fix(rpc-json)!: resolve Core 23 nested platform addresses (breaking field/accessor renames)#808
Open
lklimek wants to merge 9 commits into
Open
fix(rpc-json)!: resolve Core 23 nested platform addresses (breaking field/accessor renames)#808lklimek wants to merge 9 commits into
lklimek wants to merge 9 commits into
Conversation
Dash Core 23.1 moved the platform P2P/HTTPS ports out of the deprecated top-level platformP2PPort/platformHTTPPort keys into a nested `addresses` object whose `platform_p2p`/`platform_https` sub-keys hold "host:port" arrays. Without reading them, platform_p2p_port deserialized to None on Core-23 entries, dropping or mis-porting validators downstream. Add MasternodeAddresses and backfill the existing port fields from the addresses object when the legacy port is absent or zero, via a private DMNStateIntermediate (mirroring DMNStateDiffIntermediate) and inside the existing TryFrom for DMNStateDiff. Legacy pre-23 JSON is untouched. Co-Authored-By: Claude Opus 4.8 (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 as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR extends ChangesMasternode Platform Address Resolution
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #808 +/- ##
==========================================
+ Coverage 72.94% 73.13% +0.18%
==========================================
Files 323 323
Lines 71989 72283 +294
==========================================
+ Hits 52511 52861 +350
+ Misses 19478 19422 -56
|
…erialize backfill Match the plain-derive style of the rest of the crate: deserialize DMNState and DMNStateDiff with no logic in the deserialize path. The nested Core 23 `addresses` object now lands as a plain public field instead of being folded into the legacy port fields during deserialization. Port resolution moves into accessor methods on both DMNState and DMNStateDiff: resolved_platform_p2p_port / resolved_platform_http_port. They check the new `addresses` object first (first non-zero, parseable port) and fall back to the deprecated top-level platformP2PPort/platformHTTPPort — new-first, legacy-fallback. Removed: the DMNStateIntermediate mirror struct and its `#[serde(from)]`, the `backfill_port` helper, and the in-deserialize backfill blocks in both From/TryFrom impls. DMNState regains its plain `#[derive(Deserialize)]`; DMNStateDiff keeps its pre-existing `try_from` with `addresses` flowing through as raw data. apply_diff now propagates `addresses` so a stored DMNState stays resolvable after diffs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
lklimek
commented
Jun 12, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Contributor
There was a problem hiding this comment.
Code Review
PR cleanly addresses Core 23+ nested platform port resolution via a new MasternodeAddresses mirror plus new-first/legacy-fallback accessors on DMNState/DMNStateDiff. Two in-scope suggestions: the addresses diff round-trip cannot clear nested addresses (compare emits None, apply ignores None), and there is no end-to-end test for apply_diff propagating addresses — the very path that motivates the PR. One nitpick on parse_port port-range validation.
🟡 2 suggestion(s) | 💬 1 nitpick(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `rpc-json/src/lib.rs`:
- [SUGGESTION] rpc-json/src/lib.rs:2375-2446: Diff round-trip cannot clear nested `addresses`
`compare_to_newer_dmn_state` sets `has_diff = true` and stores `newer.addresses.clone()` whenever `self.addresses != newer.addresses` (lines 2375-2380). When the transition is `Some(_) -> None`, the resulting `DMNStateDiff.addresses` is `None`, which is indistinguishable from "no change". `apply_diff` then guards with `if addresses.is_some()` (lines 2444-2446), so the stored `addresses` is never cleared and the compare→apply round-trip silently loses the transition. Because `resolved_platform_p2p_port()`/`resolved_platform_http_port()` prefer nested addresses over legacy fields, stale ports can persist after the round-trip. The same `Option<T>` pattern is reused for `platform_p2p_port`/`platform_http_port` and has the same limitation, but it is more impactful now that port resolution depends on `addresses`. Either model `addresses` as `Option<Option<MasternodeAddresses>>` (matching `pose_ban_height` / `operator_payout_address`) so absence and clearing are distinct, or document the limitation and add a test pinning the chosen behavior.
- [SUGGESTION] rpc-json/src/lib.rs:2389-2447: No test exercises `apply_diff` propagating `addresses` (the path the PR exists to fix)
The PR's stated production failure is `protx listdiff updatedMNs` entries with `platformP2PPort: 0` + nested `addresses` being merged into a long-lived stored `DMNState`. The added tests at lines 3540-3653 cover (1) `DMNState` deserialization, (2) `DMNStateDiff` deserialization, and (3) accessor resolution rules in isolation — but none call `DMNState::apply_diff` with a diff carrying `addresses` and then assert `resolved_platform_p2p_port()` on the merged state. Add a regression test: start with a stored `DMNState` (legacy ports `Some(0)`, `addresses: None`), apply a `DMNStateDiff { addresses: Some(...platform_p2p: ["host:36656"]...), .. }`, and assert `resolved_platform_p2p_port() == Some(36656)`. Without it, a future refactor of `apply_diff` could silently drop `addresses` propagation again and reintroduce the exact devnet incident.
…learable diff Address self-review on the Core 23 platform-address work. Rename the raw legacy fields on DMNState and DMNStateDiff to legacy_platform_p2p_port / legacy_platform_http_port (serde wire renames intact) so callers reach for the accessors instead of the deprecated fields. The resolver accessors become platform_p2p_address / platform_http_address and return Option<(String, u32)> instead of a bare port. On DMNState the legacy fallback pairs the port with the node IP from `service`, since Dash deploys platform services on the masternode's core IP; on DMNStateDiff there is no node IP to pair with, so the legacy path is not resolved. Zero ports never surface: they are dropped in both the addresses path and the legacy fallback. Ports parse through u16 then widen to u32, rejecting out-of-range values like "host:70000". DMNStateDiff::addresses becomes Option<Option<MasternodeAddresses>>, mirroring pose_ban_height: None = unchanged, Some(None) = cleared, Some(Some(_)) = set. A custom serde helper distinguishes absent / null / object on the wire. compare_to_newer_dmn_state now encodes a Some->None clear as Some(None) and apply_diff applies the inner value, so the clear survives the round-trip. Tests cover diff addresses propagation through apply_diff, the Some->None clear round-trip, zero-port-to-None, and out-of-range rejection. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…resses' into fix/rpc-json-core23-platform-addresses
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
lklimek
changed the title
Contributor
Author
|
@coderabbitai review all |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Get the list of changed files in this PR
gh pr view 808 --repo dashpay/rust-dashcore --json files --jq '.files[].path'Length of output: 197 🏁 Script executed: #!/bin/bash
# Get the diff for this PR
gh pr diff 808 --repo dashpay/rust-dashcoreLength of output: 23990
Overall AssessmentThis is a well-structured fix. The root cause is clearly identified, the design (bare-data deserialization layer + explicit accessor resolution) is appropriate, the three-state Issues Found1. Legacy port fallback has no range validation (minor)File: fn legacy_platform_address(&self, port: Option<u32>) -> Option<(String, u32)> {
port.filter(|&p| p != 0).map(|p| (self.service.ip().to_string(), p))
}The fn legacy_platform_address(&self, port: Option<u32>) -> Option<(String, u32)> {
port.filter(|&p| p != 0 && p <= u32::from(u16::MAX))
.map(|p| (self.service.ip().to_string(), p))
}2.
|
| Item | Severity | Required? |
|---|---|---|
Legacy port not range-validated in legacy_platform_address |
Minor | Suggested |
DMNStateDiff direct Deserialize loses three-state semantics for null |
Minor | Suggested |
first_valid_host_port as static vs free function |
Nitpick | Optional |
The first two are worth addressing for correctness/clarity. The rest is approval-ready.
thepastaclaw
left a comment
thepastaclaw
left a comment
Contributor
There was a problem hiding this comment.
Code Review
Targeted, well-scoped fix for the Core 23 nested-addresses regression. The accessor-based design correctly preserves the wire shape and the three-state Option<Option<MasternodeAddresses>> round-trips through compare/apply. Verified findings are all non-blocking: a #[deprecated] annotation asymmetry between DMNState and DMNStateDiff (introduced by c427684), a legacy-port accessor that bypasses the documented in-range invariant, a missing wire-level test for addresses: null, and a doc/behavior mismatch in parse_host_port for unbracketed IPv6.
🟡 3 suggestion(s) | 💬 1 nitpick(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `rpc-json/src/lib.rs`:
- [SUGGESTION] rpc-json/src/lib.rs:2121-2124: Mirror `#[deprecated]` on DMNState legacy port fields
Commit c4276848 added `#[deprecated(note = "...")]` to `DMNStateDiff::legacy_platform_p2p_port`/`legacy_platform_http_port` (2181-2184) but the same-named fields on `DMNState` (2121-2124) are left bare. Both hold the identical wire value and require the same migration (call `platform_p2p_address()` / `platform_http_address()` instead). The asymmetry is the worst of both worlds: a consumer reading `state.legacy_platform_p2p_port` gets no compiler nudge while a consumer reading the equivalent diff field does. `DMNState` is the more commonly-held type for downstream callers, so this defeats the migration signal where it matters most. Either deprecate both or neither.
- [SUGGESTION] rpc-json/src/lib.rs:2159-2161: Legacy port fallback bypasses the accessor's in-range invariant
`platform_p2p_address` / `platform_http_address` document that they return non-zero in-range ports, and the nested-addresses path enforces this by parsing through `u16`. The legacy fallback at `legacy_platform_address` only filters out `0`, so a wire value like `platformP2PPort: 70000` is returned as `Some((service_ip, 70000))`. Callers that trust the accessor contract and narrow the result to `u16` would silently truncate, reintroducing exactly the class of bad endpoint values these accessors are meant to prevent. Fold the legacy port through `u16::try_from` so both paths share one invariant.
- [SUGGESTION] rpc-json/src/lib.rs:3388-3400: Add a wire-level test for `addresses: null` deserialization
`deserialize_addresses_2opt` exists specifically to distinguish absent (None = unchanged), JSON `null` (Some(None) = cleared), and an object (Some(Some(_)) = set). The added tests cover absent (`dmn_state_legacy_*`) and object (`dmn_state_core23_*`), and exercise `Some(None)` only programmatically through `compare_to_newer_dmn_state` → `apply_diff` (`dmn_state_diff_clears_addresses`). The wire-format path the helper was written for — a `DMNStateDiff` JSON payload with `"addresses": null` — is not asserted. A future refactor (e.g. swapping the custom helper for plain `#[serde(default)]`, which would regress to None = unchanged) would not fail any test, and the bug would only surface when stored `addresses` silently stayed populated. A one-shot deserialize assertion locks the contract.
…DMNState fields Address a fresh review pass on the Core 23 platform-address work. - Mirror the `#[deprecated]` annotation onto DMNState::legacy_platform_p2p_port / legacy_platform_http_port so both DMNState and DMNStateDiff signal the same deprecation; add `#[allow(deprecated)]` to the two accessor methods, the apply_diff write sites, and the tests that read the fields to keep `clippy -D warnings` green. - legacy_platform_address now validates through `u16::try_from`, so an out-of-range legacy port (e.g. platformP2PPort: 70000) yields None instead of Some((ip, 70000)), matching the addresses path and the accessor's documented in-range contract. - parse_host_port rejects unbracketed multi-colon (bare IPv6) hosts rather than silently mangling them via rsplit_once; bracketed IPv6 and IPv4 still parse. The doc now states the supported forms accurately. Tests: wire-level `addresses: null` -> Some(None) (and absent -> None) round trip, out-of-range legacy port rejection, and parse_host_port IPv6 handling. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
rpc-json/src/lib.rs (1)
3032-3035: ⚡ Quick winMirror the legacy-port deprecation on the public intermediate type.
DMNStateDiffIntermediateis public, so consumers can still use these raw legacy fields without the warning they get onDMNState/DMNStateDiff. Add the same#[deprecated]notes here, with scoped#[allow(deprecated)]at internal conversion sites if needed.Proposed update
+ #[deprecated(note = "Core 23+ nested addresses.platform_p2p should be used instead")] #[serde(default, rename = "platformP2PPort")] pub legacy_platform_p2p_port: Option<u32>, + #[deprecated(note = "Core 23+ nested addresses.platform_https should be used instead")] #[serde(default, rename = "platformHTTPPort")] pub legacy_platform_http_port: Option<u32>,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rpc-json/src/lib.rs` around lines 3032 - 3035, Add deprecation warnings to the legacy port fields in `DMNStateDiffIntermediate` to match the deprecation already applied to similar fields in `DMNState`/`DMNStateDiff`. Add `#[deprecated]` attributes to both `legacy_platform_p2p_port` and `legacy_platform_http_port` fields with appropriate deprecation messages. Then identify any internal code that constructs or uses these fields from `DMNStateDiffIntermediate` and wrap those usages with `#[allow(deprecated)]` scoped attributes to suppress the warnings at conversion sites.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rpc-json/src/lib.rs`:
- Around line 2398-2418: The legacy_platform_p2p_port and
legacy_platform_http_port fields in the diff comparison (at rpc-json/src/lib.rs
lines 2398-2418 and 2479-2490) are storing None when values differ, but
apply_diff treats None as unchanged, leaving stale legacy ports in the state. To
fix this, either adopt the same clearable diff shape used for the addresses
field by changing the type to Option<Option<u32>> with matching serde handling
to support wire-level null clearing, or if legacy-port clears are intentionally
unsupported, add an explicit regression test documenting this limitation. Apply
the chosen solution consistently across both affected locations.
- Around line 2088-2094: The `parse_host_port` function currently accepts empty
host strings (e.g., ":36656" returns Some(("", 36656))), which should be
rejected as invalid input. Add a validation check after extracting the host from
the rsplit_once call to ensure the host is not empty, returning None if it is.
Place this check alongside the existing IPv6 bracket validation logic in the
function to treat empty hosts the same as malformed input.
---
Nitpick comments:
In `@rpc-json/src/lib.rs`:
- Around line 3032-3035: Add deprecation warnings to the legacy port fields in
`DMNStateDiffIntermediate` to match the deprecation already applied to similar
fields in `DMNState`/`DMNStateDiff`. Add `#[deprecated]` attributes to both
`legacy_platform_p2p_port` and `legacy_platform_http_port` fields with
appropriate deprecation messages. Then identify any internal code that
constructs or uses these fields from `DMNStateDiffIntermediate` and wrap those
usages with `#[allow(deprecated)]` scoped attributes to suppress the warnings at
conversion sites.
🪄 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: 69ac54fd-1649-46a7-adfc-d0764dc3d36f
📒 Files selected for processing (1)
rpc-json/src/lib.rs
…ediate legacy ports Addresses CodeRabbit review on PR #808: - parse_host_port now rejects empty hosts (":36656" -> None) - DMNStateDiffIntermediate legacy_platform_*_port mirror the #[deprecated] of DMNState/DMNStateDiff Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
2 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.
Warning
Breaking change. Public
rpc-jsonAPI changes onDMNState/DMNStateDiff: the rawplatform_p2p_port/platform_http_portfields are renamedlegacy_platform_p2p_port/legacy_platform_http_port; the resolver accessors are renamed and now return(host, port)(platform_p2p_address()/platform_http_address() -> Option<(String, u32)>); a new publicaddressesfield is added to both structs. See Breaking changes for the consumer migration.Why this PR exists
Problem:
rpc-json'sDMNState/DMNStateDiffmodel only the deprecated top-levelplatformP2PPort/platformHTTPPortkeys. WithDEPLOYMENT_V24active, Dash Core 23.1.x returns ProTx version-3 masternodes whoseprotx listdiffdiff entries (updatedMNs) carry legacyplatformP2PPort: 0/platformHTTPPort: 0, while the real ports live only in the new nestedaddressesobject (core_p2p/platform_p2p/platform_https, each an array of"host:port").rpc-jsonignoresaddressesand so reports the port as0.What breaks — confirmed live on devnet:
drive-abcibuilds its masternode list by applying these diffs incrementally; the legacy0overwrites the stored port → the masternode'splatform_p2p_portbecomes0→ it is captured into the validator at quorum creation (platform_types/validator/v0/mod.rs— a validator is dropped only onNone, not on0, so the0survives and is cast tou16 0) → the validator is advertised to Tenderdash with port0→ validator-set divergence / consensus connectivity failure. Reproduced on devnet paloma (Core 23.1.3):The bad value first enters via a state diff (e.g. a PoSe-revival update); the full-state
addedMNsentry still reconstructs the legacy port correctly, which is why only the incremental diff path is affected.Impact by network:
DEPLOYMENT_V24active — i.e. devnets (paloma, active since 2025-07-01). The development line (v3.1-dev, rust-dashcore pindev) is actively broken there. Thev3.0.2release (pinv0.41.0) is byte-identical in the DMN region — equally code-vulnerable.DEPLOYMENT_V24.nStartTime = NEVER_ACTIVE(verified inchainparams.cppfor v23.0.0 and v23.1.3), so no mainnet masternode can beversion: 3. Verified empirically: a mainnetprotx listdiffversion histogram is{v1: 1789, v2: 1152, v3: 0}, all 350 Evo nodes carrying correct non-zero ports. This fix must be live in the mainnet release before a future Core schedules a real mainnet V24 activation, or the first v3 masternode upgrade will start corrupting validator ports.What was done
This crate keeps RPC structs as a plain data-mapping layer, so deserialization stays a bare
#[derive(Deserialize)]with no logic in the deserialize path — the legacy fields and the newaddressesobject land raw — and the legacy-vs-new resolution lives in explicit accessor methods:MasternodeAddresses { core_p2p, platform_p2p, platform_https: Vec<String> }(serde-default per field; tolerant of unknown purposes), exposed asaddresseson bothDMNStateandDMNStateDiff.legacy_platform_p2p_port/legacy_platform_http_port(serde wire renamesplatformP2PPort/platformHTTPPortintact) so callers reach for the accessors instead of the deprecated fields.(host, port)pair, new-first / legacy-fallback:DMNState::platform_p2p_address(&self) -> Option<(String, u32)>DMNState::platform_http_address(&self) -> Option<(String, u32)>DMNStateDiff::platform_p2p_address(&self) -> Option<(String, u32)>DMNStateDiff::platform_http_address(&self) -> Option<(String, u32)>addressesentry (host + port verbatim) when its port is valid (non-zero, in-range); otherwise, onDMNState, fall back to the legacy port paired with the node IP fromservice(Dash deploys platform services on the masternode's core IP). A diff carries no node IP, so its accessors do not resolve the legacy port — read it vialegacy_platform_*_port.0is dropped in both theaddressespath and the legacy fallback.u16then widen tou32, rejecting out-of-range values (e.g."host:70000"). IPv4/IPv6-safe (rsplit_once(':')). Serialize output unchanged.DMNStateDiff::addressesisOption<Option<MasternodeAddresses>>(mirrorspose_ban_height):None= unchanged,Some(None)= cleared,Some(Some(_))= set.compare_to_newer_dmn_stateencodes a clear asSome(None)andapply_diffapplies the inner value, so aSome → Nonetransition survives the compare→apply round-trip. A custom serde helper distinguishes absent /null/ object on the wire.Testing
0+addresses→ addresses port (the exact devnet failure shape); legacy0+ noaddresses→None; out-of-range port →None;apply_diffpropagatesaddressesinto a storedDMNState;Some → Noneaddressesclear survives the round-trip; raw fields remain the untouched wire values.13/13rpc-json lib tests green.cargo fmt+cargo clippy --workspace --all-targets --all-features -- -D warningsclean.Breaking changes
addressesfield toDMNState(Option<MasternodeAddresses>) andDMNStateDiff(Option<Option<MasternodeAddresses>>). Downstream struct literals must add the field or use... No wire/serialize change; legacy deserialization is unchanged.platform_p2p_port/platform_http_portare renamedlegacy_platform_p2p_port/legacy_platform_http_porton bothDMNStateandDMNStateDiff(serde wire names unchanged).resolved_platform_p2p_port()/resolved_platform_http_port()→platform_p2p_address()/platform_http_address()returningOption<(String, u32)>.platform_p2p_address()/platform_http_address()accessors instead of reading the rawlegacy_platform_*_portfields. The raw fields intentionally still reflect the wire value (which is0for v3 entries).Checklist
fmt,clippy -D warningsclean(host, port); clearable nested-addressesdiff; zero-port →None; port-range hardening;apply_diffregression test)🤖 Co-authored by Claudius the Magnificent AI Agent
Summary by CodeRabbit