fix: make discv4 packets adhere to eip-8 (#8039)

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:
 /// <https://github.com/ethereum/devp2p/blob/master/enr-entries/eth.md#entry-format>
 ///
 /// 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<Self> {
+        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<ForkId> 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<u8> {
+            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 }
+            );
+        }
+    }
 }

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

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<Self> {
+        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<NodeRecord>,
@@ -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<Self> {
+        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<Self> {
+        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<u8> {
+            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<NodeRecord> = 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);
+        }
+    }
 }