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 - Ethereum Client - Reject finalized updates without a sync committee in next store period #4482

Conversation

claravanstaden
Copy link
Contributor

This is a cherry-pick from master of https://github.com/paritytech/polkadot-sdk/pull/3790 and https://github.com/paritytech/polkadot-sdk/pull/3727

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

…c committee in next store period (paritytech#4478)

While syncing Ethereum consensus updates to the Snowbridge Ethereum
light client, the syncing process stalled due to error
`InvalidSyncCommitteeUpdate` when importing the next sync committee for
period `1087`.

This bug manifested specifically because our light client checkpoint is
a few weeks old (submitted to governance weeks ago) and had to catchup
until a recent block. Since then, we have done thorough testing of the
catchup sync process.

### Symptoms
- Import next sync committee for period `1086` (essentially period
`1087`). Light client store period = `1086`.
- Import header in period `1087`. Light client store period = `1087`.
The current and next sync committee is not updated, and is now in an
outdated state. (current sync committee = `1086` and current sync
committee = `1087`, where it should be current sync committee = `1087`
and current sync committee = `None`)
- Import next sync committee for period `1087` (essentially period
`1088`) fails because the expected next sync committee's roots don't
match.

### Bug
The bug here is that the current and next sync committee's didn't
handover when an update in the next period was received.

### Fix
There are two possible fixes here:
1. Correctly handover sync committees when a header in the next period
is received.
2. Reject updates in the next period until the next sync committee
period is known.

We opted for solution 2, which is more conservative and requires less
changes.

### Polkadot-sdk versions
This fix should be backported in polkadot-sdk versions 1.7 and up.

Snowfork PR: #145

---------

Co-authored-by: Vincent Geddes <[email protected]>
@paritytech-review-bot paritytech-review-bot bot requested a review from a team May 16, 2024 14:28
@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/6224002

@claravanstaden
Copy link
Contributor Author

claravanstaden commented May 16, 2024

@bkontur clippy is failing because the Cargo.lock file should be updated. Should I update it? My changes didn't require a lock file update.

Also PR doc is failing because the format in 1.7 was different, I think. Should I remove the PR doc? Add it to 1.7.0 folder? 🤔

@acatangiu acatangiu merged commit 0ebc130 into paritytech:release-crates-io-v1.7.0 May 16, 2024
18 of 123 checks passed
fellowship-merge-bot bot pushed a commit to polkadot-fellows/runtimes that referenced this pull request May 19, 2024
Upgrades Snowbridge with an Ethereum client fixes: 
- paritytech/polkadot-sdk#4504
- paritytech/polkadot-sdk#4482

Adds a migration to reset the Ethereum checkpoint. Will be reset again
to the moment recent checkpoint before merged.

TODO:
- [x] Check migration works
- [x] Check converting Sync Committee to prepared keys is OK for a
migration

---------

Co-authored-by: Adrian Catangiu <[email protected]>
Co-authored-by: Vincent Geddes <[email protected]>
Co-authored-by: ron <[email protected]>
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