Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Prune messages from confirmation tx body, not from the on_idle #2211

Merged
merged 1 commit into from
Jun 16, 2023
Merged

Prune messages from confirmation tx body, not from the on_idle #2211

merged 1 commit into from
Jun 16, 2023

Conversation

svyatonik
Copy link
Contributor

@svyatonik svyatonik commented Jun 15, 2023 •
edited
Loading

closes #2210

See #2210 for rationale. I've also updated weights to see the effect don't look at the absolute numbers, only at db reads + writes.

I've left audit-maybe label - it'll probably need to be audited as a part of dynamic-lanes audit.

IMPORTANT: runtime storage upgrade required: #2211 (comment)

@svyatonik svyatonik added P-Runtime PR-audit-maybe Would be good to have the PR audited, but it doesn't look critical. labels Jun 15, 2023
svyatonik
svyatonik commented Jun 15, 2023
View reviewed changes
modules/messages/src/outbound_lane.rs
@@ -150,41 +145,17 @@ impl<S: OutboundLaneStorage> OutboundLane<S> {

ensure_unrewarded_relayers_are_correct(confirmed_messages.end, relayers)?;

// prune all confirmed messages
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two things to note here:

  1. this removal is accounted by our benchmarks (see changes in weights db writes);
  2. we do weight refund in delivery confirmation transaction, but it doesn't need to be changed because it does refund based on total_messages from the call arguments and confirmed_messages range, returned by this function. So all should be fine here

acatangiu
acatangiu approved these changes Jun 16, 2023
View reviewed changes
Copy link
Collaborator

@acatangiu acatangiu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@svyatonik svyatonik merged commit fa7e240 into master Jun 16, 2023
@svyatonik svyatonik deleted the prune-messages-in-confirmation-tx branch June 16, 2023 09:59
@svyatonik svyatonik mentioned this pull request Jun 19, 2023
Closed
@svyatonik svyatonik added the PR-breaksruntime A PR that is going to break runtime bridge compatibility. We need to be careful with upgrade. label Jul 20, 2023
@svyatonik
Copy link
Contributor Author

Forgot to add this warning: when we'll be upgrading existing pallets to this version, we shall not forget to prune old messages and update lane state accordingly

bkontur pushed a commit that referenced this pull request May 7, 2024
@svyatonik @bkontur
prune messages from confirmation tx, not from the on_idle (#2211)
fb7e124
@bkontur bkontur mentioned this pull request Jun 13, 2024
Draft
5 tasks
bkontur added a commit to paritytech/polkadot-sdk that referenced this pull request Jul 4, 2024
@bkontur
prune messages from confirmation tx, not from the on_idle (#2211)
e6dea3a 
Original PR with more context: paritytech/parity-bridges-common#2211
@bkontur bkontur mentioned this pull request Jul 4, 2024
Merged
4 tasks
bkontur added a commit to paritytech/polkadot-sdk that referenced this pull request Jul 4, 2024
@bkontur @svyatonik
prune messages from confirmation tx, not from the on_idle (#2211)
b352ea0 
Original PR with more context: paritytech/parity-bridges-common#2211


Signed-off-by: Branislav Kontur <[email protected]>
Co-authored-by: Svyatoslav Nikolsky <[email protected]>
bkontur added a commit to paritytech/polkadot-sdk that referenced this pull request Jul 4, 2024
@bkontur @svyatonik
prune messages from confirmation tx, not from the on_idle (#2211)
4fbda94 
Original PR with more context: paritytech/parity-bridges-common#2211


Signed-off-by: Branislav Kontur <[email protected]>
Co-authored-by: Svyatoslav Nikolsky <[email protected]>
acatangiu pushed a commit to paritytech/polkadot-sdk that referenced this pull request Jul 10, 2024
@bkontur @svyatonik
[bridges] Prune messages from confirmation tx body, not from the on_i…
7c875be 
…dle (#4944)

Original PR with more context:
paritytech/parity-bridges-common#2211
Relates to:
paritytech/parity-bridges-common#2210

## TODO

- [x] fresh weighs for `pallet_bridge_messages`
- [x] add `try_state` for `pallet_bridge_messages` which checks for
unpruned messages - relates to the
[comment](paritytech/parity-bridges-common#2211 (comment))
- [x] ~prepare migration, that prunes leftovers, which would be pruned
eventually from `on_idle` the
[comment](paritytech/parity-bridges-common#2211 (comment)
can be done also by `set_storage` / `kill_storage` or with
`OnRuntimeUpgrade` implementatino when `do_try_state_for_outbound_lanes`
detects problem.

## Open question

- [ ] Do we really need `oldest_unpruned_nonce` afterwards?
- after the runtime upgrade and when `do_try_state_for_outbound_lanes`
pass, we won't need any migrations here
    - we won't even need `do_try_state_for_outbound_lanes`

---------

Signed-off-by: Branislav Kontur <[email protected]>
Co-authored-by: Svyatoslav Nikolsky <[email protected]>
Co-authored-by: command-bot <>
@bkontur bkontur mentioned this pull request Jul 11, 2024
Merged
4 tasks
bkontur added a commit to paritytech/polkadot-sdk that referenced this pull request Jul 11, 2024
@bkontur @svyatonik
prune messages from confirmation tx, not from the on_idle (#2211)
b4c8d95 
Original PR with more context: paritytech/parity-bridges-common#2211


Signed-off-by: Branislav Kontur <[email protected]>
Co-authored-by: Svyatoslav Nikolsky <[email protected]>
bkontur added a commit to paritytech/polkadot-sdk that referenced this pull request Jul 11, 2024
@bkontur @svyatonik
prune messages from confirmation tx, not from the on_idle (#2211)
9b90f4b 
Original PR with more context: paritytech/parity-bridges-common#2211


Signed-off-by: Branislav Kontur <[email protected]>
Co-authored-by: Svyatoslav Nikolsky <[email protected]>
bkontur added a commit to paritytech/polkadot-sdk that referenced this pull request Jul 17, 2024
@bkontur @svyatonik
prune messages from confirmation tx, not from the on_idle (#2211)
84ff166 
Original PR with more context: paritytech/parity-bridges-common#2211


Signed-off-by: Branislav Kontur <[email protected]>
Co-authored-by: Svyatoslav Nikolsky <[email protected]>
bkontur added a commit to paritytech/polkadot-sdk that referenced this pull request Jul 23, 2024
@bkontur @svyatonik
prune messages from confirmation tx, not from the on_idle (#2211)
9a0a37b 
Original PR with more context: paritytech/parity-bridges-common#2211


Signed-off-by: Branislav Kontur <[email protected]>
Co-authored-by: Svyatoslav Nikolsky <[email protected]>
bkontur added a commit to paritytech/polkadot-sdk that referenced this pull request Jul 26, 2024
@bkontur @svyatonik
prune messages from confirmation tx, not from the on_idle (#2211)
de72d65 
Original PR with more context: paritytech/parity-bridges-common#2211


Signed-off-by: Branislav Kontur <[email protected]>
Co-authored-by: Svyatoslav Nikolsky <[email protected]>
bkontur added a commit to paritytech/polkadot-sdk that referenced this pull request Jul 27, 2024
@bkontur @svyatonik
prune messages from confirmation tx, not from the on_idle (#2211)
fb038d2 
Original PR with more context: paritytech/parity-bridges-common#2211


Signed-off-by: Branislav Kontur <[email protected]>
Co-authored-by: Svyatoslav Nikolsky <[email protected]>
bkontur added a commit to paritytech/polkadot-sdk that referenced this pull request Jul 29, 2024
@bkontur @svyatonik
prune messages from confirmation tx, not from the on_idle (#2211)
fc83163 
Original PR with more context: paritytech/parity-bridges-common#2211


Signed-off-by: Branislav Kontur <[email protected]>
Co-authored-by: Svyatoslav Nikolsky <[email protected]>
bkontur added a commit to paritytech/polkadot-sdk that referenced this pull request Jul 30, 2024
@bkontur @svyatonik
prune messages from confirmation tx, not from the on_idle (#2211)
d63d620 
Original PR with more context: paritytech/parity-bridges-common#2211


Signed-off-by: Branislav Kontur <[email protected]>
Co-authored-by: Svyatoslav Nikolsky <[email protected]>
bkontur added a commit to paritytech/polkadot-sdk that referenced this pull request Jul 31, 2024
@bkontur @svyatonik
prune messages from confirmation tx, not from the on_idle (#2211)
712a690 
Original PR with more context: paritytech/parity-bridges-common#2211


Signed-off-by: Branislav Kontur <[email protected]>
Co-authored-by: Svyatoslav Nikolsky <[email protected]>
github-merge-queue bot pushed a commit to paritytech/polkadot-sdk that referenced this pull request Aug 23, 2024
@bkontur @serban300 @svyatonik
[bridges] Prune messages from confirmation tx body, not from the on_i…
e4ffba6 
…dle (#5006)

(Please, do not merge until SA, reverted and restored of
#4944)

Original PR with more context:
paritytech/parity-bridges-common#2211
Relates to:
paritytech/parity-bridges-common#2210

## TODO

- [x] fresh weighs for `pallet_bridge_messages`
- [x] add `try_state` for `pallet_bridge_messages` which checks for
unpruned messages - relates to the
[comment](paritytech/parity-bridges-common#2211 (comment))
- [x] ~prepare migration, that prunes leftovers, which would be pruned
eventually from `on_idle` the
[comment](paritytech/parity-bridges-common#2211 (comment)
can be done also by `set_storage` / `kill_storage` or with
`OnRuntimeUpgrade` implementatino when `do_try_state_for_outbound_lanes`
detects problem.

## Open question

- [ ] Do we really need `oldest_unpruned_nonce` afterwards?
- after the runtime upgrade and when `do_try_state_for_outbound_lanes`
pass, we won't need any migrations here
    - we won't even need `do_try_state_for_outbound_lanes`
- please check comments bellow:
#4944 (comment)

---------

Signed-off-by: Branislav Kontur <[email protected]>
Co-authored-by: Serban Iorga <[email protected]>
Co-authored-by: Svyatoslav Nikolsky <[email protected]>
Co-authored-by: command-bot <>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@acatangiu acatangiu acatangiu approved these changes

@serban300 serban300 serban300 approved these changes

Assignees
No one assigned
Labels
P-Runtime PR-audit-maybe Would be good to have the PR audited, but it doesn't look critical. PR-breaksruntime A PR that is going to break runtime bridge compatibility. We need to be careful with upgrade.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Prune confirmed messages in the confirmation transaction
3 participants
@svyatonik @serban300 @acatangiu