Skip to content

Commit

Permalink
Snowbridge Beacon header age check (paritytech#3727)
Browse files Browse the repository at this point in the history
## 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!>
  • Loading branch information
claravanstaden authored and claravanstaden committed Mar 22, 2024
1 parent 44c6148 commit 78aa78c
Show file tree
Hide file tree
Showing 2 changed files with 71 additions and 1 deletion.
15 changes: 15 additions & 0 deletions bridges/snowbridge/parachain/pallets/ethereum-client/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,10 @@ pub mod pallet {
InvalidExecutionHeaderProof,
InvalidAncestryMerkleProof,
InvalidBlockRootsRootMerkleProof,
/// The gap between the finalized headers is larger than the sync committee period,
/// rendering execution headers unprovable using ancestry proofs (blocks root size is
/// the same as the sync committee period slots).
InvalidFinalizedHeaderGap,
HeaderNotFinalized,
BlockBodyHashTreeRootFailed,
HeaderHashTreeRootFailed,
Expand Down Expand Up @@ -398,6 +402,17 @@ pub mod pallet {
Error::<T>::IrrelevantUpdate
);

// Verify the finalized header gap between the current finalized header and new imported
// header is not larger than the sync committee period, otherwise we cannot do
// ancestry proofs for execution headers in the gap.
ensure!(
latest_finalized_state
.slot
.saturating_add(config::SLOTS_PER_HISTORICAL_ROOT as u64) >=
update.finalized_header.slot,
Error::<T>::InvalidFinalizedHeaderGap
);

// Verify that the `finality_branch`, if present, confirms `finalized_header` to match
// the finalized checkpoint root saved in the state of `attested_header`.
let finalized_block_root: H256 = update
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ use crate::mock::{

pub use crate::mock::*;

use crate::config::{EPOCHS_PER_SYNC_COMMITTEE_PERIOD, SLOTS_PER_EPOCH};
use crate::config::{EPOCHS_PER_SYNC_COMMITTEE_PERIOD, SLOTS_PER_EPOCH, SLOTS_PER_HISTORICAL_ROOT};
use frame_support::{assert_err, assert_noop, assert_ok};
use hex_literal::hex;
use primitives::{
Expand Down Expand Up @@ -884,6 +884,61 @@ fn submit_execution_header_not_finalized() {
});
}

/// Check that a gap of more than 8192 slots between finalized headers is not allowed.
#[test]
fn submit_finalized_header_update_with_too_large_gap() {
let checkpoint = Box::new(load_checkpoint_update_fixture());
let update = Box::new(load_sync_committee_update_fixture());
let mut next_update = Box::new(load_next_sync_committee_update_fixture());

// Adds 8193 slots, so that the next update is still in the next sync committee, but the
// gap between the finalized headers is more than 8192 slots.
let slot_with_large_gap = checkpoint.header.slot + SLOTS_PER_HISTORICAL_ROOT as u64 + 1;

next_update.finalized_header.slot = slot_with_large_gap;
// Adding some slots to the attested header and signature slot since they need to be ahead
// of the finalized header.
next_update.attested_header.slot = slot_with_large_gap + 33;
next_update.signature_slot = slot_with_large_gap + 43;

new_tester().execute_with(|| {
assert_ok!(EthereumBeaconClient::process_checkpoint_update(&checkpoint));
assert_ok!(EthereumBeaconClient::submit(RuntimeOrigin::signed(1), update.clone()));
assert!(<NextSyncCommittee<Test>>::exists());
assert_err!(
EthereumBeaconClient::submit(RuntimeOrigin::signed(1), next_update.clone()),
Error::<Test>::InvalidFinalizedHeaderGap
);
});
}

/// Check that a gap of 8192 slots between finalized headers is allowed.
#[test]
fn submit_finalized_header_update_with_gap_at_limit() {
let checkpoint = Box::new(load_checkpoint_update_fixture());
let update = Box::new(load_sync_committee_update_fixture());
let mut next_update = Box::new(load_next_sync_committee_update_fixture());

next_update.finalized_header.slot = checkpoint.header.slot + SLOTS_PER_HISTORICAL_ROOT as u64;
// Adding some slots to the attested header and signature slot since they need to be ahead
// of the finalized header.
next_update.attested_header.slot =
checkpoint.header.slot + SLOTS_PER_HISTORICAL_ROOT as u64 + 33;
next_update.signature_slot = checkpoint.header.slot + SLOTS_PER_HISTORICAL_ROOT as u64 + 43;

new_tester().execute_with(|| {
assert_ok!(EthereumBeaconClient::process_checkpoint_update(&checkpoint));
assert_ok!(EthereumBeaconClient::submit(RuntimeOrigin::signed(1), update.clone()));
assert!(<NextSyncCommittee<Test>>::exists());
assert_err!(
EthereumBeaconClient::submit(RuntimeOrigin::signed(1), next_update.clone()),
// The test should pass the InvalidFinalizedHeaderGap check, and will fail at the
// next check, the merkle proof, because we changed the next_update slots.
Error::<Test>::InvalidHeaderMerkleProof
);
});
}

/* IMPLS */

#[test]
Expand Down

0 comments on commit 78aa78c

Please sign in to comment.