Snowbridge Beacon header age check #3727
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
vgeddes
reviewed
Mar 22, 2024
| @@ -15,8 +15,6 @@ | |||
| //! ## Consensus Updates | |||
| //! | |||
| //! * [`Call::submit`]: Submit a finalized beacon header with an optional sync committee update | |||
| //! * [`Call::submit_execution_header`]: Submit an execution header together with an ancestry proof | |||
There was a problem hiding this comment.
This does not seem relevant?
There was a problem hiding this comment.
Correct, reverted in 27ecafd.
| @@ -286,8 +286,8 @@ impl Contains<RuntimeCall> for SafeCallFilter { | |||
| match call { | |||
| RuntimeCall::System(frame_system::Call::set_storage { items }) | |||
| if items.iter().all(|(k, _)| { | |||
| k.eq(&bridging::XcmBridgeHubRouterByteFee::key()) | | |||
There was a problem hiding this comment.
While this is certainly a bug you're fixing, its still not relevant to the issue in the beacon light client. I suggest updating the PR description, saying you added some miscellaneous unrelated fixes.
There was a problem hiding this comment.
I reverted this change to keep the PR as focused as possible. I added the change in this PR: #3761
added 2 commits
March 22, 2024 10:36
acatangiu
approved these changes
Mar 22, 2024
acatangiu
added
the
R0-silent
bkontur
approved these changes
Mar 22, 2024
Merged
via the queue into
paritytech:master
with commit Mar 22, 2024
3410dfb
133 of 140 checks passed
claravanstaden
added a commit
to Snowfork/polkadot-sdk
that referenced
this pull request
Mar 22, 2024
## Bug Explanation Adds a check that prevents finalized headers with a gap larger than the sync committee period being imported, which could cause execution headers in the gap being unprovable. The current version of the Ethereum client checks that there is a header at least every sync committee, but it doesn't check that the headers are within a sync period of each other. For example: Header 100 (sync committee period 1) Header 9000 (sync committee period 2) (8900 blocks apart) These headers are in adjacent sync committees, but more than the sync committee period (8192 blocks) apart. The reason we need a header every 8192 slots at least, is the header is used to prove messages within the last 8192 blocks. If we import header 9000, and we receive a message to be verified at header 200, the `block_roots` field of header 9000 won't contain the header in order to do the ancestry check. ## Environment While running in Rococo, this edge case was discovered after the relayer was offline for a few days. It is unlikely, but not impossible, to happen again and so it should be backported to polkadot-sdk 1.7.0 (so that [polkadot-fellows/runtimes](https://github.com/polkadot-fellows/runtimes) can be updated with the fix). Our Ethereum client has been operational on Rococo for the past few months, and this been the only major issue discovered so far. ### Unrelated Change An unrelated nit: Removes a left over file that should have been deleted when the `parachain` directory was removed. --------- Co-authored-by: claravanstaden <Cats 4 life!>
dharjeezy
pushed a commit
to dharjeezy/polkadot-sdk
that referenced
this pull request
Mar 24, 2024
## Bug Explanation Adds a check that prevents finalized headers with a gap larger than the sync committee period being imported, which could cause execution headers in the gap being unprovable. The current version of the Ethereum client checks that there is a header at least every sync committee, but it doesn't check that the headers are within a sync period of each other. For example: Header 100 (sync committee period 1) Header 9000 (sync committee period 2) (8900 blocks apart) These headers are in adjacent sync committees, but more than the sync committee period (8192 blocks) apart. The reason we need a header every 8192 slots at least, is the header is used to prove messages within the last 8192 blocks. If we import header 9000, and we receive a message to be verified at header 200, the `block_roots` field of header 9000 won't contain the header in order to do the ancestry check. ## Environment While running in Rococo, this edge case was discovered after the relayer was offline for a few days. It is unlikely, but not impossible, to happen again and so it should be backported to polkadot-sdk 1.7.0 (so that [polkadot-fellows/runtimes](https://github.com/polkadot-fellows/runtimes) can be updated with the fix). Our Ethereum client has been operational on Rococo for the past few months, and this been the only major issue discovered so far. ### Unrelated Change An unrelated nit: Removes a left over file that should have been deleted when the `parachain` directory was removed. --------- Co-authored-by: claravanstaden <Cats 4 life!>
claravanstaden
added a commit
to Snowfork/polkadot-sdk
that referenced
this pull request
Mar 25, 2024
…nsure safety margins (paritytech#3791) This is a cherry-pick from master of paritytech#3727 and paritytech#3790 Expected patches for (1.7.0): snowbridge-pallet-ethereum-client snowbridge-pallet-inbound-queue snowbridge-pallet-outbound-queue snowbridge-outbound-queue-runtime-api snowbridge-pallet-system snowbridge-core
claravanstaden
added a commit
to Snowfork/polkadot-sdk
that referenced
this pull request
Mar 25, 2024
…nsure safety margins (paritytech#3791) This is a cherry-pick from master of paritytech#3727 and paritytech#3790 Expected patches for (1.7.0): snowbridge-pallet-ethereum-client snowbridge-pallet-inbound-queue snowbridge-pallet-outbound-queue snowbridge-outbound-queue-runtime-api snowbridge-pallet-system snowbridge-core
acatangiu
added a commit
that referenced
this pull request
Mar 26, 2024
…nsure safety margins (#3814) This is a cherry-pick from master of #3790 and #3727 Expected patches for (1.8.0): snowbridge-pallet-ethereum-client snowbridge-pallet-inbound-queue snowbridge-pallet-outbound-queue snowbridge-outbound-queue-runtime-api snowbridge-pallet-system snowbridge-core --------- Co-authored-by: Vincent Geddes <[email protected]> Co-authored-by: Vincent Geddes <[email protected]> Co-authored-by: Adrian Catangiu <[email protected]>
acatangiu
added a commit
that referenced
this pull request
Mar 26, 2024
…nsure safety margins (#3815) This is a cherry-pick from master of #3790 and #3727 Expected patches for (1.9.0): snowbridge-pallet-ethereum-client snowbridge-pallet-inbound-queue snowbridge-pallet-outbound-queue snowbridge-outbound-queue-runtime-api snowbridge-pallet-system snowbridge-core --------- Co-authored-by: Vincent Geddes <[email protected]> Co-authored-by: Vincent Geddes <[email protected]> Co-authored-by: Adrian Catangiu <[email protected]>
acatangiu
referenced
this pull request
May 16, 2024
…c committee in next store period (#4482) This is a cherry-pick from master of [#3790 and https://github.com/paritytech/polkadot-sdk/pull/3727](https://github.com/paritytech/polkadot-sdk/pull/4478) Expected patches for (1.7.0): snowbridge-pallet-ethereum-client Co-authored-by: Vincent Geddes <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Bug Explanation
Adds a check that prevents finalized headers with a gap larger than the sync committee period being imported, which could cause execution headers in the gap being unprovable. The current version of the Ethereum client checks that there is a header at least every sync committee, but it doesn't check that the headers are within a sync period of each other. For example:
Header 100 (sync committee period 1)
Header 9000 (sync committee period 2)
(8900 blocks apart)
These headers are in adjacent sync committees, but more than the sync committee period (8192 blocks) apart.
The reason we need a header every 8192 slots at least, is the header is used to prove messages within the last 8192 blocks. If we import header 9000, and we receive a message to be verified at header 200, the
block_rootsfield of header 9000 won't contain the header in order to do the ancestry check.Environment
While running in Rococo, this edge case was discovered after the relayer was offline for a few days. It is unlikely, but not impossible, to happen again and so it should be backported to polkadot-sdk 1.7.0 (so that polkadot-fellows/runtimes can be updated with the fix).
Our Ethereum client has been operational on Rococo for the past few months, and this been the only major issue discovered so far.
Unrelated Change
An unrelated nit: Removes a left over file that should have been deleted when the
parachaindirectory was removed.