Free consensus updates border condition

[claravanstaden]

In the current implementation, there exists a border condition where the consensus update is free, where it should be paid.

The condition is:

• A next sync committee update is provided for a sync committee that has already successfully been imported (should not be free)
• The header update is also not new (same finalized header that has already been imported)

Test output:
submit 1 =============
update_has_next_sync_committee true
update.attested_header.slot 129
latest_finalized_state.slot 64
condition 1
SyncCommitteeUpdated at period 0
Pays::No

submit 2 =============
update_has_next_sync_committee false
update.attested_header.slot 128
latest_finalized_state.slot 64
SyncCommitteeUpdated at period 0
Pays::No (should be Yes)

Summary

To articulate the problem: It is possible to send an update with a sync committee, where neither the finalized header nor the sync committee would be updated, yet the update is free. We need to filter out irrelevant updates.

Resolves: SNO-1142

[claravanstaden]

Perhaps this should rather be:

Suggested change

	update.attested_header.slot > latest_finalized_state.slot ||
	update.finalized_header.slot > latest_finalized_state.slot ||

[claravanstaden]

It would deviate from the spec though: https://github.com/ethereum/consensus-specs/blob/dev/specs/altair/light-client/sync-protocol.md#validate_light_client_update

[yrong]

I'd prefer to not make this change deviate from the spec. To avoid spam of free consensus update maybe we can check the update interval even for sync committee updates?

polkadot-sdk/bridges/snowbridge/pallets/ethereum-client/src/lib.rs

Lines 661 to 663 in dfee885

	if update.next_sync_committee_update.is_some() {
	return Pays::No;
	}

[claravanstaden]

What happens if the sync committee update slot is smaller than the current finalized header? Then using the interval check does not work

[claravanstaden]

The reason why I think my change is fundamentally the correct one is because we assume if an update makes it through verify_update, it is relevant and will change the light client state. This border case passes through verify_update to apply_update, but makes no light client state change. Which should not happen imo.

[yrong]

if an update makes it through verify_update, it is relevant and will change the light client state.

I'd assume it's a normal case update.finalized_header.slot < latest_finalized_state.slot for sync committee updates. Still prefer to strictly follow the spec unless absolute nessessary to change.

[yrong]

What happens if the sync committee update slot is smaller than the current finalized header? Then using the interval check does not work

What I mean is something like this to avoid spamming.

			if update.next_sync_committee_update.is_some() &&
				update.finalized_header.slot >=
					latest_slot.saturating_add((T::FreeHeadersInterval::get() / 2) as u64)
			{
				return Pays::No;
			}

[claravanstaden]

if an update makes it through verify_update, it is relevant and will change the light client state.

I'd assume it's a normal case update.finalized_header.slot < latest_finalized_state.slot for sync committee updates. Still prefer to strictly follow the spec unless absolute nessessary to change.

Yes, but if update.finalized_header.slot < latest_finalized_state.slot then the update must contain a sync committee update and the NextSyncCommittee must be empty: https://github.com/paritytech/polkadot-sdk/blob/master/bridges/snowbridge/pallets/ethereum-client/src/lib.rs#L332

For the case I highlighted in the PR description, update_has_next_sync_committee is false and the finalized update is older than the finalized header.

I understand your reluctance to deviate from the spec, but fundamentally it is worrying to be that an update is essentially irrelevant but can somehow pass the validation and make it to the apply function.

[claravanstaden]

What happens if the sync committee update slot is smaller than the current finalized header? Then using the interval check does not work

What I mean is something like this to avoid spamming.

			if update.next_sync_committee_update.is_some() &&
				update.finalized_header.slot >=
					latest_slot.saturating_add((T::FreeHeadersInterval::get() / 2) as u64)
			{
				return Pays::No;
			}

I don't understand the logic? Why divide FreeHeadersInterval by 2 here?

[yrong]

To add some loose check for sync committee update avoid spamming, just an example may not be a correct way.

// If the next sync committee is not known and this update sets it, the update is free.
// If the sync committee update is in a period that we have not received an update for,
// the update is free.

I like the impl in your recent commit.

[claravanstaden]

Move this up so we can check if the NextSyncCommittee is empty before updating this update.

[claravanstaden]

@yrong @alistair-singh @vgeddes @acatangiu I have prototyped a fix for this without changing the spec.

However, I think my original change is the most correct solution. Our use case differs from the Ethereum consensus spec because we don't track the head of the chain, only finalized updates. So my suggested change is more correct and reduces the need for other hacks to filter our irrelevant sync committee updates.

I checked it in the Ethereum foundation R&D: #172 (comment)

[alistair-singh]

+1