fix(dashcore): Address<NetworkChecked> serde deserialize no longer hardcodes Mainnet#657
Open
QuantumExplorer wants to merge 2 commits intov0.42-devfrom
Open
fix(dashcore): Address<NetworkChecked> serde deserialize no longer hardcodes Mainnet#657QuantumExplorer wants to merge 2 commits intov0.42-devfrom
QuantumExplorer wants to merge 2 commits intov0.42-devfrom
Conversation
…rdcodes Mainnet `Address<NetworkChecked>::deserialize` was using `addr_unchecked.require_network(Network::Mainnet)`, which silently broke every testnet/regtest/devnet round-trip: - Any testnet `Address` serialized via serde would fail to deserialize with a "network mismatch" error. - Structs that embed `Address<NetworkChecked>` in serde-derived types (e.g. `InputDetail` in `key-wallet`'s `TransactionRecord`) would fail to decode on non-mainnet networks, and the failure cascaded as "skip entire TransactionRecord" for any consumer that log-and-skipped decode errors. The existing doc comment on the impl literally admitted it: "This is a limitation of deserializing without network context. Users should use Address<NetworkUnchecked> for serde when the network is not known at compile time." Meanwhile the native bincode `Decode` impl for `Address` at address.rs:864-874 already does the right thing — it decodes to `Address<NetworkUnchecked>` and calls `assume_checked()`. Align the serde impl with that behavior so the two serialization paths agree and `TransactionRecord` round-trips cleanly regardless of network. Callers that need actual network validation should deserialize as `Address<NetworkUnchecked>` and call `require_network(N)` explicitly. Flagged as a critical bug by rust-quality-engineer during the holistic review of Phase 10 Item 6c (evo-tool's `wallet_transactions.record` bincode-serde persistence path). Without this fix, Item 6c is broken on every non-mainnet deployment. 29 dashcore address tests pass. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThe change fixes deserialization logic for Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v0.42-dev #657 +/- ##
=============================================
+ Coverage 68.08% 68.12% +0.03%
=============================================
Files 319 319
Lines 67646 67700 +54
=============================================
+ Hits 46057 46120 +63
+ Misses 21589 21580 -9
|
ZocoLini
previously approved these changes
Apr 18, 2026
Collaborator
ZocoLini
left a comment
ZocoLini
left a comment
There was a problem hiding this comment.
quick question about the serde/bincode situation, is there any reason to keep bincode?? library users can use bincode::serde module to serialize/deserialize stuff through the serde framework. is there any serialization feature builtin bincode that is not available through bincode::serde that we must support?? or this is more about retrocompability??
xdustinface
previously approved these changes
Apr 18, 2026
Collaborator
xdustinface
left a comment
xdustinface
left a comment
There was a problem hiding this comment.
Maybe add a test for it?
Member
Author
|
@ZocoLini all of platform consensus uses bincode and not serde. There are actually a lot of historical reasons for it, and I don't regret this decision either. bincode 2 is independent of serde. |
…serialize Pin the fix from the previous commit with four focused tests: - `serde_deserialize_network_checked_testnet_round_trip`: a testnet `Address<NetworkChecked>` survives a serde round-trip. Before the fix this failed with a "network mismatch" error. - `serde_deserialize_network_checked_devnet_round_trip`: same for the logical devnet/regtest case. Raw bytes round-trip even though the `NetworkChecked` side cannot prove which network the caller had in mind. - `serde_deserialize_network_checked_agrees_with_bincode_decode`: the serde and native `assume_checked()` paths must produce equal addresses; guards against a future "tighten serde with a hardcoded network" regression. - `serde_deserialize_network_unchecked_require_network_still_enforces`: callers who want network validation still opt in via `Address<NetworkUnchecked>` + `require_network(..)`, and that path must still reject mismatches. Verified that reverting the fix causes the first three tests to fail with the exact pre-fix error; the fourth correctly keeps passing because the opt-in validation path is unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
QuantumExplorer
dismissed stale reviews from xdustinface and ZocoLini
via
5a41e78
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
dash/src/address.rs (1)
2148-2166: Construct an actual devnet/regtest address in this test.Line 2157 parses the same
y-prefixed address as testnet, andFromStrmaps that prefix toNetwork::Testnet, so this does not exercise aNetwork::DevnetorNetwork::Regtestvalue.🧪 Proposed test adjustment
- fn serde_deserialize_network_checked_devnet_round_trip() { - let original: Address = - Address::from_str("yWZBnVvSxS5xSq27dHVAJpuqbt7vvwGFL1").unwrap().assume_checked(); + fn serde_deserialize_network_checked_shared_prefix_round_trip() { + let payload = Address::<NetworkUnchecked>::from_str("yWZBnVvSxS5xSq27dHVAJpuqbt7vvwGFL1") + .unwrap() + .payload() + .clone(); - let json = serde_json::to_string(&original).expect("serialize"); - let decoded: Address = serde_json::from_str(&json).expect("deserialize NetworkChecked"); + for network in [Network::Devnet, Network::Regtest] { + let original: Address = Address::new(network, payload.clone()); + let json = serde_json::to_string(&original).expect("serialize"); + let decoded: Address = serde_json::from_str(&json).expect("deserialize NetworkChecked"); - // Round-trip preserves the raw address bytes (and thus script_pubkey) - // even though the `NetworkChecked` side cannot prove which network the - // caller had in mind. - assert_eq!(decoded.script_pubkey(), original.script_pubkey()); + // Round-trip preserves the raw address bytes (and thus script_pubkey) + // even though the `NetworkChecked` side cannot prove which network the + // caller had in mind. + assert_eq!(decoded.to_string(), original.to_string()); + assert_eq!(decoded.script_pubkey(), original.script_pubkey()); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash/src/address.rs` around lines 2148 - 2166, The test serde_deserialize_network_checked_devnet_round_trip currently parses a y-prefixed string that FromStr maps to Network::Testnet, so update the test to actually produce an Address on Network::Devnet or Network::Regtest: either use a canonical devnet/regtest base58 address string (one whose prefix maps to Network::Devnet/Network::Regtest) when calling Address::from_str, or construct the Address directly from the underlying script/pubkey and set the network to Network::Devnet/Network::Regtest before calling assume_checked(), so the decoded round-trip truly exercises Network::Devnet/Network::Regtest handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dash/src/address.rs`:
- Around line 2168-2189: The test
serde_deserialize_network_checked_agrees_with_bincode_decode currently only
compares serde against assume_checked() and never exercises bincode; update the
test to also perform a bincode roundtrip: for each unchecked
Address::<NetworkUnchecked>, call
bincode::serialize(&unchecked.clone().assume_checked()) and
bincode::deserialize::<Address>(&bytes) to get via_bincode, then assert equality
of via_bincode.to_string() and script_pubkey() against both via_serde and
via_assume_checked; use the existing symbols Address, NetworkUnchecked,
assume_checked, serde_json and bincode::serialize/deserialize and keep unwraps
for test failures.
---
Nitpick comments:
In `@dash/src/address.rs`:
- Around line 2148-2166: The test
serde_deserialize_network_checked_devnet_round_trip currently parses a
y-prefixed string that FromStr maps to Network::Testnet, so update the test to
actually produce an Address on Network::Devnet or Network::Regtest: either use a
canonical devnet/regtest base58 address string (one whose prefix maps to
Network::Devnet/Network::Regtest) when calling Address::from_str, or construct
the Address directly from the underlying script/pubkey and set the network to
Network::Devnet/Network::Regtest before calling assume_checked(), so the decoded
round-trip truly exercises Network::Devnet/Network::Regtest handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 42bf03b5-a7a5-48eb-b60a-db3fdc467fd1
📒 Files selected for processing (1)
dash/src/address.rs
Comment on lines
+2168
to
+2189
| /// Serde round-trip must agree with the native bincode `Decode` impl. | ||
| /// Both paths now use `assume_checked()`; this test guards the invariant | ||
| /// so a future "tighten serde with a hardcoded network" regression would | ||
| /// trip here rather than silently diverging. | ||
| #[test] | ||
| #[cfg(feature = "serde")] | ||
| fn serde_deserialize_network_checked_agrees_with_bincode_decode() { | ||
| let testnet_strings = [ | ||
| "yWZBnVvSxS5xSq27dHVAJpuqbt7vvwGFL1", // P2PKH | ||
| ]; | ||
| for s in testnet_strings { | ||
| let unchecked = Address::<NetworkUnchecked>::from_str(s).unwrap(); | ||
|
|
||
| let json = serde_json::to_string(&unchecked.clone().assume_checked()).unwrap(); | ||
| let via_serde: Address = serde_json::from_str(&json).unwrap(); | ||
|
|
||
| let via_assume_checked: Address = unchecked.assume_checked(); | ||
|
|
||
| assert_eq!(via_serde.to_string(), via_assume_checked.to_string()); | ||
| assert_eq!(via_serde.script_pubkey(), via_assume_checked.script_pubkey()); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Show whether the claimed bincode parity test actually invokes bincode APIs.
rg -n -C4 'serde_deserialize_network_checked_agrees_with_bincode_decode|bincode::(encode_to_vec|decode_from_slice)' dash/src/address.rsRepository: dashpay/rust-dashcore
Length of output: 488
🏁 Script executed:
# Check for bincode dependency and features in Cargo.toml files
find dash -name "Cargo.toml" -type f | head -5 | xargs grep -l "bincode" 2>/dev/nullRepository: dashpay/rust-dashcore
Length of output: 80
🏁 Script executed:
# Look for existing bincode usage patterns in the codebase to verify the API
rg -n "bincode::(encode_to_vec|decode_from_slice|encode|decode)" dash/src --type rs -B2 -A2Repository: dashpay/rust-dashcore
Length of output: 91
🏁 Script executed:
# Check if there are other tests that use both serde and bincode features
rg -n "#\[cfg.*all.*serde.*bincode" dash/src --type rsRepository: dashpay/rust-dashcore
Length of output: 91
🏁 Script executed:
# Check the feature flags defined in the Cargo.toml
fd "Cargo.toml" dash | xargs grep -A30 "\[features\]"Repository: dashpay/rust-dashcore
Length of output: 1636
🏁 Script executed:
# Search for bincode usage patterns (corrected rg syntax)
rg "bincode::" dash/src/address.rs -A2Repository: dashpay/rust-dashcore
Length of output: 2056
🏁 Script executed:
# Check bincode version requirement
grep -A5 "^bincode" dash/Cargo.tomlRepository: dashpay/rust-dashcore
Length of output: 817
🏁 Script executed:
# Look for any encode_to_vec or decode_from_slice patterns in other tests
rg "encode_to_vec|decode_from_slice" dash/srcRepository: dashpay/rust-dashcore
Length of output: 1955
Exercise bincode to actually verify parity.
This test claims to verify parity with the native bincode Decode implementation but never invokes bincode—it only compares serde against assume_checked(). A future bincode divergence would pass this test silently.
Add bincode encode/decode roundtrip and compare all three paths (serde, bincode, and assume_checked()) to provide meaningful coverage:
Proposed fix
#[test]
- #[cfg(feature = "serde")]
+ #[cfg(all(feature = "serde", feature = "bincode"))]
fn serde_deserialize_network_checked_agrees_with_bincode_decode() {
let testnet_strings = [
"yWZBnVvSxS5xSq27dHVAJpuqbt7vvwGFL1", // P2PKH
];
for s in testnet_strings {
let unchecked = Address::<NetworkUnchecked>::from_str(s).unwrap();
let json = serde_json::to_string(&unchecked.clone().assume_checked()).unwrap();
let via_serde: Address = serde_json::from_str(&json).unwrap();
let via_assume_checked: Address = unchecked.assume_checked();
+ let encoded =
+ bincode::encode_to_vec(&via_assume_checked, bincode::config::standard()).unwrap();
+ let (via_bincode, _): (Address, usize) =
+ bincode::decode_from_slice(&encoded, bincode::config::standard()).unwrap();
- assert_eq!(via_serde.to_string(), via_assume_checked.to_string());
- assert_eq!(via_serde.script_pubkey(), via_assume_checked.script_pubkey());
+ assert_eq!(via_bincode.to_string(), via_assume_checked.to_string());
+ assert_eq!(via_bincode.script_pubkey(), via_assume_checked.script_pubkey());
+ assert_eq!(via_serde.to_string(), via_bincode.to_string());
+ assert_eq!(via_serde.script_pubkey(), via_bincode.script_pubkey());
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /// Serde round-trip must agree with the native bincode `Decode` impl. | |
| /// Both paths now use `assume_checked()`; this test guards the invariant | |
| /// so a future "tighten serde with a hardcoded network" regression would | |
| /// trip here rather than silently diverging. | |
| #[test] | |
| #[cfg(feature = "serde")] | |
| fn serde_deserialize_network_checked_agrees_with_bincode_decode() { | |
| let testnet_strings = [ | |
| "yWZBnVvSxS5xSq27dHVAJpuqbt7vvwGFL1", // P2PKH | |
| ]; | |
| for s in testnet_strings { | |
| let unchecked = Address::<NetworkUnchecked>::from_str(s).unwrap(); | |
| let json = serde_json::to_string(&unchecked.clone().assume_checked()).unwrap(); | |
| let via_serde: Address = serde_json::from_str(&json).unwrap(); | |
| let via_assume_checked: Address = unchecked.assume_checked(); | |
| assert_eq!(via_serde.to_string(), via_assume_checked.to_string()); | |
| assert_eq!(via_serde.script_pubkey(), via_assume_checked.script_pubkey()); | |
| } | |
| } | |
| /// Serde round-trip must agree with the native bincode `Decode` impl. | |
| /// Both paths now use `assume_checked()`; this test guards the invariant | |
| /// so a future "tighten serde with a hardcoded network" regression would | |
| /// trip here rather than silently diverging. | |
| #[test] | |
| #[cfg(all(feature = "serde", feature = "bincode"))] | |
| fn serde_deserialize_network_checked_agrees_with_bincode_decode() { | |
| let testnet_strings = [ | |
| "yWZBnVvSxS5xSq27dHVAJpuqbt7vvwGFL1", // P2PKH | |
| ]; | |
| for s in testnet_strings { | |
| let unchecked = Address::<NetworkUnchecked>::from_str(s).unwrap(); | |
| let json = serde_json::to_string(&unchecked.clone().assume_checked()).unwrap(); | |
| let via_serde: Address = serde_json::from_str(&json).unwrap(); | |
| let via_assume_checked: Address = unchecked.assume_checked(); | |
| let encoded = | |
| bincode::encode_to_vec(&via_assume_checked, bincode::config::standard()).unwrap(); | |
| let (via_bincode, _): (Address, usize) = | |
| bincode::decode_from_slice(&encoded, bincode::config::standard()).unwrap(); | |
| assert_eq!(via_bincode.to_string(), via_assume_checked.to_string()); | |
| assert_eq!(via_bincode.script_pubkey(), via_assume_checked.script_pubkey()); | |
| assert_eq!(via_serde.to_string(), via_bincode.to_string()); | |
| assert_eq!(via_serde.script_pubkey(), via_bincode.script_pubkey()); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dash/src/address.rs` around lines 2168 - 2189, The test
serde_deserialize_network_checked_agrees_with_bincode_decode currently only
compares serde against assume_checked() and never exercises bincode; update the
test to also perform a bincode roundtrip: for each unchecked
Address::<NetworkUnchecked>, call
bincode::serialize(&unchecked.clone().assume_checked()) and
bincode::deserialize::<Address>(&bytes) to get via_bincode, then assert equality
of via_bincode.to_string() and script_pubkey() against both via_serde and
via_assume_checked; use the existing symbols Address, NetworkUnchecked,
assume_checked, serde_json and bincode::serialize/deserialize and keep unwraps
for test failures.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Address<NetworkChecked>::deserializewas usingaddr_unchecked.require_network(Network::Mainnet), silently breaking every testnet / regtest / devnet serde round-trip. Any testnetAddressserialized via serde failed to deserialize with a "network mismatch" error, and structs that embedAddress<NetworkChecked>in serde-derived types (e.g.InputDetailinkey-wallet'sTransactionRecord) failed to decode on non-mainnet — cascading as "skip entire TransactionRecord" for consumers that log-and-skip decode errors.The existing doc comment on the impl admitted the limitation, and the native bincode
Decodeimpl forAddress(address.rs:864-874) already does the right thing — decodes toAddress<NetworkUnchecked>and callsassume_checked(). This change aligns the serde impl with that behavior so the two serialization paths agree.Callers that need actual network validation should deserialize as
Address<NetworkUnchecked>and callrequire_network(N)explicitly.Extracted from
Cherry-picked from
feat/platform-wallet2(draft PR #655). Surfaced as a critical bug during review of a bincode-serde persistence path that storesTransactionRecords on non-mainnet networks.Test plan
cargo build -p dashcore --all-featurescargo test -p dashcore --all-features --lib address— 29 passed / 0 faileddash/src/address.rs, +11 / −4).🤖 Extracted with Claude Code
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests