fix(hashes): make SerdeHash tolerant of ContentDeserializer's HR-quirk (#729)

* fix(hashes): make SerdeHash tolerant of ContentDeserializer's HR-quirk

This is the same kind of serde-tag incompatibility fixed for `OutPoint`
in #708, applied to the hash-newtype family (sha256, sha256d, hash160,
hash_x11, ripemd160, sha1, sha512 — affecting Txid, BlockHash, ProTxHash,
PubkeyHash, QuorumHash, and every other type generated by `hash_newtype!`
or `serde_impl!`).

`SerdeHash::deserialize` used two separate visitors — a string-only HR
visitor (`HexVisitor`) and a bytes-only non-HR visitor (`BytesVisitor`).
That works fine in isolation but breaks the moment a hash-bearing struct
is wrapped by an internally-tagged enum (`#[serde(tag = "...")]`),
`flatten`, or an untagged enum. Serde routes those through
`ContentDeserializer`, a format-agnostic intermediate buffer that always
reports `is_human_readable() == true` regardless of the upstream format.
A value originally written by a non-HR encoder is therefore replayed
into the HR branch as raw bytes, which the previous `HexVisitor::visit_str`
saw as "32 chars" instead of "64-char hex" and rejected with
`bad hex string length 32 (expected 64)`.

This was hit downstream in dashpay/platform when validators / validator
sets (which contain `ProTxHash`, `PubkeyHash`, `QuorumHash`) were
configured for the dpp `tag = "$formatVersion"` versioning convention.

## Fix

Rework `SerdeHash::deserialize` to use a single `AnyShapeVisitor` that
accepts every shape a hash can arrive in:

- `visit_str` / `visit_borrowed_str` — ASCII hex (canonical HR form).
- `visit_bytes` / `visit_borrowed_bytes` — disambiguated by length:
  exactly `N` bytes → raw hash, exactly `2*N` bytes → UTF-8 hex.
  Any other length is rejected.
- `visit_seq` — length-prefixed `u8` sequence (used by bincode and
  other non-self-describing formats).

Use `deserialize_any` in the HR branch so the actual content shape —
not the reported HR flag — drives dispatch. Keep `deserialize_bytes`
in the non-HR branch since bincode is non-self-describing and does not
support `deserialize_any`.

## Trade-off

Raw JSON now also accepts the byte-form (`"\x11..."` UTF-8 bytes vs.
`"11..."` hex string) because `deserialize_any` in serde_json's
self-describing mode dispatches based on the JSON token. We disambiguate
strictly by length in `visit_bytes`, so anything that's neither N bytes
nor 2*N bytes still errors. This is consistent with the OutPoint fix
in #708 — accept any shape, validate by length.

## Implementation note: no_std / no alloc

`dashcore_hashes` does not enable `serde/alloc` (it has only `serde-std`
which transitively gates that), so `Visitor::visit_byte_buf` and
`visit_string` (defined behind serde's `alloc` feature) are unavailable.
The `visit_seq` path uses a stack array sized to fit the largest hash
(64 bytes — sha512) instead of a `Vec`, keeping the crate's no-alloc
posture.

## Tests

Two new regression tests in `dash/src/hash_types.rs`:

- `serde_round_trip_through_internally_tagged_enum` — wraps a `Txid`
  in a `#[serde(tag = "type")]` enum, round-trips through
  `serde_json::Value` (which forces buffering through
  `ContentDeserializer`), and asserts the round-trip is identity. Also
  verifies the canonical hex-string form still deserializes and that
  bincode round-trip still succeeds via the byte/seq path.
- `serde_round_trip_through_internally_tagged_enum_pubkey_hash` —
  same shape with `PubkeyHash` (20-byte hash) to exercise the
  smaller-length disambiguation path.

`bincode` dev-dep updated to `features = ["serde"]` (same change as #708)
so the bincode regression assertion compiles.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(test): rustfmt + correct comment about N being in bytes

* review: address PR feedback — real visit_seq test + debug_assert overflow

Two in-scope fixes from review:

1. The Txid round-trip test had an abandoned `raw_txid_bytes` literal
   followed by `let _ = raw_txid_bytes; // documentation only` — leftover
   exploration that misled readers into thinking the bytes were used.
   Replace with a real assertion that constructs a `serde_json::Value::Array`
   of u8 numbers, wraps it in a `#[serde(tag = "type")]` enum, and
   round-trips through `serde_json::from_value`. This now actually
   exercises the new `visit_seq` path through `ContentDeserializer` —
   the security review noted that the prior test only hit `visit_str`,
   leaving `visit_bytes`/`visit_seq` regression coverage thin.

2. The `MAX_HASH_BYTES = 64` overflow check in `visit_seq` was returning
   a runtime error with a debug-prose string ("recompile with larger
   MAX") that leaked an internal type name to user error logs. Convert
   to `debug_assert!` — failure mode is now a test panic in debug builds
   (caught at CI time when adding a wider hash type), zero overhead in
   release. The condition is unreachable in any release build that
   compiled at all, since adding a wider digest would require updating
   `serde_impl!` invocations.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: shumkov

dash/Cargo.toml

@@ -67,7 +67,7 @@ serde_json = "1.0.140"
 serde_test = "1.0.177"
 serde_derive = "1.0.219"
 secp256k1 = { features = [ "recovery", "rand", "hashes" ], version="0.30.0" }
-bincode = { version = "2.0.1" }
+bincode = { version = "2.0.1", features = ["serde"] }
 assert_matches = "1.5.0"
 dashcore = { path = ".", features = ["core-block-hash-use-x11", "message_verification", "quorum_validation", "signer"] }
 criterion = "0.5"

dash/src/hash_types.rs

@@ -389,3 +389,117 @@ mod newtypes {
         }
     }
 }
+
+#[cfg(all(test, feature = "serde"))]
+mod tests {
+    use super::*;
+    use serde_derive::{Deserialize, Serialize};
+
+    /// Regression test for the bug where hash newtypes' `Deserialize` errored
+    /// with "bad hex string length 32 (expected 64)" (or similar) when an
+    /// hash-bearing struct was wrapped by an internally-tagged enum and
+    /// round-tripped through serde's intermediate `ContentDeserializer`.
+    /// `ContentDeserializer` always reports `is_human_readable() == true`,
+    /// so a value originally produced by a non-human-readable encoder ends up
+    /// replayed into the HR branch as raw bytes — which the previous
+    /// string-only `HexVisitor` rejected because `from_str` saw 32 UTF-8 chars
+    /// instead of the expected 64-char hex form.
+    #[test]
+    fn serde_round_trip_through_internally_tagged_enum() {
+        #[derive(Debug, PartialEq, Serialize, Deserialize)]
+        struct WithTxid {
+            txid: Txid,
+        }
+
+        #[derive(Debug, PartialEq, Serialize, Deserialize)]
+        #[serde(tag = "type")]
+        enum Tagged {
+            A(WithTxid),
+        }
+
+        let original = Tagged::A(WithTxid {
+            txid: "5df6e0e2761359d30a8275058e299fcc0381534545f55cf43e41983f5d4c9456"
+                .parse()
+                .unwrap(),
+        });
+
+        // Round-trip through serde_json::Value forces serde to buffer the
+        // value into a Content tree, then replay it through
+        // ContentDeserializer when resolving the internally-tagged enum
+        // variant. The canonical HR form serializes a hash as a hex string,
+        // so this exercises the visit_str path through ContentDeserializer.
+        let value = serde_json::to_value(&original).unwrap();
+        let restored: Tagged = serde_json::from_value(value).unwrap();
+        assert_eq!(original, restored);
+
+        // Hand-build the array-of-numbers form of the Txid inside the tagged
+        // enum and deserialize from it. This routes through `ContentDeserializer`
+        // (because of the internally-tagged enum) and exercises the
+        // `visit_seq` path on the unified visitor — the exact shape produced
+        // downstream when a non-human-readable encoder hands hash bytes to a
+        // tagged-enum-bearing context. Before the fix the visitor only had
+        // string/bytes-disjoint visitors and rejected this shape.
+        let raw_txid_bytes: [u8; 32] = [
+            0x56, 0x94, 0x4c, 0x5d, 0x3f, 0x98, 0x41, 0x3e, 0xf4, 0x5c, 0xf5, 0x45, 0x45, 0x53,
+            0x81, 0x03, 0xcc, 0x9f, 0x29, 0x8e, 0x05, 0x75, 0x82, 0x0a, 0xd3, 0x59, 0x13, 0x76,
+            0xe2, 0xe0, 0xf6, 0x5d,
+        ];
+        let arr_value = serde_json::Value::Array(
+            raw_txid_bytes.iter().map(|b| serde_json::Value::Number((*b).into())).collect(),
+        );
+        let map_form = serde_json::json!({
+            "type": "A",
+            "txid": arr_value,
+        });
+        let from_arr: Tagged = serde_json::from_value(map_form).unwrap();
+        assert_eq!(original, from_arr);
+
+        // The canonical HR string form must still deserialize, so existing
+        // JSON producers do not break.
+        let from_string: Txid = serde_json::from_str(
+            "\"5df6e0e2761359d30a8275058e299fcc0381534545f55cf43e41983f5d4c9456\"",
+        )
+        .unwrap();
+        assert_eq!(
+            from_string,
+            "5df6e0e2761359d30a8275058e299fcc0381534545f55cf43e41983f5d4c9456"
+                .parse::<Txid>()
+                .unwrap(),
+        );
+
+        // Plain bincode (non-human-readable) round-trip of a `Txid` must
+        // still succeed via the byte-shape branch — guards against breaking
+        // the `visit_seq` path used by length-prefixed sequence formats.
+        let raw: Txid =
+            "5df6e0e2761359d30a8275058e299fcc0381534545f55cf43e41983f5d4c9456".parse().unwrap();
+        let cfg = bincode::config::standard();
+        let bytes = bincode::serde::encode_to_vec(raw, cfg).unwrap();
+        let (decoded, _): (Txid, _) = bincode::serde::decode_from_slice(&bytes, cfg).unwrap();
+        assert_eq!(raw, decoded);
+    }
+
+    /// 20-byte hash (PubkeyHash) goes through the same path. Smaller hash
+    /// length exercises a different `raw_len_bytes` / `hex_len_bytes`
+    /// disambiguation in the visitor.
+    #[test]
+    fn serde_round_trip_through_internally_tagged_enum_pubkey_hash() {
+        #[derive(Debug, PartialEq, Serialize, Deserialize)]
+        struct WithPubkeyHash {
+            pkh: PubkeyHash,
+        }
+
+        #[derive(Debug, PartialEq, Serialize, Deserialize)]
+        #[serde(tag = "type")]
+        enum Tagged {
+            A(WithPubkeyHash),
+        }
+
+        let original = Tagged::A(WithPubkeyHash {
+            pkh: PubkeyHash::from_hex("e8b43025641eea4fd21190f01bd870ef90f1a8b1").unwrap(),
+        });
+
+        let value = serde_json::to_value(&original).unwrap();
+        let restored: Tagged = serde_json::from_value(value).unwrap();
+        assert_eq!(original, restored);
+    }
+}

hashes/src/serde_macros.rs

@@ -23,60 +23,120 @@ pub mod serde_details {
     use core::{fmt, ops, str};
 
     use crate::Error;
-    struct HexVisitor<ValueT>(PhantomData<ValueT>);
     use serde::{de, Deserializer, Serializer};
 
-    impl<'de, ValueT> de::Visitor<'de> for HexVisitor<ValueT>
+    /// Single visitor that accepts every shape a hash can arrive in: an ASCII
+    /// hex string (`visit_str`), a UTF-8 byte slice that decodes as hex
+    /// (`visit_bytes` of length `2*N` — note `N` is in BYTES per the macro,
+    /// see `serde_impl!` invocation in `internal_macros.rs`), a raw byte
+    /// slice of the hash's length-in-bytes (`visit_bytes` of length `N`),
+    /// or a length-prefixed sequence of `u8` from non-self-describing
+    /// formats (`visit_seq`, used by bincode).
+    ///
+    /// Required to interoperate with serde's `ContentDeserializer`, the
+    /// format-agnostic intermediate buffer serde uses to dispatch
+    /// internally-tagged enums (`#[serde(tag = "...")]`), `flatten`, and
+    /// untagged enums. `ContentDeserializer` always reports
+    /// `is_human_readable() == true` regardless of the upstream format. This
+    /// is intentional in serde's source — see long-standing upstream issues;
+    /// the maintainers consider it working-as-intended and the recommended
+    /// pattern is **"don't branch on `is_human_readable()` for shape dispatch
+    /// — accept any shape."** A value originally written by a
+    /// non-human-readable encoder (raw bytes) can therefore be replayed into
+    /// the human-readable branch as bytes / a byte-buf and must be accepted
+    /// there. See the regression tests in
+    /// `dash/src/hash_types.rs::serde_round_trip_through_internally_tagged_enum`.
+    struct AnyShapeVisitor<ValueT>(PhantomData<ValueT>);
+
+    impl<'de, ValueT> de::Visitor<'de> for AnyShapeVisitor<ValueT>
     where
-        ValueT: FromStr,
+        ValueT: SerdeHash,
         <ValueT as FromStr>::Err: fmt::Display,
     {
         type Value = ValueT;
 
         fn expecting(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
-            formatter.write_str("an ASCII hex string")
+            formatter.write_str("an ASCII hex string or a byte string of the hash length")
         }
 
-        fn visit_bytes<E>(self, v: &[u8]) -> Result<Self::Value, E>
+        fn visit_str<E>(self, v: &str) -> Result<Self::Value, E>
         where
             E: de::Error,
         {
-            if let Ok(hex) = str::from_utf8(v) {
-                Self::Value::from_str(hex).map_err(E::custom)
-            } else {
-                Err(E::invalid_value(de::Unexpected::Bytes(v), &self))
-            }
+            Self::Value::from_str(v).map_err(E::custom)
         }
 
-        fn visit_str<E>(self, v: &str) -> Result<Self::Value, E>
+        fn visit_borrowed_str<E>(self, v: &'de str) -> Result<Self::Value, E>
         where
             E: de::Error,
         {
             Self::Value::from_str(v).map_err(E::custom)
         }
-    }
-
-    struct BytesVisitor<ValueT>(PhantomData<ValueT>);
-
-    impl<'de, ValueT> de::Visitor<'de> for BytesVisitor<ValueT>
-    where
-        ValueT: SerdeHash,
-        <ValueT as FromStr>::Err: fmt::Display,
-    {
-        type Value = ValueT;
 
-        fn expecting(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
-            formatter.write_str("a bytestring")
+        fn visit_bytes<E>(self, v: &[u8]) -> Result<Self::Value, E>
+        where
+            E: de::Error,
+        {
+            // Disambiguate by length. A correctly-sized raw hash byte string
+            // is exactly `N` bytes (`N` is in bytes per the macro — see
+            // `serde_impl!` invocation in `internal_macros.rs`); a hex-encoded
+            // form of that hash is `2*N` ASCII bytes. Any other length is
+            // rejected.
+            let raw_len_bytes = ValueT::N;
+            let hex_len_bytes = raw_len_bytes * 2;
+            if v.len() == raw_len_bytes {
+                SerdeHash::from_slice_delegated(v)
+                    .map_err(|_| E::invalid_length(v.len(), &stringify!(N)))
+            } else if v.len() == hex_len_bytes {
+                if let Ok(hex) = str::from_utf8(v) {
+                    Self::Value::from_str(hex).map_err(E::custom)
+                } else {
+                    Err(E::invalid_value(de::Unexpected::Bytes(v), &self))
+                }
+            } else {
+                Err(E::invalid_length(v.len(), &self))
+            }
         }
 
-        fn visit_bytes<E>(self, v: &[u8]) -> Result<Self::Value, E>
+        fn visit_borrowed_bytes<E>(self, v: &'de [u8]) -> Result<Self::Value, E>
         where
             E: de::Error,
         {
-            SerdeHash::from_slice_delegated(v).map_err(|_| {
-                // from_slice only errors on incorrect length
-                E::invalid_length(v.len(), &stringify!(N))
-            })
+            self.visit_bytes(v)
+        }
+
+        fn visit_seq<A>(self, mut seq: A) -> Result<Self::Value, A::Error>
+        where
+            A: de::SeqAccess<'de>,
+        {
+            // Used by bincode and any non-self-describing format that emits a
+            // length-prefixed sequence of u8. Use a stack buffer sized to fit
+            // the largest hash this trait services (sha512 / Hmac<sha512> at
+            // 64 bytes) so we keep `no_std`-compatible (no `Vec`/`alloc`).
+            // Bumping `MAX_HASH_BYTES` is only needed if a wider digest type
+            // is added — `debug_assert!` catches that in tests.
+            const MAX_HASH_BYTES: usize = 64;
+            let raw_len_bytes = ValueT::N;
+            debug_assert!(
+                raw_len_bytes <= MAX_HASH_BYTES,
+                "hash byte-length {} exceeds AnyShapeVisitor stack buffer ({}); bump MAX_HASH_BYTES",
+                raw_len_bytes,
+                MAX_HASH_BYTES,
+            );
+            let mut buf = [0u8; MAX_HASH_BYTES];
+            let mut len: usize = 0;
+            while let Some(b) = seq.next_element::<u8>()? {
+                if len == raw_len_bytes {
+                    return Err(de::Error::invalid_length(len + 1, &stringify!(N)));
+                }
+                buf[len] = b;
+                len += 1;
+            }
+            if len != raw_len_bytes {
+                return Err(de::Error::invalid_length(len, &stringify!(N)));
+            }
+            SerdeHash::from_slice_delegated(&buf[..len])
+                .map_err(|_| de::Error::invalid_length(len, &stringify!(N)))
         }
     }
 
@@ -90,7 +150,7 @@ pub mod serde_details {
             + ops::Index<ops::RangeFull, Output = [u8]>,
         <Self as FromStr>::Err: fmt::Display,
     {
-        /// Size, in bits, of the hash.
+        /// Size of the hash, in bytes.
         const N: usize;
 
         /// Helper function to turn a deserialized slice into the correct hash type.
@@ -106,11 +166,22 @@ pub mod serde_details {
         }
 
         /// Do serde deserialization.
+        ///
+        /// Uses a single visitor that accepts every shape a hash can arrive
+        /// in (ASCII hex string, raw byte slice, length-prefixed `u8`
+        /// sequence). The HR branch dispatches via `deserialize_any` to
+        /// handle both true human-readable deserializers (where the visitor
+        /// receives `visit_str`) and serde's `ContentDeserializer` (which
+        /// reports `is_human_readable() == true` even when wrapping bytes
+        /// from a non-HR source — internally-tagged enums, `flatten`, and
+        /// untagged enums all route through it). The non-HR branch keeps
+        /// `deserialize_bytes` because bincode is non-self-describing and
+        /// does not support `deserialize_any`.
         fn deserialize<'de, D: Deserializer<'de>>(d: D) -> Result<Self, D::Error> {
             if d.is_human_readable() {
-                d.deserialize_str(HexVisitor::<Self>(PhantomData))
+                d.deserialize_any(AnyShapeVisitor::<Self>(PhantomData))
             } else {
-                d.deserialize_bytes(BytesVisitor::<Self>(PhantomData))
+                d.deserialize_bytes(AnyShapeVisitor::<Self>(PhantomData))
             }
         }
     }