Skip to content

Commit

Permalink
Snowbridge - Ethereum Client - Reject finalized updates without a syn…
Browse files Browse the repository at this point in the history
…c committee in next store period (#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: Snowfork#145

---------

Co-authored-by: Vincent Geddes <117534+vgeddes@users.noreply.github.com>
  • Loading branch information
claravanstaden and vgeddes committed May 16, 2024
1 parent fb2c9ca commit 707ae1a
Show file tree
Hide file tree
Showing 3 changed files with 33 additions and 1 deletion.
7 changes: 7 additions & 0 deletions bridges/snowbridge/pallets/ethereum-client/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,7 @@ pub mod pallet {
#[pallet::error]
pub enum Error<T> {
SkippedSyncCommitteePeriod,
SyncCommitteeUpdateRequired,
/// Attested header is older than latest finalized header.
IrrelevantUpdate,
NotBootstrapped,
Expand Down Expand Up @@ -393,6 +394,7 @@ pub mod pallet {

// Verify update is relevant.
let update_attested_period = compute_period(update.attested_header.slot);
let update_finalized_period = compute_period(update.finalized_header.slot);
let update_has_next_sync_committee = !<NextSyncCommittee<T>>::exists() &&
(update.next_sync_committee_update.is_some() &&
update_attested_period == store_period);
Expand Down Expand Up @@ -468,6 +470,11 @@ pub mod pallet {
),
Error::<T>::InvalidSyncCommitteeMerkleProof
);
} else {
ensure!(
update_finalized_period == store_period,
Error::<T>::SyncCommitteeUpdateRequired
);
}

// Verify sync committee aggregate signature.
Expand Down
14 changes: 13 additions & 1 deletion bridges/snowbridge/pallets/ethereum-client/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -451,20 +451,32 @@ fn submit_update_with_sync_committee_in_current_period() {
}

#[test]
fn submit_update_in_next_period() {
fn reject_submit_update_in_next_period() {
let checkpoint = Box::new(load_checkpoint_update_fixture());
let sync_committee_update = Box::new(load_sync_committee_update_fixture());
let update = Box::new(load_next_finalized_header_update_fixture());
let sync_committee_period = compute_period(sync_committee_update.finalized_header.slot);
let next_sync_committee_period = compute_period(update.finalized_header.slot);
assert_eq!(sync_committee_period + 1, next_sync_committee_period);
let next_sync_committee_update = Box::new(load_next_sync_committee_update_fixture());

new_tester().execute_with(|| {
assert_ok!(EthereumBeaconClient::process_checkpoint_update(&checkpoint));
assert_ok!(EthereumBeaconClient::submit(
RuntimeOrigin::signed(1),
sync_committee_update.clone()
));
// check an update in the next period is rejected
assert_err!(
EthereumBeaconClient::submit(RuntimeOrigin::signed(1), update.clone()),
Error::<Test>::SyncCommitteeUpdateRequired
);
// submit update with next sync committee
assert_ok!(EthereumBeaconClient::submit(
RuntimeOrigin::signed(1),
next_sync_committee_update
));
// check same header in the next period can now be submitted successfully
assert_ok!(EthereumBeaconClient::submit(RuntimeOrigin::signed(1), update.clone()));
let block_root: H256 = update.finalized_header.clone().hash_tree_root().unwrap();
assert!(<FinalizedBeaconState<Test>>::contains_key(block_root));
Expand Down
13 changes: 13 additions & 0 deletions prdoc/pr_4478.prdoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0
# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json

title: Snowbridge - Ethereum Client - Reject finalized updates without a sync committee in next store period

doc:
- audience: Runtime Dev
description: |
Bug fix in the Ethereum light client that stalls the light client when an update in the next sync committee period is received without receiving the next sync committee update in the next period.

crates:
- name: snowbridge-pallet-ethereum-client
bump: patch

0 comments on commit 707ae1a

Please sign in to comment.