From 14d91c3ba0b043e6ab9e97ca9c76936bdb40f594 Mon Sep 17 00:00:00 2001 From: Oliver Nordbjerg Date: Thu, 2 May 2024 15:58:17 +0200 Subject: [PATCH] fix: make discv4 packets adhere to eip-8 (#8039) --- Cargo.lock | 1 + crates/ethereum-forks/src/forkid.rs | 75 ++++++++++- crates/net/discv4/Cargo.toml | 8 +- crates/net/discv4/src/proto.rs | 199 +++++++++++++++++++++++++++- 4 files changed, 275 insertions(+), 8 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 0a831ae80..2c587c8c7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6583,6 +6583,7 @@ name = "reth-discv4" version = "0.2.0-beta.6" dependencies = [ "alloy-rlp", + "assert_matches", "discv5", "enr", "generic-array", diff --git a/crates/ethereum-forks/src/forkid.rs b/crates/ethereum-forks/src/forkid.rs index 3be3e3ab8..ee4edb8bd 100644 --- a/crates/ethereum-forks/src/forkid.rs +++ b/crates/ethereum-forks/src/forkid.rs @@ -4,7 +4,7 @@ use crate::Head; use alloy_primitives::{hex, BlockNumber, B256}; -use alloy_rlp::*; +use alloy_rlp::{Error as RlpError, *}; #[cfg(any(test, feature = "arbitrary"))] use arbitrary::Arbitrary; use crc::*; @@ -116,19 +116,51 @@ pub struct ForkId { } /// Represents a forward-compatible ENR entry for including the forkid in a node record via -/// EIP-868. Forward compatibility is achieved by allowing trailing fields. +/// EIP-868. Forward compatibility is achieved via EIP-8. /// /// See: /// /// /// for how geth implements ForkId values and forward compatibility. -#[derive(Debug, Clone, PartialEq, Eq, RlpEncodable, RlpDecodable)] -#[rlp(trailing)] +#[derive(Debug, Clone, PartialEq, Eq, RlpEncodable)] pub struct EnrForkIdEntry { /// The inner forkid pub fork_id: ForkId, } +impl Decodable for EnrForkIdEntry { + // NOTE(onbjerg): Manual implementation to satisfy EIP-8. + // + // See https://eips.ethereum.org/EIPS/eip-8 + fn decode(buf: &mut &[u8]) -> alloy_rlp::Result { + let b = &mut &**buf; + let rlp_head = Header::decode(b)?; + if !rlp_head.list { + return Err(RlpError::UnexpectedString) + } + let started_len = b.len(); + + let this = Self { fork_id: Decodable::decode(b)? }; + + // NOTE(onbjerg): Because of EIP-8, we only check that we did not consume *more* than the + // payload length, i.e. it is ok if payload length is greater than what we consumed, as we + // just discard the remaining list items + let consumed = started_len - b.len(); + if consumed > rlp_head.payload_length { + return Err(RlpError::ListLengthMismatch { + expected: rlp_head.payload_length, + got: consumed, + }) + } + + let rem = rlp_head.payload_length - consumed; + b.advance(rem); + *buf = *b; + + Ok(this) + } +} + impl From for EnrForkIdEntry { fn from(fork_id: ForkId) -> Self { Self { fork_id } @@ -652,4 +684,39 @@ mod tests { assert!(fork_filter.set_head_priv(Head { number: b2, ..Default::default() }).is_some()); assert_eq!(fork_filter.current(), h2); } + + mod eip8 { + use super::*; + + fn junk_enr_fork_id_entry() -> Vec { + let mut buf = Vec::new(); + // enr request is just an expiration + let fork_id = ForkId { hash: ForkHash(hex!("deadbeef")), next: 0xBADDCAFE }; + + // add some junk + let junk: u64 = 112233; + + // rlp header encoding + let payload_length = fork_id.length() + junk.length(); + alloy_rlp::Header { list: true, payload_length }.encode(&mut buf); + + // fields + fork_id.encode(&mut buf); + junk.encode(&mut buf); + + buf + } + + #[test] + fn eip8_decode_enr_fork_id_entry() { + let enr_fork_id_entry_with_junk = junk_enr_fork_id_entry(); + + let mut buf = enr_fork_id_entry_with_junk.as_slice(); + let decoded = EnrForkIdEntry::decode(&mut buf).unwrap(); + assert_eq!( + decoded.fork_id, + ForkId { hash: ForkHash(hex!("deadbeef")), next: 0xBADDCAFE } + ); + } + } } diff --git a/crates/net/discv4/Cargo.toml b/crates/net/discv4/Cargo.toml index bd7e99ee6..49e9b4ecc 100644 --- a/crates/net/discv4/Cargo.toml +++ b/crates/net/discv4/Cargo.toml @@ -21,7 +21,12 @@ reth-network-types.workspace = true # ethereum alloy-rlp = { workspace = true, features = ["derive"] } discv5.workspace = true -secp256k1 = { workspace = true, features = ["global-context", "rand-std", "recovery", "serde"] } +secp256k1 = { workspace = true, features = [ + "global-context", + "rand-std", + "recovery", + "serde", +] } enr.workspace = true # async/futures tokio = { workspace = true, features = ["io-util", "net", "time"] } @@ -36,6 +41,7 @@ generic-array = "0.14" serde = { workspace = true, optional = true } [dev-dependencies] +assert_matches.workspace = true rand.workspace = true tokio = { workspace = true, features = ["macros"] } reth-tracing.workspace = true diff --git a/crates/net/discv4/src/proto.rs b/crates/net/discv4/src/proto.rs index da84dc05a..62dd9235d 100644 --- a/crates/net/discv4/src/proto.rs +++ b/crates/net/discv4/src/proto.rs @@ -215,7 +215,7 @@ impl NodeEndpoint { } /// A [FindNode packet](https://github.com/ethereum/devp2p/blob/master/discv4.md#findnode-packet-0x03). -#[derive(Clone, Copy, Debug, Eq, PartialEq, RlpEncodable, RlpDecodable)] +#[derive(Clone, Copy, Debug, Eq, PartialEq, RlpEncodable)] pub struct FindNode { /// The target node's ID, a 64-byte secp256k1 public key. pub id: PeerId, @@ -223,8 +223,41 @@ pub struct FindNode { pub expire: u64, } +impl Decodable for FindNode { + // NOTE(onbjerg): Manual implementation to satisfy EIP-8. + // + // See https://eips.ethereum.org/EIPS/eip-8 + fn decode(buf: &mut &[u8]) -> alloy_rlp::Result { + let b = &mut &**buf; + let rlp_head = Header::decode(b)?; + if !rlp_head.list { + return Err(RlpError::UnexpectedString) + } + let started_len = b.len(); + + let this = Self { id: Decodable::decode(b)?, expire: Decodable::decode(b)? }; + + // NOTE(onbjerg): Because of EIP-8, we only check that we did not consume *more* than the + // payload length, i.e. it is ok if payload length is greater than what we consumed, as we + // just discard the remaining list items + let consumed = started_len - b.len(); + if consumed > rlp_head.payload_length { + return Err(RlpError::ListLengthMismatch { + expected: rlp_head.payload_length, + got: consumed, + }) + } + + let rem = rlp_head.payload_length - consumed; + b.advance(rem); + *buf = *b; + + Ok(this) + } +} + /// A [Neighbours packet](https://github.com/ethereum/devp2p/blob/master/discv4.md#neighbors-packet-0x04). -#[derive(Clone, Debug, Eq, PartialEq, RlpEncodable, RlpDecodable)] +#[derive(Clone, Debug, Eq, PartialEq, RlpEncodable)] pub struct Neighbours { /// The list of nodes containing IP, UDP port, TCP port, and node ID. pub nodes: Vec, @@ -232,16 +265,82 @@ pub struct Neighbours { pub expire: u64, } +impl Decodable for Neighbours { + // NOTE(onbjerg): Manual implementation to satisfy EIP-8. + // + // See https://eips.ethereum.org/EIPS/eip-8 + fn decode(buf: &mut &[u8]) -> alloy_rlp::Result { + let b = &mut &**buf; + let rlp_head = Header::decode(b)?; + if !rlp_head.list { + return Err(RlpError::UnexpectedString) + } + let started_len = b.len(); + + let this = Self { nodes: Decodable::decode(b)?, expire: Decodable::decode(b)? }; + + // NOTE(onbjerg): Because of EIP-8, we only check that we did not consume *more* than the + // payload length, i.e. it is ok if payload length is greater than what we consumed, as we + // just discard the remaining list items + let consumed = started_len - b.len(); + if consumed > rlp_head.payload_length { + return Err(RlpError::ListLengthMismatch { + expected: rlp_head.payload_length, + got: consumed, + }) + } + + let rem = rlp_head.payload_length - consumed; + b.advance(rem); + *buf = *b; + + Ok(this) + } +} + /// A [ENRRequest packet](https://github.com/ethereum/devp2p/blob/master/discv4.md#enrrequest-packet-0x05). /// /// This packet is used to request the current version of a node's Ethereum Node Record (ENR). -#[derive(Clone, Copy, Debug, Eq, PartialEq, RlpEncodable, RlpDecodable)] +#[derive(Clone, Copy, Debug, Eq, PartialEq, RlpEncodable)] pub struct EnrRequest { /// The expiration timestamp for the request. No reply should be sent if it refers to a time in /// the past. pub expire: u64, } +impl Decodable for EnrRequest { + // NOTE(onbjerg): Manual implementation to satisfy EIP-8. + // + // See https://eips.ethereum.org/EIPS/eip-8 + fn decode(buf: &mut &[u8]) -> alloy_rlp::Result { + let b = &mut &**buf; + let rlp_head = Header::decode(b)?; + if !rlp_head.list { + return Err(RlpError::UnexpectedString) + } + let started_len = b.len(); + + let this = Self { expire: Decodable::decode(b)? }; + + // NOTE(onbjerg): Because of EIP-8, we only check that we did not consume *more* than the + // payload length, i.e. it is ok if payload length is greater than what we consumed, as we + // just discard the remaining list items + let consumed = started_len - b.len(); + if consumed > rlp_head.payload_length { + return Err(RlpError::ListLengthMismatch { + expected: rlp_head.payload_length, + got: consumed, + }) + } + + let rem = rlp_head.payload_length - consumed; + b.advance(rem); + *buf = *b; + + Ok(this) + } +} + /// A [ENRResponse packet](https://github.com/ethereum/devp2p/blob/master/discv4.md#enrresponse-packet-0x06). /// /// This packet is used to respond to an ENRRequest packet and includes the requested ENR along with @@ -442,6 +541,7 @@ mod tests { test_utils::{rng_endpoint, rng_ipv4_record, rng_ipv6_record, rng_message}, DEFAULT_DISCOVERY_PORT, SAFE_MAX_DATAGRAM_NEIGHBOUR_RECORDS, }; + use assert_matches::assert_matches; use enr::EnrPublicKey; use rand::{thread_rng, Rng, RngCore}; use reth_primitives::{hex, ForkHash}; @@ -769,4 +869,97 @@ mod tests { ); assert!(decoded_enr.verify()); } + + mod eip8 { + use super::*; + + fn junk_enr_request() -> Vec { + let mut buf = Vec::new(); + // enr request is just an expiration + let expire: u64 = 123456; + + // add some junk + let junk: u64 = 112233; + + // rlp header encoding + let payload_length = expire.length() + junk.length(); + alloy_rlp::Header { list: true, payload_length }.encode(&mut buf); + + // fields + expire.encode(&mut buf); + junk.encode(&mut buf); + + buf + } + + // checks that junk data at the end of the packet is discarded according to eip-8 + #[test] + fn eip8_decode_enr_request() { + let enr_request_with_junk = junk_enr_request(); + + let mut buf = enr_request_with_junk.as_slice(); + let decoded = EnrRequest::decode(&mut buf).unwrap(); + assert_eq!(decoded.expire, 123456); + } + + // checks that junk data at the end of the packet is discarded according to eip-8 + // + // test vector from eip-8: https://eips.ethereum.org/EIPS/eip-8 + #[test] + fn eip8_decode_findnode() { + let findnode_with_junk = hex!("c7c44041b9f7c7e41934417ebac9a8e1a4c6298f74553f2fcfdcae6ed6fe53163eb3d2b52e39fe91831b8a927bf4fc222c3902202027e5e9eb812195f95d20061ef5cd31d502e47ecb61183f74a504fe04c51e73df81f25c4d506b26db4517490103f84eb840ca634cae0d49acb401d8a4c6b6fe8c55b70d115bf400769cc1400f3258cd31387574077f301b421bc84df7266c44e9e6d569fc56be00812904767bf5ccd1fc7f8443b9a35582999983999999280dc62cc8255c73471e0a61da0c89acdc0e035e260add7fc0c04ad9ebf3919644c91cb247affc82b69bd2ca235c71eab8e49737c937a2c396"); + + let buf = findnode_with_junk.as_slice(); + let decoded = Message::decode(buf).unwrap(); + + let expected_id = hex!("ca634cae0d49acb401d8a4c6b6fe8c55b70d115bf400769cc1400f3258cd31387574077f301b421bc84df7266c44e9e6d569fc56be00812904767bf5ccd1fc7f"); + assert_matches!(decoded.msg, Message::FindNode(FindNode { id, expire: 1136239445 }) if id == expected_id); + } + + // checks that junk data at the end of the packet is discarded according to eip-8 + // + // test vector from eip-8: https://eips.ethereum.org/EIPS/eip-8 + #[test] + fn eip8_decode_neighbours() { + let neighbours_with_junk = hex!("c679fc8fe0b8b12f06577f2e802d34f6fa257e6137a995f6f4cbfc9ee50ed3710faf6e66f932c4c8d81d64343f429651328758b47d3dbc02c4042f0fff6946a50f4a49037a72bb550f3a7872363a83e1b9ee6469856c24eb4ef80b7535bcf99c0004f9015bf90150f84d846321163782115c82115db8403155e1427f85f10a5c9a7755877748041af1bcd8d474ec065eb33df57a97babf54bfd2103575fa829115d224c523596b401065a97f74010610fce76382c0bf32f84984010203040101b840312c55512422cf9b8a4097e9a6ad79402e87a15ae909a4bfefa22398f03d20951933beea1e4dfa6f968212385e829f04c2d314fc2d4e255e0d3bc08792b069dbf8599020010db83c4d001500000000abcdef12820d05820d05b84038643200b172dcfef857492156971f0e6aa2c538d8b74010f8e140811d53b98c765dd2d96126051913f44582e8c199ad7c6d6819e9a56483f637feaac9448aacf8599020010db885a308d313198a2e037073488203e78203e8b8408dcab8618c3253b558d459da53bd8fa68935a719aff8b811197101a4b2b47dd2d47295286fc00cc081bb542d760717d1bdd6bec2c37cd72eca367d6dd3b9df738443b9a355010203b525a138aa34383fec3d2719a0"); + + let buf = neighbours_with_junk.as_slice(); + let decoded = Message::decode(buf).unwrap(); + + let _ = NodeRecord { + address: "99.33.22.55".parse().unwrap(), + tcp_port: 4444, + udp_port: 4445, + id: hex!("3155e1427f85f10a5c9a7755877748041af1bcd8d474ec065eb33df57a97babf54bfd2103575fa829115d224c523596b401065a97f74010610fce76382c0bf32").into(), + }.length(); + + let expected_nodes: Vec = vec![ + NodeRecord { + address: "99.33.22.55".parse().unwrap(), + tcp_port: 4444, + udp_port: 4445, + id: hex!("3155e1427f85f10a5c9a7755877748041af1bcd8d474ec065eb33df57a97babf54bfd2103575fa829115d224c523596b401065a97f74010610fce76382c0bf32").into(), + }, + NodeRecord { + address: "1.2.3.4".parse().unwrap(), + tcp_port: 1, + udp_port: 1, + id: hex!("312c55512422cf9b8a4097e9a6ad79402e87a15ae909a4bfefa22398f03d20951933beea1e4dfa6f968212385e829f04c2d314fc2d4e255e0d3bc08792b069db").into(), + }, + NodeRecord { + address: "2001:db8:3c4d:15::abcd:ef12".parse().unwrap(), + tcp_port: 3333, + udp_port: 3333, + id: hex!("38643200b172dcfef857492156971f0e6aa2c538d8b74010f8e140811d53b98c765dd2d96126051913f44582e8c199ad7c6d6819e9a56483f637feaac9448aac").into(), + }, + NodeRecord { + address: "2001:db8:85a3:8d3:1319:8a2e:370:7348".parse().unwrap(), + tcp_port: 999, + udp_port: 1000, + id: hex!("8dcab8618c3253b558d459da53bd8fa68935a719aff8b811197101a4b2b47dd2d47295286fc00cc081bb542d760717d1bdd6bec2c37cd72eca367d6dd3b9df73").into(), + }, + ]; + assert_matches!(decoded.msg, Message::Neighbours(Neighbours { nodes, expire: 1136239445 }) if nodes == expected_nodes); + } + } }