Skip to content
This repository has been archived by the owner on Oct 22, 2024. It is now read-only.

Free consensus updates border condition #172

Merged
merged 8 commits into from
Sep 10, 2024
Merged

Free consensus updates border condition #172

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
8a27360
illustrates border condition
claravanstaden Sep 2, 2024
209274b
adds test payloads
claravanstaden Sep 2, 2024
c8307f7
cleanup
claravanstaden Sep 3, 2024
c2ed92d
update comment
claravanstaden Sep 3, 2024
25c83b5
fmt
claravanstaden Sep 3, 2024
3997df7
shuffle things around
claravanstaden Sep 3, 2024
6919815
a few more test checks
claravanstaden Sep 10, 2024
9ad72e8
Update bridges/snowbridge/pallets/ethereum-client/src/lib.rs
claravanstaden Sep 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions bridges/snowbridge/pallets/ethereum-client/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,6 +328,11 @@ pub mod pallet {
let update_has_next_sync_committee = !<NextSyncCommittee<T>>::exists() &&
(update.next_sync_committee_update.is_some() &&
update_attested_period == store_period);

println!("update_has_next_sync_committee {}", update_has_next_sync_committee);
println!("update.attested_header.slot {}", update.attested_header.slot);
println!("latest_finalized_state.slot {}", latest_finalized_state.slot);

ensure!(
update.attested_header.slot > latest_finalized_state.slot ||
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps this should rather be:

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Screenshot 2024-09-02 at 09 28 34

Copy link

@yrong yrong Sep 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@yrong yrong Sep 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@yrong yrong Sep 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
			}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link

@yrong yrong Sep 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

update_has_next_sync_committee,
Expand Down Expand Up @@ -457,10 +462,13 @@ pub mod pallet {
<Error<T>>::InvalidSyncCommitteeUpdate
);
<NextSyncCommittee<T>>::set(sync_committee_prepared);
println!("condition 1");
} else if update_finalized_period == store_period + 1 {
<CurrentSyncCommittee<T>>::set(<NextSyncCommittee<T>>::get());
<NextSyncCommittee<T>>::set(sync_committee_prepared);
println!("condition 2");
}
println!("SyncCommitteeUpdated at period {}", update_finalized_period);
log::info!(
target: LOG_TARGET,
"💫 SyncCommitteeUpdated at period {}.",
Expand Down
20 changes: 20 additions & 0 deletions bridges/snowbridge/pallets/ethereum-client/src/mock.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,26 @@ pub fn load_next_finalized_header_update_fixture() -> snowbridge_beacon_primitiv
load_fixture("next-finalized-header-update.json".to_string()).unwrap()
}

pub fn load_test_checkpoint_update_fixture(
) -> snowbridge_beacon_primitives::CheckpointUpdate<{ config::SYNC_COMMITTEE_SIZE }> {
load_fixture("initial-checkpoint-test.json".to_string()).unwrap()
}

pub fn load_test_sync_committee_update_fixture() -> snowbridge_beacon_primitives::Update<
{ config::SYNC_COMMITTEE_SIZE },
{ config::SYNC_COMMITTEE_BITS_SIZE },
> {
load_fixture("sync-committee-update-test.json".to_string()).unwrap()
}

pub fn load_second_test_sync_committee_update_fixture() -> snowbridge_beacon_primitives::Update<
{ config::SYNC_COMMITTEE_SIZE },
{ config::SYNC_COMMITTEE_BITS_SIZE },
> {
load_fixture("second-sync-committee-update-test.json".to_string()).unwrap()
}


pub fn get_message_verification_payload() -> (Log, Proof) {
let inbound_fixture = snowbridge_pallet_ethereum_client_fixtures::make_inbound_fixture();
(inbound_fixture.message.event_log, inbound_fixture.message.proof)
Expand Down
24 changes: 24 additions & 0 deletions bridges/snowbridge/pallets/ethereum-client/src/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -698,6 +698,30 @@ fn duplicate_sync_committee_updates_are_not_free() {
});
}

#[test]
fn sync_committee_update_for_sync_committee_already_imported_are_not_free() {
let checkpoint = Box::new(load_test_checkpoint_update_fixture());
let sync_committee_update = Box::new(load_test_sync_committee_update_fixture());
let second_sync_committee_update = Box::new(load_second_test_sync_committee_update_fixture());

new_tester().execute_with(|| {
println!("checkpoint =============");
assert_ok!(EthereumBeaconClient::process_checkpoint_update(&checkpoint));
println!("submit 1 =============");
let result =
EthereumBeaconClient::submit(RuntimeOrigin::signed(1), sync_committee_update.clone());
assert_ok!(result);
assert_eq!(result.unwrap().pays_fee, Pays::No);

println!("submit 2 =============");
// Check that if the same update is submitted, the update is not free.
let second_result =
EthereumBeaconClient::submit(RuntimeOrigin::signed(1), second_sync_committee_update);
//assert_err!(second_result, Error::<Test>::IrrelevantUpdate);
assert_eq!(second_result.unwrap_err().post_info.pays_fee, Pays::Yes);
});
}

/* IMPLS */

#[test]
Expand Down
Loading