Skip to content

Commit cab6d7d

Browse files
shumkovclaude
shumkov
and
claude
authored
fix(dashcore): make OutPoint serde tolerant of ContentDeserializer's HR-quirk (#708)
* fix(dashcore): make `OutPoint` serde survive ContentDeserializer replays `OutPoint`'s `Deserialize` impl branched on `is_human_readable()` and used two unrelated visitors — a string-only `StringVisitor` for HR and a struct-only `StructVisitor` for non-HR. Serde's `ContentDeserializer` (used internally for `#[serde(tag = "...")]`, `flatten`, and untagged enums) always reports `is_human_readable() == true` regardless of the upstream format, so a value originally produced as a non-HR struct could be replayed into the HR branch as a map and fail with `invalid type: map, expected an OutPoint`. Fix the `serde_struct_human_string_impl!` macro to use a single visitor that accepts all three input shapes (`visit_str`, `visit_seq`, `visit_map`) and use `deserialize_any` in the HR branch so the actual content shape is picked up regardless of what `is_human_readable` reports. The non-HR branch keeps `deserialize_struct` so bincode (which doesn't support `deserialize_any`) still works. The serialize side is unchanged. The trade-off: raw JSON now also accepts the struct/seq forms in addition to the canonical \`\"txid:vout\"\` string form, and tolerates unknown fields in the struct form. This is intentional and required — internally-tagged enums replay the entire buffered content (including the tag field) when resolving each variant, so \`visit_map\` must accept unknown fields. Canonical JSON output is unchanged because the serialize side still emits the string form. Add a regression test that round-trips an \`OutPoint\` through an internally-tagged enum via \`serde_json::Value\` (forcing the \`ContentDeserializer\` path) and through bincode. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(test): drop bincode round-trip from OutPoint serde regression test The dev-dependency on `bincode` doesn't enable the `serde` feature, so `bincode::serde::encode_to_vec` is not in scope. The non-human-readable struct path is already exercised by the existing dashcore test suite which uses bincode extensively, so the regression test only needs to lock in the `ContentDeserializer` behavior — kept the JSON-via-tagged- enum and string-form assertions. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(dashcore): restore bincode side of OutPoint serde tagged-enum test Adding `features = ["serde"]` to the bincode dev-dep so the regression test can exercise the symmetric non-human-readable struct path through the tagged enum, not just the human-readable JSON path. Reword the macro doc comment so it frames the underlying issue as serde's `ContentDeserializer` HR-quirk rather than implying a bug specific to dashcore — the same sharp edge bites every custom `Deserialize` that uses `is_human_readable()` for shape dispatch and the upstream stance is "accept any shape, don't branch". Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(dashcore): use plain OutPoint bincode round-trip instead of tagged A bincode round-trip *via* a `#[serde(tag = ...)]` enum can't work — serde internally-tagged dispatch buffers content by calling `deserialize_any` on the upstream deserializer, and bincode is not self-describing (`Serde(AnyNotSupported)`). That limitation is orthogonal to the bug this PR fixes — the original failing case in dashpay/platform routes through `platform_value::Value`, which *does* support `deserialize_any`. Replace the bogus bincode-tagged-enum assertion with a plain `OutPoint` bincode round-trip, which still locks the non-human-readable struct path (`visit_seq`). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(test): drop needless borrow flagged by clippy `bincode::serde::encode_to_vec` takes the value by `Serialize` bound and `OutPoint` is `Copy`, so the `&raw` borrow is redundant (`clippy::needless_borrows_for_generic_args`). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * review: add map-form regression test and reject duplicate fields Address CodeRabbit review feedback on the OutPoint serde fix: 1. Add a test that hand-builds the JSON object/map shape of `OutPoint` inside a `#[serde(tag = ...)]` enum and deserializes from it. This is the exact shape the original bug exhibited downstream — serde serialized as a map (because the upstream format was non-human- readable), then the tagged enum replayed through `ContentDeserializer` into the HR branch where the old string-only visitor errored. The `serde_json::to_value` round-trip was only exercising the visit_str path through `ContentDeserializer` because canonical OutPoint HR serialization is a string. 2. Reject duplicate field keys in `serde_struct_human_string_impl!`'s `visit_map`. Previously a duplicate silently kept the last value; now it errors with `duplicate field` like serde's derive. Add a test covering this — parse from a raw JSON string (not `json!`) because `Value::Object` deduplicates on construction. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 56fe09d commit cab6d7d

2 files changed

Lines changed: 224 additions & 105 deletions

File tree

‎dash/src/blockdata/transaction/outpoint.rs‎

Lines changed: 107 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -337,6 +337,113 @@ mod tests {
337337
assert!(tx.out_point_buffer(1).is_none());
338338
}
339339

340+
/// Regression test for the bug where `OutPoint::deserialize` errored with
341+
/// "invalid type: map, expected an OutPoint" when an OutPoint-bearing
342+
/// struct was wrapped by an internally-tagged enum and round-tripped
343+
/// through serde's intermediate `ContentDeserializer`. `ContentDeserializer`
344+
/// always reports `is_human_readable() == true`, so a value originally
345+
/// produced by a non-human-readable encoder ends up replayed into the HR
346+
/// branch as a map — which the previous string-only HR visitor rejected.
347+
#[cfg(feature = "serde")]
348+
#[test]
349+
fn serde_round_trip_through_internally_tagged_enum() {
350+
use serde_derive::{Deserialize, Serialize};
351+
352+
#[derive(Debug, PartialEq, Serialize, Deserialize)]
353+
struct WithOutPoint {
354+
out_point: OutPoint,
355+
}
356+
357+
#[derive(Debug, PartialEq, Serialize, Deserialize)]
358+
#[serde(tag = "type")]
359+
enum Tagged {
360+
A(WithOutPoint),
361+
}
362+
363+
let original = Tagged::A(WithOutPoint {
364+
out_point: OutPoint {
365+
txid: "5df6e0e2761359d30a8275058e299fcc0381534545f55cf43e41983f5d4c9456"
366+
.parse()
367+
.unwrap(),
368+
vout: 7,
369+
},
370+
});
371+
372+
// Round-trip through serde_json::Value forces serde to buffer the value
373+
// into a Content tree, then replay it through ContentDeserializer when
374+
// resolving the internally-tagged enum variant. The canonical HR form
375+
// serializes the OutPoint as a string, so this exercises the visit_str
376+
// path through ContentDeserializer.
377+
let value = serde_json::to_value(&original).unwrap();
378+
let restored: Tagged = serde_json::from_value(value).unwrap();
379+
assert_eq!(original, restored);
380+
381+
// Hand-build the struct/map form of the OutPoint inside the tagged
382+
// enum and deserialize from it. This is the exact shape that triggered
383+
// the original bug downstream (`platform_value::Value` produces struct
384+
// shapes for OutPoint because it is non-human-readable, and the tagged
385+
// enum then replays those through `ContentDeserializer` with
386+
// `is_human_readable() == true`). Before the fix this failed with
387+
// `invalid type: map, expected an OutPoint`.
388+
let map_form = serde_json::json!({
389+
"type": "A",
390+
"out_point": {
391+
"txid": "5df6e0e2761359d30a8275058e299fcc0381534545f55cf43e41983f5d4c9456",
392+
"vout": 7,
393+
},
394+
});
395+
let from_map: Tagged = serde_json::from_value(map_form).unwrap();
396+
assert_eq!(original, from_map);
397+
398+
// The canonical HR string form must still deserialize, so existing
399+
// JSON producers do not break.
400+
let from_string: OutPoint = serde_json::from_str(
401+
"\"5df6e0e2761359d30a8275058e299fcc0381534545f55cf43e41983f5d4c9456:7\"",
402+
)
403+
.unwrap();
404+
assert_eq!(
405+
from_string,
406+
OutPoint {
407+
txid: "5df6e0e2761359d30a8275058e299fcc0381534545f55cf43e41983f5d4c9456"
408+
.parse()
409+
.unwrap(),
410+
vout: 7,
411+
},
412+
);
413+
414+
// Plain bincode (non-human-readable) round-trip of an `OutPoint`
415+
// must still succeed via the struct-shape branch — guards against
416+
// breaking the `visit_seq` path. (Note: a bincode round-trip of the
417+
// tagged enum above is *not* possible in serde at all — internally-
418+
// tagged enum dispatch requires `deserialize_any` on the upstream
419+
// deserializer, and bincode is not self-describing. That's a serde
420+
// limitation orthogonal to this bug.)
421+
let raw = OutPoint {
422+
txid: "5df6e0e2761359d30a8275058e299fcc0381534545f55cf43e41983f5d4c9456"
423+
.parse()
424+
.unwrap(),
425+
vout: 7,
426+
};
427+
let cfg = bincode::config::standard();
428+
let bytes = bincode::serde::encode_to_vec(raw, cfg).unwrap();
429+
let (decoded, _): (OutPoint, _) = bincode::serde::decode_from_slice(&bytes, cfg).unwrap();
430+
assert_eq!(raw, decoded);
431+
432+
// Duplicate field in the map form must error with `duplicate field`,
433+
// not silently keep the last value. Parse from a JSON string (rather
434+
// than `serde_json::json!`) because `Value::Object` deduplicates keys
435+
// on construction — the duplicate must reach the visitor as separate
436+
// map entries, which only happens during streaming parse.
437+
let err = serde_json::from_str::<OutPoint>(
438+
r#"{"txid":"5df6e0e2761359d30a8275058e299fcc0381534545f55cf43e41983f5d4c9456","txid":"0000000000000000000000000000000000000000000000000000000000000000","vout":7}"#,
439+
)
440+
.unwrap_err();
441+
assert!(
442+
err.to_string().contains("duplicate field"),
443+
"expected duplicate-field error, got: {err}",
444+
);
445+
}
446+
340447
// #[test]
341448
// fn out_point_parse() {
342449
// let mut tx = Transaction {

‎dash/src/serde_utils.rs‎

Lines changed: 117 additions & 105 deletions
Original file line numberDiff line numberDiff line change
@@ -358,143 +358,155 @@ pub(crate) use {serde_string_deserialize_impl, serde_string_impl, serde_string_s
358358

359359
/// A combination macro where the human-readable serialization is done like
360360
/// serde_string_impl and the non-human-readable impl is done as a struct.
361+
///
362+
/// The deserialize impl uses a *single* visitor that handles all three shapes
363+
/// (`visit_str`, `visit_seq`, `visit_map`) — required to interoperate with
364+
/// serde's `ContentDeserializer`, the format-agnostic intermediate buffer
365+
/// serde uses to dispatch internally-tagged enums (`#[serde(tag = "...")]`),
366+
/// `flatten`, and untagged enums. `ContentDeserializer` always reports
367+
/// `is_human_readable() == true` regardless of the upstream format (this is
368+
/// intentional in serde's source: see long-standing upstream issues — the
369+
/// recommended pattern is "don't branch on `is_human_readable()` for shape
370+
/// dispatch — accept any shape"). A value originally written by a
371+
/// non-human-readable encoder can therefore be replayed into the
372+
/// human-readable branch as a map and must be accepted there. See the
373+
/// regression test
374+
/// `outpoint::tests::serde_round_trip_through_internally_tagged_enum`.
361375
macro_rules! serde_struct_human_string_impl {
362376
($name:ident, $expecting:literal, $($fe:ident),*) => (
363377
impl<'de> $crate::serde::Deserialize<'de> for $name {
364378
fn deserialize<D>(deserializer: D) -> Result<$name, D::Error>
365379
where
366380
D: $crate::serde::de::Deserializer<'de>,
367381
{
368-
if deserializer.is_human_readable() {
369-
use core::fmt::{self, Formatter};
370-
use core::str::FromStr;
371-
372-
struct Visitor;
373-
impl<'de> $crate::serde::de::Visitor<'de> for Visitor {
374-
type Value = $name;
382+
use core::fmt::{self, Formatter};
383+
use core::str::FromStr;
384+
use $crate::serde::de::IgnoredAny;
375385

376-
fn expecting(&self, f: &mut Formatter) -> fmt::Result {
377-
f.write_str($expecting)
378-
}
386+
#[allow(non_camel_case_types)]
387+
enum Enum { Unknown__Field, $($fe),* }
379388

380-
fn visit_str<E>(self, v: &str) -> Result<Self::Value, E>
381-
where
382-
E: $crate::serde::de::Error,
383-
{
384-
$name::from_str(v).map_err(E::custom)
385-
}
389+
struct EnumVisitor;
390+
impl<'de> $crate::serde::de::Visitor<'de> for EnumVisitor {
391+
type Value = Enum;
386392

393+
fn expecting(&self, f: &mut Formatter) -> fmt::Result {
394+
f.write_str("a field name")
387395
}
388396

389-
deserializer.deserialize_str(Visitor)
390-
} else {
391-
use core::fmt::{self, Formatter};
392-
use $crate::serde::de::IgnoredAny;
393-
394-
#[allow(non_camel_case_types)]
395-
enum Enum { Unknown__Field, $($fe),* }
396-
397-
struct EnumVisitor;
398-
impl<'de> $crate::serde::de::Visitor<'de> for EnumVisitor {
399-
type Value = Enum;
400-
401-
fn expecting(&self, f: &mut Formatter) -> fmt::Result {
402-
f.write_str("a field name")
403-
}
404-
405-
fn visit_str<E>(self, v: &str) -> Result<Self::Value, E>
406-
where
407-
E: $crate::serde::de::Error,
408-
{
409-
match v {
410-
$(
411-
stringify!($fe) => Ok(Enum::$fe)
412-
),*,
413-
_ => Ok(Enum::Unknown__Field)
414-
}
397+
fn visit_str<E>(self, v: &str) -> Result<Self::Value, E>
398+
where
399+
E: $crate::serde::de::Error,
400+
{
401+
match v {
402+
$(
403+
stringify!($fe) => Ok(Enum::$fe)
404+
),*,
405+
_ => Ok(Enum::Unknown__Field)
415406
}
416407
}
408+
}
417409

418-
impl<'de> $crate::serde::Deserialize<'de> for Enum {
419-
fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
420-
where
421-
D: $crate::serde::de::Deserializer<'de>,
422-
{
423-
deserializer.deserialize_str(EnumVisitor)
424-
}
410+
impl<'de> $crate::serde::Deserialize<'de> for Enum {
411+
fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
412+
where
413+
D: $crate::serde::de::Deserializer<'de>,
414+
{
415+
deserializer.deserialize_str(EnumVisitor)
425416
}
417+
}
426418

427-
struct Visitor;
428-
429-
impl<'de> $crate::serde::de::Visitor<'de> for Visitor {
430-
type Value = $name;
419+
struct Visitor;
431420

432-
fn expecting(&self, f: &mut Formatter) -> fmt::Result {
433-
f.write_str("a struct")
434-
}
421+
impl<'de> $crate::serde::de::Visitor<'de> for Visitor {
422+
type Value = $name;
435423

436-
fn visit_seq<V>(self, mut seq: V) -> Result<Self::Value, V::Error>
437-
where
438-
V: $crate::serde::de::SeqAccess<'de>,
439-
{
440-
use $crate::serde::de::Error;
424+
fn expecting(&self, f: &mut Formatter) -> fmt::Result {
425+
f.write_str($expecting)
426+
}
441427

442-
let length = 0;
443-
$(
444-
let $fe = seq.next_element()?.ok_or_else(|| {
445-
Error::invalid_length(length, &self)
446-
})?;
447-
#[allow(unused_variables)]
448-
let length = length + 1;
449-
)*
450-
451-
let ret = $name {
452-
$($fe),*
453-
};
428+
fn visit_str<E>(self, v: &str) -> Result<Self::Value, E>
429+
where
430+
E: $crate::serde::de::Error,
431+
{
432+
$name::from_str(v).map_err(E::custom)
433+
}
454434

455-
Ok(ret)
456-
}
435+
fn visit_seq<V>(self, mut seq: V) -> Result<Self::Value, V::Error>
436+
where
437+
V: $crate::serde::de::SeqAccess<'de>,
438+
{
439+
use $crate::serde::de::Error;
440+
441+
let length = 0;
442+
$(
443+
let $fe = seq.next_element()?.ok_or_else(|| {
444+
Error::invalid_length(length, &self)
445+
})?;
446+
#[allow(unused_variables)]
447+
let length = length + 1;
448+
)*
449+
450+
let ret = $name {
451+
$($fe),*
452+
};
453+
454+
Ok(ret)
455+
}
457456

458-
fn visit_map<A>(self, mut map: A) -> Result<Self::Value, A::Error>
459-
where
460-
A: $crate::serde::de::MapAccess<'de>,
461-
{
462-
use $crate::serde::de::Error;
457+
fn visit_map<A>(self, mut map: A) -> Result<Self::Value, A::Error>
458+
where
459+
A: $crate::serde::de::MapAccess<'de>,
460+
{
461+
use $crate::serde::de::Error;
463462

464-
$(let mut $fe = None;)*
463+
$(let mut $fe = None;)*
465464

466-
loop {
467-
match map.next_key::<Enum>()? {
468-
Some(Enum::Unknown__Field) => {
469-
map.next_value::<IgnoredAny>()?;
470-
}
471-
$(
472-
Some(Enum::$fe) => {
473-
$fe = Some(map.next_value()?);
474-
}
475-
)*
476-
None => { break; }
465+
loop {
466+
match map.next_key::<Enum>()? {
467+
Some(Enum::Unknown__Field) => {
468+
map.next_value::<IgnoredAny>()?;
477469
}
470+
$(
471+
Some(Enum::$fe) => {
472+
if $fe.is_some() {
473+
return Err(A::Error::duplicate_field(stringify!($fe)));
474+
}
475+
$fe = Some(map.next_value()?);
476+
}
477+
)*
478+
None => { break; }
478479
}
480+
}
479481

480-
$(
481-
let $fe = match $fe {
482-
Some(x) => x,
483-
None => return Err(A::Error::missing_field(stringify!($fe))),
484-
};
485-
)*
486-
487-
let ret = $name {
488-
$($fe),*
482+
$(
483+
let $fe = match $fe {
484+
Some(x) => x,
485+
None => return Err(A::Error::missing_field(stringify!($fe))),
489486
};
487+
)*
490488

491-
Ok(ret)
492-
}
489+
let ret = $name {
490+
$($fe),*
491+
};
492+
493+
Ok(ret)
493494
}
494-
// end type defs
495+
}
496+
// end type defs
495497

496-
static FIELDS: &'static [&'static str] = &[$(stringify!($fe)),*];
498+
static FIELDS: &'static [&'static str] = &[$(stringify!($fe)),*];
497499

500+
if deserializer.is_human_readable() {
501+
// Self-describing format (raw JSON, or a `ContentDeserializer`
502+
// replaying buffered content for an internally-tagged enum).
503+
// `deserialize_any` lets the deserializer dispatch to whichever
504+
// visit_* method matches the actual data shape — so we accept
505+
// the canonical "txid:vout" string form, plus the struct/seq
506+
// forms that show up when a non-human-readable struct is
507+
// re-deserialized via `ContentDeserializer`.
508+
deserializer.deserialize_any(Visitor)
509+
} else {
498510
deserializer.deserialize_struct(stringify!($name), FIELDS, Visitor)
499511
}
500512
}

0 commit comments

Comments
 (0)