What the developer wrote
Authored by Mitchell Bagot
Fix Unknown NetworkMessage encoding
The NetworkMessageEncoder and old Encodable implementation for
NetworkMessage make use of the Vec<u8> encoding for the raw payload
data. This introduces a vector length prefix to the start of the
encoding which causes the roundtrip for Unknown NetworkMessage types to
fail. In practice, NetworkMessage will only ever be decoded through the
RawNetworkMessage, where a payload length will be known. As such, this
length prefix is not necessary in the encoding and can be trivially
removed.
Change Unknown NetworkMessage encoding to directly write the vector of
bytes instead of using Vec<u8> encoding logic.
What changed, and why it matters
This commit fixes a bug in how custom or unknown Bitcoin peer-to-peer messages were being serialized. Previously, the code accidentally added an extra length number to the front of the raw payload bytes, so if you decoded an unknown message and re-encoded it, the bytes would not match. The fix writes the raw payload bytes directly without the extra length prefix, and adds a test to confirm round-trip encoding works.
Review whether any downstream code relied on the previous prefixed encoding for Unknown messages, and consider adding additional round-trip tests for other NetworkMessage variants. The fix should be included in the next release.
Security signals we found
Serialization round-trip failure for Unknown NetworkMessage variants
Incorrect length prefix in raw payload encoding
Potential for protocol interoperability issues with unknown P2P message types
Evidence from the diff
The Encodable implementation for NetworkMessage was calling consensus_encode on the Vec
Changed components
p2p/src/message.rsNetworkMessage::Unknown encodingNetworkMessageEncoderRawNetworkMessage decoding pathInspect captured patch +19 / −1
diff --git a/p2p/src/message.rs b/p2p/src/message.rs
index 0c30c15a..ef67827f 100644
--- a/p2p/src/message.rs
+++ b/p2p/src/message.rs
@@ -796,7 +796,8 @@ impl Encodable for NetworkMessage {
| Self::WtxidRelay
| Self::FilterClear
| Self::SendAddrV2 => Ok(0),
- Self::Unknown { payload: ref data, .. } => data.consensus_encode(writer),
+ // Don't use consensus_encode so as not to add a length suffix.
+ Self::Unknown { payload: ref data, .. } => writer.write(data),
}
}
}
@@ -2248,4 +2249,21 @@ mod test {
let headers_message = HeadersMessage(vec![block_900_000, block_900_001, block_900_002]);
assert!(headers_message.is_connected());
}
+
+ #[test]
+ fn network_message_decode() {
+ use encoding::Decoder;
+
+ let data = hex!("010101010101");
+
+ let mut decoder = NetworkMessageDecoder::new(
+ CommandString::try_from_static("unknown").unwrap(),
+ 6,
+ );
+ let _ = decoder.push_bytes(&mut data.as_slice());
+ let decoded = decoder.end().unwrap();
+
+ let enc = serialize(&decoded);
+ assert_eq!(data.as_slice(), enc.as_slice());
+ }
}
Why this scored 37/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.