feat(platform-wallet): unban a PoSe-banned masternode — ProUpServTx orchestration, FFI, Swift - #4507
feat(platform-wallet): unban a PoSe-banned masternode — ProUpServTx orchestration, FFI, Swift#4507QuantumExplorer wants to merge 3 commits into
Conversation
Pin lineage chore/sync-fixes-without-swept + a cherry-pick of dashpay/rust-dashcore#991 (merged to dev as 5f2de2e0), pushed as chore/sync-fixes-payload-seam. The seam adds TransactionBuilder::set_payload_finalizer, needed to build ProUpServTx: its inputs_hash + operator-BLS payload_sig are only knowable after input selection and must land before input signing. Staying off dev head because dev's TransactionsSwept changes are not absorbed here yet. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ed masternode execute_masternode_update_service builds, operator-BLS-signs, funds, input-signs, and broadcasts the provider update service transaction that revives a PoSe-banned masternode or evonode, re-asserting its current service values from the live masternode-list entry (revive-only by design). The build rides key-wallet's new payload-finalization seam: selection reserves the funding inputs, the finalizer writes inputs_hash and the operator's basic-scheme BLS signature over base_payload_hash (the same convention verify_message_digest checks real mainnet signatures with), and only then are inputs ECDSA-signed, since their sighashes cover the finished payload. Guards, all before network work where possible: the operator secret must match the list entry's operator key under either serialization; an evonode payload requires the caller-supplied platform P2P port (the list does not carry it) and the entry's node id + HTTP port; and the operator payout script follows the owner-decided rule — operatorReward 0 (read from the fetched ProRegTx) always sends the empty script and forbids an address, non-zero requires the address explicitly, so an unban can never silently clear an operator payout on-chain. New PlatformWalletError::MasternodeListUnavailable mirrors the locator's list-unavailable outcome for FFI mapping. Nine tests, including an end-to-end funded build against a recording broadcaster with the BLS signature verified over base_payload_hash, and the IPv4-mapped LE encoding pinned against dashcore's known testnet ProUpServTx vector. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…wrappers Two additive entry points mirroring the withdraw pair, both funding the L1 fee from the wallet through the mnemonic resolver: - platform_wallet_manager_masternode_update_service — wallet-owned nodes; derives the operator key at the record's operator_key_index with the same three-phase seed resolution as platform_wallet_provider_key_at_index (resolver never under a wallet guard). - platform_wallet_manager_tracked_masternode_update_service — tracked nodes; parses the host-vaulted operator key text through parse_secret_for_role like the verify-key path. out_txid (32 wire-order bytes) is zero-initialised on every path and written on definitive success; an ambiguous broadcast returns the existing ErrorTransactionBroadcastUnconfirmed (never retry — inputs stay reserved). PlatformWalletError::MasternodeListUnavailable maps to the existing code 46. No new error codes; the tracked_masternode helpers are promoted to pub(crate) instead of copied. Swift: masternodeUpdateService / trackedMasternodeUpdateService follow the masternodeWithdraw resolver marshalling, returning the txid Data. Both existing Swift error cases (transactionBroadcastUnconfirmed, masternodeListUnavailable) already cover the new outcomes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 31 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (10)
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 |
|
⛔ Blockers found — Opus deferred (commit 9c7d370) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v4.2-dev #4507 +/- ##
============================================
+ Coverage 82.66% 83.38% +0.71%
============================================
Files 2744 2773 +29
Lines 370075 373250 +3175
============================================
+ Hits 305916 311223 +5307
+ Misses 64159 62027 -2132
🚀 New features to boost your workflow:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The payload finalization and signing order is sound, but two in-scope blockers remain: the payout guard trusts an unbound DAPI transaction, and v3 extended service entries are silently reduced to a v2 payload that downgrades and discards service information. The new FFI functions also violate their zero-on-error output contract and make avoidable non-zeroizing copies of the operator secret.
Source: reviewer backend gpt-5.6-sol (general, security-auditor, rust-quality, and ffi-engineer); final verifier backend gpt-5.6-sol. Orchestration only, not reviewer evidence: openclaw-agent/cliproxy/gpt-5.6-sol.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— rust-quality (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 2 blocking | 🟡 2 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-platform-wallet/src/masternode/update_service.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/masternode/update_service.rs:138-145: Verify the fetched ProRegTx matches the requested proTxHash
`Sdk::get_transaction` only decodes the bytes returned by DAPI; it does not verify that the decoded transaction has the requested txid. This code therefore trusts an unauthenticated response's `operator_reward`. A faulty or malicious endpoint can return an unrelated zero-reward ProRegTx for a target whose real reward is non-zero, causing `resolve_operator_payout_script` to accept an empty script. The resulting valid ProUpServTx targets the real masternode and clears its operator payout, defeating this PR's explicit payout-protection guarantee. Compare the decoded txid with the SPV-authenticated DML `pro_tx_hash` before reading the payload, and test a mismatched response.
- [BLOCKING] packages/rs-platform-wallet/src/masternode/update_service.rs:222-229: Reject or preserve v3 extended masternode service information
`MasternodeListSummary::from_entry` reduces both legacy and v3 `MasternodeNetInfo::Extended` entries to `primary_service_address()`, so this code cannot tell that an IPv4/IPv6 primary came from an extended map containing additional Core P2P, Platform P2P/HTTPS, domain, or fallback endpoints. It then always creates `ProviderUpdateServicePayload::CURRENT_VERSION`, which the pinned rust-dashcore defines as v2. After the v24 deployment, Dash Core accepts a v2 service update for a v3 state, sets the state version to the transaction version, and replaces the complete `netInfo` map with the supplied legacy address. The purported revive-only operation therefore downgrades a v3 entry and discards live endpoints instead of reasserting them, potentially taking services offline. Preserve the entry version and full extended network information and emit a v3 payload, or reject extended entries before funding until rust-dashcore supports that payload; add coverage for an extended entry with a routable primary address.
In `packages/rs-platform-wallet-ffi/src/masternode_update_service.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/masternode_update_service.rs:137-143: Do not copy the operator secret through an unzeroized array
`private` is a `Zeroizing<Vec<u8>>`, but `try_into()` copies it into the plain `bytes: [u8; 32]`. Because the array is `Copy`, passing it to `Zeroizing::new` leaves the named stack value outside zeroizing storage until the function returns. Copy directly into a zeroizing destination instead. The tracked-key arm at line 301 should likewise move the existing `Zeroizing<[u8; 32]>` out of `LocatorSecret::Bls` directly rather than dereferencing it into `Zeroizing::new(*secret)`.
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/masternode_update_service.rs:226-230: Zero the txid output before other pointer checks
`check_ptr!` returns immediately, so a valid `out_txid` remains unchanged whenever an earlier required input is null. This contradicts the PR's stated contract that `out_txid` is zero-initialized on every path and lets C-family callers observe stale transaction bytes after an error. Validate `out_txid` first, clear it, and only then validate the input pointers. Apply the same ordering to the tracked-masternode extern at lines 286-291, and initialize the null-pointer tests with a non-zero sentinel.
| match fetched.transaction.special_transaction_payload { | ||
| Some(TransactionPayload::ProviderRegistrationPayloadType(registration)) => { | ||
| Ok(registration.operator_reward) | ||
| } | ||
| _ => Err(PlatformWalletError::InvalidParameter(format!( | ||
| "transaction {display} is not a provider registration transaction" | ||
| ))), | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Verify the fetched ProRegTx matches the requested proTxHash
Sdk::get_transaction only decodes the bytes returned by DAPI; it does not verify that the decoded transaction has the requested txid. This code therefore trusts an unauthenticated response's operator_reward. A faulty or malicious endpoint can return an unrelated zero-reward ProRegTx for a target whose real reward is non-zero, causing resolve_operator_payout_script to accept an empty script. The resulting valid ProUpServTx targets the real masternode and clears its operator payout, defeating this PR's explicit payout-protection guarantee. Compare the decoded txid with the SPV-authenticated DML pro_tx_hash before reading the payload, and test a mismatched response.
| match fetched.transaction.special_transaction_payload { | |
| Some(TransactionPayload::ProviderRegistrationPayloadType(registration)) => { | |
| Ok(registration.operator_reward) | |
| } | |
| _ => Err(PlatformWalletError::InvalidParameter(format!( | |
| "transaction {display} is not a provider registration transaction" | |
| ))), | |
| } | |
| let fetched_txid = fetched.transaction.txid(); | |
| let expected_txid = Txid::from_byte_array(*pro_tx_hash); | |
| if fetched_txid != expected_txid { | |
| return Err(PlatformWalletError::InvalidIdentityData(format!( | |
| "DAPI returned transaction {fetched_txid} for requested registration transaction \ | |
| {display}" | |
| ))); | |
| } | |
| match fetched.transaction.special_transaction_payload { | |
| Some(TransactionPayload::ProviderRegistrationPayloadType(registration)) => { | |
| Ok(registration.operator_reward) | |
| } | |
| _ => Err(PlatformWalletError::InvalidParameter(format!( | |
| "transaction {display} is not a provider registration transaction" | |
| ))), | |
| } |
source: ['codex']
| let service = entry.service_address.ok_or_else(|| { | ||
| PlatformWalletError::InvalidParameter( | ||
| "the masternode's service address is not a plain IP:port entry, so it cannot be \ | ||
| re-asserted from the masternode list" | ||
| .to_string(), | ||
| ) | ||
| })?; | ||
| let (ip_address, port) = service_payload_fields(service); |
There was a problem hiding this comment.
🔴 Blocking: Reject or preserve v3 extended masternode service information
MasternodeListSummary::from_entry reduces both legacy and v3 MasternodeNetInfo::Extended entries to primary_service_address(), so this code cannot tell that an IPv4/IPv6 primary came from an extended map containing additional Core P2P, Platform P2P/HTTPS, domain, or fallback endpoints. It then always creates ProviderUpdateServicePayload::CURRENT_VERSION, which the pinned rust-dashcore defines as v2. After the v24 deployment, Dash Core accepts a v2 service update for a v3 state, sets the state version to the transaction version, and replaces the complete netInfo map with the supplied legacy address. The purported revive-only operation therefore downgrades a v3 entry and discards live endpoints instead of reasserting them, potentially taking services offline. Preserve the entry version and full extended network information and emit a v3 payload, or reject extended entries before funding until rust-dashcore supports that payload; add coverage for an extended entry with a routable primary address.
source: ['codex']
| let bytes: [u8; 32] = private.as_slice().try_into().map_err(|_| { | ||
| PlatformWalletFFIResult::err( | ||
| PlatformWalletFFIResultCode::ErrorWalletOperation, | ||
| "the derived operator private key is not 32 bytes", | ||
| ) | ||
| })?; | ||
| Ok(Zeroizing::new(bytes)) |
There was a problem hiding this comment.
🟡 Suggestion: Do not copy the operator secret through an unzeroized array
private is a Zeroizing<Vec<u8>>, but try_into() copies it into the plain bytes: [u8; 32]. Because the array is Copy, passing it to Zeroizing::new leaves the named stack value outside zeroizing storage until the function returns. Copy directly into a zeroizing destination instead. The tracked-key arm at line 301 should likewise move the existing Zeroizing<[u8; 32]> out of LocatorSecret::Bls directly rather than dereferencing it into Zeroizing::new(*secret).
| let bytes: [u8; 32] = private.as_slice().try_into().map_err(|_| { | |
| PlatformWalletFFIResult::err( | |
| PlatformWalletFFIResultCode::ErrorWalletOperation, | |
| "the derived operator private key is not 32 bytes", | |
| ) | |
| })?; | |
| Ok(Zeroizing::new(bytes)) | |
| if private.len() != 32 { | |
| return Err(PlatformWalletFFIResult::err( | |
| PlatformWalletFFIResultCode::ErrorWalletOperation, | |
| "the derived operator private key is not 32 bytes", | |
| )); | |
| } | |
| let mut bytes = Zeroizing::new([0u8; 32]); | |
| bytes.copy_from_slice(private.as_slice()); | |
| Ok(bytes) |
source: ['codex']
| check_ptr!(wallet_id); | ||
| check_ptr!(pro_tx_hash); | ||
| check_ptr!(mnemonic_resolver_handle); | ||
| check_ptr!(out_txid); | ||
| *out_txid = [0u8; 32]; |
There was a problem hiding this comment.
🟡 Suggestion: Zero the txid output before other pointer checks
check_ptr! returns immediately, so a valid out_txid remains unchanged whenever an earlier required input is null. This contradicts the PR's stated contract that out_txid is zero-initialized on every path and lets C-family callers observe stale transaction bytes after an error. Validate out_txid first, clear it, and only then validate the input pointers. Apply the same ordering to the tracked-masternode extern at lines 286-291, and initialize the null-pointer tests with a non-zero sentinel.
| check_ptr!(wallet_id); | |
| check_ptr!(pro_tx_hash); | |
| check_ptr!(mnemonic_resolver_handle); | |
| check_ptr!(out_txid); | |
| *out_txid = [0u8; 32]; | |
| check_ptr!(out_txid); | |
| *out_txid = [0u8; 32]; | |
| check_ptr!(wallet_id); | |
| check_ptr!(pro_tx_hash); | |
| check_ptr!(mnemonic_resolver_handle); |
source: ['codex']
2 participants
What
Adds the full below-app stack for unbanning a PoSe-banned masternode/evonode from the mobile wallets: a ProUpServTx orchestrator in
platform-wallet, two additive FFI entry points mirroring the withdraw pair, and swift-sdk wrappers. Second of the three-PR sequence (rust-dashcore#991 → this → the wallet UI).How
Pin bump (commit 1): rust-dashcore moves
3d13d983→4db5c367— the existingchore/sync-fixes-without-sweptlineage plus a cherry-pick of the merged dashpay/rust-dashcore#991 payload-finalization seam (pushed aschore/sync-fixes-payload-seam). Staying offdevhead because itsTransactionsSweptbreaking changes are not absorbed here yet.Orchestrator (commit 2):
execute_masternode_update_servicere-asserts the node's current service values from the live DML entry (revive-only by design) and rides the seam: selection reserves the funding inputs →inputs_hash→ operator BLSpayload_sig(basic scheme overbase_payload_hash, modern serialization — the same conventionverify_message_digestchecks real mainnet signatures with) → input ECDSA signing (sighashes cover the finished payload) → broadcast viaCoreWallet::finalize_transaction+broadcast_finalized_transaction(release on Rejected, keep reserved on MaybeSent).Guards, before network work wherever possible:
verify_masternode_key);mn_typeset explicitly so the serializer can't silently drop the triplet;operatorRewardis read from the fetched ProRegTx; reward 0 ⇒ always the empty script and an address is refused (consensus forbids one anyway); non-zero ⇒ the address must be supplied explicitly — the payload replaces the payout script on-chain, so an unban can never silently clear an operator payout.New
PlatformWalletError::MasternodeListUnavailablemaps to the existing FFI code 46. No new error codes.FFI + Swift (commit 3):
platform_wallet_manager_masternode_update_service(wallet-owned; operator key derived at the record'soperator_key_indexwith the three-phase seed resolution fromplatform_wallet_provider_key_at_index) andplatform_wallet_manager_tracked_masternode_update_service(host-vaulted key text viaparse_secret_for_role). Both fund the fee from the wallet through the mnemonic resolver.out_txidzero-initialised on every path, written on definitive success; ambiguous broadcasts return the existingErrorTransactionBroadcastUnconfirmed(never retry). Swift wrappers follow themasternodeWithdrawmarshalling; both needed Swift error cases already exist.Tests / verification
base_payload_hash, and the IPv4-mapped LE encoding pinned against dashcore's known testnet ProUpServTx vector.cargo fmt, workspace clippy-D warnings,cargo check --workspace --all-features(which compilesrs-unified-sdk-jniagainst the new externs — additive only, nothing existing changed), and the three wallet-crate test suites: platform-wallet 937 passed with the one pre-existing v4.2-dev failure (regression_reports_max_from_usable_suffix_not_total_account_balance, fixture invalidated by feat(dpp)!: rebalance the shielded fee constants for protocol 14 #4467 — unrelated, this diff doesn't touch shielded selection); platform-wallet-ffi and platform-wallet-storage fully green.build_ios.sh --target simsucceeds end to end including the SwiftExampleApp link, so the new Swift wrappers compile against the regenerated headers.🤖 Generated with Claude Code