p2p: make SendCmpct encode/decode idempotent
What the developer wrote
Authored by yancy
p2p: make SendCmpct encode/decode idempotent
Normalizing the mode bit makes serialization/deserealiztion _not_
idempotent. That's due to the checksum that's created before
normalization which if serialized again, no longer has the same
checksum. This cases a failure to serialize the same `SendCmpct` which
was received if the mode bit is anything other then 0 or 1.
Furthermore, the spec https://bips.dev/152/ defines only 0
(low-bandwidth mode) or 1 (high-bandwidth mode) so anything other than
0 or 1 is unspecified and should result in an error.
As quoted from spec https://bips.dev/152/:
The first integer SHALL be interpreted as a boolean (and MUST have a
value of either 1 or 0)
What changed, and why it matters
This change fixes a serialization bug in how the rust-bitcoin library handles a Bitcoin peer-to-peer message called `sendcmpct`. Previously, if a malformed or unusual message arrived with a mode value other than 0 or 1, the library would silently normalize it to true/false. That normalization changed the message bytes, so if the library later tried to re-send the exact message it had received, the message checksum would no longer match, causing a serialization failure. The patch now rejects any mode value other than 0 or 1, matching the BIP 152 specification, and reports a clear decoding error instead of silently altering the data.
Review whether any other P2P message decoders silently normalize multi-value fields into booleans or smaller types, as similar idempotency issues may exist elsewhere. Consider adding roundtrip tests for all P2P messages with edge-case byte values.
Security signals we found
Non-idempotent encode/decode roundtrip for network messages
Silent normalization of out-of-spec protocol field
Checksum mismatch on re-serialization of received messages
Validation now enforces BIP 152 boolean constraint (0 or 1)
Evidence from the diff
The SendCmpct decoder in p2p/src/message_compact_blocks.rs previously converted the first byte to a boolean using u8::from_le_bytes(send_cmpct) != 0, which accepted any non-zero byte as true. Because the struct stores a normalized bool, re-encoding a received message with a mode byte like 2 would produce a serialized byte 1, changing the payload and therefore its checksum. The patch introduces an explicit validity check (send_cmpct[0] == 1 || send_cmpct[0] == 0) and a new InvalidMode error variant, making decode fail fast on out-of-spec values and preserving encode/decode idempotency for valid messages.
Changed components
p2p/src/message_compact_blocks.rsSendCmpct message decoderSendCmpctDecoderError error typeInspect captured patch +44 / −8
diff --git a/p2p/src/message_compact_blocks.rs b/p2p/src/message_compact_blocks.rs
index f6612bb2..ef50d74d 100644
--- a/p2p/src/message_compact_blocks.rs
+++ b/p2p/src/message_compact_blocks.rs
@@ -44,12 +44,21 @@ crate::decoder_newtype! {
#[derive(Debug, Default, Clone)]
pub struct SendCmpctDecoder(SendCmpctInnerDecoder);
+ fn map_push_bytes_err(e: <SendCmpctInnerDecoder as encoding::Decoder>::Error) -> SendCmpctDecoderError {
+ SendCmpctDecoderError::eof(e)
+ }
+
fn end(
result: Result<([u8; 1], [u8; 8]), <SendCmpctInnerDecoder as encoding::Decoder>::Error>
) -> Result<SendCmpct, SendCmpctDecoderError> {
- let (send_cmpct, version) = result.map_err(SendCmpctDecoderError)?;
- let send_compact = u8::from_le_bytes(send_cmpct) != 0;
- Ok(SendCmpct { send_compact, version: u64::from_le_bytes(version) })
+ let (send_cmpct, version) = result.map_err(SendCmpctDecoderError::eof)?;
+
+ if send_cmpct[0] == 1 || send_cmpct[0] == 0 {
+ let send_compact = u8::from_le_bytes(send_cmpct) != 0;
+ Ok(SendCmpct { send_compact, version: u64::from_le_bytes(version) })
+ } else {
+ Err(SendCmpctDecoderError::invalid_mode())
+ }
}
}
@@ -64,13 +73,35 @@ pub mod error {
use internals::write_err;
+ use crate::message_compact_blocks::SendCmpctInnerDecoder;
+
/// Errors occurring when decoding a [`SendCmpct`] message.
///
/// [`SendCmpct`]: super::SendCmpct
#[derive(Debug, Clone, PartialEq, Eq)]
- pub struct SendCmpctDecoderError(
- pub(super) <super::SendCmpctInnerDecoder as encoding::Decoder>::Error,
- );
+ pub struct SendCmpctDecoderError(pub(super) SendCmpctDecoderErrorInner);
+
+ impl SendCmpctDecoderError {
+ /// Constructs an EOF error.
+ #[inline]
+ pub(super) fn eof(e: <SendCmpctInnerDecoder as encoding::Decoder>::Error) -> Self {
+ Self(SendCmpctDecoderErrorInner::UnexpectedEof(e))
+ }
+ /// Constructs an invalid mode error.
+ #[inline]
+ pub(super) fn invalid_mode() -> Self { Self(SendCmpctDecoderErrorInner::InvalidMode) }
+ }
+
+ /// Errors occuring when decoding a [`SendCmpct`] message.
+ ///
+ /// [`SendCmpct`]: super::SendCmpct
+ #[derive(Debug, Clone, PartialEq, Eq)]
+ pub(super) enum SendCmpctDecoderErrorInner {
+ /// First byte was not a boolean value (0 or 1).
+ InvalidMode,
+ /// `UnexpectedEofError` Error by way of associated type on `ArrayDecoder<N>`.
+ UnexpectedEof(<super::SendCmpctInnerDecoder as encoding::Decoder>::Error),
+ }
impl From<Infallible> for SendCmpctDecoderError {
fn from(never: Infallible) -> Self { match never {} }
@@ -78,13 +109,18 @@ pub mod error {
impl fmt::Display for SendCmpctDecoderError {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
- write_err!(f, "sendcmpct error"; self.0)
+ write_err!(f, "sendcmpct error"; self)
}
}
#[cfg(feature = "std")]
impl std::error::Error for SendCmpctDecoderError {
- fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { Some(&self.0) }
+ fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
+ match self.0 {
+ SendCmpctDecoderErrorInner::InvalidMode => None,
+ SendCmpctDecoderErrorInner::UnexpectedEof(ref e) => Some(e),
+ }
+ }
}
}
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.