Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Snowbridge free consensus updates #5201

Merged
merged 23 commits into from
Sep 2, 2024
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
5dde91c
Free finalized Ethereum updates (#159)
claravanstaden Jul 31, 2024
e8e0616
cleanup
claravanstaden Jul 31, 2024
8aa9d7e
more cleanup
claravanstaden Jul 31, 2024
ea85bf5
import fix
claravanstaden Aug 1, 2024
3c55dd8
prdoc
claravanstaden Aug 1, 2024
a74bcc2
Merge branch 'master' into free-consensus-updates
claravanstaden Aug 1, 2024
e07082e
prdoc bump corrections
claravanstaden Aug 1, 2024
622d9e2
Merge remote-tracking branch 'origin/free-consensus-updates' into fre…
claravanstaden Aug 1, 2024
4994b52
Merge branch 'master' into free-consensus-updates
claravanstaden Aug 2, 2024
9391a36
saturating add
claravanstaden Aug 2, 2024
f768444
Merge branch 'master' into free-consensus-updates
claravanstaden Aug 3, 2024
6417823
Merge branch 'master' into free-consensus-updates
claravanstaden Aug 6, 2024
3321bc0
Merge branch 'master' into free-consensus-updates
claravanstaden Aug 7, 2024
6ad199b
Merge branch 'master' into free-consensus-updates
claravanstaden Aug 7, 2024
c0077e1
Update bridges/snowbridge/pallets/ethereum-client/src/lib.rs
claravanstaden Aug 26, 2024
ab8739c
pr comments
claravanstaden Aug 26, 2024
88d944d
Merge branch 'master' into free-consensus-updates
claravanstaden Aug 26, 2024
34dda83
adds westend free headers conf
claravanstaden Aug 26, 2024
ba9560a
fix prdoc
claravanstaden Aug 26, 2024
8521c51
Merge branch 'master' into free-consensus-updates
claravanstaden Aug 27, 2024
67a120b
Merge branch 'master' into free-consensus-updates
claravanstaden Sep 2, 2024
82f05de
fmt
claravanstaden Sep 2, 2024
dbea452
clippy
claravanstaden Sep 2, 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
53 changes: 42 additions & 11 deletions bridges/snowbridge/pallets/ethereum-client/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,10 @@ mod tests;
mod benchmarking;

use frame_support::{
dispatch::DispatchResult, pallet_prelude::OptionQuery, traits::Get, transactional,
dispatch::{DispatchResult, PostDispatchInfo},
pallet_prelude::OptionQuery,
traits::Get,
transactional,
};
use frame_system::ensure_signed;
use snowbridge_beacon_primitives::{
Expand Down Expand Up @@ -82,6 +85,9 @@ pub mod pallet {
type RuntimeEvent: From<Event<Self>> + IsType<<Self as frame_system::Config>::RuntimeEvent>;
#[pallet::constant]
type ForkVersions: Get<ForkVersions>;
/// Minimum gap between finalized headers for an update to be free.
#[pallet::constant]
type FreeHeadersInterval: Get<u32>;
type WeightInfo: WeightInfo;
}

Expand Down Expand Up @@ -204,11 +210,10 @@ pub mod pallet {
#[transactional]
claravanstaden marked this conversation as resolved.
Show resolved Hide resolved
/// Submits a new finalized beacon header update. The update may contain the next
/// sync committee.
pub fn submit(origin: OriginFor<T>, update: Box<Update>) -> DispatchResult {
pub fn submit(origin: OriginFor<T>, update: Box<Update>) -> DispatchResultWithPostInfo {
ensure_signed(origin)?;
ensure!(!Self::operating_mode().is_halted(), Error::<T>::Halted);
Self::process_update(&update)?;
Ok(())
Self::process_update(&update)
}

/// Halt or resume all pallet operations. May only be called by root.
Expand Down Expand Up @@ -280,10 +285,9 @@ pub mod pallet {
Ok(())
}

pub(crate) fn process_update(update: &Update) -> DispatchResult {
pub(crate) fn process_update(update: &Update) -> DispatchResultWithPostInfo {
Self::verify_update(update)?;
Self::apply_update(update)?;
Ok(())
Self::apply_update(update)
}

/// References and strictly follows <https://github.com/ethereum/consensus-specs/blob/dev/specs/altair/light-client/sync-protocol.md#validate_light_client_update>
Expand Down Expand Up @@ -432,8 +436,9 @@ pub mod pallet {
/// Reference and strictly follows <https://github.com/ethereum/consensus-specs/blob/dev/specs/altair/light-client/sync-protocol.md#apply_light_client_update
/// Applies a finalized beacon header update to the beacon client. If a next sync committee
/// is present in the update, verify the sync committee by converting it to a
/// SyncCommitteePrepared type. Stores the provided finalized header.
fn apply_update(update: &Update) -> DispatchResult {
/// SyncCommitteePrepared type. Stores the provided finalized header. Updates are free
/// if the certain conditions specified in `check_refundable` are met.
fn apply_update(update: &Update) -> DispatchResultWithPostInfo {
let latest_finalized_state =
FinalizedBeaconState::<T>::get(LatestFinalizedBlockRoot::<T>::get())
.ok_or(Error::<T>::NotBootstrapped)?;
Expand Down Expand Up @@ -465,11 +470,17 @@ pub mod pallet {
});
};

let pays_fee = Self::check_refundable(update, latest_finalized_state.slot);
let actual_weight = match update.next_sync_committee_update {
None => T::WeightInfo::submit(),
Some(_) => T::WeightInfo::submit_with_sync_committee(),
};

if update.finalized_header.slot > latest_finalized_state.slot {
Self::store_finalized_header(update.finalized_header, update.block_roots_root)?;
}

Ok(())
Ok(PostDispatchInfo { actual_weight: Some(actual_weight), pays_fee })
Comment on lines +473 to +483
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you double-check and confirm that this doesn't allow free spamming for this call with using the same "valid" update (with update.next_sync_committee_update.is_some() == true) over and over for free, potentially DoS-ing BH blocks?

It looks like there is a compute_period verification for updating sync committee, that would not allow the DoS I described above, but I got a bit lost in the details and would like you to confirm.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@acatangiu that's a valid point, thanks! There is a specific test for that (duplicate_sync_committee_updates_are_not_free): https://github.com/paritytech/polkadot-sdk/pull/5201/files#diff-ea6025b3cac6719f750d1f0065ba30d70a16d386a5ce374e4a6b79299f48a3eeR684

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, didn't yet look at tests 🙈

Copy link
Contributor

Choose a reason for hiding this comment

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

The test tries to submit the exact same proof twice, which will fail the second time because the attested header in the proof is no longer newer than the stored one on-chain.

But what about the following scenario:

  • Relayer submits valid update for slot N in period P with sync committee S provided in Some(next_sync_committee_update)
  • Relayer then provides update for slot N+1 in period P with sync committee S provided in Some(next_sync_committee_update)?

Basically try to "farm" the Some(next_sync_committee_update) tx fee freebie, to spam update one slot at a time.

Copy link
Contributor

Choose a reason for hiding this comment

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

The above is a valid issue, and is being fixed in Snowfork#172

}

/// Computes the signing root for a given beacon header and domain. The hash tree root
Expand Down Expand Up @@ -634,11 +645,31 @@ pub mod pallet {
config::SLOTS_PER_EPOCH as u64,
));
let domain_type = config::DOMAIN_SYNC_COMMITTEE.to_vec();
// Domains are used for for seeds, for signatures, and for selecting aggregators.
// Domains are used for seeds, for signatures, and for selecting aggregators.
let domain = Self::compute_domain(domain_type, fork_version, validators_root)?;
// Hash tree root of SigningData - object root + domain
let signing_root = Self::compute_signing_root(header, domain)?;
Ok(signing_root)
}

/// Updates are free if the update is successful and the interval between the latest
/// finalized header in storage and the newly imported header is large enough. All
/// successful sync committee updates are free.
pub(super) fn check_refundable(update: &Update, latest_slot: u64) -> Pays {
// If the sync committee was successfully updated, the update may be free.
if update.next_sync_committee_update.is_some() {
return Pays::No;
}

// If free headers are allowed and the latest finalized header is larger than the
Copy link
Contributor

Choose a reason for hiding this comment

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

This part of the comment, "If free headers are allowed". I don't see it reflected in the code. Should it be another condition apart from the inequality?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, in an older implementation it was optional. Comment updated here: ab8739c#diff-adbbba03e39a047dd71745d3ed0eaf33b4e982ac58d64f273f1574816ff44940R664

// minimum slot interval, the header import transaction is free.
if update.finalized_header.slot >=
latest_slot.saturating_add(T::FreeHeadersInterval::get() as u64)
{
return Pays::No;
}

Pays::Yes
}
}
}
2 changes: 2 additions & 0 deletions bridges/snowbridge/pallets/ethereum-client/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ use sp_std::default::Default;
use std::{fs::File, path::PathBuf};

type Block = frame_system::mocking::MockBlock<Test>;
use frame_support::traits::ConstU32;
use sp_runtime::BuildStorage;

fn load_fixture<T>(basename: String) -> Result<T, serde_json::Error>
Expand Down Expand Up @@ -111,6 +112,7 @@ parameter_types! {
impl ethereum_beacon_client::Config for Test {
type RuntimeEvent = RuntimeEvent;
type ForkVersions = ChainForkVersions;
type FreeHeadersInterval = ConstU32<96>;
type WeightInfo = ();
}

Expand Down
Loading
Loading