From 5df1c08ca6eed7a0534942f6ba96a464d5c34dfd Mon Sep 17 00:00:00 2001 From: Jun Kimura Date: Mon, 23 Dec 2024 18:57:38 +0900 Subject: [PATCH 1/5] EELC2: improve validation of `Fraction` Signed-off-by: Jun Kimura --- crates/light-client-cli/src/client.rs | 3 +- crates/light-client-verifier/src/consensus.rs | 18 +++++------ crates/light-client-verifier/src/context.rs | 30 +++++++++++++++---- crates/light-client-verifier/src/errors.rs | 4 ++- 4 files changed, 38 insertions(+), 17 deletions(-) diff --git a/crates/light-client-cli/src/client.rs b/crates/light-client-cli/src/client.rs index e7aaae0..c612d9d 100644 --- a/crates/light-client-cli/src/client.rs +++ b/crates/light-client-cli/src/client.rs @@ -70,7 +70,8 @@ impl< verifier: Default::default(), genesis_time, genesis_validators_root, - trust_level: trust_level.unwrap_or(Fraction::new(2, 3)), + // safe to unwrap: `2/3` is valid fraction + trust_level: trust_level.unwrap_or(Fraction::new(2, 3).unwrap()), } } diff --git a/crates/light-client-verifier/src/consensus.rs b/crates/light-client-verifier/src/consensus.rs index e32508a..f713f6e 100644 --- a/crates/light-client-verifier/src/consensus.rs +++ b/crates/light-client-verifier/src/consensus.rs @@ -194,9 +194,9 @@ pub fn verify_sync_committee_attestation< participants, ctx.min_sync_committee_participants(), )); - } else if participants as u64 * ctx.signature_threshold().denominator + } else if participants as u64 * ctx.signature_threshold().denominator() < consensus_update.sync_aggregate().sync_committee_bits.len() as u64 - * ctx.signature_threshold().numerator + * ctx.signature_threshold().numerator() { return Err(Error::InsufficientParticipants( participants as u64, @@ -881,7 +881,7 @@ mod tests { genesis_validators_root, // NOTE: this is workaround. we must get the correct timestamp from beacon state. minimal::get_config().min_genesis_time, - Fraction::new(2, 3), + Fraction::new(2, 3).unwrap(), 1729846322.into(), ); assert!(verifier.validate_boostrap(&ctx, &bootstrap, None).is_ok()); @@ -928,7 +928,7 @@ mod tests { genesis_validators_root, // NOTE: this is workaround. we must get the correct timestamp from beacon state. minimal::get_config().min_genesis_time, - Fraction::new(2, 3), + Fraction::new(2, 3).unwrap(), 1729846322.into(), ); assert!(verifier.validate_boostrap(&ctx, &bootstrap, None).is_ok()); @@ -1039,7 +1039,7 @@ mod tests { config::minimal::get_config(), Default::default(), Default::default(), - Fraction::new(2, 3), + Fraction::new(2, 3).unwrap(), SystemTime::now() .duration_since(SystemTime::UNIX_EPOCH) .unwrap() @@ -1458,7 +1458,7 @@ mod tests { config::minimal::get_config(), Default::default(), Default::default(), - Fraction::new(2, 3), + Fraction::new(2, 3).unwrap(), SystemTime::now() .duration_since(SystemTime::UNIX_EPOCH) .unwrap() @@ -1779,7 +1779,7 @@ mod tests { .as_ref(), ), 1731420304.into(), - Fraction::new(2, 3), + Fraction::new(2, 3).unwrap(), SystemTime::now() .duration_since(SystemTime::UNIX_EPOCH) .unwrap() @@ -1806,7 +1806,7 @@ mod tests { .as_ref(), ), 1731420304.into(), - Fraction::new(2, 3), + Fraction::new(2, 3).unwrap(), SystemTime::now() .duration_since(SystemTime::UNIX_EPOCH) .unwrap() @@ -1971,7 +1971,7 @@ mod tests { .as_ref(), ), 1731420304.into(), - Fraction::new(2, 3), + Fraction::new(2, 3).unwrap(), SystemTime::now() .duration_since(SystemTime::UNIX_EPOCH) .unwrap() diff --git a/crates/light-client-verifier/src/context.rs b/crates/light-client-verifier/src/context.rs index 97126b0..907b05d 100644 --- a/crates/light-client-verifier/src/context.rs +++ b/crates/light-client-verifier/src/context.rs @@ -1,3 +1,4 @@ +use crate::errors::Error; use ethereum_consensus::{ beacon::{Epoch, Root, Slot}, compute::{compute_epoch_at_slot, compute_slot_at_timestamp}, @@ -6,19 +7,36 @@ use ethereum_consensus::{ fork::{ForkParameters, ForkSpec}, types::U64, }; + +/// Fraction is a struct representing a fraction for a threshold. #[derive(Clone, Default, Debug, PartialEq, Eq, serde::Serialize, serde::Deserialize)] pub struct Fraction { - pub numerator: u64, - pub denominator: u64, + numerator: u64, + denominator: u64, } impl Fraction { - pub fn new(numerator: u64, denominator: u64) -> Self { - Self { - numerator, - denominator, + pub fn new(numerator: u64, denominator: u64) -> Result { + if denominator == 0 || numerator > denominator { + Err(Error::InvalidFraction(Self { + numerator, + denominator, + })) + } else { + Ok(Self { + numerator, + denominator, + }) } } + + pub fn numerator(&self) -> u64 { + self.numerator + } + + pub fn denominator(&self) -> u64 { + self.denominator + } } pub trait ConsensusVerificationContext { diff --git a/crates/light-client-verifier/src/errors.rs b/crates/light-client-verifier/src/errors.rs index 7fa9a6e..fb6430e 100644 --- a/crates/light-client-verifier/src/errors.rs +++ b/crates/light-client-verifier/src/errors.rs @@ -1,4 +1,4 @@ -use crate::internal_prelude::*; +use crate::{context::Fraction, internal_prelude::*}; use displaydoc::Display; use ethereum_consensus::{ beacon::{BeaconBlockHeader, Epoch, Root, Slot}, @@ -91,6 +91,8 @@ pub enum Error { InvalidExecutionBlockNumberMerkleBranch(MerkleError), /// inconsistent next sync committee: `store:{0:?}` != `update:{1:?}` InconsistentNextSyncCommittee(PublicKey, PublicKey), + /// invalid fraction: `fraction={0:?}` + InvalidFraction(Fraction), /// other error: `{description}` Other { description: String }, } From c6a6216246aacce3348bfed1cd691020edb0c958 Mon Sep 17 00:00:00 2001 From: Jun Kimura Date: Mon, 23 Dec 2024 19:30:31 +0900 Subject: [PATCH 2/5] ci: fix rust version Signed-off-by: Jun Kimura --- .github/workflows/test.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index c736db9..cca34da 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -22,6 +22,8 @@ jobs: steps: - uses: actions/checkout@v4 - uses: actions-rust-lang/setup-rust-toolchain@v1 + with: + toolchain: 1.82.0 - uses: Swatinem/rust-cache@v2 - run: cargo test - run: cargo build From a9202d187f6422f73c9c36ec6ce39fd80a419d1a Mon Sep 17 00:00:00 2001 From: Jun Kimura Date: Mon, 23 Dec 2024 20:35:57 +0900 Subject: [PATCH 3/5] S-1: fix typo Signed-off-by: Jun Kimura --- crates/consensus/src/bls.rs | 8 ++++---- crates/consensus/src/errors.rs | 2 +- crates/consensus/src/sync_protocol.rs | 4 ++-- crates/light-client-verifier/src/consensus.rs | 8 ++++---- crates/light-client-verifier/src/updates.rs | 2 +- 5 files changed, 12 insertions(+), 12 deletions(-) diff --git a/crates/consensus/src/bls.rs b/crates/consensus/src/bls.rs index a6106ca..5cbd2a9 100644 --- a/crates/consensus/src/bls.rs +++ b/crates/consensus/src/bls.rs @@ -195,7 +195,7 @@ impl TryFrom for BLSSignature { } } -pub fn aggreate_public_key(keys: &[BLSPublicKey]) -> Result { +pub fn aggregate_public_key(keys: &[BLSPublicKey]) -> Result { Ok(BLSAggregatePublicKey::into_aggregate(keys)?) } @@ -204,13 +204,13 @@ pub fn fast_aggregate_verify( msg: H256, signature: BLSSignature, ) -> Result { - let aggregate_pubkey = aggreate_public_key(&pubkeys)?; + let aggregate_pubkey = aggregate_public_key(&pubkeys)?; let aggregate_signature = BLSAggregateSignature::from_signature(&signature); Ok(aggregate_signature.fast_aggregate_verify_pre_aggregated(msg.as_bytes(), &aggregate_pubkey)) } -pub fn is_equal_pubkeys_and_aggreate_pub_key( +pub fn is_equal_pubkeys_and_aggregate_pub_key( pubkeys: &Vector, aggregate_pubkey: &PublicKey, ) -> Result<(), Error> { @@ -218,7 +218,7 @@ pub fn is_equal_pubkeys_and_aggreate_pub_key( .iter() .map(|k| k.clone().try_into().unwrap()) .collect(); - let agg_pubkey: PublicKey = aggreate_public_key(&pubkeys)?.into(); + let agg_pubkey: PublicKey = aggregate_public_key(&pubkeys)?.into(); if aggregate_pubkey == &agg_pubkey { Ok(()) } else { diff --git a/crates/consensus/src/errors.rs b/crates/consensus/src/errors.rs index 5772ecd..d1281a0 100644 --- a/crates/consensus/src/errors.rs +++ b/crates/consensus/src/errors.rs @@ -21,7 +21,7 @@ pub enum Error { InvalidBLSSignatureLenght(usize, usize), /// invalid bls public key length: `expected={0} actual={1}` InvalidBLSPublicKeyLength(usize, usize), - /// bls aggreate public key mismatch: `{0:?} != {1:?}` + /// bls aggregate public key mismatch: `{0:?} != {1:?}` BLSAggregatePublicKeyMismatch(PublicKey, PublicKey), /// invalid address length: `expected={0} actual={1}` InvalidAddressLength(usize, usize), diff --git a/crates/consensus/src/sync_protocol.rs b/crates/consensus/src/sync_protocol.rs index eec4a20..aa99c47 100644 --- a/crates/consensus/src/sync_protocol.rs +++ b/crates/consensus/src/sync_protocol.rs @@ -1,5 +1,5 @@ use crate::{ - bls::{is_equal_pubkeys_and_aggreate_pub_key, PublicKey, Signature}, + bls::{is_equal_pubkeys_and_aggregate_pub_key, PublicKey, Signature}, errors::Error, internal_prelude::*, types::U64, @@ -21,7 +21,7 @@ pub struct SyncCommittee { impl SyncCommittee { pub fn validate(&self) -> Result<(), Error> { - is_equal_pubkeys_and_aggreate_pub_key(&self.pubkeys, &self.aggregate_pubkey) + is_equal_pubkeys_and_aggregate_pub_key(&self.pubkeys, &self.aggregate_pubkey) } } diff --git a/crates/light-client-verifier/src/consensus.rs b/crates/light-client-verifier/src/consensus.rs index f713f6e..74b3170 100644 --- a/crates/light-client-verifier/src/consensus.rs +++ b/crates/light-client-verifier/src/consensus.rs @@ -387,7 +387,7 @@ pub mod test_utils { use ethereum_consensus::ssz_rs::Vector; use ethereum_consensus::{ beacon::{BlockNumber, Checkpoint, Epoch, Slot}, - bls::{aggreate_public_key, PublicKey, Signature}, + bls::{aggregate_public_key, PublicKey, Signature}, fork::deneb, merkle::MerkleTree, preset::minimal::DenebBeaconBlock, @@ -441,7 +441,7 @@ pub mod test_utils { for v in self.committee.iter() { pubkeys.push(v.public_key()); } - let aggregate_pubkey = aggreate_public_key(&pubkeys.to_vec()).unwrap(); + let aggregate_pubkey = aggregate_public_key(&pubkeys.to_vec()).unwrap(); SyncCommittee { pubkeys: Vector::from_iter(pubkeys.into_iter().map(PublicKey::from)), aggregate_pubkey: PublicKey::from(aggregate_pubkey), @@ -855,7 +855,7 @@ mod tests { }; use ethereum_consensus::{ beacon::Version, - bls::aggreate_public_key, + bls::aggregate_public_key, config::{minimal, Config}, fork::{ altair::ALTAIR_FORK_SPEC, bellatrix::BELLATRIX_FORK_SPEC, ForkParameter, @@ -897,7 +897,7 @@ mod tests { .iter() .map(|k| k.clone().try_into().unwrap()) .collect(); - let aggregated_key = aggreate_public_key(&pubkeys).unwrap(); + let aggregated_key = aggregate_public_key(&pubkeys).unwrap(); let pubkey = BLSPublicKey { point: aggregated_key.point, }; diff --git a/crates/light-client-verifier/src/updates.rs b/crates/light-client-verifier/src/updates.rs index 8fa7e2f..c0da19e 100644 --- a/crates/light-client-verifier/src/updates.rs +++ b/crates/light-client-verifier/src/updates.rs @@ -77,7 +77,7 @@ pub trait ConsensusUpdate: /// validate the basic properties of the update fn validate_basic(&self, ctx: &C) -> Result<(), Error> { - // ensure that sync committee's aggreated key matches pubkeys + // ensure that sync committee's aggregated key matches pubkeys if let Some(next_sync_committee) = self.next_sync_committee() { next_sync_committee.validate()?; } From a9e03c9d32a4b4280f6a0349bbca93aeac57a889 Mon Sep 17 00:00:00 2001 From: Jun Kimura Date: Tue, 24 Dec 2024 10:06:39 +0900 Subject: [PATCH 4/5] fix lint error since rust 1.83 Signed-off-by: Jun Kimura --- .github/workflows/test.yml | 2 -- crates/consensus/src/types.rs | 2 +- crates/light-client-cli/src/context.rs | 1 - 3 files changed, 1 insertion(+), 4 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index cca34da..c736db9 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -22,8 +22,6 @@ jobs: steps: - uses: actions/checkout@v4 - uses: actions-rust-lang/setup-rust-toolchain@v1 - with: - toolchain: 1.82.0 - uses: Swatinem/rust-cache@v2 - run: cargo test - run: cargo build diff --git a/crates/consensus/src/types.rs b/crates/consensus/src/types.rs index 062ffdf..170016c 100644 --- a/crates/consensus/src/types.rs +++ b/crates/consensus/src/types.rs @@ -148,7 +148,7 @@ impl<'de> serde::Deserialize<'de> for U64 { { struct MyVisitor; - impl<'de> serde::de::Visitor<'de> for MyVisitor { + impl serde::de::Visitor<'_> for MyVisitor { type Value = U64; fn expecting(&self, fmt: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { diff --git a/crates/light-client-cli/src/context.rs b/crates/light-client-cli/src/context.rs index 7ef892c..6d0bb1c 100644 --- a/crates/light-client-cli/src/context.rs +++ b/crates/light-client-cli/src/context.rs @@ -49,7 +49,6 @@ impl< } /// Store accessors - pub fn get_bootstrap( &self, ) -> Result< From b9ce959c18703429610ff51f6287a176a2059b0a Mon Sep 17 00:00:00 2001 From: Jun Kimura Date: Tue, 24 Dec 2024 11:27:04 +0900 Subject: [PATCH 5/5] improve url validation Signed-off-by: Jun Kimura --- crates/light-client-cli/src/context.rs | 2 +- crates/lodestar-rpc/src/client.rs | 24 ++++++++++++++++++++---- 2 files changed, 21 insertions(+), 5 deletions(-) diff --git a/crates/light-client-cli/src/context.rs b/crates/light-client-cli/src/context.rs index 6d0bb1c..ac539f1 100644 --- a/crates/light-client-cli/src/context.rs +++ b/crates/light-client-cli/src/context.rs @@ -29,8 +29,8 @@ impl< pub fn build(network: Network, opts: Opts) -> Result { let home_dir = opts.home_dir(); if !home_dir.exists() { - info!("directory {:?} is created", home_dir); std::fs::create_dir(&home_dir)?; + info!("directory {:?} is created", home_dir); } Ok(Self { config: network.config(), diff --git a/crates/lodestar-rpc/src/client.rs b/crates/lodestar-rpc/src/client.rs index db1ae8f..7fbab2a 100644 --- a/crates/lodestar-rpc/src/client.rs +++ b/crates/lodestar-rpc/src/client.rs @@ -8,7 +8,7 @@ use ethereum_consensus::beacon::Slot; use ethereum_consensus::sync_protocol::SyncCommitteePeriod; use ethereum_consensus::types::H256; use log::debug; -use reqwest::{Client, StatusCode}; +use reqwest::{Client, StatusCode, Url}; use serde::de::DeserializeOwned; type Result = core::result::Result; @@ -20,9 +20,25 @@ pub struct RPCClient { impl RPCClient { pub fn new(endpoint: impl Into) -> Self { + let url = Url::parse(&endpoint.into()).expect("Invalid URL"); + if url.scheme() != "http" && url.scheme() != "https" { + panic!("Invalid URL scheme: {}", url.scheme()); + } + if url.path() != "/" { + panic!("Invalid URL path: {}", url.path()); + } + if url.host().is_none() { + panic!("Invalid URL host: {}", url.host().unwrap()); + } + if url.query().is_some() { + panic!("Invalid URL query: {}", url.query().unwrap()); + } + if url.fragment().is_some() { + panic!("Invalid URL fragment: {}", url.fragment().unwrap()); + } Self { http_client: reqwest::Client::new(), - endpoint: endpoint.into(), + endpoint: url.as_str().strip_suffix("/").unwrap().to_string(), } } @@ -168,8 +184,8 @@ impl RPCClient { #[derive(serde::Serialize, serde::Deserialize)] struct InternalServerError { - #[serde(rename = "statusCode")] + #[serde(alias = "statusCode", alias = "code")] status_code: u64, - error: String, + error: Option, message: String, }