Repository navigation
fix(platform-wallet): a ProUpServTx always carries an output - #5105
Merged
QuantumExplorer merged 2 commits intoSep 28, 2026
Merged
QuantumExplorer merged 2 commits into
QuantumExplorer merged 2 commits into
Conversation
…on every ProUpServTx The update-service (unban) flow now takes the service address as an explicit, caller-confirmed input instead of copying it from the synced masternode list: IP:port always, and for an evonode the platform node id, platform P2P port and platform HTTP port. The payload is built from that input alone. The confirmed type is checked against the registration transaction, which is already bound to the requested proTxHash, and values no payload can carry (a non-IPv4 or unspecified address, a zero port, an empty node id, clashing ports) are refused before any network work. A read-only suggestion accessor (library, FFI and Swift) returns what the list shows so hosts can prefill their form, including whether the list shows the node PoSe-banned. Nothing refuses on that status. A ProUpServTx always carries an output: the builder adds a zero-value data output, the same placeholder Dash Core's protx update_service adds, so a selection whose leftover is below the dust limit no longer yields a transaction with no outputs. The payload finalizer also refuses an output-less transaction before signing, which releases the reservation. BREAKING CHANGE: MasternodeUpdateServiceParams takes a ConfirmedMasternodeService in place of platform_p2p_port. The four update-service FFI entry points take service_address, platform_node_id, platform_p2p_port and platform_http_port in place of has_platform_p2p_port and platform_p2p_port. The Swift wrappers take a MasternodeServiceConfirmation in place of platformP2PPort. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
github-actions
Bot
added
the
waiting-bots
Contributor
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: dashpay/platform/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (6)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughMasternode service updates now use caller-confirmed regular or evonode service data. The Rust wallet validates and builds payloads from those values, exposes synced-list suggestions, and signs funded transactions. The FFI and Swift APIs pass the new fields for wallet-owned and tracked masternodes. ChangesMasternode Service Updates
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~60 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant PlatformWalletManager
participant RustFFI
participant prepare_masternode_update_service
participant update_service_builder
PlatformWalletManager->>RustFFI: Submit confirmed service values
RustFFI->>prepare_masternode_update_service: Marshal parameters and prepare update
prepare_masternode_update_service->>update_service_builder: Build and finalize transaction
update_service_builder-->>RustFFI: Return prepared transaction
RustFFI-->>PlatformWalletManager: Return transaction result
Suggested reviewers: Merge Risk: ⚪ Minimal · up to No actionable issue remains; the change is mergeable after normal checks. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to The update now relies on the calling app to confirm service values before signing. The inspected wallet paths retain operator and registration checks, and no new bypass was established, but the app-level confirmation flow remains unverified. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
Collaborator
|
🔍 Review in progress — actively reviewing now (commit c939f01) · triage: critical |
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
Verified the supplied findings against head b23e7ec. Two non-blocking issues remain in the new caller-confirmed service path: embedded NULs can truncate the confirmed address, and semantic validation occurs after mnemonic resolution in wallet-owned calls. The duplicate validation-order finding is consolidated; the pre-existing panic-hardening observation is excluded.
🟡 2 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: architecture-layering); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: ffi-engineer); reviewer 4: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 5: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 6: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 7: gpt-6-astra (agent: phase2-reviewer, role: ffi-engineer); reviewer 8: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
- Triage:
criticalbygpt-6-astra(effort low) — The large cross-language change directly modifies signed ProUpServTx payload construction and transaction funding/output behavior in packages/rs-platform-wallet/src/masternode/update_service.rs, including prepare_masternode_update_service and build_sign_update_service, meeting the intricacy and funds-movement/signature-surface criteria. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— architecture-layering (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— ffi-engineer (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 below 15% reserve: weekly 13% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort xhigh); 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/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerMasternodes.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerMasternodes.swift:591-596: Reject embedded NUL characters before marshalling the confirmed address
The shared preflight accepts embedded U+0000 characters, but `withFFIArguments()` passes the address through `withCString`, and Rust's `marshal_params()` reads it with `CStr::from_ptr`. Consequently, `"203.0.113.7:9999\u{0}:19999"` reaches Rust as the valid address `203.0.113.7:9999`; the malformed complete string is never validated. All four Swift update-service methods use this path, so an otherwise valid request can sign a truncated address rather than reject the caller-confirmed input. Reject NUL characters here, before crossing the C-string boundary, and add a regression test covering a valid address followed by a NUL and suffix.
In `packages/rs-platform-wallet-ffi/src/masternode_update_service.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/masternode_update_service.rs:205-211: Validate confirmed service values before resolving the operator mnemonic
`marshal_params()` parses the endpoint and checks the regular-masternode shape, but accepts values that `validate_confirmed_service()` will unconditionally reject, such as an all-zero evonode node id or zero/clashing platform ports. Both wallet-owned entry points call `wallet_operator_secret()` after marshalling but before reaching that validator in `prepare_masternode_update_service()`. For external-signable or watch-only wallets, this invokes the host mnemonic resolver for a request already known to be invalid, potentially requiring Keychain authentication; a resolver failure also masks the service validation error. This falls short of the new call-site intent that malformed input fail before resolver use. Expose and reuse the wallet's semantic validator during marshalling, retain the prepare-time check, and add a test asserting that invalid service values do not invoke the resolver. The tracked-key entry points do not have this early mnemonic-resolution side effect.
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.
- Crate-wide panic-across-
extern "C"hardening — Pre-existing broad hardening outside this PR's scope: runtime.rs still propagates worker panics through expect, and this PR does not change that behavior or establish a new reachable panic. The supplied claim that a Rust panic would unwind into Swift as undefined behavior is also inaccurate for these non-unwinding extern "C" functions: a Rust panic reaching that boundary aborts rather than unwinding into the caller.- Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.
Contributor
|
Your move: thepastaclaw left review threads unresolved; resolve them. |
github-actions
Bot
added
waiting-self-review
Keep only the output fix in this change. The service address, masternode type and platform fields come from the synced masternode list as on v4.2-dev, and the FFI entry points and Swift methods are back to their v4.2-dev form. Every ProUpServTx gets a zero-value OP_RETURN output, as Dash Core's FundSpecialTx adds for protx update_service, and the payload finalizer refuses a transaction without outputs before signing. The comment on reading the registration transaction now says the fetched transaction must hash to the requested proTxHash, that only the operator reward is read from it, and that the service values come from the synced list. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
QuantumExplorer
changed the title
github-actions
Bot
added
waiting-bots
QuantumExplorer
deleted the
claude/masternode-update-service-confirmed-address
branch
September 28, 2026 18:10
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
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.
Basic explanation
A masternode operator sends an "update service" transaction (ProUpServTx) to state where their node lives; sending one also lifts a PoSe ban. This PR makes sure the wallet never builds one without an output. Every ProUpServTx now carries a small zero-value data output, the same one Dash Core adds, so the network accepts it however small the coin that pays the fee.
Example:
bad-txns-vout-empty), and the app reported it as maybe sent with the coin left reserved.protx update_servicedoes, so the same wallet sends a valid transaction. Change is still returned whenever the leftover clears the dust limit, and a transaction without outputs is refused before signing, releasing the coin.Value: A wallet holding only small coins can still update its node, and a transaction the network would refuse is never signed or sent.
Risks: Every ProUpServTx carries one extra output of about 11 bytes, which costs about 11 duffs of fee. The service address handling is unchanged from v4.2-dev, and so are the FFI and Swift APIs.
Issue being fixed or feature implemented
The update-service builder added no outputs of its own. When coin selection booked the whole leftover of a small coin as fee (anything from about 342 to 888 duffs in a single coin), the ProUpServTx had no outputs. The network refuses a transaction without outputs, and the selected coin stayed reserved while the wallet waited on a broadcast that could not confirm.
What was done?
rs-platform-wallet(masternode/update_service.rs):update_service_builderadds a zero-value OP_RETURN output to every ProUpServTx, as Dash Core'sFundSpecialTxdoes.finalize_update_service_payload, refuses a transaction without outputs before signing anything. key-wallet releases the build's reservation on any finalizer error, so the refusal leaves the coin spendable.The service address, masternode type and platform fields still come from the synced masternode list, exactly as on v4.2-dev. No FFI or Swift file changes. This is wallet code only; no consensus or protocol-version change.
How Has This Been Tested?
cargo test -p platform-wallet --lib masternode::(66 passed), including:should_carry_an_output_when_the_only_utxo_leaves_no_change([600]: one zero-value data output, fee 600)should_carry_an_output_when_selection_prefers_the_small_utxo([1_000_000_000, 600])should_fail_before_broadcast_when_the_fee_cannot_be_covered([200]: insufficient-funds error, nothing sent)should_release_the_reservation_when_the_transaction_has_no_outputs(a builder without the data output is refused by the finalizer, and the same coin funds the next build)builds_signs_and_broadcasts_a_pro_up_serv_txnow also asserts the transaction carries an output.[600],[1_000_000_000, 600]and reservation-release tests fail, confirming they cover the change.cargo clippy -p platform-wallet --tests -- -D warnings,cargo fmt --all -- --check, andcargo check -p platform-wallet-ffi(the FFI crate is unchanged).Breaking Changes
None. The Rust, FFI and Swift update-service APIs are unchanged.
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 ·
c939f01/skip-botsproceeds without the ones not yet reported/self-reviewedonce the bots are doners-platform-wallet(packages/rs-platform-wallet/src/masternode/update_service.rs) — HashEngineering or ZocoLini or llbartekll or romchornyiWhen every box is checked the
PR Hygienecheck passes and this can merge.Summary by CodeRabbit