Repository navigation
fix(platform-wallet): refuse ports Dash Core v24 refuses before building a version-3 ProUpServTx - #5629
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the
 |
|
Waiting for bot review — thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
github-actions
Bot
added
the
waiting-bots
|
✅ Final review complete — no blockers (commit 8ca225b) · triage: low |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
The V24 port guard correctly rejects Core's invalid individual ports before constructing or funding the update transaction, and the wallet test and Rust wallet CI checks are successful. However, the generated version-3 address map can still contain duplicate IP:port entries, which Dash Core rejects with bad-protx-dup-netinfo-entry; this leaves a concrete failure mode in the same conversion path.
🟡 1 suggestion(s)
Review provenance
Source: reviewer 1: glm-5.3-flash (agent: phase1-reviewer, role: general); reviewer 2: glm-5.3-flash (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
lowbygpt-6.1-sol(effort low) — This is a small, well-contained Rust wallet validation change with focused tests and no consensus, cryptographic, signing, or direct coin-selection logic. - Phase 1 reviewers:
glm-5.3-flash— general (completed, effort high); agentphase1-reviewer,glm-5.3-flash— rust-quality (completed, effort high); agentphase1-reviewer - Phase 1 model:
glm-5.3-flash— zai quota: 5h 98% left, weekly 94% left; passed overgemini-3.8-flash-high(antigravity quota lookup failed) - Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort medium); agentphase2-reviewer
🤖 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-platform-wallet/src/masternode/update_service.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/masternode/update_service.rs:480-486: Reject duplicate endpoints in the generated version-3 address map
The guard validates each port independently, but Dash Core v24 also rejects an ExtNetInfo map when two entries have the same IP:port. `legacy_evonode_net_info` writes the legacy service IP under CoreP2P, PlatformP2P, and PlatformHttps, so equal ports such as `convert(26656, 26656, 1443)` produce duplicate entries and Core returns `bad-protx-dup-netinfo-entry`. The transaction can therefore still be built and broadcast without the fail-fast behavior this change intends to provide. Add a pairwise-distinct check for the three generated ports and cover it with a test.
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Pre-V24 version-2 updates still lack fail-fast scalar-port validation — Dash Core v24's scalar validation for the pre-V24 version-2 payload also rejects pairwise duplicate platform/core ports, the network default port, and invalid mainnet platform ports. The unchanged pre-V24 branch can therefore retain the same broadcast-and-reservation failure pattern for those inputs. This predates the diff and the PR specifically targets the V24 legacy-to-version-3 conversion, so it should be tracked separately.
- Follow-up: Open a separate issue to pre-validate the version-2 scalar port rules before building the pre-V24 update payload.
|
Bots are done — your move: post |
github-actions
Bot
added
waiting-self-review
|
Waiting for bot review — thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
github-actions
Bot
added
waiting-bots
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Updated after discussion: 1 finding(s) of this review were settled in the inline threads; 1 blocking finding(s) still stand.
- ⏭️ Deferred (maintainer decision): 🟡 Keep Core network validation in the upstream transaction layer (tracked in dashpay/rust-dashcore#1226)
The V24 legacy conversion still accepts port 443 for CoreP2P and PlatformP2P because the implementation does not classify 443 as refused before applying the HTTPS exception. The added regression test explicitly exercises those cases and will fail, while the wallet test job is still in progress. The copied Core compatibility rules also belong upstream in rust-dashcore alongside the transaction/network-info types to avoid future drift.
🔴 1 blocking | 🟡 1 0 suggestion(s)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
normalbygpt-6.1-sol(effort low) — This is a contained wallet-side validation change in masternode/update_service.rs that affects transaction construction and error handling but does not itself change consensus, cryptography, key handling, or storage. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort high); agentphase2-reviewer
🤖 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-platform-wallet/src/masternode/update_service.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/masternode/update_service.rs:604-607: Reject port 443 for non-HTTPS purposes
`is_bad_port` only covers ports 1–1023 and `CORE_BAD_PORTS`, which does not contain 443. Therefore `require_port_core_accepts(NetInfoPurpose::CoreP2P, 443)` and the equivalent PlatformP2P call return `Ok(())`; the HTTPS exception has no effect because `is_bad_port` is false. This contradicts the documented Core rule and the new test, which expects 443 to be refused for CoreP2P and PlatformP2P while allowing it only for PlatformHttps. A legacy conversion using either non-HTTPS purpose on port 443 can still build the version-3 transaction that Core rejects.
- [SUGGESTION → DEFERRED by a maintainer; not blocking, do not fix in this PR] packages/rs-platform-wallet/src/masternode/update_service.rs:563-607: Keep Core network validation in the upstream transaction layer
The patch independently copies Dash Core's `IsBadPort` rules, the purpose-specific HTTPS exception, and duplicate-address semantics into `rs-platform-wallet`. The repository's ownership table assigns Core transaction construction to `dashpay/rust-dashcore/key-wallet`, while `rs-platform-wallet` should orchestrate the conversion. The pinned and current rust-dashcore sources expose `ExtNetInfo` and its entries but do not provide a reusable validator, so the minimal durable fix is an upstream compatibility-validation helper alongside those types, with the wallet mapping its error into `PlatformWalletError`. Otherwise another ProUpServTx builder, or a future Core rule change, can diverge from this copied validation.
|
Your move: thepastaclaw requested changes on this head; dismiss the review or push a fix. |
github-actions
Bot
added
waiting-self-review
…ing a version-3 ProUpServTx Once V24 is active, reviving a legacy evonode converts its service address and platform ports into a version-3 address map. Core v24 checks every IP entry of that map (`ExtNetInfo::ValidateService`) and refuses the transaction with TX_BAD_SPECIAL `bad-protx-netinfo-port` for port 0 and every port `IsBadPort` lists (1-1023 and a fixed list of well-known ports), except 443 for Platform HTTPS. Pre-v3 entries were never held to that rule, so the wallet could build and broadcast a transaction that never confirms and keeps its inputs reserved until the TTL expires. The conversion now checks the Core P2P, Platform P2P and Platform HTTPS ports with Core's exact rule, copied from src/netbase.cpp and src/evo/netinfo.cpp at Dash Core v24.0.0-rc.4, and returns an InvalidParameter error naming the purpose and port before any payload is built or input reserved. Not consensus-relevant: this is client-side wallet transaction building and changes no Platform protocol version's behaviour. Test would have caught this in CI: should_refuse_a_legacy_evonode_conversion_with_a_port_core_v24_refuses ✖ before the fix (a version-3 payload with CoreP2P port 1 is built) — ✔ after. Closes #5503 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…n-3 address map Dash Core v24 also refuses a version-3 address map that lists one address twice (`ExtNetInfo::HasAddrPortDuplicates`, TX_BAD_SPECIAL `bad-protx-dup-netinfo-entry`). The conversion of a legacy evonode gives the Core P2P, Platform P2P and Platform HTTPS entries the same IP, so a caller's Platform P2P port equal to the Core P2P or Platform HTTPS port built a transaction that never confirms, like the refused ports did. The conversion now refuses such ports with an InvalidParameter error naming both services and the port, before anything is built. A pre-v3 entry cannot share its Core P2P and Platform HTTPS ports, since Core refused that already. Not consensus-relevant: client-side wallet transaction building. Test would have caught this in CI: should_refuse_a_legacy_evonode_conversion_that_repeats_an_address ✖ before the fix (a version-3 payload whose Platform P2P port repeats Core P2P port 20001 is built) — ✔ after. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
d5e8e32 to
8ca225b
Compare
|
Waiting for bot review — thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
github-actions
Bot
added
waiting-bots
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The change correctly rejects prohibited ports and duplicate endpoints before constructing a version-3 legacy-evonode payload, while preserving the existing version-2 and already-registered version-3 paths. The wallet-side Core rule copy is an accepted, tracked compatibility workaround for the pinned 5.0.x dependency; the upstream replacement is tracked by rust-dashcore#1226 and platform#5637. No in-scope blocking issues or suggestions remain. Rust workspace tests were still in progress and Kotlin checks were queued in the supplied CI snapshot, with no failed code checks.
🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)
Review provenance
Source: reviewer 1: glm-5.3-flash (agent: phase1-reviewer, role: general); reviewer 2: glm-5.3-flash (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
lowbygpt-6.1-sol(effort low) — This is a small, self-contained wallet transaction-building validation change with focused tests and no consensus, funds movement, cryptographic, or other critical-surface modifications. - Phase 1 reviewers:
glm-5.3-flash— general (completed, effort high); agentphase1-reviewer,glm-5.3-flash— rust-quality (completed, effort high); agentphase1-reviewer - Phase 1 model:
glm-5.3-flash— zai quota: 5h 99% left, weekly 93% left; passed overgemini-3.8-flash-high(antigravity quota lookup failed) - Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort medium); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.
No unresolved findings remain from the prior review on this head.
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Move Core network-info validation to rust-dashcore when the compatibility API is available — The port and duplicate-entry rules duplicate Dash Core semantics in rs-platform-wallet rather than living beside rust-dashcore's ExtNetInfo model. This is appropriate for the pinned 5.0.x dependency, but the copy should be removed once rust-dashcore#1226 lands and the v5.1 dependency update in platform#5637 makes the upstream validator available.
- Follow-up: Replace the wallet-side constants and helpers with rust-dashcore's ExtNetInfo::validate_ports_and_duplicates during the v5.1 dependency update.
|
Bots are done — your move: post |
github-actions
Bot
added
waiting-self-review
Issue being fixed or feature implemented
Closes #5503.
Release line: this PR targets 5.0.x (
v5.0-dev) and reaches v5.1 through the forward merge.ExtNetInfo::validate_ports_and_duplicates(feat(dash): check address maps against Core v24 port and duplicate rules rust-dashcore#1226) once the rust-dashcore bump chore(platform)!: update rust-dashcore to dev 7329126e #5637 lands.Once V24 is active, reviving a legacy (pre-v3) evonode with
prepare_masternode_update_serviceconverts the evonode's service address and platform ports into a version-3 address map. Dash Core v24 checks every IP entry of that map (ExtNetInfo::ValidateService) and refuses the transaction withTX_BAD_SPECIALbad-protx-netinfo-portfor:IsBadPortlists: 1–1023 plus a fixed list of well-known ports;Pre-v3 entries were never held to that rule, so the wallet could build and broadcast a transaction Core never accepts. It never confirmed, its inputs stayed reserved until the broadcast TTL expired, and the user got no error to act on.
What was done?
require_port_core_accepts(purpose, port)applies Core's exact rule. The port list is copied fromsrc/netbase.cppIsBadPortand the rule fromsrc/evo/netinfo.cppExtNetInfo::ValidateService, both at Dash Core v24.0.0-rc.4, with the purpose-scoped 443 exception. The code comment cites that version.prepare_update_service_placeholder, the V24 conversion of a legacy evonode checks the Core P2P, Platform P2P and Platform HTTPS ports before the payload is built. A refused port returnsPlatformWalletError::InvalidParameternaming the purpose and port, and tells the operator to move the service and set it with Core'sprotx update_service_evo. No input is reserved and nothing is broadcast.require_distinct_portsapplies Core v24's duplicate-entry rule (ExtNetInfo::HasAddrPortDuplicates,TX_BAD_SPECIALbad-protx-dup-netinfo-entry): a version-3 address map may not list one address twice. The conversion gives all three services the legacy IP. A caller's Platform P2P port equal to the Core P2P or Platform HTTPS port would therefore repeat an address and build the same never-confirming transaction, so it is refused with an error naming both services and the port. A pre-v3 entry cannot share its Core P2P and Platform HTTPS ports, because Core already refused that (bad-protx-platform-dup-ports). (Raised in review.)IsBadPortto the version-2 payload used before V24 (MnNetInfo::ValidateServiceand the scalar-port checks inCheckProviderNetworkFields).Protocol-version impact
None. This is wallet-side transaction building in
rs-platform-wallet. It touches no Platform consensus or validation code and no protocol version's behaviour, so no newvNor version-table change is needed.How Has This Been Tested?
New test
should_refuse_a_legacy_evonode_conversion_with_a_port_core_v24_refusesinmasternode/update_service.rs:The test spells out Core's list itself instead of reusing the implementation's constant.
New test
should_refuse_a_legacy_evonode_conversion_that_repeats_an_address:Red → green, re-run on
origin/v5.0-devafter the move:v5.0-devunchanged. Their diff is line for line the one reviewed onv5.1-dev.Also run on the
v5.0-devbase:cargo test -p platform-wallet -p platform-wallet-storage -p platform-wallet-ffi --all-features --locked --lib --bins --tests -- --skip wallet::shielded::, with CI's env (RUST_MIN_STACK,CARGO_PROFILE_DEV_DEBUG=0,CODEGEN_UNITS=256): 2,800 passed, 0 failed. This is the CI wallet job's package set and filter.platform-wallet,platform-wallet-storage,platform-wallet-ffi,rs-unified-sdk-ffi,rs-unified-sdk-jni;--all-targets --all-features --locked -- --no-deps -D warnings): clean.cargo fmt --all -- --check: clean.Breaking Changes
None.
Checklist:
structure.rs, regeneratedgrovedb-structure.json, and checked the structure viewer link posted on this pull requestFor repository code-owners and collaborators only
🤖 Generated with Claude Code
PR Hygiene ·
8ca225b/self-reviewedrs-platform-wallet(packages/rs-platform-wallet/src/masternode/update_service.rs) — HashEngineering or ZocoLini or llbartekll or romchornyiWhen every merge requirement is met, the
PR Hygienecheck passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.Summary by CodeRabbit