p2p_core%fix!: align P2P framing, inventory types and header limits with the reference implementation

pkgs/p2p_core/Cargo.toml

@@ -57,6 +57,7 @@ thiserror = { workspace = true }
 dash-dev = { version = "0.0.0", path = "../dev", features = ["full"] }
 hex-conservative = { workspace = true, features = ["std"] }
 rstest = { workspace = true }
+serde = { workspace = true, features = ["derive", "std"] }
 
 [lints]
 workspace = true

pkgs/p2p_core/corpus/inv.json5

@@ -52,5 +52,64 @@
         ]
       }
     },
-  }
+  },
+  // Generated with the reference implementation (dashpay/dash v24.0.0-rc.3,
+  // 0364ed626e). To run the program, paste it in `src/bitcoin-util.cpp` at the
+  // specified revision, build it with `make -C src dash-util` and run
+  // `./src/dash-util`. The output should match the section.
+/*
+#include <protocol.h>
+
+#include <cstdio>
+
+int main()
+{
+    for (uint32_t type = 0; type <= 36; ++type) {
+        const CInv inv{type, uint256{}};
+        std::printf("    { \"type\": %u, \"command\": %s%s%s, \"tx\": %s, \"block\": %s },\n", type,
+                    inv.IsKnownType() ? "\"" : "", inv.IsKnownType() ? inv.GetCommand().c_str() : "null",
+                    inv.IsKnownType() ? "\"" : "", inv.IsGenTxMsg() ? "true" : "false",
+                    inv.IsGenBlkMsg() ? "true" : "false");
+    }
+}
+*/
+  "inv_type": [
+    { "type": 0, "command": null, "tx": false, "block": false },
+    { "type": 1, "command": "tx", "tx": true, "block": false },
+    { "type": 2, "command": "block", "tx": false, "block": true },
+    { "type": 3, "command": "merkleblock", "tx": false, "block": true },
+    { "type": 4, "command": null, "tx": false, "block": false },
+    { "type": 5, "command": null, "tx": false, "block": false },
+    { "type": 6, "command": "spork", "tx": false, "block": false },
+    { "type": 7, "command": null, "tx": false, "block": false },
+    { "type": 8, "command": null, "tx": false, "block": false },
+    { "type": 9, "command": null, "tx": false, "block": false },
+    { "type": 10, "command": null, "tx": false, "block": false },
+    { "type": 11, "command": null, "tx": false, "block": false },
+    { "type": 12, "command": null, "tx": false, "block": false },
+    { "type": 13, "command": null, "tx": false, "block": false },
+    { "type": 14, "command": null, "tx": false, "block": false },
+    { "type": 15, "command": null, "tx": false, "block": false },
+    { "type": 16, "command": "dstx", "tx": true, "block": false },
+    { "type": 17, "command": "govobj", "tx": false, "block": false },
+    { "type": 18, "command": "govobjvote", "tx": false, "block": false },
+    { "type": 19, "command": null, "tx": false, "block": false },
+    { "type": 20, "command": "cmpctblock", "tx": false, "block": true },
+    { "type": 21, "command": "qfcommit", "tx": false, "block": false },
+    { "type": 22, "command": null, "tx": false, "block": false },
+    { "type": 23, "command": "qcontrib", "tx": false, "block": false },
+    { "type": 24, "command": "qcomplaint", "tx": false, "block": false },
+    { "type": 25, "command": "qjustify", "tx": false, "block": false },
+    { "type": 26, "command": "qpcommit", "tx": false, "block": false },
+    { "type": 27, "command": null, "tx": false, "block": false },
+    { "type": 28, "command": "qsigrec", "tx": false, "block": false },
+    { "type": 29, "command": "clsig", "tx": false, "block": false },
+    { "type": 30, "command": null, "tx": false, "block": false },
+    { "type": 31, "command": "isdlock", "tx": false, "block": false },
+    { "type": 32, "command": "dsq", "tx": false, "block": false },
+    { "type": 33, "command": "platformban", "tx": false, "block": false },
+    { "type": 34, "command": "tx", "tx": true, "block": false },
+    { "type": 35, "command": null, "tx": false, "block": false },
+    { "type": 36, "command": null, "tx": false, "block": false },
+  ],
 }

pkgs/p2p_core/src/bip324.rs

@@ -6,12 +6,15 @@
 
 //! BIP324 V2 message framing.
 
+use crate::codec::MAX_P2P_PAYLOAD_SIZE;
 use crate::command::CommandString;
 use crate::msg::P2pMsg;
 use crate::prelude::*;
 use crate::short_id::ShortId;
 use crate::P2pDecodeError;
 
+use dash_types::codec::BaseCodec;
+
 /// Encodes a `P2pMsg` into V2 framed bytes.
 pub fn encode_v2(msg: &P2pMsg, buf: &mut Vec<u8>) {
   match msg.short_id() {
@@ -28,35 +31,43 @@ pub fn encode_v2(msg: &P2pMsg, buf: &mut Vec<u8>) {
   msg.encode_payload(buf);
 }
 
-/// Decodes V2 framed bytes into a `P2pMsg`.
-pub fn decode_v2(payload: &[u8]) -> Result<P2pMsg, P2pDecodeError> {
-  if payload.is_empty() {
-    return Err(P2pDecodeError::Consensus(String::from(
-      "unexpected eof: needed 1 byte, 0 remaining",
-    )));
-  }
-
-  let short_id = payload[0];
-  let rest = &payload[1..];
+/// The type byte, a 12-byte command and a [`MAX_P2P_PAYLOAD_SIZE`] payload.
+const MAX_V2_CONTENTS_SIZE: usize = 1 + 12 + MAX_P2P_PAYLOAD_SIZE;
 
-  if short_id == 0 {
-    // Long format: next 12 bytes are the command string.
-    if rest.len() < 12 {
-      return Err(P2pDecodeError::Consensus(format!(
-        "unexpected eof: needed 12 bytes, {} remaining",
-        rest.len()
-      )));
-    }
-    let mut cmd_bytes = [0u8; 12];
-    cmd_bytes.copy_from_slice(&rest[..12]);
-    let cmd = CommandString::from_bytes(cmd_bytes);
-    P2pMsg::decode_payload(&cmd, &rest[12..])
+/// Decodes V2 message contents into a `P2pMsg`.
+///
+/// The size bound applies to the whole contents and comes before any check
+/// of the type. A short-ID payload may thus be 12 bytes larger than a
+/// long-format payload.
+///
+/// # Errors
+///
+/// Returns `PayloadTooLarge` above the bound, with an empty command for an
+/// unknown short ID. Returns `MissingCommand`, `UnknownShortId` or
+/// `InvalidCommand` for a type the transport refuses, and otherwise the error
+/// decoding the message.
+pub fn decode_v2(contents: &[u8]) -> Result<P2pMsg, P2pDecodeError> {
+  let mut payload = contents;
+  let short_id = u8::decode(&mut payload).map_err(|_| P2pDecodeError::MissingCommand)?;
+  let id = ShortId(short_id);
+  let cmd = if id == ShortId::LONG_FORMAT {
+    Some(CommandString::decode(&mut payload).map_err(|_| P2pDecodeError::MissingCommand)?)
+  } else if id.is_in_table() {
+    Some(id.to_command().unwrap_or(CommandString::from_static("")))
   } else {
-    // Short ID: resolve to command, then decode payload.
-    let sid = ShortId(short_id);
-    let cmd = sid
-      .to_command()
-      .ok_or(P2pDecodeError::UnknownShortId { id: short_id })?;
-    P2pMsg::decode_payload(&cmd, rest)
+    None
+  };
+
+  if contents.len() > MAX_V2_CONTENTS_SIZE {
+    return Err(P2pDecodeError::PayloadTooLarge {
+      command: cmd.unwrap_or(CommandString::from_static("")),
+      size: contents.len(),
+      max: MAX_V2_CONTENTS_SIZE,
+    });
+  }
+  let cmd = cmd.ok_or(P2pDecodeError::UnknownShortId { id: short_id })?;
+  if id == ShortId::LONG_FORMAT && !cmd.is_valid_v2() {
+    return Err(P2pDecodeError::InvalidCommand { command: cmd });
   }
+  P2pMsg::decode_unbounded(&cmd, payload)
 }

pkgs/p2p_core/src/codec.rs

@@ -6,8 +6,8 @@
 
 //! Codec helpers for P2P message types.
 
-/// Maximum buffered P2P message payload.
-pub(crate) const MAX_P2P_PAYLOAD_SIZE: usize = 3_145_728;
+/// Largest P2P message payload, in bytes.
+pub const MAX_P2P_PAYLOAD_SIZE: usize = 3_145_728;
 
 /// Generates `Encode` + `Decode` with P2P payload size limit.
 macro_rules! impl_p2p {

pkgs/p2p_core/src/command.rs

@@ -51,12 +51,31 @@ impl CommandString {
   }
 
   /// Returns the command as a `&str` (trimmed of null padding).
+  ///
+  /// A command that is not UTF-8 up to its first NUL gives the empty string.
   pub fn as_str(&self) -> &str {
     let end = self.0.iter().position(|&b| b == 0).unwrap_or(12);
-    // The bytes are always valid ASCII written by from_static or
-    // validated on decode, so this conversion is sound.
     core::str::from_utf8(&self.0[..end]).unwrap_or("")
   }
+
+  /// Whether a V2 transport accepts the command as a long-form type.
+  ///
+  /// Bytes before the first NUL must be in 0x20 to 0x7f, and every byte after
+  /// it NUL.
+  pub const fn is_valid_v2(&self) -> bool {
+    let mut padding = false;
+    let mut i = 0;
+    while i < self.0.len() {
+      let b = self.0[i];
+      if b == 0 {
+        padding = true;
+      } else if padding || b < b' ' || b > 0x7f {
+        return false;
+      }
+      i += 1;
+    }
+    true
+  }
 }
 
 type_cvrt!(From<[u8; 12]> for CommandString, |bytes| Self(*bytes));

pkgs/p2p_core/src/error.rs

@@ -6,7 +6,7 @@
 
 //! P2P-layer decoding errors.
 
-use crate::prelude::*;
+use crate::command::CommandString;
 
 use dash_types::codec::DecodeError;
 use dash_types::type_id::Unencodable;
@@ -16,27 +16,37 @@ use core::fmt;
 /// An error encountered during P2P message decoding.
 #[derive(Clone, Debug, Eq, Hash, PartialEq, thiserror::Error, Unencodable)]
 pub enum P2pDecodeError {
-  /// Underlying consensus decoding error.
-  #[error("{0}")]
-  Consensus(String),
+  /// A field failed to decode.
+  #[error(transparent)]
+  Consensus(#[from] DecodeError),
   /// Unrecognised 12-byte command string.
   #[error(fmt = fmt_unknown_command)]
   UnknownCommand {
     /// The raw command bytes.
     bytes: [u8; 12],
   },
-  /// V2 short ID does not map to a known message.
+  /// V2 contents end before the message type does.
+  #[error("missing command")]
+  MissingCommand,
+  /// A command that fails [`CommandString::is_valid_v2`].
+  #[error(fmt = fmt_invalid_command)]
+  InvalidCommand {
+    /// The command as received.
+    command: CommandString,
+  },
+  /// V2 short ID outside the short ID tables.
   #[error("unknown v2 short id: {id}")]
   UnknownShortId {
     /// The short ID byte.
     id: u8,
   },
   /// Message payload exceeds the allowed size.
-  #[error("{command} payload too large: {size} bytes, max {max}")]
+  #[error(fmt = fmt_payload_too_large)]
   PayloadTooLarge {
-    /// Wire command name.
-    command: &'static str,
-    /// Size of the raw payload, in bytes.
+    /// The message's command.
+    command: CommandString,
+    /// Size of the payload, or of the whole contents for `decode_v2`, in
+    /// bytes.
     size: usize,
     /// Maximum allowed size, in bytes.
     max: usize,
@@ -57,15 +67,31 @@ pub enum P2pDecodeError {
   },
 }
 
-impl From<DecodeError> for P2pDecodeError {
-  fn from(e: DecodeError) -> Self {
-    Self::Consensus(format!("{e}"))
-  }
-}
-
 /// Formats `P2pDecodeError::UnknownCommand`, escaping non-printable bytes.
 fn fmt_unknown_command(bytes: &[u8; 12], f: &mut fmt::Formatter<'_>) -> fmt::Result {
   f.write_str("unknown command: ")?;
+  write_command(bytes, f)
+}
+
+/// Formats `P2pDecodeError::InvalidCommand`, escaping non-printable bytes.
+fn fmt_invalid_command(command: &CommandString, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+  f.write_str("invalid command: ")?;
+  write_command(command.as_bytes(), f)
+}
+
+/// Formats `P2pDecodeError::PayloadTooLarge`, escaping non-printable bytes.
+fn fmt_payload_too_large(
+  command: &CommandString,
+  size: &usize,
+  max: &usize,
+  f: &mut fmt::Formatter<'_>,
+) -> fmt::Result {
+  write!(f, "payload too large: {size} bytes, max {max}, command ")?;
+  write_command(command.as_bytes(), f)
+}
+
+/// Writes command bytes up to the first NUL, escaping non-printable bytes.
+fn write_command(bytes: &[u8; 12], f: &mut fmt::Formatter<'_>) -> fmt::Result {
   for &b in bytes.iter().take_while(|&&b| b != 0) {
     if b.is_ascii_graphic() || b == b' ' {
       write!(f, "{}", b as char)?;

pkgs/p2p_core/src/lib.rs

@@ -31,6 +31,7 @@ pub mod __private {
   pub use dash_types;
 }
 pub use bip324::{decode_v2, encode_v2};
+pub use codec::MAX_P2P_PAYLOAD_SIZE;
 pub use command::CommandString;
 pub use error::P2pDecodeError;
 pub use magic::Magic;