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.

Sign up for GitHub

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

Jump to bottom

Make snowbridge-ethereum-beacon-client generic over the network it is built for #788

Closed
wants to merge 4 commits into from
Closed

Make snowbridge-ethereum-beacon-client generic over the network it is built for #788

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
794f9e1
Use config constant for consistency
nazar-pc Mar 13, 2023
80c2ad4
Make `snowbridge-ethereum-beacon-client` generic over the network it …
nazar-pc Mar 13, 2023
494ab1c
add additional where bound to Config in the benchmarks
ParthDesai Mar 15, 2023
b683f20
fix tests using configuration
ParthDesai Mar 15, 2023
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
1 change: 1 addition & 0 deletions parachain/pallets/ethereum-beacon-client/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,4 +67,5 @@ runtime-benchmarks = [
"frame-system/runtime-benchmarks",
"hex-literal"
]
# Feature only used for testing and benchmarking
minimal = []
1 change: 1 addition & 0 deletions parachain/pallets/ethereum-beacon-client/src/benchmarking/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ mod data_mainnet;
use data_mainnet::*;

benchmarks! {
where_clause { where [(); T::SYNC_COMMITTEE_SIZE]: Sized }
sync_committee_period_update {
let caller: T::AccountId = whitelisted_caller();

Expand Down
13 changes: 2 additions & 11 deletions parachain/pallets/ethereum-beacon-client/src/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,5 @@
#[cfg(feature = "minimal")]
mod minimal;

#[cfg(not(feature = "minimal"))]
mod mainnet;

#[cfg(feature = "minimal")]
pub use minimal::*;

#[cfg(not(feature = "minimal"))]
pub use mainnet::*;
pub mod mainnet;
pub mod minimal;

pub const CURRENT_SYNC_COMMITTEE_INDEX: u64 = 22;
pub const CURRENT_SYNC_COMMITTEE_DEPTH: u64 = 5;
Expand Down
71 changes: 48 additions & 23 deletions parachain/pallets/ethereum-beacon-client/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
//! # Ethereum Beacon Client
#![cfg_attr(not(feature = "std"), no_std)]
#![allow(incomplete_features)]
#![feature(generic_const_exprs)]
Copy link
Collaborator

@vgeddes vgeddes Mar 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

We can't really use unstable features (and especially this one, which is still in development) in the common-good BridgeHub parachain. That's where this pallet is eventually going to be installed.

But simple const generics, which were stabilized in Rust 1.51, are totally fine. So then would it not be possible to support both the minimal and mainnet using a more dynamic approach, like the following:

const A: u32 = 2;
const B: u32 = 3;

function foo<const FOO: u32>() { ... }

function main() {
  if ... {
    foo<A>();
  } else {
    foo<B>();
  }
}

Another option is doing away with the minimal feature. That will increase the duration of our integration tests though. But perhaps it is better to test always against mainnet configs. Let me think over it for a bit.

Copy link
Author

Choose a reason for hiding this comment

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

Those features were only added so we can use constants defined on Config in function calls (that also have const generics) otherwise they are considered expressions in current Rust version for some reason ({} part). There are apparently more advanced versions of this that are not complete, but we literally use the most basic version of the feature possible, so I'd say the risk of using it is low.

And as I said before, runtime already requires nightly Rust (and you run tests on nightly Rust too), so I don't see an issue there either.

Dynamic approach means bigger runtime and less flexibility. With this PR you can provide whatever values you want (you might want to use Ethereum or any of the numerous forks), with "dynamic" approach we're limited to a few hardcoded options only.

Why are you saying that you can't use unstable features, is there a requirement for this somewhere?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well the issue is that our snowbridge runtimes/parachain in this repo are just for testing. Our pallets are eventually going to be included in the BridgeHub runtime in the cumulus repo: https://github.com/paritytech/cumulus/tree/master/parachains/runtimes/bridge-hubs

So we'd have to ask the Cumulus team if they are willing to support the generic_const_exprs feature. I can't find a clear policy anywhere, but I suspect that Parity-developed core parachains can't use unstable features, even if the runtimes are built using nightly.

I believe Substrate relies on rust nightly mainly because it supports wasm compilation, rather than because of needing other unstable features.

Copy link
Collaborator

@vgeddes vgeddes Mar 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

If pallet is made instantiable (I didn't implement it in this PR as it might be more controversial) then it would be possible to have this pallet included multiple times in the same runtime with different configurations, which wasn't possible before.

I don't have a strong objection against making the pallet instantiable. Just bear in mind that operating even a single instance of the pallet consumes a lot of block weight due to BLS signature verification. I wouldn't add more 2-3 instances in a runtime (and that is still dependent on BLS host functions being merged into Substrate).

Copy link
Author

Choose a reason for hiding this comment

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

Here is why they use nightly: paritytech/substrate#1252

TL;DR: It will soon become stable compatible: paritytech/substrate#13580

Still it seems to be useful to have ability to customize it and this is the only way. I don't see usage of nightly compiler as a big obstacle for Substrate ecosystem so far.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for those links, some that is news to me! I asked some Parity devs, and it was pointed out to me that paritytech/substrate#13580 pretty much answers this question. Using rust nightly is on the way out. And unstable features won't be accepted on the BridgeHub parachain, because it will add instability during compilation and possibly in production settings.

So I think we need to find another solution here for you.

Copy link
Author

Choose a reason for hiding this comment

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

Got it, will close for now then, but if you have specific steps you'd like to make I might be able to help. Thanks for review!


pub mod config;
mod merkleization;
Expand Down Expand Up @@ -102,6 +104,11 @@ pub mod pallet {
type ForkVersions: Get<ForkVersions>;
type WeightInfo: WeightInfo;
type WeakSubjectivityPeriodSeconds: Get<u64>;

const SLOTS_PER_EPOCH: u64;
const EPOCHS_PER_SYNC_COMMITTEE_PERIOD: u64;
const SYNC_COMMITTEE_SIZE: usize;
const BLOCK_ROOT_AT_INDEX_PROOF_DEPTH: u64;
}

#[pallet::event]
Expand Down Expand Up @@ -211,12 +218,15 @@ pub mod pallet {
}

#[pallet::genesis_build]
impl<T: Config> GenesisBuild<T> for GenesisConfig<T> {
impl<T: Config> GenesisBuild<T> for GenesisConfig<T>
where
[(); T::SYNC_COMMITTEE_SIZE]: Sized,
{
fn build(&self) {
log::info!(
target: "ethereum-beacon-client",
"💫 Sync committee size is: {}",
config::SYNC_COMMITTEE_SIZE
T::SYNC_COMMITTEE_SIZE
);

if let Some(initial_sync) = self.initial_sync.clone() {
Expand All @@ -226,7 +236,10 @@ pub mod pallet {
}

#[pallet::call]
impl<T: Config> Pallet<T> {
impl<T: Config> Pallet<T>
where
[(); T::SYNC_COMMITTEE_SIZE]: Sized,
{
#[pallet::call_index(0)]
#[pallet::weight(T::WeightInfo::sync_committee_period_update())]
#[transactional]
Expand Down Expand Up @@ -355,9 +368,12 @@ pub mod pallet {
}
}

impl<T: Config> Pallet<T> {
impl<T: Config> Pallet<T>
where
[(); T::SYNC_COMMITTEE_SIZE]: Sized,
{
fn process_initial_sync(initial_sync: InitialSyncOf<T>) -> DispatchResult {
Self::verify_sync_committee(
Self::verify_sync_committee::<{ T::SYNC_COMMITTEE_SIZE }>(
initial_sync.current_sync_committee.clone(),
initial_sync.current_sync_committee_branch,
initial_sync.header.state_root,
Expand Down Expand Up @@ -398,11 +414,12 @@ pub mod pallet {
update.attested_header.slot >= update.finalized_header.slot,
Error::<T>::InvalidSyncCommitteeHeaderUpdate
);
let sync_committee_bits =
get_sync_committee_bits(update.sync_aggregate.sync_committee_bits.clone())
.map_err(|_| Error::<T>::InvalidSyncCommitteeBits)?;
let sync_committee_bits = get_sync_committee_bits::<_, { T::SYNC_COMMITTEE_SIZE }>(
update.sync_aggregate.sync_committee_bits.clone(),
)
.map_err(|_| Error::<T>::InvalidSyncCommitteeBits)?;
Self::sync_committee_participation_is_supermajority(sync_committee_bits.clone())?;
Self::verify_sync_committee(
Self::verify_sync_committee::<{ T::SYNC_COMMITTEE_SIZE }>(
update.next_sync_committee.clone(),
update.next_sync_committee_branch,
update.attested_header.state_root,
Expand Down Expand Up @@ -501,9 +518,10 @@ pub mod pallet {
return Err(Error::<T>::BridgeBlocked.into())
}

let sync_committee_bits =
get_sync_committee_bits(update.sync_aggregate.sync_committee_bits.clone())
.map_err(|_| Error::<T>::InvalidSyncCommitteeBits)?;
let sync_committee_bits = get_sync_committee_bits::<_, { T::SYNC_COMMITTEE_SIZE }>(
update.sync_aggregate.sync_committee_bits.clone(),
)
.map_err(|_| Error::<T>::InvalidSyncCommitteeBits)?;
Self::sync_committee_participation_is_supermajority(sync_committee_bits.clone())?;

let block_root: H256 =
Expand Down Expand Up @@ -608,9 +626,10 @@ pub mod pallet {
let sync_committee = Self::get_sync_committee_for_period(current_period)?;

let validators_root = <ValidatorsRoot<T>>::get();
let sync_committee_bits =
get_sync_committee_bits(update.sync_aggregate.sync_committee_bits.clone())
.map_err(|_| Error::<T>::InvalidSyncCommitteeBits)?;
let sync_committee_bits = get_sync_committee_bits::<_, { T::SYNC_COMMITTEE_SIZE }>(
update.sync_aggregate.sync_committee_bits.clone(),
)
.map_err(|_| Error::<T>::InvalidSyncCommitteeBits)?;

Self::verify_signed_header(
sync_committee_bits,
Expand Down Expand Up @@ -706,14 +725,14 @@ pub mod pallet {

log::info!(
target: "ethereum-beacon-client",
"💫 Depth: {} leaf_index: {}", config::BLOCK_ROOT_AT_INDEX_PROOF_DEPTH, leaf_index
"💫 Depth: {} leaf_index: {}", T::BLOCK_ROOT_AT_INDEX_PROOF_DEPTH, leaf_index
);

ensure!(
Self::is_valid_merkle_branch(
beacon_block_root,
block_root_proof,
config::BLOCK_ROOT_AT_INDEX_PROOF_DEPTH,
T::BLOCK_ROOT_AT_INDEX_PROOF_DEPTH,
leaf_index,
finalized_block_root_hash
),
Expand Down Expand Up @@ -751,7 +770,7 @@ pub mod pallet {

let fork_version = Self::compute_fork_version(Self::compute_epoch_at_slot(
signature_slot,
config::SLOTS_PER_EPOCH,
T::SLOTS_PER_EPOCH,
));
let domain_type = config::DOMAIN_SYNC_COMMITTEE.to_vec();
// Domains are used for for seeds, for signatures, and for selecting aggregators.
Expand Down Expand Up @@ -831,15 +850,18 @@ pub mod pallet {
Ok(hash_root.into())
}

fn verify_sync_committee(
fn verify_sync_committee<const SYNC_COMMITTEE_SIZE: usize>(
sync_committee: SyncCommitteeOf<T>,
sync_committee_branch: BoundedVec<H256, T::MaxProofBranchSize>,
header_state_root: H256,
depth: u64,
index: u64,
) -> DispatchResult {
let sync_committee_root = merkleization::hash_tree_root_sync_committee(sync_committee)
.map_err(|_| Error::<T>::SyncCommitteeHashTreeRootFailed)?;
let sync_committee_root = merkleization::hash_tree_root_sync_committee::<
_,
SYNC_COMMITTEE_SIZE,
>(sync_committee)
.map_err(|_| Error::<T>::SyncCommitteeHashTreeRootFailed)?;

ensure!(
Self::is_valid_merkle_branch(
Expand Down Expand Up @@ -968,7 +990,7 @@ pub mod pallet {
}

pub(super) fn compute_current_sync_period(slot: u64) -> u64 {
slot / config::SLOTS_PER_EPOCH / config::EPOCHS_PER_SYNC_COMMITTEE_PERIOD
slot / T::SLOTS_PER_EPOCH / T::EPOCHS_PER_SYNC_COMMITTEE_PERIOD
}

/// Return the domain for the domain_type and fork_version.
Expand Down Expand Up @@ -1127,7 +1149,10 @@ pub mod pallet {
}
}

impl<T: Config> Verifier for Pallet<T> {
impl<T: Config> Verifier for Pallet<T>
where
[(); T::SYNC_COMMITTEE_SIZE]: Sized,
{
/// Verify a message by verifying the existence of the corresponding
/// Ethereum log in a block. Returns the log if successful.
fn verify(message: &Message) -> Result<(Log, u64), DispatchError> {
Expand Down
30 changes: 18 additions & 12 deletions parachain/pallets/ethereum-beacon-client/src/merkleization.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,18 +82,20 @@ impl TryFrom<BeaconHeader> for SSZBeaconBlockHeader {
}
}

impl<SyncCommitteeBitsSize: Get<u32>, SignatureSize: Get<u32>>
TryFrom<SyncAggregate<SyncCommitteeBitsSize, SignatureSize>> for SSZSyncAggregate
impl<
SyncCommitteeBitsSize: Get<u32>,
SignatureSize: Get<u32>,
const SYNC_COMMITTEE_SIZE: usize,
> TryFrom<SyncAggregate<SyncCommitteeBitsSize, SignatureSize>>
for SSZSyncAggregate<SYNC_COMMITTEE_SIZE>
{
type Error = MerkleizationError;

fn try_from(
sync_aggregate: SyncAggregate<SyncCommitteeBitsSize, SignatureSize>,
) -> Result<Self, Self::Error> {
Ok(SSZSyncAggregate {
sync_committee_bits: Bitvector::<{ config::SYNC_COMMITTEE_SIZE }>::deserialize(
&sync_aggregate.sync_committee_bits,
)?,
sync_committee_bits: Bitvector::deserialize(&sync_aggregate.sync_committee_bits)?,
sync_committee_signature: Vector::<u8, 96>::from_iter(
sync_aggregate.sync_committee_signature,
),
Expand Down Expand Up @@ -121,21 +123,22 @@ pub fn hash_tree_root_execution_header<
hash_tree_root(ssz_execution_payload)
}

pub fn hash_tree_root_sync_committee<S: Get<u32>>(
pub fn hash_tree_root_sync_committee<S: Get<u32>, const SYNC_COMMITTEE_SIZE: usize>(
sync_committee: SyncCommittee<S>,
) -> Result<[u8; 32], MerkleizationError> {
let mut pubkeys_vec = Vec::new();

for pubkey in sync_committee.pubkeys.iter() {
let conv_pubkey = Vector::<u8, 48>::from_iter(pubkey.0);
let conv_pubkey = Vector::<u8, { config::PUBKEY_SIZE }>::from_iter(pubkey.0);

pubkeys_vec.push(conv_pubkey);
}

let pubkeys =
Vector::<Vector<u8, 48>, { config::SYNC_COMMITTEE_SIZE }>::from_iter(pubkeys_vec.clone());
let pubkeys = Vector::<Vector<u8, { config::PUBKEY_SIZE }>, SYNC_COMMITTEE_SIZE>::from_iter(
pubkeys_vec.clone(),
);

let agg = Vector::<u8, 48>::from_iter(sync_committee.aggregate_pubkey.0);
let agg = Vector::<u8, { config::PUBKEY_SIZE }>::from_iter(sync_committee.aggregate_pubkey.0);

hash_tree_root(SSZSyncCommittee { pubkeys, aggregate_pubkey: agg })
}
Expand Down Expand Up @@ -168,10 +171,13 @@ pub fn hash_tree_root<T: SimpleSerializeTrait>(
}
}

pub fn get_sync_committee_bits<SyncCommitteeBitsSize: Get<u32>>(
pub fn get_sync_committee_bits<
SyncCommitteeBitsSize: Get<u32>,
const SYNC_COMMITTEE_SIZE: usize,
>(
bits_hex: BoundedVec<u8, SyncCommitteeBitsSize>,
) -> Result<Vec<u8>, MerkleizationError> {
let bitv = Bitvector::<{ config::SYNC_COMMITTEE_SIZE }>::deserialize(&bits_hex).map_err(
let bitv = Bitvector::<SYNC_COMMITTEE_SIZE>::deserialize(&bits_hex).map_err(
//|_| MerkleizationError::InvalidInput
|e| -> MerkleizationError {
match e {
Expand Down
29 changes: 21 additions & 8 deletions parachain/pallets/ethereum-beacon-client/src/mock.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ use std::{fs::File, path::PathBuf};

pub mod mock_minimal {
use super::*;

use crate::config::minimal;
type UncheckedExtrinsic = frame_system::mocking::MockUncheckedExtrinsic<Test>;
type Block = frame_system::mocking::MockBlock<Test>;

Expand Down Expand Up @@ -69,7 +69,7 @@ pub mod mock_minimal {
}

parameter_types! {
pub const MaxSyncCommitteeSize: u32 = config::SYNC_COMMITTEE_SIZE as u32;
pub const MaxSyncCommitteeSize: u32 = minimal::SYNC_COMMITTEE_SIZE as u32;
pub const MaxProofBranchSize: u32 = 20;
pub const MaxExtraDataSize: u32 = config::MAX_EXTRA_DATA_BYTES as u32;
pub const MaxLogsBloomSize: u32 = config::MAX_LOGS_BLOOM_SIZE as u32;
Expand Down Expand Up @@ -110,11 +110,17 @@ pub mod mock_minimal {
type ForkVersions = ChainForkVersions;
type WeakSubjectivityPeriodSeconds = WeakSubjectivityPeriodSeconds;
type WeightInfo = ();

const SLOTS_PER_EPOCH: u64 = minimal::SLOTS_PER_EPOCH;
const EPOCHS_PER_SYNC_COMMITTEE_PERIOD: u64 = minimal::EPOCHS_PER_SYNC_COMMITTEE_PERIOD;
const SYNC_COMMITTEE_SIZE: usize = minimal::SYNC_COMMITTEE_SIZE;
const BLOCK_ROOT_AT_INDEX_PROOF_DEPTH: u64 = minimal::BLOCK_ROOT_AT_INDEX_PROOF_DEPTH;
}
}

pub mod mock_mainnet {
use super::*;
use crate::config::mainnet;

type UncheckedExtrinsic = frame_system::mocking::MockUncheckedExtrinsic<Test>;
type Block = frame_system::mocking::MockBlock<Test>;
Expand Down Expand Up @@ -171,7 +177,7 @@ pub mod mock_mainnet {
}

parameter_types! {
pub const MaxSyncCommitteeSize: u32 = config::SYNC_COMMITTEE_SIZE as u32;
pub const MaxSyncCommitteeSize: u32 = mainnet::SYNC_COMMITTEE_SIZE as u32;
pub const MaxProofBranchSize: u32 = 20;
pub const MaxExtraDataSize: u32 = config::MAX_EXTRA_DATA_BYTES as u32;
pub const MaxLogsBloomSize: u32 = config::MAX_LOGS_BLOOM_SIZE as u32;
Expand Down Expand Up @@ -212,6 +218,11 @@ pub mod mock_mainnet {
type ForkVersions = ChainForkVersions;
type WeakSubjectivityPeriodSeconds = WeakSubjectivityPeriodSeconds;
type WeightInfo = ();

const SLOTS_PER_EPOCH: u64 = mainnet::SLOTS_PER_EPOCH;
const EPOCHS_PER_SYNC_COMMITTEE_PERIOD: u64 = mainnet::EPOCHS_PER_SYNC_COMMITTEE_PERIOD;
const SYNC_COMMITTEE_SIZE: usize = mainnet::SYNC_COMMITTEE_SIZE;
const BLOCK_ROOT_AT_INDEX_PROOF_DEPTH: u64 = mainnet::BLOCK_ROOT_AT_INDEX_PROOF_DEPTH;
}
}

Expand Down Expand Up @@ -276,11 +287,13 @@ fn header_update_from_file<T: crate::Config>(
serde_json::from_reader(File::open(&filepath).unwrap()).unwrap()
}

fn get_config_setting() -> String {
return match config::IS_MINIMAL {
true => "minimal".to_owned(),
false => "mainnet".to_owned(),
}
#[cfg(feature = "minimal")]
fn get_config_setting() -> &'static str {
"minimal"
}
#[cfg(not(feature = "minimal"))]
fn get_config_setting() -> &'static str {
"mainnet"
}

fn add_file_prefix(name: &str) -> String {
Expand Down
8 changes: 4 additions & 4 deletions parachain/pallets/ethereum-beacon-client/src/ssz.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,14 @@ pub struct SSZBeaconBlockHeader {
}

#[derive(Default, SimpleSerialize)]
pub struct SSZSyncCommittee {
pub pubkeys: Vector<Vector<u8, { config::PUBKEY_SIZE }>, { config::SYNC_COMMITTEE_SIZE }>,
pub struct SSZSyncCommittee<const SYNC_COMMITTEE_SIZE: usize> {
pub pubkeys: Vector<Vector<u8, { config::PUBKEY_SIZE }>, SYNC_COMMITTEE_SIZE>,
pub aggregate_pubkey: Vector<u8, { config::PUBKEY_SIZE }>,
}

#[derive(Default, Debug, SimpleSerialize, Clone)]
pub struct SSZSyncAggregate {
pub sync_committee_bits: Bitvector<{ config::SYNC_COMMITTEE_SIZE }>,
pub struct SSZSyncAggregate<const SYNC_COMMITTEE_SIZE: usize> {
pub sync_committee_bits: Bitvector<SYNC_COMMITTEE_SIZE>,
pub sync_committee_signature: Vector<u8, { config::SIGNATURE_SIZE }>,
}

Expand Down
Loading