fix(rpc-json)!: resolve Core 23 nested platform addresses (breaking field/accessor renames)

[lklimek]

Warning

Breaking change. Public rpc-json API changes on DMNState / DMNStateDiff: the raw platform_p2p_port / platform_http_port fields are renamed legacy_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 public addresses field is added to both structs. See Breaking changes for the consumer migration.

Why this PR exists

• Problem: rpc-json's DMNState / DMNStateDiff model only the deprecated top-level platformP2PPort / platformHTTPPort keys. With DEPLOYMENT_V24 active, Dash Core 23.1.x returns ProTx version-3 masternodes whose protx listdiff diff entries (updatedMNs) carry legacy platformP2PPort: 0 / platformHTTPPort: 0, while the real ports live only in the new nested addresses object (core_p2p / platform_p2p / platform_https, each an array of "host:port"). rpc-json ignores addresses and so reports the port as 0.

• What breaks — confirmed live on devnet: drive-abci builds its masternode list by applying these diffs incrementally; the legacy 0 overwrites the stored port → the masternode's platform_p2p_port becomes 0 → it is captured into the validator at quorum creation (platform_types/validator/v0/mod.rs — a validator is dropped only on None, not on 0, so the 0 survives and is cast to u16 0) → the validator is advertised to Tenderdash with port 0 → validator-set divergence / consensus connectivity failure. Reproduced on devnet paloma (Core 23.1.3):

  proTxHash:        63aa0d6fcbe1229ce844a91f74e913f56b9a700ca281948cd303195e2e05757c
  version:          3
  platformP2PPort:  0          ← legacy top-level, zeroed
  platformHTTPPort: 0          ← legacy top-level, zeroed
  addresses.platform_p2p:   ["68.67.122.87:36656"]   ← real port, present
  addresses.platform_https: ["68.67.122.87:1443"]    ← real port, present
  

  The bad value first enters via a state diff (e.g. a PoSe-revival update); the full-state addedMNs entry still reconstructs the legacy port correctly, which is why only the incremental diff path is affected.

• Impact by network:

  • Affected now: any platform network with DEPLOYMENT_V24 active — i.e. devnets (paloma, active since 2025-07-01). The development line (v3.1-dev, rust-dashcore pin dev) is actively broken there. The v3.0.2 release (pin v0.41.0) is byte-identical in the DMN region — equally code-vulnerable.
  • Mainnet — latent, not yet triggerable: Core hard-codes mainnet (and testnet) DEPLOYMENT_V24.nStartTime = NEVER_ACTIVE (verified in chainparams.cpp for v23.0.0 and v23.1.3), so no mainnet masternode can be version: 3. Verified empirically: a mainnet protx listdiff version 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 new addresses object land raw — and the legacy-vs-new resolution lives in explicit accessor methods:

• New MasternodeAddresses { core_p2p, platform_p2p, platform_https: Vec<String> } (serde-default per field; tolerant of unknown purposes), exposed as addresses on both DMNState and DMNStateDiff.
• The deprecated raw fields are renamed legacy_platform_p2p_port / legacy_platform_http_port (serde wire renames platformP2PPort / platformHTTPPort intact) so callers reach for the accessors instead of the deprecated fields.
• New accessors return a full (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)>
  • Rule: prefer the addresses entry (host + port verbatim) when its port is valid (non-zero, in-range); otherwise, on DMNState, fall back to the legacy port paired with the node IP from service (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 via legacy_platform_*_port.
• Zero ports never surface: 0 is dropped in both the addresses path and the legacy fallback.
• Ports parse through u16 then widen to u32, rejecting out-of-range values (e.g. "host:70000"). IPv4/IPv6-safe (rsplit_once(':')). Serialize output unchanged.
• DMNStateDiff::addresses is Option<Option<MasternodeAddresses>> (mirrors pose_ban_height): None = unchanged, Some(None) = cleared, Some(Some(_)) = set. compare_to_newer_dmn_state encodes a clear as Some(None) and apply_diff applies the inner value, so a Some → None transition survives the compare→apply round-trip. A custom serde helper distinguishes absent / null / object on the wire.

Testing

• RED-first TDD, assertions go through the accessors: legacy-only → legacy port paired with node IP; legacy 0 + addresses → addresses port (the exact devnet failure shape); legacy 0 + no addresses → None; out-of-range port → None; apply_diff propagates addresses into a stored DMNState; Some → None addresses clear survives the round-trip; raw fields remain the untouched wire values.
• 13/13 rpc-json lib tests green. cargo fmt + cargo clippy --workspace --all-targets --all-features -- -D warnings clean.

Breaking changes

• Adds a public addresses field to DMNState (Option<MasternodeAddresses>) and DMNStateDiff (Option<Option<MasternodeAddresses>>). Downstream struct literals must add the field or use ... No wire/serialize change; legacy deserialization is unchanged.
• The raw fields platform_p2p_port / platform_http_port are renamed legacy_platform_p2p_port / legacy_platform_http_port on both DMNState and DMNStateDiff (serde wire names unchanged).
• The resolver accessors are renamed and now return an address pair: resolved_platform_p2p_port() / resolved_platform_http_port() → platform_p2p_address() / platform_http_address() returning Option<(String, u32)>.
• Consumer action required: to get the correct port (or full address) for Core-23 (ProTx v3) entries, callers must use the new platform_p2p_address() / platform_http_address() accessors instead of reading the raw legacy_platform_*_port fields. The raw fields intentionally still reflect the wire value (which is 0 for v3 entries).

Checklist

• RED→GREEN tests, fmt, clippy -D warnings clean
• Backward-compatible deserialization (legacy wire fields unchanged; plain derive)
• Root cause confirmed against live devnet data (Core 23.1.3, paloma)
• Self-review + bot-review comments addressed (accessors return (host, port); clearable nested-addresses diff; zero-port → None; port-range hardening; apply_diff regression test)
• Reviewer confirmation

_{🤖 Co-authored by Claudius the Magnificent AI Agent}

[coderabbitai]

Warning

Review limit reached

@lklimek, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 56 minutes and 51 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more credits in the billing tab to continue.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: c0d2d441-5233-42c6-b698-a036901b79ee

📥 Commits

Reviewing files that changed from the base of the PR and between 981e97f and b4d77bd.

📒 Files selected for processing (1)

• rpc-json/src/lib.rs

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/rpc-json-core23-platform-addresses

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[codecov]

Codecov Report

❌ Patch coverage is 98.34983% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.03%. Comparing base (981e97f) to head (b4d77bd).

Files with missing lines	Patch %	Lines
rpc-json/src/lib.rs	98.34%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              dev     #808      +/-   ##
==========================================
+ Coverage   72.97%   73.03%   +0.05%     
==========================================
  Files         323      323              
  Lines       71989    72278     +289     
==========================================
+ Hits        52534    52786     +252     
- Misses      19455    19492      +37     

Flag	Coverage Δ
core	76.75% <ø> (ø)
ffi	46.28% <ø> (-1.12%)	⬇️
rpc	32.90% <98.34%> (+12.89%)	⬆️
spv	90.19% <ø> (-0.03%)	⬇️
wallet	71.64% <ø> (ø)

Files with missing lines	Coverage Δ
rpc-json/src/lib.rs	64.44% <98.34%> (+24.11%)	⬆️

... and 20 files with indirect coverage changes

[thepastaclaw]

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.

[lklimek]

@coderabbitai review all

[coderabbitai]

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-dashcore

Length of output: 23990

---

---

@lklimek — Full review of PR #808 below.

---

Overall Assessment

This 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 Option<Option<_>> diff encoding is correct, and the tests exercise every meaningful case including the exact devnet failure shape. A few minor items are worth addressing before merging.

---

Issues Found

1. Legacy port fallback has no range validation (minor)

File: rpc-json/src/lib.rs — DMNState::legacy_platform_address

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 addresses path rejects out-of-range ports via port.parse::<u16>(), but the legacy path only filters 0. A wire value of, say, 70000 would be accepted and returned. This is not a regression (the old code was the same), but the inconsistency is now visible because the two paths sit next to each other. Suggest adding p <= u32::from(u16::MAX) to the filter, or reusing the same u16-bounded parse helper:

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. DMNStateDiff direct Deserialize can't distinguish null (clear) from absent (unchanged)

File: rpc-json/src/lib.rs — DMNStateDiff

DMNStateDiff derives Deserialize without the deserialize_addresses_2opt helper:

#[derive(Clone, PartialEq, Eq, Debug, Deserialize)]
pub struct DMNStateDiff {
    // ...
    pub addresses: Option<Option<MasternodeAddresses>>,  // no deserialize_with here
}

Under the plain derived Deserialize, a JSON null for addresses yields None (outer), not Some(None) — the three-state semantics are lost. The production path goes through DMNStateDiffIntermediate which has the correct custom deserializer, so there is no runtime bug. But the #[derive(Deserialize)] on DMNStateDiff is misleading — a caller who deserializes a DMNStateDiff directly from wire JSON (e.g., a future test or integration path) will silently get wrong semantics for a null clear.

Options:

• Add #[serde(default, deserialize_with = "deserialize_addresses_2opt")] to DMNStateDiff.addresses as well, or
• Add a // NOTE: deserialize via DMNStateDiffIntermediate comment on the type to document the intended path, or
• If direct deserialization is never intended, remove Deserialize from DMNStateDiff entirely.

---

3. first_valid_host_port is a static method but doesn't use the receiver (nitpick)

impl MasternodeAddresses {
    fn first_valid_host_port(addrs: &[String]) -> Option<(String, u32)> { ... }
}

This is an associated function (no self) that only uses its argument. Moving it to a free function alongside parse_host_port would be more idiomatic, and the two parsing utilities would sit together. Not a blocking concern.

---

Positive Notes

• rsplit_once(':') for IPv4/bracketed-IPv6 safety is the right approach and is well-documented.
• Some(newer.addresses.clone()) in compare_to_newer_dmn_state correctly wraps None as Some(None) for the clear case.
• if let Some(addresses) = addresses { self.addresses = addresses; } in apply_diff cleanly handles all three states of Option<Option<_>>.
• #[deprecated] on the raw DMNStateDiff fields guides consumers toward the accessors without a hard break — good ergonomics.
• The deserialize_addresses_2opt helper is the canonical double-Option serde pattern and is correctly paired with #[serde(default)].
• Test coverage is excellent: all nine cases, including the exact devnet failure shape (platformP2PPort: 0 + addresses present) and the Some → None clear round-trip.
• No zero port ever surfaces from any accessor — both paths are guarded.

---

Summary Table

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]

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.