fix(dashcore): Address<NetworkChecked> serde deserialize no longer hardcodes Mainnet#657
Merged
xdustinface merged 7 commits intov0.42-devfrom Apr 20, 2026
Merged
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
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThe change updates serde deserialization 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.12% 68.18% +0.06%
=============================================
Files 319 319
Lines 67660 67702 +42
=============================================
+ Hits 46093 46163 +70
+ Misses 21567 21539 -28
|
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
`bincode::encode_to_vec<E: Encode>(val: E, cfg)` takes the value by value. Passing `&addr.clone().assume_checked()` wraps it in the `&T: Encode` blanket impl for no reason — clippy's needless_borrows_for_generic_args (now firing on 1.95+) flags it. Address implements Encode directly, so pass it owned. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
xdustinface
reviewed
Apr 19, 2026
| /// this case. | ||
| #[test] | ||
| #[cfg(feature = "serde")] | ||
| fn serde_deserialize_network_checked_devnet_round_trip() { |
Collaborator
There was a problem hiding this comment.
This seems just like a copy of serde_deserialize_network_checked_testnet_round_trip ? I don't see how it tests something specific to devnet. Either adjust the address or drop if there is no difference between devnet/testnet.
Per xdustinface review: the `_devnet_round_trip` test used the same `y`-prefix address and the same round-trip as the testnet test — serde carries raw bytes and doesn't distinguish testnet/regtest/devnet at the codec layer (network awareness happens at `require_network`). Keeping the testnet test plus the bincode-agreement test is sufficient coverage for the fix.
xdustinface
approved these changes
Apr 20, 2026
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
Bug Fixes
Tests