diff --git a/dash/Cargo.toml b/dash/Cargo.toml index a6574f853..22d697b9b 100644 --- a/dash/Cargo.toml +++ b/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" diff --git a/dash/src/blockdata/transaction/outpoint.rs b/dash/src/blockdata/transaction/outpoint.rs index d596a8fd6..46095882f 100644 --- a/dash/src/blockdata/transaction/outpoint.rs +++ b/dash/src/blockdata/transaction/outpoint.rs @@ -337,6 +337,113 @@ mod tests { assert!(tx.out_point_buffer(1).is_none()); } + /// Regression test for the bug where `OutPoint::deserialize` errored with + /// "invalid type: map, expected an OutPoint" when an OutPoint-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 a map — which the previous string-only HR visitor rejected. + #[cfg(feature = "serde")] + #[test] + fn serde_round_trip_through_internally_tagged_enum() { + use serde_derive::{Deserialize, Serialize}; + + #[derive(Debug, PartialEq, Serialize, Deserialize)] + struct WithOutPoint { + out_point: OutPoint, + } + + #[derive(Debug, PartialEq, Serialize, Deserialize)] + #[serde(tag = "type")] + enum Tagged { + A(WithOutPoint), + } + + let original = Tagged::A(WithOutPoint { + out_point: OutPoint { + txid: "5df6e0e2761359d30a8275058e299fcc0381534545f55cf43e41983f5d4c9456" + .parse() + .unwrap(), + vout: 7, + }, + }); + + // 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 the OutPoint as a 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 struct/map form of the OutPoint inside the tagged + // enum and deserialize from it. This is the exact shape that triggered + // the original bug downstream (`platform_value::Value` produces struct + // shapes for OutPoint because it is non-human-readable, and the tagged + // enum then replays those through `ContentDeserializer` with + // `is_human_readable() == true`). Before the fix this failed with + // `invalid type: map, expected an OutPoint`. + let map_form = serde_json::json!({ + "type": "A", + "out_point": { + "txid": "5df6e0e2761359d30a8275058e299fcc0381534545f55cf43e41983f5d4c9456", + "vout": 7, + }, + }); + let from_map: Tagged = serde_json::from_value(map_form).unwrap(); + assert_eq!(original, from_map); + + // The canonical HR string form must still deserialize, so existing + // JSON producers do not break. + let from_string: OutPoint = serde_json::from_str( + "\"5df6e0e2761359d30a8275058e299fcc0381534545f55cf43e41983f5d4c9456:7\"", + ) + .unwrap(); + assert_eq!( + from_string, + OutPoint { + txid: "5df6e0e2761359d30a8275058e299fcc0381534545f55cf43e41983f5d4c9456" + .parse() + .unwrap(), + vout: 7, + }, + ); + + // Plain bincode (non-human-readable) round-trip of an `OutPoint` + // must still succeed via the struct-shape branch — guards against + // breaking the `visit_seq` path. (Note: a bincode round-trip of the + // tagged enum above is *not* possible in serde at all — internally- + // tagged enum dispatch requires `deserialize_any` on the upstream + // deserializer, and bincode is not self-describing. That's a serde + // limitation orthogonal to this bug.) + let raw = OutPoint { + txid: "5df6e0e2761359d30a8275058e299fcc0381534545f55cf43e41983f5d4c9456" + .parse() + .unwrap(), + vout: 7, + }; + let cfg = bincode::config::standard(); + let bytes = bincode::serde::encode_to_vec(raw, cfg).unwrap(); + let (decoded, _): (OutPoint, _) = bincode::serde::decode_from_slice(&bytes, cfg).unwrap(); + assert_eq!(raw, decoded); + + // Duplicate field in the map form must error with `duplicate field`, + // not silently keep the last value. Parse from a JSON string (rather + // than `serde_json::json!`) because `Value::Object` deduplicates keys + // on construction — the duplicate must reach the visitor as separate + // map entries, which only happens during streaming parse. + let err = serde_json::from_str::( + r#"{"txid":"5df6e0e2761359d30a8275058e299fcc0381534545f55cf43e41983f5d4c9456","txid":"0000000000000000000000000000000000000000000000000000000000000000","vout":7}"#, + ) + .unwrap_err(); + assert!( + err.to_string().contains("duplicate field"), + "expected duplicate-field error, got: {err}", + ); + } + // #[test] // fn out_point_parse() { // let mut tx = Transaction { diff --git a/dash/src/serde_utils.rs b/dash/src/serde_utils.rs index 7110c9678..cf8048501 100644 --- a/dash/src/serde_utils.rs +++ b/dash/src/serde_utils.rs @@ -358,6 +358,20 @@ pub(crate) use {serde_string_deserialize_impl, serde_string_impl, serde_string_s /// A combination macro where the human-readable serialization is done like /// serde_string_impl and the non-human-readable impl is done as a struct. +/// +/// The deserialize impl uses a *single* visitor that handles all three shapes +/// (`visit_str`, `visit_seq`, `visit_map`) — 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 +/// 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 can therefore be replayed into the +/// human-readable branch as a map and must be accepted there. See the +/// regression test +/// `outpoint::tests::serde_round_trip_through_internally_tagged_enum`. macro_rules! serde_struct_human_string_impl { ($name:ident, $expecting:literal, $($fe:ident),*) => ( impl<'de> $crate::serde::Deserialize<'de> for $name { @@ -365,136 +379,134 @@ macro_rules! serde_struct_human_string_impl { where D: $crate::serde::de::Deserializer<'de>, { - if deserializer.is_human_readable() { - use core::fmt::{self, Formatter}; - use core::str::FromStr; - - struct Visitor; - impl<'de> $crate::serde::de::Visitor<'de> for Visitor { - type Value = $name; + use core::fmt::{self, Formatter}; + use core::str::FromStr; + use $crate::serde::de::IgnoredAny; - fn expecting(&self, f: &mut Formatter) -> fmt::Result { - f.write_str($expecting) - } + #[allow(non_camel_case_types)] + enum Enum { Unknown__Field, $($fe),* } - fn visit_str(self, v: &str) -> Result - where - E: $crate::serde::de::Error, - { - $name::from_str(v).map_err(E::custom) - } + struct EnumVisitor; + impl<'de> $crate::serde::de::Visitor<'de> for EnumVisitor { + type Value = Enum; + fn expecting(&self, f: &mut Formatter) -> fmt::Result { + f.write_str("a field name") } - deserializer.deserialize_str(Visitor) - } else { - use core::fmt::{self, Formatter}; - use $crate::serde::de::IgnoredAny; - - #[allow(non_camel_case_types)] - enum Enum { Unknown__Field, $($fe),* } - - struct EnumVisitor; - impl<'de> $crate::serde::de::Visitor<'de> for EnumVisitor { - type Value = Enum; - - fn expecting(&self, f: &mut Formatter) -> fmt::Result { - f.write_str("a field name") - } - - fn visit_str(self, v: &str) -> Result - where - E: $crate::serde::de::Error, - { - match v { - $( - stringify!($fe) => Ok(Enum::$fe) - ),*, - _ => Ok(Enum::Unknown__Field) - } + fn visit_str(self, v: &str) -> Result + where + E: $crate::serde::de::Error, + { + match v { + $( + stringify!($fe) => Ok(Enum::$fe) + ),*, + _ => Ok(Enum::Unknown__Field) } } + } - impl<'de> $crate::serde::Deserialize<'de> for Enum { - fn deserialize(deserializer: D) -> Result - where - D: $crate::serde::de::Deserializer<'de>, - { - deserializer.deserialize_str(EnumVisitor) - } + impl<'de> $crate::serde::Deserialize<'de> for Enum { + fn deserialize(deserializer: D) -> Result + where + D: $crate::serde::de::Deserializer<'de>, + { + deserializer.deserialize_str(EnumVisitor) } + } - struct Visitor; - - impl<'de> $crate::serde::de::Visitor<'de> for Visitor { - type Value = $name; + struct Visitor; - fn expecting(&self, f: &mut Formatter) -> fmt::Result { - f.write_str("a struct") - } + impl<'de> $crate::serde::de::Visitor<'de> for Visitor { + type Value = $name; - fn visit_seq(self, mut seq: V) -> Result - where - V: $crate::serde::de::SeqAccess<'de>, - { - use $crate::serde::de::Error; + fn expecting(&self, f: &mut Formatter) -> fmt::Result { + f.write_str($expecting) + } - let length = 0; - $( - let $fe = seq.next_element()?.ok_or_else(|| { - Error::invalid_length(length, &self) - })?; - #[allow(unused_variables)] - let length = length + 1; - )* - - let ret = $name { - $($fe),* - }; + fn visit_str(self, v: &str) -> Result + where + E: $crate::serde::de::Error, + { + $name::from_str(v).map_err(E::custom) + } - Ok(ret) - } + fn visit_seq(self, mut seq: V) -> Result + where + V: $crate::serde::de::SeqAccess<'de>, + { + use $crate::serde::de::Error; + + let length = 0; + $( + let $fe = seq.next_element()?.ok_or_else(|| { + Error::invalid_length(length, &self) + })?; + #[allow(unused_variables)] + let length = length + 1; + )* + + let ret = $name { + $($fe),* + }; + + Ok(ret) + } - fn visit_map(self, mut map: A) -> Result - where - A: $crate::serde::de::MapAccess<'de>, - { - use $crate::serde::de::Error; + fn visit_map(self, mut map: A) -> Result + where + A: $crate::serde::de::MapAccess<'de>, + { + use $crate::serde::de::Error; - $(let mut $fe = None;)* + $(let mut $fe = None;)* - loop { - match map.next_key::()? { - Some(Enum::Unknown__Field) => { - map.next_value::()?; - } - $( - Some(Enum::$fe) => { - $fe = Some(map.next_value()?); - } - )* - None => { break; } + loop { + match map.next_key::()? { + Some(Enum::Unknown__Field) => { + map.next_value::()?; } + $( + Some(Enum::$fe) => { + if $fe.is_some() { + return Err(A::Error::duplicate_field(stringify!($fe))); + } + $fe = Some(map.next_value()?); + } + )* + None => { break; } } + } - $( - let $fe = match $fe { - Some(x) => x, - None => return Err(A::Error::missing_field(stringify!($fe))), - }; - )* - - let ret = $name { - $($fe),* + $( + let $fe = match $fe { + Some(x) => x, + None => return Err(A::Error::missing_field(stringify!($fe))), }; + )* - Ok(ret) - } + let ret = $name { + $($fe),* + }; + + Ok(ret) } - // end type defs + } + // end type defs - static FIELDS: &'static [&'static str] = &[$(stringify!($fe)),*]; + static FIELDS: &'static [&'static str] = &[$(stringify!($fe)),*]; + if deserializer.is_human_readable() { + // Self-describing format (raw JSON, or a `ContentDeserializer` + // replaying buffered content for an internally-tagged enum). + // `deserialize_any` lets the deserializer dispatch to whichever + // visit_* method matches the actual data shape — so we accept + // the canonical "txid:vout" string form, plus the struct/seq + // forms that show up when a non-human-readable struct is + // re-deserialized via `ContentDeserializer`. + deserializer.deserialize_any(Visitor) + } else { deserializer.deserialize_struct(stringify!($name), FIELDS, Visitor) } }