From 733b1dff01c53c3a93b50854db0b1b10b34fea70 Mon Sep 17 00:00:00 2001 From: Jimmy Chen Date: Wed, 26 Jun 2024 05:41:30 +1000 Subject: [PATCH] Update `csc` format in ENR and spec tests for devnet-1 (#5966) * Update `csc` format in ENR. * Add spec tests for `recover_cells_and_kzg_proofs`. * Add tests for ENR. * Fix failing tests. * Add protection against invalid csc value in ENR. * Fix lint --- beacon_node/lighthouse_network/src/config.rs | 2 +- .../lighthouse_network/src/discovery/enr.rs | 79 +++++++++++++-- .../src/peer_manager/peerdb.rs | 6 +- consensus/types/src/data_column_subnet_id.rs | 12 +-- testing/ef_tests/Makefile | 2 +- testing/ef_tests/check_all_files_accessed.py | 6 +- testing/ef_tests/src/cases.rs | 2 + .../cases/kzg_recover_cells_and_kzg_proofs.rs | 95 +++++++++++++++++++ testing/ef_tests/src/handler.rs | 20 ++++ testing/ef_tests/tests/tests.rs | 6 ++ 10 files changed, 211 insertions(+), 19 deletions(-) create mode 100644 testing/ef_tests/src/cases/kzg_recover_cells_and_kzg_proofs.rs diff --git a/beacon_node/lighthouse_network/src/config.rs b/beacon_node/lighthouse_network/src/config.rs index 30f35064586..7c95977140e 100644 --- a/beacon_node/lighthouse_network/src/config.rs +++ b/beacon_node/lighthouse_network/src/config.rs @@ -42,7 +42,7 @@ pub struct Config { pub network_dir: PathBuf, /// IP addresses to listen on. - listen_addresses: ListenAddress, + pub(crate) listen_addresses: ListenAddress, /// The address to broadcast to peers about which address we are listening on. None indicates /// that no discovery address has been set in the CLI args. diff --git a/beacon_node/lighthouse_network/src/discovery/enr.rs b/beacon_node/lighthouse_network/src/discovery/enr.rs index 67bfe0fd809..fc8d3809e2d 100644 --- a/beacon_node/lighthouse_network/src/discovery/enr.rs +++ b/beacon_node/lighthouse_network/src/discovery/enr.rs @@ -25,7 +25,7 @@ pub const ATTESTATION_BITFIELD_ENR_KEY: &str = "attnets"; /// The ENR field specifying the sync committee subnet bitfield. pub const SYNC_COMMITTEE_BITFIELD_ENR_KEY: &str = "syncnets"; /// The ENR field specifying the peerdas custody subnet count. -pub const PEERDAS_CUSTODY_SUBNET_COUNT_ENR_KEY: &str = "custody_subnet_count"; +pub const PEERDAS_CUSTODY_SUBNET_COUNT_ENR_KEY: &str = "csc"; /// Extension trait for ENR's within Eth2. pub trait Eth2Enr { @@ -68,7 +68,9 @@ impl Eth2Enr for Enr { /// defined in the spec. fn custody_subnet_count(&self, spec: &ChainSpec) -> u64 { self.get(PEERDAS_CUSTODY_SUBNET_COUNT_ENR_KEY) - .and_then(|custody_bytes| u64::from_ssz_bytes(custody_bytes).ok()) + .and_then(|custody_bytes| custody_bytes.try_into().map(u64::from_be_bytes).ok()) + // If value supplied in ENR is invalid, fallback to `custody_requirement` + .filter(|csc| csc <= &spec.data_column_sidecar_subnet_count) .unwrap_or(spec.custody_requirement) } @@ -243,10 +245,8 @@ pub fn build_enr( spec.custody_requirement }; - builder.add_value( - PEERDAS_CUSTODY_SUBNET_COUNT_ENR_KEY, - &custody_subnet_count.as_ssz_bytes(), - ); + let csc_bytes = custody_subnet_count.to_be_bytes(); + builder.add_value(PEERDAS_CUSTODY_SUBNET_COUNT_ENR_KEY, &csc_bytes.as_slice()); builder .build(enr_key) @@ -309,3 +309,70 @@ pub fn save_enr_to_disk(dir: &Path, enr: &Enr, log: &slog::Logger) { } } } + +#[cfg(test)] +mod test { + use super::*; + use crate::config::Config as NetworkConfig; + use types::MainnetEthSpec; + + type E = MainnetEthSpec; + + #[test] + fn custody_subnet_count_default() { + let config = NetworkConfig { + subscribe_all_data_column_subnets: false, + ..NetworkConfig::default() + }; + let spec = E::default_spec(); + let enr = build_enr_with_config(config, &spec).0; + + assert_eq!( + enr.custody_subnet_count::(&spec), + spec.custody_requirement, + ); + } + + #[test] + fn custody_subnet_count_all() { + let config = NetworkConfig { + subscribe_all_data_column_subnets: true, + ..NetworkConfig::default() + }; + let spec = E::default_spec(); + let enr = build_enr_with_config(config, &spec).0; + + assert_eq!( + enr.custody_subnet_count::(&spec), + spec.data_column_sidecar_subnet_count, + ); + } + + #[test] + fn custody_subnet_count_fallback_default() { + let config = NetworkConfig::default(); + let spec = E::default_spec(); + let (mut enr, enr_key) = build_enr_with_config(config, &spec); + let invalid_subnet_count = 999u64; + + enr.insert( + PEERDAS_CUSTODY_SUBNET_COUNT_ENR_KEY, + &invalid_subnet_count.to_be_bytes().as_slice(), + &enr_key, + ) + .unwrap(); + + assert_eq!( + enr.custody_subnet_count::(&spec), + spec.custody_requirement, + ); + } + + fn build_enr_with_config(config: NetworkConfig, spec: &ChainSpec) -> (Enr, CombinedKey) { + let keypair = libp2p::identity::secp256k1::Keypair::generate(); + let enr_key = CombinedKey::from_secp256k1(&keypair); + let enr_fork_id = EnrForkId::default(); + let enr = build_enr::(&enr_key, &config, &enr_fork_id, spec).unwrap(); + (enr, enr_key) + } +} diff --git a/beacon_node/lighthouse_network/src/peer_manager/peerdb.rs b/beacon_node/lighthouse_network/src/peer_manager/peerdb.rs index ec9b011d365..66ba0980392 100644 --- a/beacon_node/lighthouse_network/src/peer_manager/peerdb.rs +++ b/beacon_node/lighthouse_network/src/peer_manager/peerdb.rs @@ -5,7 +5,6 @@ use peer_info::{ConnectionDirection, PeerConnectionStatus, PeerInfo}; use rand::seq::SliceRandom; use score::{PeerAction, ReportSource, Score, ScoreState}; use slog::{crit, debug, error, trace, warn}; -use ssz::Encode; use std::net::IpAddr; use std::time::Instant; use std::{cmp::Ordering, fmt::Display}; @@ -687,7 +686,10 @@ impl PeerDB { if supernode { enr.insert( PEERDAS_CUSTODY_SUBNET_COUNT_ENR_KEY, - &spec.data_column_sidecar_subnet_count.as_ssz_bytes(), + &spec + .data_column_sidecar_subnet_count + .to_be_bytes() + .as_slice(), &enr_key, ) .expect("u64 can be encoded"); diff --git a/consensus/types/src/data_column_subnet_id.rs b/consensus/types/src/data_column_subnet_id.rs index 34a0302cc79..403216977df 100644 --- a/consensus/types/src/data_column_subnet_id.rs +++ b/consensus/types/src/data_column_subnet_id.rs @@ -107,15 +107,15 @@ impl From for DataColumnSubnetId { } } -impl Into for DataColumnSubnetId { - fn into(self) -> u64 { - self.0 +impl From for u64 { + fn from(val: DataColumnSubnetId) -> Self { + val.0 } } -impl Into for &DataColumnSubnetId { - fn into(self) -> u64 { - self.0 +impl From<&DataColumnSubnetId> for u64 { + fn from(val: &DataColumnSubnetId) -> Self { + val.0 } } diff --git a/testing/ef_tests/Makefile b/testing/ef_tests/Makefile index 5dc3d2a0404..59e08e5d259 100644 --- a/testing/ef_tests/Makefile +++ b/testing/ef_tests/Makefile @@ -1,4 +1,4 @@ -TESTS_TAG := v1.5.0-alpha.2 +TESTS_TAG := v1.5.0-alpha.3 TESTS = general minimal mainnet TARBALLS = $(patsubst %,%-$(TESTS_TAG).tar.gz,$(TESTS)) diff --git a/testing/ef_tests/check_all_files_accessed.py b/testing/ef_tests/check_all_files_accessed.py index 67cdbe9b969..1a279771979 100755 --- a/testing/ef_tests/check_all_files_accessed.py +++ b/testing/ef_tests/check_all_files_accessed.py @@ -35,12 +35,12 @@ "tests/.*/.*/ssz_static/LightClientStore", # LightClientSnapshot "tests/.*/.*/ssz_static/LightClientSnapshot", + # Unused container for das + "tests/.*/.*/ssz_static/MatrixEntry", # Unused kzg methods - "tests/.*/.*/kzg/compute_cells", - "tests/.*/.*/kzg/recover_all_cells", "tests/.*/.*/kzg/verify_cell_kzg_proof", # One of the EF researchers likes to pack the tarballs on a Mac - ".*\.DS_Store.*", + ".*/.DS_Store.*", # More Mac weirdness. "tests/mainnet/bellatrix/operations/deposit/pyspec_tests/deposit_with_previous_fork_version__valid_ineffective/._meta.yaml", "tests/mainnet/eip7594/networking/get_custody_columns/pyspec_tests/get_custody_columns__short_node_id/._meta.yaml", diff --git a/testing/ef_tests/src/cases.rs b/testing/ef_tests/src/cases.rs index 2137452d46d..3a9fc023624 100644 --- a/testing/ef_tests/src/cases.rs +++ b/testing/ef_tests/src/cases.rs @@ -23,6 +23,7 @@ mod kzg_blob_to_kzg_commitment; mod kzg_compute_blob_kzg_proof; mod kzg_compute_cells_and_kzg_proofs; mod kzg_compute_kzg_proof; +mod kzg_recover_cells_and_kzg_proofs; mod kzg_verify_blob_kzg_proof; mod kzg_verify_blob_kzg_proof_batch; mod kzg_verify_cell_kzg_proof_batch; @@ -56,6 +57,7 @@ pub use kzg_blob_to_kzg_commitment::*; pub use kzg_compute_blob_kzg_proof::*; pub use kzg_compute_cells_and_kzg_proofs::*; pub use kzg_compute_kzg_proof::*; +pub use kzg_recover_cells_and_kzg_proofs::*; pub use kzg_verify_blob_kzg_proof::*; pub use kzg_verify_blob_kzg_proof_batch::*; pub use kzg_verify_cell_kzg_proof_batch::*; diff --git a/testing/ef_tests/src/cases/kzg_recover_cells_and_kzg_proofs.rs b/testing/ef_tests/src/cases/kzg_recover_cells_and_kzg_proofs.rs new file mode 100644 index 00000000000..c177094d0b1 --- /dev/null +++ b/testing/ef_tests/src/cases/kzg_recover_cells_and_kzg_proofs.rs @@ -0,0 +1,95 @@ +use super::*; +use crate::case_result::compare_result; +use kzg::{CellsAndKzgProofs, KzgProof}; +use serde::Deserialize; +use std::convert::Infallible; +use std::marker::PhantomData; + +#[derive(Debug, Clone, Deserialize)] +#[serde(deny_unknown_fields)] +pub struct KZGRecoverCellsAndKzgProofsInput { + pub cell_indices: Vec, + pub cells: Vec, + pub proofs: Vec, +} + +#[derive(Debug, Clone, Deserialize)] +#[serde(bound = "E: EthSpec", deny_unknown_fields)] +pub struct KZGRecoverCellsAndKZGProofs { + pub input: KZGRecoverCellsAndKzgProofsInput, + pub output: Option<(Vec, Vec)>, + #[serde(skip)] + _phantom: PhantomData, +} + +impl LoadCase for KZGRecoverCellsAndKZGProofs { + fn load_from_dir(path: &Path, _fork_name: ForkName) -> Result { + decode::yaml_decode_file(path.join("data.yaml").as_path()) + } +} + +impl Case for KZGRecoverCellsAndKZGProofs { + fn is_enabled_for_fork(fork_name: ForkName) -> bool { + fork_name == ForkName::Deneb + } + + fn result(&self, _case_index: usize, _fork_name: ForkName) -> Result<(), Error> { + let parse_input = |input: &KZGRecoverCellsAndKzgProofsInput| { + // Proofs are not used for `recover_cells_and_compute_kzg_proofs`, they are only checked + // to satisfy the spec tests. + if input.proofs.len() != input.cell_indices.len() { + return Err(Error::SkippedKnownFailure); + } + + let proofs = input + .proofs + .iter() + .map(|s| parse_proof(s)) + .collect::, Error>>()?; + + let cells = input + .cells + .iter() + .map(|s| parse_cell(s)) + .collect::, Error>>()?; + + Ok((proofs, cells, input.cell_indices.clone())) + }; + + let result = + parse_input(&self.input).and_then(|(input_proofs, input_cells, cell_indices)| { + let (cells, proofs) = KZG + .recover_cells_and_compute_kzg_proofs( + cell_indices.as_slice(), + input_cells.as_slice(), + ) + .map_err(|e| { + Error::InternalError(format!( + "Failed to recover cells and kzg proofs: {e:?}" + )) + })?; + + // Check recovered proofs matches inputs proofs. This is done only to satisfy the + // spec tests, as the ckzg library recomputes all proofs and does not require + // proofs to recover. + for (input_proof, cell_id) in input_proofs.iter().zip(cell_indices) { + if let Err(e) = compare_result::( + &Ok(*input_proof), + &proofs.get(cell_id as usize).cloned(), + ) { + return Err(e); + } + } + + Ok((cells, proofs)) + }); + + let expected = self + .output + .as_ref() + .and_then(|(cells, proofs)| parse_cells_and_proofs(cells, proofs).ok()) + .map(|(cells, proofs)| (cells.try_into().unwrap(), proofs.try_into().unwrap())); + + compare_result::(&result, &expected) + } +} diff --git a/testing/ef_tests/src/handler.rs b/testing/ef_tests/src/handler.rs index 39dbc6ed88b..a89edd36007 100644 --- a/testing/ef_tests/src/handler.rs +++ b/testing/ef_tests/src/handler.rs @@ -886,6 +886,26 @@ impl Handler for KZGVerifyCellKZGProofBatchHandler { } } +#[derive(Derivative)] +#[derivative(Default(bound = ""))] +pub struct KZGRecoverCellsAndKZGProofHandler(PhantomData); + +impl Handler for KZGRecoverCellsAndKZGProofHandler { + type Case = cases::KZGRecoverCellsAndKZGProofs; + + fn config_name() -> &'static str { + "general" + } + + fn runner_name() -> &'static str { + "kzg" + } + + fn handler_name(&self) -> String { + "recover_cells_and_kzg_proofs".into() + } +} + #[derive(Derivative)] #[derivative(Default(bound = ""))] pub struct MerkleProofValidityHandler(PhantomData); diff --git a/testing/ef_tests/tests/tests.rs b/testing/ef_tests/tests/tests.rs index 11c504c75b0..2c13284b3b6 100644 --- a/testing/ef_tests/tests/tests.rs +++ b/testing/ef_tests/tests/tests.rs @@ -755,6 +755,12 @@ fn kzg_verify_cell_proof_batch() { .run_for_feature(ForkName::Deneb, FeatureName::Eip7594); } +#[test] +fn kzg_recover_cells_and_proofs() { + KZGRecoverCellsAndKZGProofHandler::::default() + .run_for_feature(ForkName::Deneb, FeatureName::Eip7594); +} + #[test] fn merkle_proof_validity() { MerkleProofValidityHandler::::default().run();