chore: bump rust-dashcore to 64d9267#3936
Conversation
…h fix) Bumps all rust-dashcore workspace deps from 5c0113e7 to 64d9267d, picking up: - key-wallet#813 — honor the account index in the DashPay derivation path (additive/backward-compatible; no platform-side change needed). - rpc-json#808 — Core 23 restructured masternode platform ports: the flat platform_p2p_port / platform_http_port are now legacy_* and a nested `addresses` (MasternodeAddresses) was added. #808 migration (behavior-preserving): drive-abci's masternode/validator state reads the legacy flat port pair, exactly as before — MasternodeStateV0 keeps its own field names, so platform's serialized state is unchanged (no state migration, no consensus change). The Core 23 nested `addresses` are intentionally left unconsumed (DMNState -> MasternodeStateV0 drops them; the reverse sets addresses: None); adopting per-protocol addresses is a separate, reviewable change. Deprecated-field reads are scoped under #[allow(deprecated)] with rationale. Workspace compiles clean (all targets, 0 warnings); masternode identity unit tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughUpdates eight dashcore-family workspace dependency rev pins in Changesdashcore rev bump and DMNState legacy port field migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
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 |
|
✅ Review complete (commit 8e19e2a) |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3936 +/- ##
=============================================
+ Coverage 72.78% 87.19% +14.40%
=============================================
Files 22 2632 +2610
Lines 3054 327764 +324710
=============================================
+ Hits 2223 285786 +283563
- Misses 831 41978 +41147
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/rs-drive-abci/tests/strategy_tests/masternodes.rs`:
- Around line 539-545: The test fixture code increments port values using plain
addition operators on legacy_platform_p2p_port and legacy_platform_http_port,
which can overflow when many updates are applied to the same masternode. Replace
the `*port += 1` operations with saturating or checked arithmetic to prevent
overflow and maintain fixture stability across larger test ranges. Use
saturating_add(1) to safely cap the port values at their maximum rather than
allowing wrapping or panicking.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6cb0cc74-e28a-4349-94c1-3296986b9180
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
Cargo.tomlpackages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_identities/create_operator_identity/v0/mod.rspackages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_identities/create_owner_identity/v0/mod.rspackages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_identities/create_voter_identity/v0/mod.rspackages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_identities/get_operator_identifier/v0/mod.rspackages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_identities/update_operator_identity/v0/mod.rspackages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_list/update_state_masternode_list/v0/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/mod.rspackages/rs-drive-abci/src/platform_types/masternode/v0/mod.rspackages/rs-drive-abci/src/platform_types/validator/v0/mod.rspackages/rs-drive-abci/tests/strategy_tests/masternode_list_item_helpers.rspackages/rs-drive-abci/tests/strategy_tests/masternodes.rs
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Dependency bump pulls in Core 23 DMNState/DMNStateDiff changes where platform ports may live only in a nested addresses field (legacy fields = None) or carry sentinel 0 legacy ports. drive-abci consumes the deprecated raw legacy_platform_*_port fields and ignores addresses, which the upstream test suite at the pinned commit explicitly demonstrates is unsafe: HPMNs are silently dropped from validator sets or assigned port 0. The deferral noted in the PR description is not behavior-preserving against Core 23 RPC output and needs the new accessors (platform_p2p_address() / platform_http_address()) wired in before merge, not after.
🔴 3 blocking | 💬 2 nitpick(s)
2 additional finding(s) omitted (not in diff).
🤖 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 `packages/rs-drive-abci/src/platform_types/validator/v0/mod.rs`:
- [BLOCKING] packages/rs-drive-abci/src/platform_types/validator/v0/mod.rs:28-53: Core 23 HPMNs with nested-only addresses are silently dropped from validator sets
`new_validator_if_masternode_in_state` destructures only `legacy_platform_p2p_port` / `legacy_platform_http_port` and early-returns `None` if either is absent. The bumped rust-dashcore (rev `64d9267`) documents in `rpc-json/src/lib.rs` (test `dmn_state_core23_addresses_resolve_platform_ports`, ~L3606) that a Core 23 HPMN deserializes with both `legacy_platform_*_port = None` and the real ports under `addresses.platform_p2p` / `addresses.platform_https`. Combined with `try_from_quorum_info_result` filter-mapping `None`, any Core 23 v3 HPMN that registers with only nested addresses disappears from the platform's quorum view — a consensus-critical state divergence the moment any quorum member is in the new format. This is exactly what the upstream accessors `DMNState::platform_p2p_address()` / `platform_http_address()` exist to prevent. The PR description scopes nested-address adoption to a follow-up, but the upstream change is breaking semantics, not syntax: the deferral is not safe. Either consume the accessors here (and store the resolved host/port pair, since the nested `platform_p2p` can publish a host different from `service.ip()`), or hard-gate the rev bump until the consumer migration lands.
In `packages/rs-drive-abci/src/platform_types/masternode/v0/mod.rs`:
- [BLOCKING] packages/rs-drive-abci/src/platform_types/masternode/v0/mod.rs:140-220: `DMNState` ↔ `MasternodeStateV0` conversion silently drops Core 23 nested addresses
The forward conversion destructures `addresses: _` and the reverse rebuilds `addresses: None`, so any masternode whose platform ports arrived under the Core 23 `addresses` object is persisted into `MasternodeStateV0` with `platform_p2p_port = None` / `platform_http_port = None`. After a restart or any later refresh, the masternode is then treated by `new_validator_if_masternode_in_state` as missing platform ports and excluded from validator sets. The on-disk `MasternodeStateV0` layout does not have to change to fix this — `From<DMNState>` just needs to fall back to `value.platform_p2p_address()` / `value.platform_http_address()` when the legacy fields are `None`, and (because nested addresses can resolve to a non-service host) the conversion should store the resolved port, ideally with a note that the host pairing is delegated to the validator path. Without this fallback, the PR pins a rust-dashcore that demonstrably loses platform-port information for Core 23 entries (see upstream test `dmn_state_core23_addresses_resolve_platform_ports`).
In `packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_list/update_state_masternode_list/v0/mod.rs`:
- [BLOCKING] packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_list/update_state_masternode_list/v0/mod.rs:57-160: Core 23 address-only diffs neither refresh validator entries nor propagate port updates
Two related problems in this file:
1. The refresh trigger at lines 150–152 only fires on `pose_ban_height || service || legacy_platform_p2p_port.is_some()`. The upstream test `dmn_state_diff_core23_addresses_resolve_platform_ports` (rpc-json/src/lib.rs ~L3641) shows that an `updatedMNs` entry whose only change is the nested `addresses` deserializes with both `legacy_platform_*_port = None`, so the trigger never fires and validator entries keep stale endpoints after a Core 23 platform port/IP change.
2. `update_masternode_in_validator_sets` reads `dmn_state_diff.legacy_platform_p2p_port` / `legacy_platform_http_port` and writes `port as u16` straight into the validator. The transitional case in the upstream test `dmn_state_zero_legacy_port_resolves_to_addresses` (rpc-json/src/lib.rs ~L3699) confirms Core 23 can emit `platformP2PPort: 0` alongside real ports in `addresses`. That sentinel currently flows through as `validator.platform_p2p_port = 0`, advertised verbatim to Tenderdash as `tcp://...:0` — a connectivity break the operator did not request. The `as u16` cast also silently truncates ports outside `u16` (upstream test `dmn_state_zero_legacy_port_no_addresses_resolves_to_none` and the 70000-port fixture demonstrate the explicit upstream contract is to reject these).
Fix should consume `DMNStateDiff::platform_p2p_address()` / `platform_http_address()` (and equivalents on `DMNState`) and gate the refresh trigger on `addresses.is_some()` as well, so v3 ProTx churn cannot inject a port-0 endpoint or skip a needed refresh.
shumkov
changed the title
Make the DashPay design docs match what actually shipped on this branch: - SYNC_CORRECTNESS_SPEC (Spec 0): REVIEWED→IMPLEMENTED — both stages shipped (paginated/high-water request sync + id-keyed contact-profile cache), durably persisted to both the SQLite persister and SwiftData. - CONTACTINFO_FORMAT_SPEC (Spec 1): the §4 contactInfo reject/block field marked DEFINED-but-NOT-ADOPTED — R1 resolved ignore as local-only, so it is not carried on contactInfo; the shipped codec is alias/note/displayHidden/acceptedAccounts. - BLOCK_SPEC: PAUSED→SUPERSEDED — replaced by the implemented per-sender, reversible, local-only Ignore (Spec 2); the contactInfo cross-device route was rejected (R1); cross-device deferred to a future encrypted profile field (contract track). - SPEC.md: added a 2026-06-18 status block mapping the Part-0 gap table (G1–G15) to its resolution, plus accountReference (keep-ours) and the account-index path fix (upstream rust-dashcore#813 via dashcore bump PR #3936). Points at TODO.md as the authoritative item-by-item status. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ectation #3886 (2026-06-14) raised the per-network protocol floors (min_protocol_version: Mainnet=11, others=12). The build-time clamp applies them as max(DEFAULT_INITIAL_PROTOCOL_VERSION, min_protocol_version(network)), which silently invalidated this test (added earlier by #3809): it still asserted the unpinned mock SDK seeds to DEFAULT_INITIAL_PROTOCOL_VERSION (10), but new_mock() defaults to Mainnet, so the seed is now 11. v3.1-dev has been red on this test since #3886; the DashPay merge pulled the breakage in. Assert the SDK's documented post-clamp contract instead (max(DEFAULT_INITIAL_PROTOCOL_VERSION, PROTOCOL_VERSION_11) == 11). Verified locally: left:11 right:10 ✖ before, 1 passed ✔ after. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…conversion Addresses the blocking review on #3936. The legacy-only port mapping read only DMNState.legacy_platform_*_port, which are `None` for Core 23 masternodes (Core 23 moved the ports into the nested `addresses` object). That stored `platform_p2p_port = None`, and new_validator_if_masternode_in_state then excluded those nodes from validator sets — a consensus bug, not just "multi-address unsupported". Fix: resolve the port through DMNState's `platform_p2p_address()` / `platform_http_address()` accessors (prefer nested `addresses`, fall back to the legacy flat field) in both From<DMNState> and new_validator_if_masternode_in_state. Core 22 entries map byte-identically; Core 23 entries now yield their port. MasternodeStateV0 still stores only a port — no on-disk layout change. The two sites no longer touch deprecated fields, so their #[allow(deprecated)] is dropped. Tests (red→green): from_dmn_state_resolves_core23_nested_platform_ports would read None and fail under the old legacy-only path; from_dmn_state_falls_back_to_legacy_ports pins the Core 22 path unchanged. Review nits also addressed: saturating_add in the port-bump strategy-test helper (overflow guard); corrected the "below"→"above" helper-location comment. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
shumkov
changed the title
new_validator_if_masternode_in_state resolves ports via the DMNState accessors instead of destructuring the struct, so the DMNState type import is unused. CI denies warnings, so remove it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative review at 3530df8. The PR successfully resolves prior blocking findings on validator/masternode conversion: nested-only Core 23 platform ports are now resolved via DMNState::platform_p2p_address()/platform_http_address() accessors with covering unit tests, and the prior nitpicks on comment direction and saturating_add are also fixed. The masternode-list diff path remains the one carried-forward blocking gap: address-only Core 23 diffs still do not refresh cached validator_sets, and the new inline rationale is factually wrong — DMNStateDiff at rev 64d9267 does expose nested-address accessors (rpc-json/src/lib.rs:2303,2316). The validator host being sourced from service.ip() rather than the nested platform host is recorded as an out-of-scope follow-up since it is pre-existing behavior the PR explicitly defers.
🔴 1 blocking
1 additional finding(s) omitted (not in diff).
🤖 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 `packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_list/update_state_masternode_list/v0/mod.rs`:
- [BLOCKING] packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_list/update_state_masternode_list/v0/mod.rs:85-162: Carried-forward prior #3 STILL VALID: Core 23 address-only diffs do not refresh validator_sets; deferral rationale is factually wrong
Two issues compound on this code path at the bumped rev:
1) The refresh trigger at lines 152-154 only fires on pose_ban_height/service/legacy_platform_p2p_port. With rust-dashcore 64d9267, DMNStateDiff carries Core 23 platform endpoint changes in `addresses: Option<Option<MasternodeAddresses>>`, and apply_diff propagates them into hpmn_masternode_list. A Core 23 HPMN whose only changed field is the nested platform address is therefore applied to state but never reaches update_masternode_in_validator_sets, so cached ValidatorV0.node_ip/platform_*_port stay stale until an unrelated legacy field changes or the next quorum rotation rebuilds from state. This is deterministic across nodes (no consensus split) but causes bounded liveness lag for cross-validator P2P after Core 23 port-only changes — and contradicts the PR's stated goal of resolving Core 23 nested platform ports in masternode conversion.
2) The new inline comment at lines 85-88 justifies the deferral by asserting "the diff type has no nested-address accessor." That is false at the pinned rev: rust-dashcore rpc-json/src/lib.rs defines DMNStateDiff::platform_p2p_address() at line 2303 and DMNStateDiff::platform_http_address() at line 2316, mirroring the DMNState accessors this PR already adopts in validator/v0 and masternode/v0. Future maintainers will read this as upstream-not-yet-ready, when in reality the accessors exist and the deferral is a scoping choice.
Resolution options: (a) extend the trigger to also fire on state_diff.platform_p2p_address().is_some() / platform_http_address().is_some() and consume them inside update_masternode_in_validator_sets; or (b) keep the deferral but rewrite the comment to honestly state that the accessor exists but adoption is deferred to a follow-up, so the rationale is not load-bearing on a false claim.
…f path too Follow-up to the From<DMNState> accessor fix: update_masternode_in_validator_sets and its trigger guard still read DMNStateDiff.legacy_platform_*_port directly, which is None for Core 23 nodes — so a Core 23 platform-port change carried in the diff's nested `addresses` would not propagate to the validator until the next full validator-set rebuild. Unlike DMNState's accessor, DMNStateDiff::platform_p2p_address() does NOT fall back to the legacy field (it reads only the nested addresses), so OR the accessor with the legacy field rather than replacing it — otherwise a Core 22 port change (in the legacy flat field) would be missed. The change-detection guard is widened the same way so a Core-23-only address change still triggers the validator-set update. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Carried-forward prior finding #1 is FIXED at d119dd1: the refresh trigger now includes state_diff.platform_p2p_address().is_some() and update_masternode_in_validator_sets reads via the DMNStateDiff::platform_p2p_address()/platform_http_address() accessors with legacy fallback. The codex agents re-frame the same code path as still-valid blockers around host discard and addresses-cleared diffs, but those concerns are explicitly deferred by this PR's scope (MasternodeStateV0 only stores the legacy port pair, host pairing is delegated to the validator path per the documented comment) and they are behavior-preserving relative to Core 22 — they do not break consensus, are not regressions introduced by this PR, and adopting nested platform hosts requires a versioned state migration that is out of scope for a dependency bump. One minor latest-delta suggestion remains: the refresh trigger only checks the p2p axis, so a Core 23 diff that updates only platform_https or carries Some(None) addresses won't fire validator refresh.
🟡 1 suggestion(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 `packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_list/update_state_masternode_list/v0/mod.rs`:
- [SUGGESTION] packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_list/update_state_masternode_list/v0/mod.rs:162-166: Validator refresh trigger ignores http-only and cleared-addresses diffs
The refresh predicate at lines 162-166 only consults the p2p axis (state_diff.platform_p2p_address().is_some() and state_diff.legacy_platform_p2p_port.is_some()). With the bumped rust-dashcore 64d9267, DMNStateDiff::addresses is three-state (None / Some(None) / Some(Some(_))): a Core 23 diff that updates only platform_https (empty platform_p2p), or that clears addresses entirely (Some(None)), leaves platform_p2p_address() returning None and the trigger never fires — so the cached validator entry keeps a stale platform_http_port and/or stale p2p port until the next pose-ban/service/p2p change. In practice today's Core 23 RPC ships p2p and https together so the dominant case is covered, but the cleanest fix is to also check the http accessor, the legacy http field, and the raw addresses presence so any addresses-bearing diff refreshes the validator entry. Validator_sets is local node cache (not consensus state), so impact is wrong-port routing/liveness rather than consensus divergence, hence suggestion-level.
…iff-port resolution Addresses the review suggestion that the validator-refresh trigger only consulted the p2p axis: a Core 23 diff changing only platform_https, or clearing `addresses` (Some(None)), left platform_p2p_address() == None and the legacy field None, so the trigger never fired and the cached validator kept a stale http/p2p port. The guard now also checks the http resolver and the raw `addresses` presence (three-state: None / Some(None) / Some(Some)). Extracted the diff port resolution into `diff_platform_p2p_port` / `diff_platform_http_port` (prefer the Core 23 nested-addresses accessor — which on DMNStateDiff, unlike DMNState, has NO legacy fallback — then fall back to the legacy flat field). This makes the resolution unit-testable and consolidates the deprecated legacy reads into the two helpers, so the #[allow(deprecated)] drops off both update_masternode_in_validator_sets and update_state_masternode_list_v0. Tests: nested-Core-23 (both ports), http-only Core-23 (the missed case), Core-22 legacy fallback, and no-change → None. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The current head is 1199b87, and the prior refresh-trigger issue is fixed by the new http/legacy/raw-address predicate. One in-scope validator-cache correctness issue remains: address-clearing or empty Core 23 address diffs update the stored HPMN state but leave existing cached validator ports unchanged.
🔴 1 blocking
🤖 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 `packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_list/update_state_masternode_list/v0/mod.rs`:
- [BLOCKING] packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_list/update_state_masternode_list/v0/mod.rs:71-77: Clear cached validators when platform ports disappear
The new refresh predicate now reaches this helper for any `state_diff.addresses.is_some()`, but the helper only writes ports when `diff_platform_p2p_port` or `diff_platform_http_port` returns `Some`. For Core 23 diffs such as `addresses: Some(None)` or an `addresses` object whose platform port list is empty/invalid, upstream `DMNState::apply_diff` removes the resolvable platform address from the already-updated HPMN state, and `ValidatorV0::new_validator_if_masternode_in_state` would return `None` because both platform ports are mandatory. The existing cached `ValidatorV0` in each validator set is still retained with its old `platform_p2p_port` and `platform_http_port`, so Tenderdash can continue receiving stale endpoint data that no longer matches Core state. Rebuild the validator entry from the updated HPMN state, or remove the cached member when the updated state no longer resolves the required platform ports.
| if let Some(p2p_port) = diff_platform_p2p_port(dmn_state_diff) { | ||
| validator.platform_p2p_port = p2p_port as u16; | ||
| } | ||
|
|
||
| if let Some(http_port) = dmn_state_diff.platform_http_port { | ||
| if let Some(http_port) = diff_platform_http_port(dmn_state_diff) { | ||
| validator.platform_http_port = http_port as u16; | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Clear cached validators when platform ports disappear
The new refresh predicate now reaches this helper for any state_diff.addresses.is_some(), but the helper only writes ports when diff_platform_p2p_port or diff_platform_http_port returns Some. For Core 23 diffs such as addresses: Some(None) or an addresses object whose platform port list is empty/invalid, upstream DMNState::apply_diff removes the resolvable platform address from the already-updated HPMN state, and ValidatorV0::new_validator_if_masternode_in_state would return None because both platform ports are mandatory. The existing cached ValidatorV0 in each validator set is still retained with its old platform_p2p_port and platform_http_port, so Tenderdash can continue receiving stale endpoint data that no longer matches Core state. Rebuild the validator entry from the updated HPMN state, or remove the cached member when the updated state no longer resolves the required platform ports.
source: ['codex']
There was a problem hiding this comment.
Resolved in 8e19e2a — Clear cached validators when platform ports disappear no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
|
Coverage note for my review at 1199b87: Claude reviewers and the Claude verifier could not run because the Claude ACP account hit its usage limit (resets 8am America/Chicago). Codex general, security-auditor, rust-quality, and Codex verifier ran successfully; CodeRabbit had 0 inline findings and was rate-limited. |
…when ports vanish
Two Core 23 masternode-diff correctness fixes (rust-dashcore#808 context):
1. Zeroed legacy port must not surface. Core 23 (ProTx v3) entries zero the
deprecated flat platformP2PPort/HTTPPort (the real port lives in the nested
`addresses`). The diff accessor already drops a zero addresses port, but the
legacy fallback in diff_platform_{p2p,http}_port read the raw legacy field — so
an edge diff (addresses unresolvable + legacy 0) would set a validator's
platform port to 0, the exact failure #808 fixed. The fallback now drops 0.
2. Remove cached validators when platform ports disappear. The widened refresh
guard reaches update_masternode_in_validator_sets for any addresses delta, but
the helper only writes ports when they resolve — so a Core 23 diff clearing
`addresses` (Some(None)) or zeroing the ports left the cached ValidatorV0 with
its stale ports, and Tenderdash kept getting a dead endpoint. After apply_diff
we now check the updated state via resolves_platform_validator_ports() (both
ports mandatory, matching new_validator_if_masternode_in_state) and drop the
validator from the sets when it no longer resolves them.
Tests: zeroed-legacy-dropped; resolves-for-core23-addresses / for-legacy;
does-not-resolve-when-ports-disappear (cleared + http-only). 10 tests green,
clippy clean.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Prior 1199b87 finding (stale cached validators when platform ports vanish) is resolved on this head by the new resolves_platform_validator_ports predicate that branches between update and remove. Remaining in-scope concerns: the new accessor-based code throws away the host string returned by platform_p2p_address()/platform_http_address(), and the persistence roundtrip collapses Core 23 addresses into legacy ports in a way that can mask a later addresses-clear diff after restart. No new blocking defects.
🟡 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 `packages/rs-drive-abci/src/platform_types/validator/v0/mod.rs`:
- [SUGGESTION] packages/rs-drive-abci/src/platform_types/validator/v0/mod.rs:30-36: Validator construction discards the platform host returned by the Core 23 accessor
`platform_p2p_address()` / `platform_http_address()` return `(host, port)`, but both the validator constructor here and `update_masternode_in_validator_sets` (update_state_masternode_list/v0/mod.rs:73-79) bind `_host` and copy only the port, then use `state.service.ip()` for `node_ip`. `ValidatorSetV0::to_update` advertises Tenderdash endpoints as `node_id@node_ip:platform_p2p_port`, so if a Core 23 ProTx v3 entry ever carries a platform host that differs from the Core service host, Platform will advertise the validator on the wrong host. The pre-existing convention has been service.ip() == platform host, so this likely does not fire in current devnet deployments — but now that the accessor explicitly surfaces a host, please either propagate it into `node_ip` (and refresh on `addresses` diffs) or document why the service IP is intentionally authoritative over the Core 23 platform host.
In `packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_list/update_state_masternode_list/v0/mod.rs`:
- [SUGGESTION] packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_list/update_state_masternode_list/v0/mod.rs:149-165: Persisted-state roundtrip can mask a Core 23 addresses-clear diff after restart
`MasternodeStateV0 -> DMNState` (packages/rs-drive-abci/src/platform_types/masternode/v0/mod.rs:217-219) reconstructs persisted Core 23 state as `addresses: None` with the resolved port written into `legacy_platform_p2p_port`/`legacy_platform_http_port`. After a restart, a subsequent Core 23 diff with `addresses: Some(None)` (explicit clear) and no legacy-port update applies as `addresses: Some(None)` + `legacy_*: Some(<old>)`. `state.platform_p2p_address()` then falls back to the synthesized legacy port, so `resolves_platform_validator_ports` returns true and the cached validator is retained even though Core has cleared the platform endpoint. Either treat an explicit `state_diff.addresses` delta as authoritative for the validity check (`Some(Some(_))` → check diff accessors; `Some(None)` → false; `None` → full-state fallback), or change the persistence shape to remember address provenance.
| let (_, platform_p2p_port) = state.platform_p2p_address()?; | ||
| let (_, platform_http_port) = state.platform_http_address()?; | ||
| let platform_node_id = state.platform_node_id?; | ||
| Some(ValidatorV0 { | ||
| pro_tx_hash, | ||
| public_key, | ||
| node_ip: service.ip().to_string(), | ||
| node_ip: state.service.ip().to_string(), |
There was a problem hiding this comment.
🟡 Suggestion: Validator construction discards the platform host returned by the Core 23 accessor
platform_p2p_address() / platform_http_address() return (host, port), but both the validator constructor here and update_masternode_in_validator_sets (update_state_masternode_list/v0/mod.rs:73-79) bind _host and copy only the port, then use state.service.ip() for node_ip. ValidatorSetV0::to_update advertises Tenderdash endpoints as node_id@node_ip:platform_p2p_port, so if a Core 23 ProTx v3 entry ever carries a platform host that differs from the Core service host, Platform will advertise the validator on the wrong host. The pre-existing convention has been service.ip() == platform host, so this likely does not fire in current devnet deployments — but now that the accessor explicitly surfaces a host, please either propagate it into node_ip (and refresh on addresses diffs) or document why the service IP is intentionally authoritative over the Core 23 platform host.
source: ['codex']
| let resolves_platform_ports = | ||
| resolves_platform_validator_ports(&hpmn_list_item.state); | ||
| // Refresh the validator entry on any change to the fields it | ||
| // carries: ban status, service IP, or either platform port. | ||
| // A platform-port change can be a resolvable p2p/http port | ||
| // (Core 23 nested addresses OR a legacy flat field) or an | ||
| // `addresses` delta that clears/empties a port — `addresses` | ||
| // is three-state (None / Some(None) / Some(Some)), so check | ||
| // its presence too, otherwise an http-only or address-clearing | ||
| // diff would leave a stale port on the cached validator. | ||
| if state_diff.pose_ban_height.is_some() | ||
| || state_diff.service.is_some() | ||
| || state_diff.platform_p2p_port.is_some() | ||
| || diff_platform_p2p_port(state_diff).is_some() | ||
| || diff_platform_http_port(state_diff).is_some() | ||
| || state_diff.addresses.is_some() | ||
| { | ||
| // we updated the ban status the IP or the platform port, we need to update the validator in the validator list | ||
| Self::update_masternode_in_validator_sets( | ||
| pro_tx_hash, | ||
| state_diff, | ||
| state.validator_sets_mut(), | ||
| ); | ||
| if resolves_platform_ports { |
There was a problem hiding this comment.
🟡 Suggestion: Persisted-state roundtrip can mask a Core 23 addresses-clear diff after restart
MasternodeStateV0 -> DMNState (packages/rs-drive-abci/src/platform_types/masternode/v0/mod.rs:217-219) reconstructs persisted Core 23 state as addresses: None with the resolved port written into legacy_platform_p2p_port/legacy_platform_http_port. After a restart, a subsequent Core 23 diff with addresses: Some(None) (explicit clear) and no legacy-port update applies as addresses: Some(None) + legacy_*: Some(<old>). state.platform_p2p_address() then falls back to the synthesized legacy port, so resolves_platform_validator_ports returns true and the cached validator is retained even though Core has cleared the platform endpoint. Either treat an explicit state_diff.addresses delta as authoritative for the validity check (Some(Some(_)) → check diff accessors; Some(None) → false; None → full-state fallback), or change the persistence shape to remember address provenance.
source: ['codex']
| /// Whether a masternode's (post-`apply_diff`) state still resolves **both** platform | ||
| /// ports — the validity condition `new_validator_if_masternode_in_state` enforces (it | ||
| /// rejects a node missing either). Uses the full-state accessors, which prefer the | ||
| /// Core 23 nested addresses and fall back to the non-zero legacy port paired with the | ||
| /// node's service IP. When this is false the node can no longer be a platform | ||
| /// validator, so its cached entry must be dropped rather than left stale. | ||
| fn resolves_platform_validator_ports(state: &DMNState) -> bool { | ||
| state.platform_p2p_address().is_some() && state.platform_http_address().is_some() | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: resolves_platform_validator_ports doc overstates equivalence with constructor
The doc comment claims the predicate mirrors the validity gate in new_validator_if_masternode_in_state, but that constructor additionally requires state.platform_node_id.is_some() (validator/v0/mod.rs:32). In practice platform_node_id is not in the refresh trigger so this edge case is not reachable today, but either tighten the predicate to mirror the full constructor (&& state.platform_node_id.is_some()) or soften the comment so it does not overstate the equivalence.
source: ['claude']
2 participants
Issue being fixed or feature implemented
Bumps all
rust-dashcoreworkspace deps5c0113e7→64d9267dto pick up rust-dashcore#813 — the DashPay friendship-path fix that honors the account index (m/9'/5'/15'/account'/…) instead of hardcoding0'. That fix is the prerequisite for non-zero / multi-account DashPay contacts in the wallet stack.Bumping two commits past the current pin also pulls in rpc-json#808 (Core 23 nested masternode platform addresses) — a breaking change to
DMNState/DMNStateDiffthat required correctly resolving masternode platform ports in drive-abci.What was done?
rust-dashcoredeps (dashcore,key-wallet*,dash-spv,dashcore-rpc, …) +Cargo.lock.DEPLOYMENT_V24, live on devnets) returns ProTx v3 masternodes whoseprotx listdiffentries zero the deprecated flatplatformP2PPort/platformHTTPPortand carry the real ports only in the new nestedaddressesobject. The flat fields were renamedlegacy_*and resolver accessorsplatform_p2p_address()/platform_http_address()added. drive-abci now resolves ports through the accessors (prefer nestedaddresses, fall back to the non-zero legacy field) everywhere it reads masternode platform ports:From<DMNState> for MasternodeStateV0andnew_validator_if_masternode_in_stateuseplatform_*_address()(Core 22 maps byte-identically; Core 23 entries are no longer dropped from validator sets).MasternodeStateV0still stores only au16port → serialized platform state unchanged.update_state_masternode_list_v0): the refresh trigger now fires on a p2p or http port change and anyaddressesdelta (three-stateNone/Some(None)/Some(Some)); the per-validator update resolves the port viadiff_platform_{p2p,http}_port(nested-first, non-zero legacy fallback — a zeroed v3 legacy port is dropped so a validator never gets port0).addresses(or zeroes the ports with no addresses) and the updated state resolves no platform ports, the now-invalid HPMN validator is removed from the validator sets rather than left advertising a stale/dead endpoint.#[allow(deprecated)]is scoped to the few sites that still name alegacy_*field directly (the two diff-port resolvers, the reverseMasternodeStateV0 → DMNStateconstruction, and test fixtures).How Has This Been Tested?
drive-abciunit tests: full-state Core-23 resolution + Core-22 fallback (from_dmn_state_*); diff-port resolution incl. http-only, zeroed-legacy-dropped, and theresolves_platform_validator_portsvalidity check (diff_*,resolves_*,does_not_resolve_when_ports_disappear).sdk_builder_default_seeds_atomic_to_floorexpectation fix (PR fix(sdk): refresh SDK protocol version to the network's on startup and network switch #3886 floor raise, was feature-branch-only onv3.1-dev).cargo check --workspace --all-targetsclean (0 errors, 0 warnings);clippy -D warningsclean;fmtclean.Breaking Changes
None for platform consumers. Masternode platform-port behavior is byte-identical to the current pin for Core 22 (legacy ports); the serialized
MasternodeStateV0format is unchanged. Core 23 (v3) entries — which the previous pin mishandled (port0) — now resolve correctly.Notes for reviewers
3f650020, predating the currentv3.1-devpin and not containing feat: make verification functions public #813). Both touch the sameCargo.tomlrev line, so they will conflict — this PR should be the canonical dashcore bump.0→ consensus connectivity failure. Mainnet/testnet are latent (V24NEVER_ACTIVE) until a future Core activation.Checklist
🤖 Generated with Claude Code
Summary by CodeRabbit