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.

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

Snowbridge Beacon header age check #3791

Merged

Conversation

claravanstaden
Copy link
Contributor

@claravanstaden claravanstaden commented Mar 22, 2024

This is a cherry-pick from master of #3727 and #3790

Expected patches for (1.7.0):
snowbridge-pallet-ethereum-client

## 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!>
@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable-int
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5614993

@acatangiu
Copy link
Contributor

why is this draft? looks ready to merge to me

@claravanstaden
Copy link
Contributor Author

why is this draft? looks ready to merge to me

Well #3790 isn't merged on master yet but I guess we can merge.

@claravanstaden claravanstaden marked this pull request as ready for review March 22, 2024 10:36
@acatangiu acatangiu merged commit 486c044 into paritytech:release-crates-io-v1.7.0 Mar 22, 2024
25 of 79 checks passed
@claravanstaden claravanstaden deleted the snowbridge-fixes-1.7 branch March 22, 2024 10:37
@claravanstaden
Copy link
Contributor Author

#3790 wasn't cherry-picked into this branch yet. I'll make a new PR...

@claravanstaden claravanstaden changed the title Snowbridge Beacon header age check and add linear fee multiplier to ensure safety margins Snowbridge Beacon header age check Mar 22, 2024
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants