From a97d77c147da1a08389d5aebc6067975270990eb Mon Sep 17 00:00:00 2001 From: Age Manning Date: Fri, 20 Sep 2024 22:14:57 +1000 Subject: [PATCH] Correct ENR decoding on extension trait (#6402) * Correct enr extension encodings * Clippy my ol friend * Correct all encoding and comparisons * Found some more encodings * Fix remaining tests --- Cargo.lock | 2 + beacon_node/lighthouse_network/Cargo.toml | 1 + .../lighthouse_network/src/discovery/enr.rs | 58 ++++++++++++++----- .../lighthouse_network/src/discovery/mod.rs | 16 +++-- boot_node/Cargo.toml | 1 + boot_node/src/config.rs | 3 +- 6 files changed, 58 insertions(+), 23 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 94eb9038447..5cfa602ef5d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1061,6 +1061,7 @@ name = "boot_node" version = "5.3.0" dependencies = [ "beacon_node", + "bytes", "clap", "clap_utils", "eth2_network_config", @@ -5063,6 +5064,7 @@ name = "lighthouse_network" version = "0.2.0" dependencies = [ "alloy-primitives", + "alloy-rlp", "async-channel", "bytes", "delay_map", diff --git a/beacon_node/lighthouse_network/Cargo.toml b/beacon_node/lighthouse_network/Cargo.toml index c666b8b4552..b0f5b9a5e1c 100644 --- a/beacon_node/lighthouse_network/Cargo.toml +++ b/beacon_node/lighthouse_network/Cargo.toml @@ -44,6 +44,7 @@ delay_map = { workspace = true } bytes = { workspace = true } either = { workspace = true } itertools = { workspace = true } +alloy-rlp = { workspace = true } # Local dependencies void = "1.0.2" diff --git a/beacon_node/lighthouse_network/src/discovery/enr.rs b/beacon_node/lighthouse_network/src/discovery/enr.rs index 6aa4e232d2f..ce29480ffdb 100644 --- a/beacon_node/lighthouse_network/src/discovery/enr.rs +++ b/beacon_node/lighthouse_network/src/discovery/enr.rs @@ -6,6 +6,7 @@ use super::enr_ext::CombinedKeyExt; use super::ENR_FILENAME; use crate::types::{Enr, EnrAttestationBitfield, EnrSyncCommitteeBitfield}; use crate::NetworkConfig; +use alloy_rlp::bytes::Bytes; use libp2p::identity::Keypair; use slog::{debug, warn}; use ssz::{Decode, Encode}; @@ -45,7 +46,7 @@ pub trait Eth2Enr { impl Eth2Enr for Enr { fn attestation_bitfield(&self) -> Result, &'static str> { - let bitfield_bytes: Vec = self + let bitfield_bytes: Bytes = self .get_decodable(ATTESTATION_BITFIELD_ENR_KEY) .ok_or("ENR attestation bitfield non-existent")? .map_err(|_| "Invalid RLP Encoding")?; @@ -57,7 +58,7 @@ impl Eth2Enr for Enr { fn sync_committee_bitfield( &self, ) -> Result, &'static str> { - let bitfield_bytes: Vec = self + let bitfield_bytes: Bytes = self .get_decodable(SYNC_COMMITTEE_BITFIELD_ENR_KEY) .ok_or("ENR sync committee bitfield non-existent")? .map_err(|_| "Invalid RLP Encoding")?; @@ -80,7 +81,7 @@ impl Eth2Enr for Enr { } fn eth2(&self) -> Result { - let eth2_bytes: Vec = self + let eth2_bytes: Bytes = self .get_decodable(ETH2_ENR_KEY) .ok_or("ENR has no eth2 field")? .map_err(|_| "Invalid RLP Encoding")?; @@ -234,17 +235,23 @@ pub fn build_enr( } // set the `eth2` field on our ENR - builder.add_value(ETH2_ENR_KEY, &enr_fork_id.as_ssz_bytes()); + builder.add_value::(ETH2_ENR_KEY, &enr_fork_id.as_ssz_bytes().into()); // set the "attnets" field on our ENR let bitfield = BitVector::::new(); - builder.add_value(ATTESTATION_BITFIELD_ENR_KEY, &bitfield.as_ssz_bytes()); + builder.add_value::( + ATTESTATION_BITFIELD_ENR_KEY, + &bitfield.as_ssz_bytes().into(), + ); // set the "syncnets" field on our ENR let bitfield = BitVector::::new(); - builder.add_value(SYNC_COMMITTEE_BITFIELD_ENR_KEY, &bitfield.as_ssz_bytes()); + builder.add_value::( + SYNC_COMMITTEE_BITFIELD_ENR_KEY, + &bitfield.as_ssz_bytes().into(), + ); // only set `csc` if PeerDAS fork epoch has been scheduled if spec.is_peer_das_scheduled() { @@ -275,16 +282,16 @@ fn compare_enr(local_enr: &Enr, disk_enr: &Enr) -> bool { && local_enr.quic4() == disk_enr.quic4() && local_enr.quic6() == disk_enr.quic6() // must match on the same fork - && local_enr.get_decodable::>(ETH2_ENR_KEY) == disk_enr.get_decodable(ETH2_ENR_KEY) + && local_enr.get_decodable::(ETH2_ENR_KEY) == disk_enr.get_decodable(ETH2_ENR_KEY) // take preference over disk udp port if one is not specified && (local_enr.udp4().is_none() || local_enr.udp4() == disk_enr.udp4()) && (local_enr.udp6().is_none() || local_enr.udp6() == disk_enr.udp6()) // we need the ATTESTATION_BITFIELD_ENR_KEY and SYNC_COMMITTEE_BITFIELD_ENR_KEY and // PEERDAS_CUSTODY_SUBNET_COUNT_ENR_KEY key to match, otherwise we use a new ENR. This will // likely only be true for non-validating nodes. - && local_enr.get_decodable::>(ATTESTATION_BITFIELD_ENR_KEY) == disk_enr.get_decodable(ATTESTATION_BITFIELD_ENR_KEY) - && local_enr.get_decodable::>(SYNC_COMMITTEE_BITFIELD_ENR_KEY) == disk_enr.get_decodable(SYNC_COMMITTEE_BITFIELD_ENR_KEY) - && local_enr.get_decodable::>(PEERDAS_CUSTODY_SUBNET_COUNT_ENR_KEY) == disk_enr.get_decodable(PEERDAS_CUSTODY_SUBNET_COUNT_ENR_KEY) + && local_enr.get_decodable::(ATTESTATION_BITFIELD_ENR_KEY) == disk_enr.get_decodable(ATTESTATION_BITFIELD_ENR_KEY) + && local_enr.get_decodable::(SYNC_COMMITTEE_BITFIELD_ENR_KEY) == disk_enr.get_decodable(SYNC_COMMITTEE_BITFIELD_ENR_KEY) + && local_enr.get_decodable::(PEERDAS_CUSTODY_SUBNET_COUNT_ENR_KEY) == disk_enr.get_decodable(PEERDAS_CUSTODY_SUBNET_COUNT_ENR_KEY) } /// Loads enr from the given directory @@ -332,6 +339,14 @@ mod test { spec } + 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) + } + #[test] fn custody_subnet_count_default() { let config = NetworkConfig { @@ -363,11 +378,22 @@ mod test { ); } - 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) + #[test] + fn test_encode_decode_eth2_enr() { + let (enr, _key) = build_enr_with_config(NetworkConfig::default(), &E::default_spec()); + // Check all Eth2 Mappings are decodeable + enr.eth2().unwrap(); + enr.attestation_bitfield::().unwrap(); + enr.sync_committee_bitfield::().unwrap(); + } + + #[test] + fn test_eth2_enr_encodings() { + let enr_str = "enr:-Mm4QEX9fFRi1n4H3M9sGIgFQ6op1IysTU4Gz6tpIiOGRM1DbJtIih1KgGgv3Xl-oUlwco3HwdXsbYuXStBuNhUVIPoBh2F0dG5ldHOIAAAAAAAAAACDY3NjBIRldGgykI-3hTFgAAA4AOH1BQAAAACCaWSCdjSCaXCErBAADoRxdWljgiMpiXNlY3AyNTZrMaECph91xMyTVyE5MVj6lBpPgz6KP2--Kr9lPbo6_GjrfRKIc3luY25ldHMAg3RjcIIjKIN1ZHCCIyg"; + //let my_enr_str = "enr:-Ma4QM2I1AxBU116QcMV2wKVrSr5Nsko90gMVkstZO4APysQCEwJJJeuTvODKmv7fDsLhVFjrlidVNhBOxSZ8sZPbCWCCcqHYXR0bmV0c4gAAAAAAAAMAIRldGgykGqVoakEAAAA__________-CaWSCdjSCaXCEJq-HPYRxdWljgiMziXNlY3AyNTZrMaECMPAnmmHQpD1k6DuOxWVoFXBoTYY6Wuv9BP4lxauAlmiIc3luY25ldHMAg3RjcIIjMoN1ZHCCIzI"; + let enr = Enr::from_str(enr_str).unwrap(); + enr.eth2().unwrap(); + enr.attestation_bitfield::().unwrap(); + enr.sync_committee_bitfield::().unwrap(); } } diff --git a/beacon_node/lighthouse_network/src/discovery/mod.rs b/beacon_node/lighthouse_network/src/discovery/mod.rs index 3356dd3cf78..e1cea3153ac 100644 --- a/beacon_node/lighthouse_network/src/discovery/mod.rs +++ b/beacon_node/lighthouse_network/src/discovery/mod.rs @@ -15,6 +15,7 @@ pub use enr::{build_enr, load_enr_from_disk, use_or_load_enr, CombinedKey, Eth2E pub use enr_ext::{peer_id_to_node_id, CombinedKeyExt, EnrExt}; pub use libp2p::identity::{Keypair, PublicKey}; +use alloy_rlp::bytes::Bytes; use enr::{ATTESTATION_BITFIELD_ENR_KEY, ETH2_ENR_KEY, SYNC_COMMITTEE_BITFIELD_ENR_KEY}; use futures::prelude::*; use futures::stream::FuturesUnordered; @@ -512,9 +513,9 @@ impl Discovery { // insert the bitfield into the ENR record self.discv5 - .enr_insert( + .enr_insert::( ATTESTATION_BITFIELD_ENR_KEY, - ¤t_bitfield.as_ssz_bytes(), + ¤t_bitfield.as_ssz_bytes().into(), ) .map_err(|e| format!("{:?}", e))?; } @@ -546,9 +547,9 @@ impl Discovery { // insert the bitfield into the ENR record self.discv5 - .enr_insert( + .enr_insert::( SYNC_COMMITTEE_BITFIELD_ENR_KEY, - ¤t_bitfield.as_ssz_bytes(), + ¤t_bitfield.as_ssz_bytes().into(), ) .map_err(|e| format!("{:?}", e))?; } @@ -582,7 +583,7 @@ impl Discovery { let _ = self .discv5 - .enr_insert(ETH2_ENR_KEY, &enr_fork_id.as_ssz_bytes()) + .enr_insert::(ETH2_ENR_KEY, &enr_fork_id.as_ssz_bytes().into()) .map_err(|e| { warn!( self.log, @@ -1289,7 +1290,10 @@ mod tests { bitfield.set(id, true).unwrap(); } - builder.add_value(ATTESTATION_BITFIELD_ENR_KEY, &bitfield.as_ssz_bytes()); + builder.add_value::( + ATTESTATION_BITFIELD_ENR_KEY, + &bitfield.as_ssz_bytes().into(), + ); builder.build(&enr_key).unwrap() } diff --git a/boot_node/Cargo.toml b/boot_node/Cargo.toml index 46ccd4566be..76d41ae11a8 100644 --- a/boot_node/Cargo.toml +++ b/boot_node/Cargo.toml @@ -21,3 +21,4 @@ slog-scope = "4.3.0" hex = { workspace = true } serde = { workspace = true } eth2_network_config = { workspace = true } +bytes = { workspace = true } diff --git a/boot_node/src/config.rs b/boot_node/src/config.rs index aaa9f084826..bb7678631fd 100644 --- a/boot_node/src/config.rs +++ b/boot_node/src/config.rs @@ -1,4 +1,5 @@ use beacon_node::{get_data_dir, set_network_config}; +use bytes::Bytes; use clap::ArgMatches; use eth2_network_config::Eth2NetworkConfig; use lighthouse_network::discv5::{self, enr::CombinedKey, Enr}; @@ -152,7 +153,7 @@ impl BootNodeConfig { // If we know of the ENR field, add it to the initial construction if let Some(enr_fork_bytes) = enr_fork { - builder.add_value("eth2", &enr_fork_bytes); + builder.add_value::("eth2", &enr_fork_bytes.into()); } builder .build(&local_key)