feat(kotlin-sdk): split build/broadcast with reservation release for BIP70-style deferred submission#4090
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 ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 |
|
⛔ Blockers found — Sonnet deferred (commit 84ef267) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
This PR adds a well-tested SignedPaymentRegistry enabling BIP70-style deferred build→broadcast/release for Core sends, with clean Rust idioms (no unwrap, correct pre-await mutex release, double-broadcast prevention). Verification against the actual code confirms one real blocking defect: registry tokens never expire even though the underlying key-wallet UTXO reservation they depend on auto-expires after 24 blocks (~1hr) and is released by raw outpoint with no ownership check, so a delayed merchant ack/nack on a long-outstanding token can silently corrupt or double-spend a completely unrelated, later-built transaction that reused the same now-expired UTXO. Several other issues (wallet-instance binding gaps, an architecture-rule violation on the Kotlin JNI boundary, and orphan-on-failure edge cases) are real but lower-impact.
Source (experiment sonnet-primary-opus-quarter-sample-20260710, cohort sonnet_opus_sample, bucket 0): reviewers codex/general=gpt-5.6-sol(completed); sonnet5/general=claude-sonnet-5(completed); opus/general=claude-opus-4-8(completed); codex/security-auditor=gpt-5.6-sol(completed); sonnet5/security-auditor=claude-sonnet-5(completed); opus/security-auditor=claude-opus-4-8(completed); codex/rust-quality=gpt-5.6-sol(completed); sonnet5/rust-quality=claude-sonnet-5(completed); opus/rust-quality=claude-opus-4-8(completed); codex/ffi-engineer=gpt-5.6-sol(completed); sonnet5/ffi-engineer=claude-sonnet-5(completed); opus/ffi-engineer=claude-opus-4-8(completed); verifier=verifier-sonnet5-4090-1783717967=claude-sonnet-5; orchestrator=openai/gpt-5.6-sol reasoning=high (orchestration-only, not a reviewer/verifier).
🔴 1 blocking | 🟡 4 suggestion(s) | 💬 2 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-platform-wallet/src/wallet/signed_payment_registry.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/signed_payment_registry.rs:130-230: Registry tokens never expire, but the UTXO reservation they depend on does — release-by-outpoint can corrupt a newer, unrelated reservation
Verified against key-wallet's `ReservationSet` (`managed_account/reservation.rs`): `RESERVATION_TTL_BLOCKS = 24` (~1hr), and any later `reserve()`/`reserved()` call sweeps entries older than that, silently returning the outpoint to the selectable pool. `release()` then removes an outpoint unconditionally (`reserved.remove(outpoint)`) with no generation/ownership check.
`SignedPaymentRegistry` entries carry no age/height and are never swept — a token stays broadcastable/releasable indefinitely until an explicit `broadcast`/`release` call. This PR's entire purpose is to widen the build→submit gap for BIP70/BIP270 flows where the merchant ack is asynchronous and can legitimately take longer than an hour (slow, unresponsive, or simply a user who steps away).
Concrete failure sequence: (1) build+register payment A, reserving UTXO X at height H; merchant never acks. (2) At height ≥ H+24, X's reservation is swept on the next unrelated `reserve`/`reserved` call. (3) A completely different payment B is built, reselecting and re-reserving X at the new height. (4) The merchant for A finally acks; `broadcastSigned(tokenA)` runs. If the network rejects A's now-conflicting tx, `broadcast_releasing_on_rejection` releases X by outpoint — which removes B's *active* reservation, letting a third build reselect X while B's transaction may still be in flight, risking a real double-spend. If A's broadcast instead succeeds first, B's later build/broadcast fails unpredictably. Either way the outcome is driven entirely by third-party (merchant) timing outside the wallet's control.
The registry needs either a bounded token lifetime enforced client-side (reject broadcast/require re-registration once the reservation could plausibly have been swept) or a generation-aware release that only frees an outpoint if it still belongs to the same token's reservation.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/signed_payment_registry.rs:183-186: Wallet-instance guard only detects a different PlatformWalletManager, not a different wallet within the same manager
Verified: `PlatformWalletManager` (`manager/mod.rs:39-47`) is documented as a "Multi-wallet coordinator" holding exactly one `wallet_manager: Arc<RwLock<WalletManager<PlatformWalletInfo>>>` shared by every registered wallet (`wallets: Arc<RwLock<BTreeMap<WalletId, Arc<PlatformWallet>>>>`), and `PlatformWallet::new`/`platform_wallet_get_core` construct every `CoreWallet` from that same cloned Arc. So `Arc::ptr_eq(&entry.core.wallet_manager, ¤t.wallet_manager)` is true for *any two wallets in the same manager* — it only catches recreation of the whole manager (e.g. the documented network-switch destroy+recreate), not per-wallet removal/recreation or cross-wallet token reuse within one live manager.
Impact is bounded, not a fund-safety bypass: the actual broadcast/release always operates through `entry.core` (the wallet captured at registration), which still resolves the correct `wallet_id`/account internally — a caller can't spend wallet B's funds with wallet A's token. But the doc comment's claim that a token is "bound to the exact wallet instance it was minted against" is only true across manager instances, not across wallets sharing one manager, and the existing test (`broadcast_rejects_a_different_wallet_instance`) only covers the cross-manager case with two entirely separate `WalletManager`s, missing the realistic same-manager, different-`wallet_id` scenario this multi-wallet coordinator is built for. Tighten the check to also compare `entry.core.wallet_id()` against `current.wallet_id()`.
In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedCoreWallet.kt`:
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedCoreWallet.kt:49-71: registerSignedPayment stitches two native round-trips in Kotlin, contrary to this repo's kotlin-sdk rule
`registerSignedPayment` calls `coreTransactionGetBytes(tx.handle)` and then `coreWalletRegisterSignedPayment(...)`, stitching the two results into one `SignedCoreTransaction` with hand-rolled `ByteBuffer` decoding. `packages/kotlin-sdk/CLAUDE.md` states explicitly: "No JNI functions that stitch together existing Rust calls — add the composite to Rust instead." Confirmed `core_wallet_transaction_get_bytes`/`coreTransactionGetBytes` has exactly one Kotlin call site — this one — so there's no other caller relying on it staying separate.
`core_wallet_signed_payment_register` (`rs-platform-wallet-ffi/src/core_wallet/signed_payment.rs:84`) already has `(*tx).bytes()` in scope (it uses it to deserialize the transaction at line 100); it could copy those bytes into an additional out-parameter and return everything in one native round trip, removing `coreTransactionGetBytes` from this path entirely.
In `packages/rs-platform-wallet-ffi/src/core_wallet/signed_payment.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/core_wallet/signed_payment.rs:84-134: Reservation-holding register() can orphan its UTXO reservation on failure paths after the side effect commits
`core_wallet_signed_payment_register` calls `SIGNED_PAYMENT_REGISTRY.register(core, transaction, ...)` at line 112 — an irreversible side effect that mints a token and keeps `build_signed`'s UTXO reservation held — before the fallible `CString::new(txid.to_string())` at line 120. If that (admittedly near-impossible, since txid hex can't contain a NUL) conversion fails, the function returns an error without ever writing `out_token`, orphaning the reservation with no token to release it.
The same class of bug reaches further down the JNI boundary: in `rs-unified-sdk-jni/src/wallet_manager.rs` (`coreWalletRegisterSignedPayment`, lines ~1053-1090), the native call already committed the registration by the time `env.byte_array_from_slice(&blob)` runs at line 1087. That call's failure path is `.unwrap_or(ptr::null_mut())` — no exception thrown, no token returned to Kotlin, same orphaned-reservation outcome (this time reachable on JVM allocation pressure rather than an impossible NUL byte).
Both paths leave the reservation to be reclaimed only by the 24-block TTL backstop. Reorder the fallible/pure steps before the registry insert on the Rust side, and release the token on the JNI marshalling-failure path.
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/core_wallet/signed_payment.rs:98: Registry captures its own CoreWallet Arc clone, bypassing core_wallet_destroy/platform_wallet_destroy teardown
`register()` clones the `CoreWallet<B>` from `CORE_WALLET_STORAGE` (line 98) and stores it inside `RegisteredPayment` in the process-global `SIGNED_PAYMENT_REGISTRY` — independent of the handle-storage lifecycle. `core_wallet_destroy`/`platform_wallet_destroy` only remove the handle-storage entry, dropping *that* Arc reference; they have no visibility into the registry.
If a caller registers a token and never calls `broadcastSigned`/`releaseReservation` on it (app killed mid-flow, or — per `packages/kotlin-sdk/CLAUDE.md`'s own documented pattern, "Network switch = destroy + new instance" — a network switch destroys the `PlatformWalletManager` while a token is outstanding), the registry entry keeps the entire `WalletManager` (accounts, keys, sync state) alive for the rest of the process. Not memory-unsafe, but it silently defeats the destroy-wallet invariant the SDK's own docs rely on, with no error surfaced to the caller.
| pub fn register( | ||
| &self, | ||
| core: CoreWallet<B>, | ||
| tx: Transaction, | ||
| account_type: Option<StandardAccountType>, | ||
| account_index: u32, | ||
| ) -> ReservationToken { | ||
| let token = self.next_token.fetch_add(1, Ordering::SeqCst); | ||
| self.entries | ||
| .lock() | ||
| .expect("signed-payment registry mutex poisoned") | ||
| .insert( | ||
| token, | ||
| RegisteredPayment { | ||
| core, | ||
| tx, | ||
| account_type, | ||
| account_index, | ||
| }, | ||
| ); | ||
| token | ||
| } | ||
|
|
||
| /// Broadcast the payment behind `token`, reconciling its UTXO reservation on | ||
| /// failure, then consume the token. | ||
| /// | ||
| /// The entry is removed **before** the send, so a repeated or concurrent | ||
| /// broadcast of the same token gets [`SignedPaymentError::StaleToken`] | ||
| /// instead of a second send. `current` must be the same wallet instance the | ||
| /// token was minted against (checked by `Arc::ptr_eq` on the shared | ||
| /// `WalletManager`); otherwise the call fails with | ||
| /// [`SignedPaymentError::WalletMismatch`] and the stale token is dropped. | ||
| /// | ||
| /// On a definitive rejection the reservation is released for an immediate | ||
| /// rebuild; on an ambiguous ("may already be on the network") failure it is | ||
| /// kept — the same policy as the non-deferred send path. | ||
| pub async fn broadcast( | ||
| &self, | ||
| token: ReservationToken, | ||
| current: &CoreWallet<B>, | ||
| ) -> Result<Txid, SignedPaymentError> { | ||
| // Remove under the lock and drop the guard *before* awaiting — a | ||
| // std::Mutex guard must never be held across an await point, and the | ||
| // atomic take is what makes a double-broadcast impossible. | ||
| let entry = { | ||
| let mut entries = self | ||
| .entries | ||
| .lock() | ||
| .expect("signed-payment registry mutex poisoned"); | ||
| entries.remove(&token) | ||
| } | ||
| .ok_or(SignedPaymentError::StaleToken(token))?; | ||
|
|
||
| if !Arc::ptr_eq(&entry.core.wallet_manager, ¤t.wallet_manager) { | ||
| // The token belongs to another wallet instance; it has been removed, | ||
| // so it can never be replayed here. | ||
| return Err(SignedPaymentError::WalletMismatch(token)); | ||
| } | ||
|
|
||
| let txid = match entry.account_type { | ||
| Some(account_type) => { | ||
| entry | ||
| .core | ||
| .broadcast_transaction_releasing_reservation( | ||
| account_type, | ||
| entry.account_index, | ||
| &entry.tx, | ||
| ) | ||
| .await? | ||
| } | ||
| None => entry.core.broadcast_transaction(&entry.tx).await?, | ||
| }; | ||
| Ok(txid) | ||
| } | ||
|
|
||
| /// Release the funding reservation behind `token` and drop it. Idempotent: | ||
| /// releasing an unknown / already-consumed token is a silent no-op, so a | ||
| /// double release (or a release after a broadcast) is harmless. | ||
| /// | ||
| /// The release acts on the wallet instance the token was minted against — | ||
| /// the one whose `ReservationSet` actually holds the inputs — so no wallet | ||
| /// handle need be threaded in. | ||
| pub async fn release(&self, token: ReservationToken) { | ||
| let entry = { | ||
| let mut entries = self | ||
| .entries | ||
| .lock() | ||
| .expect("signed-payment registry mutex poisoned"); | ||
| entries.remove(&token) | ||
| }; | ||
| let Some(entry) = entry else { | ||
| // Unknown / already consumed — idempotent no-op. | ||
| return; | ||
| }; | ||
| if let Some(account_type) = entry.account_type { | ||
| entry | ||
| .core | ||
| .release_transaction_reservation(account_type, entry.account_index, &entry.tx) | ||
| .await; | ||
| } | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Registry tokens never expire, but the UTXO reservation they depend on does — release-by-outpoint can corrupt a newer, unrelated reservation
Verified against key-wallet's ReservationSet (managed_account/reservation.rs): RESERVATION_TTL_BLOCKS = 24 (~1hr), and any later reserve()/reserved() call sweeps entries older than that, silently returning the outpoint to the selectable pool. release() then removes an outpoint unconditionally (reserved.remove(outpoint)) with no generation/ownership check.
SignedPaymentRegistry entries carry no age/height and are never swept — a token stays broadcastable/releasable indefinitely until an explicit broadcast/release call. This PR's entire purpose is to widen the build→submit gap for BIP70/BIP270 flows where the merchant ack is asynchronous and can legitimately take longer than an hour (slow, unresponsive, or simply a user who steps away).
Concrete failure sequence: (1) build+register payment A, reserving UTXO X at height H; merchant never acks. (2) At height ≥ H+24, X's reservation is swept on the next unrelated reserve/reserved call. (3) A completely different payment B is built, reselecting and re-reserving X at the new height. (4) The merchant for A finally acks; broadcastSigned(tokenA) runs. If the network rejects A's now-conflicting tx, broadcast_releasing_on_rejection releases X by outpoint — which removes B's active reservation, letting a third build reselect X while B's transaction may still be in flight, risking a real double-spend. If A's broadcast instead succeeds first, B's later build/broadcast fails unpredictably. Either way the outcome is driven entirely by third-party (merchant) timing outside the wallet's control.
The registry needs either a bounded token lifetime enforced client-side (reject broadcast/require re-registration once the reservation could plausibly have been swept) or a generation-aware release that only frees an outpoint if it still belongs to the same token's reservation.
source: ['claude', 'codex']
There was a problem hiding this comment.
Still valid at a428f54 — the new 20-block registry guard reads synced_height(), while the production reservation is stamped with last_processed_height(), so the clocks can diverge and the original reservation-ownership risk remains. The current-head blocking thread is #4090 (comment).
The earlier automated resolution reply was corrected because title-based reconciliation failed to match the reworded carried-forward finding.
| if !Arc::ptr_eq(&entry.core.wallet_manager, ¤t.wallet_manager) { | ||
| // The token belongs to another wallet instance; it has been removed, | ||
| // so it can never be replayed here. | ||
| return Err(SignedPaymentError::WalletMismatch(token)); |
There was a problem hiding this comment.
🟡 Suggestion: Wallet-instance guard only detects a different PlatformWalletManager, not a different wallet within the same manager
Verified: PlatformWalletManager (manager/mod.rs:39-47) is documented as a "Multi-wallet coordinator" holding exactly one wallet_manager: Arc<RwLock<WalletManager<PlatformWalletInfo>>> shared by every registered wallet (wallets: Arc<RwLock<BTreeMap<WalletId, Arc<PlatformWallet>>>>), and PlatformWallet::new/platform_wallet_get_core construct every CoreWallet from that same cloned Arc. So Arc::ptr_eq(&entry.core.wallet_manager, ¤t.wallet_manager) is true for any two wallets in the same manager — it only catches recreation of the whole manager (e.g. the documented network-switch destroy+recreate), not per-wallet removal/recreation or cross-wallet token reuse within one live manager.
Impact is bounded, not a fund-safety bypass: the actual broadcast/release always operates through entry.core (the wallet captured at registration), which still resolves the correct wallet_id/account internally — a caller can't spend wallet B's funds with wallet A's token. But the doc comment's claim that a token is "bound to the exact wallet instance it was minted against" is only true across manager instances, not across wallets sharing one manager, and the existing test (broadcast_rejects_a_different_wallet_instance) only covers the cross-manager case with two entirely separate WalletManagers, missing the realistic same-manager, different-wallet_id scenario this multi-wallet coordinator is built for. Tighten the check to also compare entry.core.wallet_id() against current.wallet_id().
| if !Arc::ptr_eq(&entry.core.wallet_manager, ¤t.wallet_manager) { | |
| // The token belongs to another wallet instance; it has been removed, | |
| // so it can never be replayed here. | |
| return Err(SignedPaymentError::WalletMismatch(token)); | |
| if !Arc::ptr_eq(&entry.core.wallet_manager, ¤t.wallet_manager) | |
| || entry.core.wallet_id() != current.wallet_id() | |
| { | |
| return Err(SignedPaymentError::WalletMismatch(token)); | |
| } |
source: ['codex']
There was a problem hiding this comment.
Resolved in a428f54 — Wallet-instance guard only detects a different PlatformWalletManager, not a different wallet within the same manager 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.
| internal fun registerSignedPayment( | ||
| tx: CoreTransaction, | ||
| ): ManagedPlatformWallet.SignedCoreTransaction { | ||
| val rawTxBytes = WalletManagerNative.coreTransactionGetBytes(tx.handle) | ||
| val blob = WalletManagerNative.coreWalletRegisterSignedPayment( | ||
| handle, | ||
| tx.handle, | ||
| tx.accountType.ffiValue, | ||
| tx.accountIndex, | ||
| ) | ||
| val buffer = java.nio.ByteBuffer.wrap(blob) // big-endian by default | ||
| val token = buffer.long | ||
| val feeDuffs = buffer.long | ||
| val txidLen = buffer.int | ||
| val txidBytes = ByteArray(txidLen) | ||
| buffer.get(txidBytes) | ||
| return ManagedPlatformWallet.SignedCoreTransaction( | ||
| txidHex = String(txidBytes, Charsets.UTF_8), | ||
| rawTxBytes = rawTxBytes, | ||
| feeDuffs = feeDuffs, | ||
| reservationToken = token, | ||
| ) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: registerSignedPayment stitches two native round-trips in Kotlin, contrary to this repo's kotlin-sdk rule
registerSignedPayment calls coreTransactionGetBytes(tx.handle) and then coreWalletRegisterSignedPayment(...), stitching the two results into one SignedCoreTransaction with hand-rolled ByteBuffer decoding. packages/kotlin-sdk/CLAUDE.md states explicitly: "No JNI functions that stitch together existing Rust calls — add the composite to Rust instead." Confirmed core_wallet_transaction_get_bytes/coreTransactionGetBytes has exactly one Kotlin call site — this one — so there's no other caller relying on it staying separate.
core_wallet_signed_payment_register (rs-platform-wallet-ffi/src/core_wallet/signed_payment.rs:84) already has (*tx).bytes() in scope (it uses it to deserialize the transaction at line 100); it could copy those bytes into an additional out-parameter and return everything in one native round trip, removing coreTransactionGetBytes from this path entirely.
source: ['claude']
There was a problem hiding this comment.
Resolved in a428f54 — registerSignedPayment stitches two native round-trips in Kotlin, contrary to this repo's kotlin-sdk rule 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.
| pub unsafe extern "C" fn core_wallet_signed_payment_register( | ||
| core_handle: Handle, | ||
| tx: *const FFICoreTransaction, | ||
| account_type: CoreAccountTypeFFI, | ||
| account_index: u32, | ||
| out_token: *mut u64, | ||
| out_fee: *mut u64, | ||
| out_txid: *mut *mut c_char, | ||
| ) -> PlatformWalletFFIResult { | ||
| check_ptr!(tx); | ||
| check_ptr!(out_token); | ||
| check_ptr!(out_fee); | ||
| check_ptr!(out_txid); | ||
|
|
||
| let core = unwrap_option_or_return!(CORE_WALLET_STORAGE.with_item(core_handle, |w| w.clone())); | ||
|
|
||
| let transaction: dashcore::Transaction = match dashcore::consensus::deserialize((*tx).bytes()) { | ||
| Ok(t) => t, | ||
| Err(e) => { | ||
| return PlatformWalletFFIResult::err( | ||
| PlatformWalletFFIResultCode::ErrorDeserialization, | ||
| format!("failed to deserialize signed transaction: {e}"), | ||
| ); | ||
| } | ||
| }; | ||
| let txid = transaction.txid(); | ||
| let fee = (*tx).fee(); | ||
|
|
||
| let token = SIGNED_PAYMENT_REGISTRY.register( | ||
| core, | ||
| transaction, | ||
| account_type.as_standard_account_type(), | ||
| account_index, | ||
| ); | ||
|
|
||
| // txid hex never contains a NUL, but handle the impossible case anyway. | ||
| let c_txid = match CString::new(txid.to_string()) { | ||
| Ok(s) => s, | ||
| Err(_) => { | ||
| return PlatformWalletFFIResult::err( | ||
| PlatformWalletFFIResultCode::ErrorUtf8Conversion, | ||
| "txid string contained an interior NUL".to_string(), | ||
| ); | ||
| } | ||
| }; | ||
|
|
||
| *out_token = token; | ||
| *out_fee = fee; | ||
| *out_txid = c_txid.into_raw(); | ||
| PlatformWalletFFIResult::ok() | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Reservation-holding register() can orphan its UTXO reservation on failure paths after the side effect commits
core_wallet_signed_payment_register calls SIGNED_PAYMENT_REGISTRY.register(core, transaction, ...) at line 112 — an irreversible side effect that mints a token and keeps build_signed's UTXO reservation held — before the fallible CString::new(txid.to_string()) at line 120. If that (admittedly near-impossible, since txid hex can't contain a NUL) conversion fails, the function returns an error without ever writing out_token, orphaning the reservation with no token to release it.
The same class of bug reaches further down the JNI boundary: in rs-unified-sdk-jni/src/wallet_manager.rs (coreWalletRegisterSignedPayment, lines ~1053-1090), the native call already committed the registration by the time env.byte_array_from_slice(&blob) runs at line 1087. That call's failure path is .unwrap_or(ptr::null_mut()) — no exception thrown, no token returned to Kotlin, same orphaned-reservation outcome (this time reachable on JVM allocation pressure rather than an impossible NUL byte).
Both paths leave the reservation to be reclaimed only by the 24-block TTL backstop. Reorder the fallible/pure steps before the registry insert on the Rust side, and release the token on the JNI marshalling-failure path.
| pub unsafe extern "C" fn core_wallet_signed_payment_register( | |
| core_handle: Handle, | |
| tx: *const FFICoreTransaction, | |
| account_type: CoreAccountTypeFFI, | |
| account_index: u32, | |
| out_token: *mut u64, | |
| out_fee: *mut u64, | |
| out_txid: *mut *mut c_char, | |
| ) -> PlatformWalletFFIResult { | |
| check_ptr!(tx); | |
| check_ptr!(out_token); | |
| check_ptr!(out_fee); | |
| check_ptr!(out_txid); | |
| let core = unwrap_option_or_return!(CORE_WALLET_STORAGE.with_item(core_handle, |w| w.clone())); | |
| let transaction: dashcore::Transaction = match dashcore::consensus::deserialize((*tx).bytes()) { | |
| Ok(t) => t, | |
| Err(e) => { | |
| return PlatformWalletFFIResult::err( | |
| PlatformWalletFFIResultCode::ErrorDeserialization, | |
| format!("failed to deserialize signed transaction: {e}"), | |
| ); | |
| } | |
| }; | |
| let txid = transaction.txid(); | |
| let fee = (*tx).fee(); | |
| let token = SIGNED_PAYMENT_REGISTRY.register( | |
| core, | |
| transaction, | |
| account_type.as_standard_account_type(), | |
| account_index, | |
| ); | |
| // txid hex never contains a NUL, but handle the impossible case anyway. | |
| let c_txid = match CString::new(txid.to_string()) { | |
| Ok(s) => s, | |
| Err(_) => { | |
| return PlatformWalletFFIResult::err( | |
| PlatformWalletFFIResultCode::ErrorUtf8Conversion, | |
| "txid string contained an interior NUL".to_string(), | |
| ); | |
| } | |
| }; | |
| *out_token = token; | |
| *out_fee = fee; | |
| *out_txid = c_txid.into_raw(); | |
| PlatformWalletFFIResult::ok() | |
| } | |
| match env.byte_array_from_slice(&blob) { | |
| Ok(array) => array.into_raw(), | |
| Err(_) => { | |
| let _ = unsafe { | |
| platform_wallet_ffi::core_wallet_signed_payment_release(token) | |
| }; | |
| ptr::null_mut() | |
| } | |
| } |
source: ['claude', 'codex']
There was a problem hiding this comment.
Resolved in a428f54 — Reservation-holding register() can orphan its UTXO reservation on failure paths after the side effect commits 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.
| check_ptr!(out_fee); | ||
| check_ptr!(out_txid); | ||
|
|
||
| let core = unwrap_option_or_return!(CORE_WALLET_STORAGE.with_item(core_handle, |w| w.clone())); |
There was a problem hiding this comment.
🟡 Suggestion: Registry captures its own CoreWallet Arc clone, bypassing core_wallet_destroy/platform_wallet_destroy teardown
register() clones the CoreWallet<B> from CORE_WALLET_STORAGE (line 98) and stores it inside RegisteredPayment in the process-global SIGNED_PAYMENT_REGISTRY — independent of the handle-storage lifecycle. core_wallet_destroy/platform_wallet_destroy only remove the handle-storage entry, dropping that Arc reference; they have no visibility into the registry.
If a caller registers a token and never calls broadcastSigned/releaseReservation on it (app killed mid-flow, or — per packages/kotlin-sdk/CLAUDE.md's own documented pattern, "Network switch = destroy + new instance" — a network switch destroys the PlatformWalletManager while a token is outstanding), the registry entry keeps the entire WalletManager (accounts, keys, sync state) alive for the rest of the process. Not memory-unsafe, but it silently defeats the destroy-wallet invariant the SDK's own docs rely on, with no error surfaced to the caller.
source: ['claude']
There was a problem hiding this comment.
Resolved in a428f54 — Registry captures its own CoreWallet Arc clone, bypassing core_wallet_destroy/platform_wallet_destroy teardown 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.
| let entry = { | ||
| let mut entries = self | ||
| .entries | ||
| .lock() | ||
| .expect("signed-payment registry mutex poisoned"); | ||
| entries.remove(&token) | ||
| } | ||
| .ok_or(SignedPaymentError::StaleToken(token))?; | ||
|
|
||
| if !Arc::ptr_eq(&entry.core.wallet_manager, ¤t.wallet_manager) { | ||
| // The token belongs to another wallet instance; it has been removed, | ||
| // so it can never be replayed here. | ||
| return Err(SignedPaymentError::WalletMismatch(token)); |
There was a problem hiding this comment.
💬 Nitpick: std::Mutex poisoning permanently disables the process-global deferred-payment surface
register/broadcast/release all call .expect("signed-payment registry mutex poisoned") on the entries lock (lines 140, 178, 217). Since SIGNED_PAYMENT_REGISTRY is a single process-global, any panic anywhere while this lock is held poisons it for the entire process — every subsequent call then panics via .expect(), permanently disabling BIP70 deferred payments (for every wallet) until the app restarts. Availability hardening rather than an exploitable defect, but worth a poisoned.into_inner() recovery given the registry's process-wide blast radius — the sibling ReservationSet::lock() in key-wallet already does exactly this.
source: ['claude']
There was a problem hiding this comment.
Resolved in a428f54 — std::Mutex poisoning permanently disables the process-global deferred-payment surface 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.
| /// Opaque handle to a registered, signed-but-unsent payment. Minted by | ||
| /// [`SignedPaymentRegistry::register`]; consumed by | ||
| /// [`SignedPaymentRegistry::broadcast`] or | ||
| /// [`SignedPaymentRegistry::release`]. Values are unique for the process | ||
| /// lifetime and never reused, so a stale token can always be recognised. | ||
| pub type ReservationToken = u64; |
There was a problem hiding this comment.
💬 Nitpick: ReservationToken is documented as opaque but is only a bare u64 alias
pub type ReservationToken = u64 gives Rust callers no domain separation for a capability-like, process-scoped handle whose uniqueness/provenance matters. A #[repr(transparent)] newtype would preserve the C ABI while catching accidental misuse of unrelated numeric IDs at compile time. Low priority — style/hygiene only.
source: ['codex']
There was a problem hiding this comment.
Still valid at a428f54 — ReservationToken remains a bare pub type ReservationToken = u64 alias. This nitpick was carried forward in the current review and intentionally not duplicated as a new inline comment.
The earlier automated resolution reply was corrected because title-based reconciliation failed to match the reworded carried-forward finding.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Follow-up review of PR #4090 at head a428f54 (prior head ee2135b). All seven prior findings were re-verified against current source, including the exact pinned key-wallet dependency (rev 1860089eddea27f66e5b3e637d18a73ae138478c) fetched from the cargo git checkout cache to resolve a factual disagreement among reviewer lanes. Carried-forward prior findings: prior-1 (blocking) is STILL VALID — first-principles inspection confirms core_wallet_tx_builder_build_signed (packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs:550) stamps the underlying ReservationSet TTL using last_processed_height(), while SignedPaymentRegistry::register (packages/rs-platform-wallet/src/wallet/signed_payment_registry.rs:210) records the wallet's synced_height() — a field independently documented (packages/rs-platform-wallet/src/wallet/identity/network/payments.rs:96-99) as decoupled from and capable of regressing relative to the monotonic last_processed_height. The new registry test helper (build_signed_tx in the registry's own test module) happens to use synced_height() for both stamping and comparison, masking this divergence in tests while the production FFI path does not share that property. Three lanes (sonnet5-general, sonnet5-rust-quality, sonnet5-ffi-engineer) incorrectly reconciled this as FIXED by conflating the test helper's clock with the production FFI clock; that reconciliation does not hold under direct source inspection and is overridden here. prior-7 (nitpick) is STILL VALID — ReservationToken remains a bare pub type ReservationToken = u64 alias. Prior-2 (wallet-instance guard now also compares wallet_id()), prior-3 (Kotlin double round-trip eliminated — core_wallet_transaction_get_bytes/coreTransactionGetBytes no longer exist, tx bytes ride back in the single register BLOB), prior-4 (fallible CString::new now runs before the registry insert; the JNI byte_array_from_slice failure path now releases the token), prior-5 (registry-held CoreWallet Arc now swept via remove_entries_for_wallet on platform_wallet_destroy), and prior-6 (mutex poisoning now recovered via a lock() helper) are all FIXED and verified directly against current code. New findings in the latest delta: the remove_entries_for_wallet sweep added to fix prior-5 matches on Arc::ptr_eq(wallet_manager) && wallet_id rather than on the specific destroyed handle, so it does not distinguish between multiple live PlatformWallet handles that alias the same logical wallet — a pattern the Kotlin SDK's own doc comments (ManagedPlatformWallet.kt:38-40) explicitly anticipate (loadPersistedWallets can hand out a fresh wrapper for a wallet id an earlier wrapper still holds). Destroying/GC'ing one such alias silently invalidates a deferred-payment token registered through a sibling, wrongly rejecting a legitimate broadcast/release with StaleToken and pinning the UTXO reservation until the 20-block TTL backstop. A second, narrower manifestation of the same root cause is a race between an in-flight core_wallet_signed_payment_register on one short-lived core handle and a concurrent platform_wallet_destroy on another alias of the same wallet. Separately, buildSignedPayment in Kotlin commits the native registry token inside withContext(Dispatchers.IO); coroutine cancellation racing the dispatcher hop back to the caller can discard the returned SignedCoreTransaction after the token was already minted, orphaning it until the TTL backstop reclaims it. Review action is REQUEST_CHANGES due to the carried-forward blocking clock-mismatch and the new alias-teardown regression, both introduced/exposed by this PR's own reservation-hardening code.
Source (experiment sonnet-primary-opus-quarter-sample-20260710, cohort sonnet_primary, bucket 1): reviewers codex/general=gpt-5.6-sol(completed); sonnet5/general=claude-sonnet-5(completed); codex/security-auditor=gpt-5.6-sol(completed); sonnet5/security-auditor=claude-sonnet-5(completed); codex/rust-quality=gpt-5.6-sol(completed); sonnet5/rust-quality=claude-sonnet-5(completed); codex/ffi-engineer=gpt-5.6-sol(completed); sonnet5/ffi-engineer=claude-sonnet-5(completed); verifier=verifier-sonnet5-4090-1783735156=claude-sonnet-5; orchestrator=openai/gpt-5.6-sol reasoning=high (orchestration-only, not a reviewer/verifier).
🔴 2 blocking | 🟡 1 suggestion(s)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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/wallet/signed_payment_registry.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/signed_payment_registry.rs:203-223: Token expiry guard measures a different height clock than the reservation it bounds (prior-1, still valid)
`register()` records `core.synced_height()` as `registered_height`, and `broadcast`/`release` compare it against a later `synced_height()` read using `RESERVATION_MAX_AGE_BLOCKS = 20`. But the underlying `ReservationSet` TTL that this guard is meant to stay under is stamped by `core_wallet_tx_builder_build_signed` (packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs:550-557) using `last_processed_height()`, not `synced_height()`. `payments.rs:96-99` documents these as intentionally decoupled clocks — `synced_height` is the filter-scan checkpoint and can regress during a DashPay rescan while `last_processed_height` remains monotonic. Whenever `synced_height` lags `last_processed_height` at registration time (or is later rolled back by a rescan), the 20-block guard can fail to trip before key-wallet's real 24-block `RESERVATION_TTL_BLOCKS` sweep (packages/rs-platform-wallet's pinned key-wallet dependency, `managed_account/reservation.rs:32,59-61`) has already reclaimed and possibly re-reserved the outpoint for an unrelated build. A broadcast or release on the stale token then acts on state it no longer owns — `ReservationSet::release`/`reserve` have no per-outpoint ownership check, so this can silently release or corrupt a newer build's reservation, the exact corruption class the guard was added to close. The new tests do not catch this because their `build_signed_tx` helper stamps and advances only `synced_height`, unlike the production FFI path. Track and pass through the exact height `build_signed` used (e.g. via `FFICoreTransaction`) instead of independently re-reading `synced_height` at registration.
In `packages/rs-platform-wallet-ffi/src/wallet.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/wallet.rs:200-214: platform_wallet_destroy's registry sweep is keyed on wallet identity, not handle identity — destroying one alias invalidates a sibling wrapper's deferred payment
`platform_wallet_destroy` now sweeps every registry entry whose `Arc::ptr_eq(wallet_manager)` and `wallet_id` match the destroyed handle's wallet (via `remove_entries_for_wallet`, packages/rs-platform-wallet/src/wallet/signed_payment_registry.rs:329-337). But a `PlatformWallet` handle is not a 1:1 proxy for the logical wallet's lifetime: `platform_wallet_manager_get_wallet` (packages/rs-platform-wallet-ffi/src/manager.rs:322-349) mints a fresh, independent handle on every call, cloning the same underlying `wallet_manager` Arc and `wallet_id`. The Kotlin SDK explicitly documents and relies on this — `ManagedPlatformWallet.kt:38-40` notes `loadPersistedWallets` can hand out a new wrapper for a wallet id an earlier live wrapper still holds. If wrapper B registers a deferred payment and wrapper A (a sibling alias of the same wallet) is then closed via `close()`/the `Cleaner` backstop, A's `platform_wallet_destroy` call sweeps B's token too, since the match is on wallet identity, not on which handle actually produced the token. B's later `broadcastSigned`/`releaseReservation` then wrongly fails with `StaleToken`, and because this sweep deliberately does not release the reservation, the funding UTXO stays locked until the 20-block TTL backstop. A narrower variant of the same root cause is a race between an in-flight `core_wallet_signed_payment_register` on a short-lived core handle and a concurrent `platform_wallet_destroy` on another alias — if the register lands after the sweep, that token is never covered by any future sweep, pinning the `WalletManager` alive indefinitely instead of just until TTL. Scope cleanup to the wallet actually being removed from the manager (or track live handle count and sweep only when the last alias is destroyed) rather than any single handle's teardown.
In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedPlatformWallet.kt`:
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedPlatformWallet.kt:265-299: Coroutine cancellation can discard a minted reservation token after buildSignedPayment's native call has already committed it
`core.registerSignedPayment(tx)` (line 295) mints a process-global registry token and starts holding the UTXO reservation, but this happens inside the dispatcher-changing `withContext(Dispatchers.IO)` wrapping the whole function (line 265). Kotlin's prompt-cancellation semantics mean a `Job` cancellation racing the dispatcher hop back to the caller can cause `withContext` to throw `CancellationException` even though the enclosed block already completed successfully and committed the native registration. In that case `buildSignedPayment` never delivers the `SignedCoreTransaction` (and its `reservationToken`) to the caller, so nothing can `broadcastSigned` or `releaseReservation` it — the registry entry sits until the `RESERVATION_MAX_AGE_BLOCKS` TTL backstop reclaims it. This is a narrow, timing-dependent window (must race cancellation against the specific dispatcher-resume point after a successful native call) and is bounded by the existing TTL guard, so it's an availability/leak concern rather than a correctness or fund-loss issue — but it is new exposure from this PR's side-effecting native call inside a cancellable coroutine scope.
| pub async fn register( | ||
| &self, | ||
| core: CoreWallet<B>, | ||
| tx: Transaction, | ||
| account_type: Option<StandardAccountType>, | ||
| account_index: u32, | ||
| ) -> ReservationToken { | ||
| let registered_height = core.synced_height().await; | ||
| let token = self.next_token.fetch_add(1, Ordering::SeqCst); | ||
| self.lock().insert( | ||
| token, | ||
| RegisteredPayment { | ||
| core, | ||
| tx, | ||
| account_type, | ||
| account_index, | ||
| registered_height, | ||
| }, | ||
| ); | ||
| token | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Token expiry guard measures a different height clock than the reservation it bounds (prior-1, still valid)
register() records core.synced_height() as registered_height, and broadcast/release compare it against a later synced_height() read using RESERVATION_MAX_AGE_BLOCKS = 20. But the underlying ReservationSet TTL that this guard is meant to stay under is stamped by core_wallet_tx_builder_build_signed (packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs:550-557) using last_processed_height(), not synced_height(). payments.rs:96-99 documents these as intentionally decoupled clocks — synced_height is the filter-scan checkpoint and can regress during a DashPay rescan while last_processed_height remains monotonic. Whenever synced_height lags last_processed_height at registration time (or is later rolled back by a rescan), the 20-block guard can fail to trip before key-wallet's real 24-block RESERVATION_TTL_BLOCKS sweep (packages/rs-platform-wallet's pinned key-wallet dependency, managed_account/reservation.rs:32,59-61) has already reclaimed and possibly re-reserved the outpoint for an unrelated build. A broadcast or release on the stale token then acts on state it no longer owns — ReservationSet::release/reserve have no per-outpoint ownership check, so this can silently release or corrupt a newer build's reservation, the exact corruption class the guard was added to close. The new tests do not catch this because their build_signed_tx helper stamps and advances only synced_height, unlike the production FFI path. Track and pass through the exact height build_signed used (e.g. via FFICoreTransaction) instead of independently re-reading synced_height at registration.
source: ['codex-general', 'codex-security-auditor', 'codex-rust-quality', 'sonnet5-security-auditor']
There was a problem hiding this comment.
Resolved in this update — Token expiry guard measures a different height clock than the reservation it bounds (prior-1, still valid) 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.
| pub unsafe extern "C" fn platform_wallet_destroy(handle: Handle) -> PlatformWalletFFIResult { | ||
| // Sweep any outstanding deferred-payment tokens bound to this wallet first, | ||
| // so the registry stops pinning its `WalletManager` (accounts, keys, sync | ||
| // state) alive for the rest of the process via the `CoreWallet` clone each | ||
| // token captured. Hooked here rather than into `core_wallet_destroy`: the | ||
| // deferred flow builds/registers on one short-lived core handle and | ||
| // broadcasts on another, so sweeping on core-handle destroy would drop | ||
| // tokens between register and broadcast. | ||
| PLATFORM_WALLET_STORAGE.with_item(handle, |wallet| { | ||
| crate::core_wallet::signed_payment::SIGNED_PAYMENT_REGISTRY | ||
| .remove_entries_for_wallet(wallet.core()); | ||
| }); | ||
| PLATFORM_WALLET_STORAGE.remove(handle); | ||
| PlatformWalletFFIResult::ok() | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: platform_wallet_destroy's registry sweep is keyed on wallet identity, not handle identity — destroying one alias invalidates a sibling wrapper's deferred payment
platform_wallet_destroy now sweeps every registry entry whose Arc::ptr_eq(wallet_manager) and wallet_id match the destroyed handle's wallet (via remove_entries_for_wallet, packages/rs-platform-wallet/src/wallet/signed_payment_registry.rs:329-337). But a PlatformWallet handle is not a 1:1 proxy for the logical wallet's lifetime: platform_wallet_manager_get_wallet (packages/rs-platform-wallet-ffi/src/manager.rs:322-349) mints a fresh, independent handle on every call, cloning the same underlying wallet_manager Arc and wallet_id. The Kotlin SDK explicitly documents and relies on this — ManagedPlatformWallet.kt:38-40 notes loadPersistedWallets can hand out a new wrapper for a wallet id an earlier live wrapper still holds. If wrapper B registers a deferred payment and wrapper A (a sibling alias of the same wallet) is then closed via close()/the Cleaner backstop, A's platform_wallet_destroy call sweeps B's token too, since the match is on wallet identity, not on which handle actually produced the token. B's later broadcastSigned/releaseReservation then wrongly fails with StaleToken, and because this sweep deliberately does not release the reservation, the funding UTXO stays locked until the 20-block TTL backstop. A narrower variant of the same root cause is a race between an in-flight core_wallet_signed_payment_register on a short-lived core handle and a concurrent platform_wallet_destroy on another alias — if the register lands after the sweep, that token is never covered by any future sweep, pinning the WalletManager alive indefinitely instead of just until TTL. Scope cleanup to the wallet actually being removed from the manager (or track live handle count and sweep only when the last alias is destroyed) rather than any single handle's teardown.
source: ['codex-general', 'codex-rust-quality', 'codex-ffi-engineer', 'sonnet5-ffi-engineer', 'sonnet5-security-auditor']
There was a problem hiding this comment.
Resolved in this update — platform_wallet_destroy's registry sweep is keyed on wallet identity, not handle identity — destroying one alias invalidates a sibling wrapper's deferred payment 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.
| ): SignedCoreTransaction = withContext(Dispatchers.IO) { | ||
| require(accountIndex >= 0) { "accountIndex must be non-negative, got $accountIndex" } | ||
| require(recipients.isNotEmpty()) { "recipients must not be empty" } | ||
| require(recipients.all { it.second > 0 }) { | ||
| "every recipient amount must be positive" | ||
| } | ||
| val builderAccountType = when (accountType) { | ||
| AccountType.BIP44 -> CoreTransactionBuilder.AccountType.BIP44 | ||
| AccountType.BIP32 -> CoreTransactionBuilder.AccountType.BIP32 | ||
| } | ||
| coreSendMutex.withLock { | ||
| mapNativeErrors { | ||
| coreWallet().use { core -> | ||
| val builder = CoreTransactionBuilder(network) | ||
| // `buildSigned` consumes the builder; `use` still safely | ||
| // destroys it on the pre-build failure paths. | ||
| val signedTx = builder.use { | ||
| for ((address, amount) in recipients) { | ||
| it.addOutput(address, amount) | ||
| } | ||
| it.setFunding(this@ManagedPlatformWallet, builderAccountType, accountIndex) | ||
| it.buildSigned( | ||
| this@ManagedPlatformWallet, | ||
| builderAccountType, | ||
| accountIndex, | ||
| coreSignerHandle, | ||
| ) | ||
| } | ||
| // Register the signed tx (holding its reservation) before the | ||
| // native transaction is freed; `use` frees it afterward. | ||
| signedTx.use { tx -> core.registerSignedPayment(tx) } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Coroutine cancellation can discard a minted reservation token after buildSignedPayment's native call has already committed it
core.registerSignedPayment(tx) (line 295) mints a process-global registry token and starts holding the UTXO reservation, but this happens inside the dispatcher-changing withContext(Dispatchers.IO) wrapping the whole function (line 265). Kotlin's prompt-cancellation semantics mean a Job cancellation racing the dispatcher hop back to the caller can cause withContext to throw CancellationException even though the enclosed block already completed successfully and committed the native registration. In that case buildSignedPayment never delivers the SignedCoreTransaction (and its reservationToken) to the caller, so nothing can broadcastSigned or releaseReservation it — the registry entry sits until the RESERVATION_MAX_AGE_BLOCKS TTL backstop reclaims it. This is a narrow, timing-dependent window (must race cancellation against the specific dispatcher-resume point after a successful native call) and is bounded by the existing TTL guard, so it's an availability/leak concern rather than a correctness or fund-loss issue — but it is new exposure from this PR's side-effecting native call inside a cancellable coroutine scope.
source: ['codex-general']
There was a problem hiding this comment.
Resolved in this update — Coroutine cancellation can discard a minted reservation token after buildSignedPayment's native call has already committed it 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.
|
Thanks — all findings were real. Bounded token lifetime added: |
|
Thanks — this summary matches the changes already present at |
bfoss765
changed the title
…BIP70 deferred submission BIP70/BIP270 (CTX/DashSpend) sends must sign, POST the raw bytes to a merchant server, and broadcast only on ack — structurally impossible on the one-shot `sendToAddresses`. Expose the existing internal build/broadcast split with an explicit reservation lifecycle, keeping `CoreTransactionBuilder` internal so the manager stays the sole driver of the setFunding/buildSigned race. Rust core (rs-platform-wallet): - New `SignedPaymentRegistry`: a generic, in-memory registry that owns a built+signed tx and its held UTXO reservation between build and submission, keyed by an opaque `ReservationToken`. `broadcast` removes the entry before sending (no double-broadcast — a repeat/concurrent call gets `StaleToken`), binds each token to its originating wallet instance (`Arc::ptr_eq` on the shared `WalletManager`, so a re-created wallet is rejected), and reconciles the reservation on failure via the existing release-on-rejection path. `release` is idempotent. Reservations are memory-only, so a crash between build and broadcast drops both the entry and the reservation on restart — the same property dashj has. - `CoreWallet::release_transaction_reservation` — the explicit "abandoned / nacked" release arm. FFI (platform-wallet-ffi) — additive C ABI: - `core_wallet_transaction_get_bytes`, `core_wallet_signed_payment_register` (token + fee + txid), `core_wallet_signed_payment_broadcast`, `core_wallet_signed_payment_release`, backed by one process-global registry pinned to `SpvBroadcaster`. - New `ErrorStaleReservationToken` (22) result code. JNI (rs-unified-sdk-jni) — additive: `coreTransactionGetBytes`, `coreWalletRegisterSignedPayment` (BLOB), `coreWalletBroadcastSignedPayment`, `coreWalletReleaseSignedPayment`. Kotlin — additive: `ManagedPlatformWallet.SignedCoreTransaction`, `buildSignedPayment` (build under coreSendMutex), `broadcastSigned(token)`, `releaseReservation(token)`; `DashSdkError.PlatformWallet.StaleReservationToken`. No existing signatures change. Refs dashpay#4089, dashpay/dash-wallet#1507 Phase 5c GAP-4. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…er/release Address review of the SignedPaymentRegistry deferred build→broadcast/release flow. BLOCKING: registry tokens never expired even though the key-wallet UTXO reservation they depend on is swept after RESERVATION_TTL_BLOCKS (24) and released by raw outpoint with no ownership check, so a long-outstanding token's broadcast/release could free or spend against an unrelated newer reservation. Bound the token lifetime: capture the wallet's synced height at register and refuse broadcast/release once the wallet has synced RESERVATION_MAX_AGE_BLOCKS (20, < TTL) past it, returning the typed StaleReservationToken WITHOUT releasing (which could free a newer build's reservation). The pinned key-wallet exposes no per-outpoint generation check, so this client-side bound is the primary guard. Also: - WalletMismatch now compares wallet_id in addition to Arc::ptr_eq on the shared WalletManager, so two wallets in one multi-wallet manager are told apart. - register() returns the raw tx bytes in the same native call and the JNI folds them into the register BLOB; the now-unused core_wallet_transaction_get_bytes / coreTransactionGetBytes is removed (one native round trip per kotlin-sdk rule). - register() does its fallible/pure marshalling before the reservation-holding insert, and the JNI releases the token if it can't hand the BLOB back to Kotlin — no orphaned reservation on a marshalling failure. - PlatformWallet teardown sweeps the registry of that wallet's tokens so a destroyed wallet's WalletManager is no longer pinned alive by a captured CoreWallet clone (hooked at platform_wallet_destroy, not the transient core-handle destroy the deferred flow cycles through). - Registry mutex recovers from poisoning instead of panicking, matching key-wallet's ReservationSet. Adds tests for token expiry (broadcast + release), same-manager different wallet_id mismatch, and the teardown sweep. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…dk-and-example-app Rebasing the split build/broadcast work onto the current base surfaced three semantic collisions the textual merge could not catch: - error code 22 was reassigned on base (ErrorCoreInsufficientFunds and the asset-lock family 22-25); moved ErrorStaleReservationToken to the next free code 26 in platform-wallet-ffi and DashSdkError's native-code mapping. - base added its own CoreWallet::release_transaction_reservation (taking AccountTypePreference, superset incl. CoinJoin) for the finalized-transaction abandon path, colliding with this PR's identically-named StandardAccountType method. Renamed this PR's deferred-payment release to release_payment_reservation (sole caller: SignedPaymentRegistry::release). - base removed the per-wallet coreSendMutex and now serializes/gates core sends through the TeardownGate (gate.op), moving send concurrency safety into the Rust reservation layer. buildSignedPayment now opens with gate.op like its sibling sendToAddresses instead of the removed mutex, which also satisfies the GateCoverageLintTest handle-borrowing fence. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
a428f54 to
bbcd88e
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
Carried-forward prior findings: all four priors remain valid at bbcd88e—two blocking reservation-lifecycle defects, one coroutine-cancellation cleanup gap, and one token type-safety nitpick. New findings in the latest delta: removing the shared send mutex leaves deferred builds on the explicitly non-atomic funding/signing sequence, and the stale-token mapping test still uses code 22 after the mapping moved to code 26. The PR has four confirmed blockers and is not ready to merge.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (failed),gpt-5.6-sol— security-auditor (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— ffi-engineer (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— security-auditor (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— ffi-engineer (failed),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)
🔴 4 blocking | 🟡 1 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-platform-wallet/src/wallet/signed_payment_registry.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/signed_payment_registry.rs:203-210: Token age uses a different height clock than the reserved inputs
`register` records `core.synced_height()`, while the production `core_wallet_tx_builder_build_signed` path stamps the underlying key-wallet reservation using `last_processed_height`. These clocks are intentionally independent: the DashPay rescan code documents that `synced_height` can regress while `last_processed_height` remains monotonic. The registry's 20-block guard can therefore continue accepting an old token after key-wallet's 24-block reservation TTL has swept and allowed the outpoint to be reserved by a newer transaction. A later release or definitively rejected broadcast for the old token removes reservations by outpoint without an ownership generation and can free the newer transaction's reservation. Pass the exact height used during transaction finalization into the registry instead of independently reading `synced_height`.
In `packages/rs-platform-wallet-ffi/src/wallet.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/wallet.rs:400-404: Destroying one alias handle invalidates another wrapper's payment token
`platform_wallet_destroy` removes every registry entry matching the underlying wallet-manager pointer and wallet ID, although each `platform_wallet_manager_get_wallet` call creates an independent handle for the same logical wallet. The Kotlin `loadPersistedWallets` path can consequently publish a new wrapper while callers still retain an older wrapper. If wrapper B registers a deferred payment and wrapper A is then closed or cleaned, destroying A consumes B's live token. B's later broadcast fails as stale, and the sweep intentionally leaves the UTXO reserved until its TTL. A handle destructor should only drop that handle; wallet-wide registry cleanup must occur when the logical wallet is removed or when its final alias is destroyed.
In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedPlatformWallet.kt`:
- [BLOCKING] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedPlatformWallet.kt:262-276: Deferred builds lost the atomic selection-and-reservation boundary
`buildSignedPayment` still calls the deprecated `setFunding` and `buildSigned` methods as two native operations, but the latest delta removed the shared per-wallet `coreSendMutex` and replaced it with `gate.op`. `TeardownGate` only counts active operations for safe teardown; it releases its mutex before running the block and does not serialize sends. Key-wallet explicitly requires `set_funding` through `assemble_unsigned` to run under one uninterrupted wallet lock because selection observes reservations before the chosen inputs are reserved. Concurrent deferred builds can therefore select the same UTXO, and an atomic immediate send can also reserve an input after the deferred path selected it but before `buildSigned` reserves it. Both paths can return signed transactions spending the same input. Use an atomic finalize-and-register native operation for deferred payments, or restore shared per-wallet serialization around the entire funding, signing, and registration sequence.
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedPlatformWallet.kt:274-278: Coroutine cancellation can orphan a committed reservation token
`core.registerSignedPayment(tx)` commits the process-global registry entry inside `gate.op`, whose implementation executes the block through `withContext(Dispatchers.IO)`. Cancellation can win during the dispatcher handoff after the native call has returned but before the completed `SignedCoreTransaction` is delivered to the caller. The caller then never receives the token required to broadcast or release the reservation, while `TeardownGate`'s `NonCancellable` cleanup only decrements its active-operation counter. Track whether token ownership was delivered and release a minted token from `NonCancellable` cleanup when delivery is cancelled.
In `packages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/errors/DashSdkErrorTest.kt`:
- [BLOCKING] packages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/errors/DashSdkErrorTest.kt:109: The stale-token mapping test still uses native code 22
The latest delta assigns code 22 to `ErrorCoreInsufficientFunds` and moves `ErrorStaleReservationToken` to code 26. The production Kotlin mapping correctly reflects those assignments, but this test constructs code 22 and asserts that it produces `StaleReservationToken`. It deterministically receives `CoreInsufficientFunds`, so the test fails and no longer verifies the code-26 mapping.
| pub async fn register( | ||
| &self, | ||
| core: CoreWallet<B>, | ||
| tx: Transaction, | ||
| account_type: Option<StandardAccountType>, | ||
| account_index: u32, | ||
| ) -> ReservationToken { | ||
| let registered_height = core.synced_height().await; |
There was a problem hiding this comment.
🔴 Blocking: Token age uses a different height clock than the reserved inputs
register records core.synced_height(), while the production core_wallet_tx_builder_build_signed path stamps the underlying key-wallet reservation using last_processed_height. These clocks are intentionally independent: the DashPay rescan code documents that synced_height can regress while last_processed_height remains monotonic. The registry's 20-block guard can therefore continue accepting an old token after key-wallet's 24-block reservation TTL has swept and allowed the outpoint to be reserved by a newer transaction. A later release or definitively rejected broadcast for the old token removes reservations by outpoint without an ownership generation and can free the newer transaction's reservation. Pass the exact height used during transaction finalization into the registry instead of independently reading synced_height.
source: ['codex-general', 'codex-security-auditor', 'codex-rust-quality', 'codex-ffi-engineer']
| PLATFORM_WALLET_STORAGE.with_item(handle, |wallet| { | ||
| crate::core_wallet::signed_payment::SIGNED_PAYMENT_REGISTRY | ||
| .remove_entries_for_wallet(wallet.core()); | ||
| }); | ||
| PLATFORM_WALLET_STORAGE.remove(handle); |
There was a problem hiding this comment.
🔴 Blocking: Destroying one alias handle invalidates another wrapper's payment token
platform_wallet_destroy removes every registry entry matching the underlying wallet-manager pointer and wallet ID, although each platform_wallet_manager_get_wallet call creates an independent handle for the same logical wallet. The Kotlin loadPersistedWallets path can consequently publish a new wrapper while callers still retain an older wrapper. If wrapper B registers a deferred payment and wrapper A is then closed or cleaned, destroying A consumes B's live token. B's later broadcast fails as stale, and the sweep intentionally leaves the UTXO reserved until its TTL. A handle destructor should only drop that handle; wallet-wide registry cleanup must occur when the logical wallet is removed or when its final alias is destroyed.
source: ['codex-general', 'codex-security-auditor', 'codex-rust-quality', 'codex-ffi-engineer']
| val signedTx = builder.use { | ||
| for ((address, amount) in recipients) { | ||
| it.addOutput(address, amount) | ||
| } | ||
| it.setFunding(this@ManagedPlatformWallet, builderAccountType, accountIndex) | ||
| it.buildSigned( | ||
| this@ManagedPlatformWallet, | ||
| builderAccountType, | ||
| accountIndex, | ||
| coreSignerHandle, | ||
| ) | ||
| } | ||
| // Register the signed tx (holding its reservation) before the | ||
| // native transaction is freed; `use` frees it afterward. | ||
| signedTx.use { tx -> core.registerSignedPayment(tx) } |
There was a problem hiding this comment.
🔴 Blocking: Deferred builds lost the atomic selection-and-reservation boundary
buildSignedPayment still calls the deprecated setFunding and buildSigned methods as two native operations, but the latest delta removed the shared per-wallet coreSendMutex and replaced it with gate.op. TeardownGate only counts active operations for safe teardown; it releases its mutex before running the block and does not serialize sends. Key-wallet explicitly requires set_funding through assemble_unsigned to run under one uninterrupted wallet lock because selection observes reservations before the chosen inputs are reserved. Concurrent deferred builds can therefore select the same UTXO, and an atomic immediate send can also reserve an input after the deferred path selected it but before buildSigned reserves it. Both paths can return signed transactions spending the same input. Use an atomic finalize-and-register native operation for deferred payments, or restore shared per-wallet serialization around the entire funding, signing, and registration sequence.
source: ['codex-general', 'codex-security-auditor', 'codex-rust-quality', 'codex-ffi-engineer']
|
|
||
| // Deferred build/broadcast: a stale/consumed/wrong-wallet reservation | ||
| // token → typed StaleReservationToken, not retryable. | ||
| val staleToken = DashSdkError.fromNative(DashSDKException(offset + 22, "stale token 7")) |
There was a problem hiding this comment.
🔴 Blocking: The stale-token mapping test still uses native code 22
The latest delta assigns code 22 to ErrorCoreInsufficientFunds and moves ErrorStaleReservationToken to code 26. The production Kotlin mapping correctly reflects those assignments, but this test constructs code 22 and asserts that it produces StaleReservationToken. It deterministically receives CoreInsufficientFunds, so the test fails and no longer verifies the code-26 mapping.
| val staleToken = DashSdkError.fromNative(DashSDKException(offset + 22, "stale token 7")) | |
| val staleToken = DashSdkError.fromNative(DashSDKException(offset + 26, "stale token 7")) |
source: ['codex-general']
| // Register the signed tx (holding its reservation) before the | ||
| // native transaction is freed; `use` frees it afterward. | ||
| signedTx.use { tx -> core.registerSignedPayment(tx) } | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Coroutine cancellation can orphan a committed reservation token
core.registerSignedPayment(tx) commits the process-global registry entry inside gate.op, whose implementation executes the block through withContext(Dispatchers.IO). Cancellation can win during the dispatcher handoff after the native call has returned but before the completed SignedCoreTransaction is delivered to the caller. The caller then never receives the token required to broadcast or release the reservation, while TeardownGate's NonCancellable cleanup only decrements its active-operation counter. Track whether token ownership was delivered and release a minted token from NonCancellable cleanup when delivery is cancelled.
source: ['codex-general', 'codex-security-auditor', 'codex-rust-quality', 'codex-ffi-engineer']
| /// Opaque handle to a registered, signed-but-unsent payment. Minted by | ||
| /// [`SignedPaymentRegistry::register`]; consumed by | ||
| /// [`SignedPaymentRegistry::broadcast`] or | ||
| /// [`SignedPaymentRegistry::release`]. Values are unique for the process | ||
| /// lifetime and never reused, so a stale token can always be recognised. | ||
| pub type ReservationToken = u64; |
There was a problem hiding this comment.
💬 Nitpick: The opaque reservation capability remains an untyped u64 alias
ReservationToken has lifecycle and provenance semantics distinct from other numeric identifiers, but a type alias provides no Rust-side domain separation. Rust callers can accidentally interchange unrelated u64 values with reservation tokens without a type error. A #[repr(transparent)] newtype would preserve the C-compatible representation while enforcing the distinction within Rust.
source: ['codex-general', 'codex-security-auditor', 'codex-rust-quality']
…n mapping The rebase onto feat/kotlin-sdk-and-example-app reassigned native code 22 to ErrorCoreInsufficientFunds and moved ErrorStaleReservationToken to code 26 (on both the Rust enum and DashSdkError's mapping), but DashSdkErrorTest still constructed code 22 and asserted StaleReservationToken. That deterministically resolved to CoreInsufficientFunds, so platformWalletCodesMapToPlatformWalletSubtree failed and :sdk:testDebugUnitTest — the "Kotlin SDK build + tests (x86_64 emulator)" CI job — went red without actually verifying the code-26 mapping. Point the assertion at code 26 so it exercises the real production mapping. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…s own height clock The SignedPaymentRegistry age guard stamped registered_height with CoreWallet::synced_height() and compared it against a later synced_height(), while the funding reservation it is meant to stay under is stamped with last_processed_height() (the height finalize_transaction / build_signed pass to set_current_height, and the clock key-wallet's ReservationSet TTL sweeps against). synced_height can regress during a rescan while last_processed_height is monotonic, so measuring the reservation's age against synced_height could let a token outlive its reservation and act on an outpoint key-wallet had already swept and re-selected for an unrelated build. Read last_processed_height() for both the registration stamp and the current comparison so the guard measures the same clock the reservation is stamped with, trips strictly before the underlying TTL, and never regresses. Add CoreWallet::last_processed_height(); drop the now-unused synced_height(). The registry's expiry tests now stamp and advance last_processed_height to match production, and outstanding() is exposed under test-utils for downstream FFI tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…llet alias is destroyed platform_wallet_destroy unconditionally called remove_entries_for_wallet, which matches every registry entry sharing the destroyed handle's WalletManager pointer + wallet_id. But platform_wallet_manager_get_wallet hands out an independent handle per alias of the same logical wallet (the loadPersistedWallets path can publish a new wrapper while callers still hold an older one). Destroying one alias therefore consumed a sibling alias's still-live deferred-payment token: the sibling's later broadcast failed as stale while the sweep left the UTXO reserved until its TTL. Gate the sweep on final-alias liveness: after removing this handle, scan the remaining PlatformWallet handles for one that shares the same (WalletManager pointer + wallet_id) — exactly the key remove_entries_for_wallet matches. While a sibling is live the destructor only drops this handle; the sweep runs (releasing the registry's WalletManager pin) only once the last alias goes. Adds HandleStorage::any for the scan and a test_support helper that builds real PlatformWallet aliases; a new FFI test proves a sibling alias's token survives one alias's destruction and is swept when the final alias is destroyed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…d-register path buildSignedPayment funded, signed, and registered a deferred payment as three separate native round-trips (setFunding + buildSigned + registerSignedPayment). Once the base branch removed the per-wallet coreSendMutex in favour of the TeardownGate — which only counts active ops for safe teardown and does not serialize sends — that split lost its atomic select-and-reserve boundary: two concurrent deferred builds, or a deferred build racing an immediate send, could select the same UTXO before either reserved it and return two signed transactions spending the same input. Restore atomicity in the Rust reservation layer, the correct home now that the Kotlin mutex is gone: add core_wallet_signed_payment_finalize, which runs the same finalize_transaction the immediate V2 path uses — selection and ReservationSet insertion commit as one unit under the wallet-manager lock, signing only after the lock drops — and then registers the built, reserved tx in the same call. buildSignedPayment now issues that single native operation (CoreTransactionBuilder.finalizeSignedPayment + coreWalletFinalizeSignedPayment), so the select+reserve window can no longer interleave. The existing concurrent_same_account_finalizers_cannot_reserve_the_same_input test already covers the atomic boundary the deferred path now shares. The deprecated split wrappers remain but are no longer on the deferred path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
Carried-forward prior findings: prior-5 remains a cancellation-safety suggestion and prior-6 remains a token-type nitpick, while prior-1 through prior-4 are fixed as originally reported. New findings in the latest delta: three blocking root causes remain—the token lifetime starts after signing instead of at reservation, registration can mint duplicate or misattributed capabilities, and wallet cleanup and identity are tied to transient wrapper aliases instead of the logical wallet generation—so the PR is not ready to merge.
Validated blockers were found in the Codex precheck. Sonnet 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)
🔴 3 blocking
2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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/wallet/signed_payment_registry.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/signed_payment_registry.rs:207-225: Registration loses the reservation's exact height
`finalize_transaction` stamps the selected inputs using `last_processed_height` before releasing the wallet lock and awaiting the external signer, but `register` independently reads the height after signing finishes. If signing spans at least five blocks, the registry's 20-block guard can still accept the token after key-wallet's 24-block TTL has swept and re-reserved its outpoint. A later release or definitive broadcast rejection can then remove the newer reservation because release operates unconditionally by outpoint. Store the height used by `set_current_height` in `SignedCoreTransaction` and transfer that exact value into the registry.
In `packages/rs-platform-wallet-ffi/src/core_wallet/signed_payment.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/core_wallet/signed_payment.rs:81-115: Registration does not preserve unique reservation ownership
`FFICoreTransaction` contains only serialized bytes and a fee, while this function independently accepts the wallet and funding account and does not consume the transaction. A valid transaction can therefore be registered repeatedly or registered against an unrelated wallet/account. Duplicate tokens defeat reservation-level single-use semantics: one token can release the outpoint, a new payment can reserve it, and another old token can later release that newer reservation. Registering wallet A's transaction against wallet B instead reconciles B and leaves A's actual reservation held until TTL. Consume a single opaque finalized-payment handle containing its originating wallet, account, reservation height, and transaction, or remove the ownership-less legacy registration path.
In `packages/rs-platform-wallet-ffi/src/wallet.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/wallet.rs:392-423: Token lifecycle is tied to transient wallet aliases
`platform_wallet_destroy` deletes every token when no sibling wrapper is currently visible, even though the manager still owns the logical wallet and can immediately return another handle for it. Closing the last current wrapper therefore makes a retained payment token stale while intentionally leaving its UTXO reserved until TTL. The `remove`, sibling scan, alias insertion, and registry sweep also use separate locks, so a concurrent lookup or registration can race the sweep. Conversely, an alias retained across logical wallet removal and recreation still matches `(WalletManager Arc, wallet_id)`, allowing an old token to operate on the replacement generation. Perform cleanup on logical wallet removal or manager shutdown and coordinate it with a stable per-wallet generation or atomic lifecycle reference count.
| pub async fn register( | ||
| &self, | ||
| core: CoreWallet<B>, | ||
| tx: Transaction, | ||
| account_type: Option<StandardAccountType>, | ||
| account_index: u32, | ||
| ) -> ReservationToken { | ||
| let registered_height = core.last_processed_height().await; | ||
| let token = self.next_token.fetch_add(1, Ordering::SeqCst); | ||
| self.lock().insert( | ||
| token, | ||
| RegisteredPayment { | ||
| core, | ||
| tx, | ||
| account_type, | ||
| account_index, | ||
| registered_height, | ||
| }, | ||
| ); |
There was a problem hiding this comment.
🔴 Blocking: Registration loses the reservation's exact height
finalize_transaction stamps the selected inputs using last_processed_height before releasing the wallet lock and awaiting the external signer, but register independently reads the height after signing finishes. If signing spans at least five blocks, the registry's 20-block guard can still accept the token after key-wallet's 24-block TTL has swept and re-reserved its outpoint. A later release or definitive broadcast rejection can then remove the newer reservation because release operates unconditionally by outpoint. Store the height used by set_current_height in SignedCoreTransaction and transfer that exact value into the registry.
source: ['codex']
| let core = unwrap_option_or_return!(CORE_WALLET_STORAGE.with_item(core_handle, |w| w.clone())); | ||
|
|
||
| let bytes = (*tx).bytes(); | ||
| let transaction: dashcore::Transaction = match dashcore::consensus::deserialize(bytes) { | ||
| Ok(t) => t, | ||
| Err(e) => { | ||
| return PlatformWalletFFIResult::err( | ||
| PlatformWalletFFIResultCode::ErrorDeserialization, | ||
| format!("failed to deserialize signed transaction: {e}"), | ||
| ); | ||
| } | ||
| }; | ||
| let txid = transaction.txid(); | ||
| let fee = (*tx).fee(); | ||
|
|
||
| // Do all fallible/pure marshalling BEFORE the registry insert — that insert | ||
| // mints a token and holds the funding reservation, so a later failure would | ||
| // orphan the reservation with no token to release it. txid hex never | ||
| // contains a NUL, but handle the impossible case anyway. | ||
| let c_txid = match CString::new(txid.to_string()) { | ||
| Ok(s) => s, | ||
| Err(_) => { | ||
| return PlatformWalletFFIResult::err( | ||
| PlatformWalletFFIResultCode::ErrorUtf8Conversion, | ||
| "txid string contained an interior NUL".to_string(), | ||
| ); | ||
| } | ||
| }; | ||
|
|
||
| let token = runtime().block_on(SIGNED_PAYMENT_REGISTRY.register( | ||
| core, | ||
| transaction, | ||
| account_type.as_standard_account_type(), | ||
| account_index, | ||
| )); |
There was a problem hiding this comment.
🔴 Blocking: Registration does not preserve unique reservation ownership
FFICoreTransaction contains only serialized bytes and a fee, while this function independently accepts the wallet and funding account and does not consume the transaction. A valid transaction can therefore be registered repeatedly or registered against an unrelated wallet/account. Duplicate tokens defeat reservation-level single-use semantics: one token can release the outpoint, a new payment can reserve it, and another old token can later release that newer reservation. Registering wallet A's transaction against wallet B instead reconciles B and leaves A's actual reservation held until TTL. Consume a single opaque finalized-payment handle containing its originating wallet, account, reservation height, and transaction, or remove the ownership-less legacy registration path.
source: ['codex']
| pub unsafe extern "C" fn platform_wallet_destroy(handle: Handle) -> PlatformWalletFFIResult { | ||
| PLATFORM_WALLET_STORAGE.remove(handle); | ||
| // Remove this handle first so it is excluded from the final-alias scan | ||
| // below (and so a concurrent lookup can no longer resolve it). | ||
| let Some(wallet) = PLATFORM_WALLET_STORAGE.remove(handle) else { | ||
| return PlatformWalletFFIResult::ok(); | ||
| }; | ||
|
|
||
| // `platform_wallet_manager_get_wallet` hands out an independent handle for | ||
| // each alias of the same logical wallet (they share the underlying | ||
| // `WalletManager` `Arc` and `wallet_id`). A deferred-payment token minted | ||
| // through one alias must NOT be invalidated when a *sibling* alias is | ||
| // destroyed — the token is still live and broadcastable through the survivor. | ||
| // | ||
| // So only sweep the registry when THIS is the final live alias: no other | ||
| // stored handle shares the same (`WalletManager` pointer + `wallet_id`) — | ||
| // exactly the key `remove_entries_for_wallet` matches on. When a sibling is | ||
| // still live, the destructor just drops this handle, leaving its tokens | ||
| // (and the shared `WalletManager` pin) in place. Once the last alias goes, | ||
| // the sweep runs, releasing the registry's pin on the wallet's | ||
| // `WalletManager` (accounts, keys, sync state) that each token's captured | ||
| // `CoreWallet` clone would otherwise keep alive for the process lifetime. | ||
| let core = wallet.core(); | ||
| let wallet_id = core.wallet_id(); | ||
| let manager = wallet.wallet_manager(); | ||
| let sibling_alias_alive = PLATFORM_WALLET_STORAGE.any(|other| { | ||
| other.wallet_id() == wallet_id | ||
| && std::sync::Arc::ptr_eq(other.wallet_manager(), manager) | ||
| }); | ||
| if !sibling_alias_alive { | ||
| crate::core_wallet::signed_payment::SIGNED_PAYMENT_REGISTRY | ||
| .remove_entries_for_wallet(core); | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Token lifecycle is tied to transient wallet aliases
platform_wallet_destroy deletes every token when no sibling wrapper is currently visible, even though the manager still owns the logical wallet and can immediately return another handle for it. Closing the last current wrapper therefore makes a retained payment token stale while intentionally leaving its UTXO reserved until TTL. The remove, sibling scan, alias insertion, and registry sweep also use separate locks, so a concurrent lookup or registration can race the sweep. Conversely, an alias retained across logical wallet removal and recreation still matches (WalletManager Arc, wallet_id), allowing an old token to operate on the replacement generation. Perform cleanup on logical wallet removal or manager shutdown and coordinate it with a stable per-wallet generation or atomic lifecycle reference count.
source: ['codex']
2 participants
Implements #4089 (design agreed there). Context: dashpay/dash-wallet#1507 Phase 5c GAP-4.
Problem
BIP70/BIP270 (CTX/DashSpend) sends sign, POST the raw bytes to a merchant server, and broadcast only on the server's ack — a flow that is structurally impossible on the one-shot
ManagedPlatformWallet.sendToAddresses(which builds, signs, and broadcasts in one uninterrupted step). This exposes the existing internal build/broadcast split with an explicit reservation lifecycle, while keepingCoreTransactionBuilderinternal so the manager remains the sole driver of thesetFunding/buildSignedselection race (its KDoc safety argument is preserved).API (purely ADDITIVE — no existing signature changes)
Kotlin, on
ManagedPlatformWallet:buildSignedPaymentrunsnew → addOutput* → setFunding → buildSignedunder the same per-walletcoreSendMutexassendToAddresses, closing the double-selection window;buildSignedreserves the UTXOs, sobroadcastSigned/releaseReservationoperate on the token later, without the mutex.Token / reservation design
SignedPaymentRegistry(inrs-platform-wallet) owns the built tx + its held reservation between build and submission, keyed by an opaqueReservationToken. The FFI layer instantiates one process-global registry pinned toSpvBroadcaster.broadcastremoves the entry before sending, so a repeated/concurrent broadcast of the same token gets a typedStaleTokenerror — never a double-broadcast. On failure it reuses the existing release-on-rejection / keep-on-ambiguous policy.Arc::ptr_eqon the sharedWalletManager), so a re-created wallet is rejected rather than spending against stale state.releaseis idempotent (unknown / already-consumed token → silent no-op) and needs no handle.ReservationSetare both in-memory, so an app crash between build and broadcast drops the entry and the reservation together — nothing leaks across a restart. This matches dashj's memory-only in-flight reservations; there is no on-disk reservation persistence to follow.Surface additions
platform-wallet-ffi):core_wallet_transaction_get_bytes,core_wallet_signed_payment_register,core_wallet_signed_payment_broadcast,core_wallet_signed_payment_release; newErrorStaleReservationToken(22).rs-unified-sdk-jni):coreTransactionGetBytes,coreWalletRegisterSignedPayment,coreWalletBroadcastSignedPayment,coreWalletReleaseSignedPayment.SignedCoreTransaction, the three methods above, andDashSdkError.PlatformWallet.StaleReservationToken.The immediate
sendToAddresses/coreWalletBroadcastTransactionpath is unchanged.Tests
Rust (
SignedPaymentRegistry, 10 tests, all green):StaleToken(network hit exactly once)StaleToken; unknown token →StaleTokenWalletMismatch(nothing sent)Full
platform-wallet/platform-wallet-ffi/rs-unified-sdk-jnisuites pass; clippy green. Kotlin: addedDashSdkErrorTestcoverage for the code-22 mapping;:sdk:compileReleaseKotlingreen. The Kotlin/JNI native path is exercised only by instrumented tests, so per convention Rust coverage + compile stands in there. The native library was not rebuilt.cc @QuantumExplorer
🤖 Generated with Claude Code