Adds finalized header gap check

[claravanstaden]

Resolves: SNO-912

This is just on the on-chain part, off-chain coming soon.

[vgeddes]

If I understand correctly, a max gap of 8192 slots is equivalent to ensuring that there is at least one imported beacon header per sync committee period?

If that is the case we could rather use the compute_period function to simplify the check (like we do with many other checks).

let header_period = compute_period(update.finalized_header.slot);
let latest_finalized_state_period = compute_period(latest_finalized_state.slot);
ensure!(
  (latest_finalized_state_period..=latest_finalized_state_period + 1).contains(&header_period)
  Error::<T>::InvalidFinalizedHeaderGap
);

[vgeddes]

Should preferably define the max gap as a constant in config.rs as its inputs are constants, and use saturating or checked arithmetic to guard against overflow. Luckily saturating_mul can be used at compile time.

pub const HEADER_IMPORT_MAX_GAP: usize = EPOCHS_PER_SYNC_COMMITTEE_PERIOD.saturating_mul(SLOTS_PER_EPOCH);

Suggested change

	let max_latency = config::EPOCHS_PER_SYNC_COMMITTEE_PERIOD * config::SLOTS_PER_EPOCH;
	ensure!(
	latest_finalized_state.slot + max_latency as u64 >= update.finalized_header.slot,
	Error::<T>::InvalidFinalizedHeaderGap
	);
	ensure!(
	update.finalized_header.slot <= latest_finalized_state.slot + HEADER_IMPORT_MAX_GAP as u64,
	Error::<T>::InvalidFinalizedHeaderGap
	);

[claravanstaden]

Thanks! This is a good suggestion, adding in c263214.

[vgeddes]

I see there is already a SLOTS_PER_HISTORICAL_ROOT constant, which has a value of 8192. Can we use reuse that here?

[yrong]

Yeah, prefer to reuse SLOTS_PER_HISTORICAL_ROOT

[claravanstaden]

If I understand correctly, a max gap of 8192 slots is equivalent to ensuring that there is at least one imported beacon header per sync committee period?

If that is the case we could rather use the compute_period function to simplify the check (like we do with many other checks).

let header_period = compute_period(update.finalized_header.slot);
let latest_finalized_state_period = compute_period(latest_finalized_state.slot);
ensure!(
  (latest_finalized_state_period..=latest_finalized_state_period + 1).contains(&header_period)
  Error::<T>::InvalidFinalizedHeaderGap
);

The two finalized headers could each be in their different sync committee periods, but more than 8192 headers apart. For example:

1. Header 100 (sync committee period 1)
2. Header 9000 (sync committee period 2)

(8900 blocks apart)

The reason we need a header every 8192 slots at least, is because the block_roots array contains 8192 slots, which is used for ancestry proofs. 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. We also can't import an older finalized header because of this check: https://github.com/Snowfork/polkadot-sdk/blob/snowbridge/bridges/snowbridge/pallets/ethereum-client/src/lib.rs#L396 So then the light client is essentially bricked because it can never do the ancestry check. This is what happened on Rococo.

[vgeddes]

The two finalized headers could each be in their different sync committee periods, but more than 8192 headers apart. For example:

Thanks, I understand better now. Makes sense 👍

[claravanstaden]

@vgeddes @yrong ready for final review 😄