p2p: Introduce a `NetworkHeader` wrapper
What the developer wrote
Authored by rustaceanrob
p2p: Introduce a `NetworkHeader` wrapper
The block headers when sent over the network include a 0 byte suffix for
the length of transactions to follow, which happens to always be 0. It
is possible this byte gets repurposed for something more useful in the
future depending on the `ProtocolVersion` sent in the request. We
should not fail the decoding if a non-zero byte is sent, as doing so
guarentees any client cannot follow the most work chain if this byte is
non-zero.
This also allows the new `encoding` traits to be implemented gracefully.
On other branches without the wrapper type, I've found this to be a
nightmare.
Note that the old traits must be implemented as we implement
`encoding::Encodable` for `NetworkMessage` _based on_ the old traits.
This is unfortunate for adding new wrapper types in the messages, as
this code will inevitably be removed, but it's too late to change the
`encoing::Encodable` for `NetworkMessage` so I think we bite the bullet
and add this temporarily.
What changed, and why it matters
This commit changes how Bitcoin block headers received over the peer-to-peer network are parsed. Previously, the code would reject a headers message if the trailing byte (which indicates how many transactions follow) was anything other than zero. The commit introduces a wrapper type that accepts any value for that byte. The stated reason is future-proofing: if Bitcoin ever uses that byte for something useful, clients using this stricter library would be unable to follow the longest chain because they'd reject valid messages. There is no claim in the commit that this fixes an active security bug, and the change is framed as a protocol-compatibility improvement.
Treat as a normal protocol-compatibility refactor rather than a security fix. Reviewers should confirm that downstream consumers of `HeadersMessage` do not silently assume `length == 0` where it now may be non-zero, and that the new wrapper does not weaken any higher-level invariant checks elsewhere in the crate.
Security signals we found
Change removes a strict parse-failure on a network input byte, increasing permissiveness
Commit message frames the change as preventing future chain-following failure, not as fixing a current vulnerability
No bounds, overflow, or memory-safety issues are evident in the diff
The wrapper preserves the byte value so callers can inspect it if needed
No authentication, signature, or consensus-rule changes are present
Evidence from the diff
The patch replaces direct consensus encoding/decoding of HeadersMessage(Vec<block::Header>) with a new NetworkHeader { header: block::Header, length: u8 } wrapper. The old decoder explicitly errored if the per-header transaction-count suffix byte was non-zero; the new decoder stores the byte and permits any u8 value. Encoding now writes the stored length field rather than a hardcoded 0. The change also adds impl_vec_wrapper! for HeadersMessage, updates is_connected() to access .header, and adjusts tests and Arbitrary implementations. The commit message explicitly notes this avoids a scenario where a future protocol version repurposes the byte and clients would fail to decode the most-work chain.
Changed components
p2p/src/message.rsHeadersMessageNetworkHeaderDecodable/Encodable traits for P2P headersInspect captured patch +55 / −41
diff --git a/p2p/src/message.rs b/p2p/src/message.rs
index 9fbab9f8..169928b2 100644
--- a/p2p/src/message.rs
+++ b/p2p/src/message.rs
@@ -745,19 +745,6 @@ impl V2NetworkMessage {
pub fn command(&self) -> CommandString { self.payload.command() }
}
-impl Encodable for HeadersMessage {
- #[inline]
- fn consensus_encode<W: Write + ?Sized>(&self, w: &mut W) -> Result<usize, io::Error> {
- let mut len = 0;
- len += w.emit_compact_size(self.0.len())?;
- for header in &self.0 {
- len += header.consensus_encode(w)?;
- len += 0u8.consensus_encode(w)?;
- }
- Ok(len)
- }
-}
-
impl Encodable for NetworkMessage {
fn consensus_encode<W: Write + ?Sized>(&self, writer: &mut W) -> Result<usize, io::Error> {
match self {
@@ -1384,9 +1371,45 @@ impl Encodable for V2NetworkMessage {
}
}
+/// Network encoded [`Header`](primitives::block::Header) with associated byte for the length of
+/// transactions that follow, which is currently always zero.
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub struct NetworkHeader {
+ /// Block header.
+ pub header: block::Header,
+ /// Length of transaction list.
+ pub length: u8,
+}
+
+impl NetworkHeader {
+ /// Create a new [`NetworkHeader`] from underlying block header.
+ pub const fn from_header(header: block::Header) -> Self {
+ Self { header, length: 0 }
+ }
+}
+
+impl Decodable for NetworkHeader {
+ fn consensus_decode<R: BufRead + ?Sized>(
+ reader: &mut R,
+ ) -> Result<Self, encode::Error> {
+ Ok(Self {
+ header: Decodable::consensus_decode(reader)?,
+ length: reader.read_u8()?,
+ })
+ }
+}
+
+impl Encodable for NetworkHeader {
+ fn consensus_encode<W: Write + ?Sized>(&self, writer: &mut W) -> Result<usize, io::Error> {
+ let mut size = self.header.consensus_encode(writer)?;
+ size += self.length.consensus_encode(writer)?;
+ Ok(size)
+ }
+}
+
/// A list of bitcoin block headers.
#[derive(Debug, Clone, PartialEq, Eq)]
-pub struct HeadersMessage(pub Vec<block::Header>);
+pub struct HeadersMessage(pub Vec<NetworkHeader>);
impl HeadersMessage {
/// Does each header point to the previous block hash in the list.
@@ -1394,36 +1417,17 @@ impl HeadersMessage {
self.0
.iter()
.zip(self.0.iter().skip(1))
- .all(|(first, second)| first.block_hash().eq(&second.prev_blockhash))
- }
-}
-
-impl Decodable for HeadersMessage {
- #[inline]
- fn consensus_decode_from_finite_reader<R: BufRead + ?Sized>(
- r: &mut R,
- ) -> Result<Self, encode::Error> {
- let len = r.read_compact_size()?;
- // should be above usual number of items to avoid
- // allocation
- let mut ret = Vec::with_capacity(core::cmp::min(1024 * 16, len as usize));
- for _ in 0..len {
- ret.push(Decodable::consensus_decode(r)?);
- if u8::consensus_decode(r)? != 0u8 {
- return Err(crate::consensus::parse_failed_error(
- "Headers message should not contain transactions",
- ));
- }
- }
- Ok(Self(ret))
+ .all(|(first, second)| first.header.block_hash().eq(&second.header.prev_blockhash))
}
- #[inline]
- fn consensus_decode<R: BufRead + ?Sized>(r: &mut R) -> Result<Self, encode::Error> {
- Self::consensus_decode_from_finite_reader(&mut r.take(MAX_MSG_SIZE.to_u64()))
+ /// Take the message as an iterator of [`Header`](primitives::block::Header).
+ pub fn into_headers(self) -> impl Iterator<Item = block::Header> {
+ self.0.into_iter().map(|network| network.header)
}
}
+impl_vec_wrapper!(HeadersMessage, NetworkHeader);
+
impl Decodable for RawNetworkMessage {
fn consensus_decode_from_finite_reader<R: BufRead + ?Sized>(
r: &mut R,
@@ -1724,6 +1728,13 @@ impl<'a> Arbitrary<'a> for CommandString {
}
}
+#[cfg(feature = "arbitrary")]
+impl<'a> Arbitrary<'a> for NetworkHeader {
+ fn arbitrary(u: &mut Unstructured<'a>) -> arbitrary::Result<Self> {
+ Ok(Self { header: u.arbitrary()?, length: u.arbitrary()? })
+ }
+}
+
#[cfg(feature = "arbitrary")]
impl<'a> Arbitrary<'a> for HeadersMessage {
fn arbitrary(u: &mut Unstructured<'a>) -> arbitrary::Result<Self> { Ok(Self(u.arbitrary()?)) }
@@ -1853,7 +1864,7 @@ mod test {
NetworkMessage::MemPool,
NetworkMessage::Tx(tx),
NetworkMessage::Block(block),
- NetworkMessage::Headers(HeadersMessage(vec![header])),
+ NetworkMessage::Headers(HeadersMessage(vec![NetworkHeader { header, length: 0 }])),
NetworkMessage::SendHeaders,
NetworkMessage::GetAddr,
NetworkMessage::Ping(15),
@@ -2244,7 +2255,10 @@ mod test {
let block_900_002 = deserialize::<block::Header>(
&hex!("0400ff3ffc834fac4e1eb2ae41f1f9776e0f8e24a6090603ffa8010000000000000000002efba7e7280aa60f0a650f29e30332d52e11af57bc58cc6e71f343851f016c676182426874370217e3615653")
).unwrap();
- let headers_message = HeadersMessage(vec![block_900_000, block_900_001, block_900_002]);
+ let header_900_000 = NetworkHeader { header: block_900_000, length: 0 };
+ let header_900_001 = NetworkHeader { header: block_900_001, length: 0 };
+ let header_900_002 = NetworkHeader { header: block_900_002, length: 0 };
+ let headers_message = HeadersMessage(vec![header_900_000, header_900_001, header_900_002]);
assert!(headers_message.is_connected());
}
}
Why this scored 34/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.