feat: include InstantLock in TransactionContext::InstantSend variant#615
Merged
xdustinface merged 7 commits intov0.42-devfrom Apr 2, 2026
Merged
feat: include InstantLock in TransactionContext::InstantSend variant#615xdustinface merged 7 commits intov0.42-devfrom
InstantLock in TransactionContext::InstantSend variant#615xdustinface merged 7 commits intov0.42-devfrom
Conversation
Collaborator
Author
|
@CodeRabbit review! |
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 16 minutes and 30 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (20)
📝 WalkthroughWalkthroughFFI and core transaction context were extended to carry full InstantLock payloads. Sync, mempool, wallet-manager, and wallet-checking layers were updated to accept and propagate Changes
Sequence Diagram(s)sequenceDiagram
participant SyncMgr as Sync Manager
participant MempoolMgr as Mempool Manager
participant WalletMgr as Wallet Manager
participant Wallet as Wallet
participant TxRecord as Transaction Record
SyncMgr->>MempoolMgr: process_instant_send(instant_lock)
activate MempoolMgr
MempoolMgr->>MempoolMgr: store (instant_lock, now) in pending_is_locks
MempoolMgr->>WalletMgr: process_instant_send_lock(instant_lock)
activate WalletMgr
WalletMgr->>Wallet: process_instant_send_lock(instant_lock)
activate Wallet
Wallet->>TxRecord: update_context(InstantSend(instant_lock))
TxRecord->>TxRecord: store lock payload
deactivate Wallet
deactivate WalletMgr
deactivate MempoolMgr
alt Mempool tx arrives with pending lock
SyncMgr->>MempoolMgr: handle_tx(tx, pending_lock: Some(instant_lock))
activate MempoolMgr
MempoolMgr->>WalletMgr: process_mempool_transaction(tx, Some(instant_lock))
activate WalletMgr
WalletMgr->>Wallet: process_mempool_transaction(tx, Some(instant_lock))
activate Wallet
Wallet->>TxRecord: create with InstantSend(instant_lock) context
deactivate Wallet
deactivate WalletMgr
deactivate MempoolMgr
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v0.42-dev #615 +/- ##
=============================================
+ Coverage 67.62% 67.64% +0.01%
=============================================
Files 321 321
Lines 68253 68422 +169
=============================================
+ Hits 46159 46285 +126
- Misses 22094 22137 +43
|
Contributor
|
✅ Actions performedReview triggered.
|
Contributor
✅ Actions performedReview triggered.
|
Collaborator
Author
|
@CodeRabbit review |
Contributor
✅ Actions performedReview triggered.
|
Collaborator
Author
|
@CodeRabbit review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
key-wallet-ffi/src/transaction.rs (1)
428-443:⚠️ Potential issue | 🟠 MajorReject
InstantLockpayloads for a different transaction.
transaction_context_from_ffi(...)only checks that the buffer deserializes. A valid lock for anothertxidwill still marktxas InstantSend here, becausecheck_core_transactionupdates state bytx.txid(), not by the lock payload. Please enforcelock.txid == tx.txid()before continuing.🛡️ Proposed fix
let context = match transaction_context_from_ffi( context_type, &block_info, islock_data, islock_len, ) { Some(ctx) => ctx, None => { FFIError::set_error( error, FFIErrorCode::InvalidInput, "Invalid transaction context: block info is zeroed for a confirmed context, or IS lock data is missing/malformed for InstantSend".to_string(), ); return false; } }; + + if let key_wallet::transaction_checking::TransactionContext::InstantSend(lock) = &context { + if lock.txid != tx.txid() { + FFIError::set_error( + error, + FFIErrorCode::InvalidInput, + "InstantLock txid does not match the provided transaction".to_string(), + ); + return false; + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/transaction.rs` around lines 428 - 443, transaction_context_from_ffi(...) can produce an InstantLock context that deserializes but belongs to a different transaction; before proceeding (after creating context variable in the match), verify that for InstantLock contexts the embedded lock's txid equals the current tx.txid() and if not set FFIError::set_error with FFIErrorCode::InvalidInput (or appropriate code) and return false; locate this check immediately after obtaining context in the block handling context (symbols: transaction_context_from_ffi, context, InstantLock, lock.txid, tx.txid, check_core_transaction) so check_core_transaction cannot be misled by a lock for a different txid.key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs (1)
245-261:⚠️ Potential issue | 🟠 MajorReturn
truewhen the record context changed, not just when a UTXO changed.
key-wallet-manager/src/process_block.rsuses this boolean to decide whether a wallet was affected by an IS lock, but the new implementation only flips it when a UTXO was marked. For outgoing transactions or records with no remaining UTXOs,record.update_context(TransactionContext::InstantSend(..))can succeed here and the method still returnsfalse, so that wallet drops out ofaffected_walletseven though stored state changed.🐛 Proposed fix
fn mark_instant_send_utxos(&mut self, txid: &Txid, lock: &InstantLock) -> bool { if !self.instant_send_locks.insert(*txid) { return false; } - let mut any_changed = false; + let mut utxos_changed = false; + let mut record_changed = false; for account in self.accounts.all_accounts_mut() { if account.mark_utxos_instant_send(txid) { - any_changed = true; + utxos_changed = true; } if let Some(record) = account.transactions_mut().get_mut(txid) { - record.update_context(TransactionContext::InstantSend(lock.clone())); + let new_context = TransactionContext::InstantSend(lock.clone()); + if record.context != new_context { + record.update_context(new_context); + record_changed = true; + } } } - if any_changed { + if utxos_changed { self.update_balance(); } - any_changed + utxos_changed || record_changed }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs` around lines 245 - 261, In mark_instant_send_utxos, the method only sets any_changed when account.mark_utxos_instant_send(txid) returns true, but it must also consider mutations from record.update_context(TransactionContext::InstantSend(lock.clone())); change the loop so that the boolean returned by record.update_context(...) is OR'ed into any_changed (e.g., let changed = account.mark_utxos_instant_send(txid) || record.update_context(...); any_changed |= changed), ensuring update_balance() and the returned value reflect record context changes as well as UTXO changes.dash-spv/src/sync/mempool/sync_manager.rs (1)
95-100:⚠️ Potential issue | 🟠 MajorGate
process_instant_sendon thevalidatedflag.The
instantsendmanager can emitSyncEvent::InstantLockReceived { validated: false, .. }when signature validation fails (seeinstantsend/manager.rsline 103). The handler at lines 95-100 ignores thevalidatedfield and processes all instant locks immediately. Either check the flag before callingprocess_instant_send, or confirm that unvalidated locks should be processed the same as validated ones.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash-spv/src/sync/mempool/sync_manager.rs` around lines 95 - 100, The handler for SyncEvent::InstantLockReceived currently calls process_instant_send unconditionally; change it to check the event's validated flag and only call self.process_instant_send(instant_lock.clone()).await when validated == true (otherwise skip or log/track the unvalidated instant lock as appropriate). Locate the match arm handling SyncEvent::InstantLockReceived and use the validated field from the event to gate the call to process_instant_send so unvalidated instant locks are not processed.key-wallet/src/transaction_checking/wallet_checker.rs (1)
81-96:⚠️ Potential issue | 🟡 MinorDon't poison
instant_send_lockswhen the lock is stale.This branch inserts into the dedup set before the
already_confirmedearly return. A late lock for a tx first seen in-block is supposed to be ignored, but it still marks the txid as "already IS-processed" and can suppress a legitimate transition after a reorg/rescan.Suggested reorder
if context.is_instant_send() { - if !self.instant_send_locks.insert(txid) { - return result; - } // Only accept IS transitions for unconfirmed transactions let already_confirmed = result.affected_accounts.iter().any(|am| { self.accounts .get_by_account_type_match(&am.account_type_match) .and_then(|a| a.transactions.get(&txid)) .map_or(false, |r| r.is_confirmed()) }); if already_confirmed { return result; } + if !self.instant_send_locks.insert(txid) { + return result; + } // Mark UTXOs as IS-locked and update the transaction context🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet/src/transaction_checking/wallet_checker.rs` around lines 81 - 96, The code currently inserts txid into self.instant_send_locks before checking whether the transaction is already confirmed, which can poison the dedup set for stale locks; move the insert of txid into self.instant_send_locks to after the already_confirmed check (i.e., first compute already_confirmed using result.affected_accounts and self.accounts.get_by_account_type_match/... .is_confirmed(), return early if true, and only then call self.instant_send_locks.insert(txid)) so that stale/late IS locks do not suppress future valid transitions in WalletChecker::... the branch handling context.is_instant_send().key-wallet-manager/src/event_tests.rs (1)
210-216:⚠️ Potential issue | 🟡 MinorUse a txid-matching lock in these API tests.
InstantLock::default()does not represent the transaction under test, so these assertions still pass with an invalid lock and will not catch the txid guard thatprocess_mempool_transactionnow needs. Usingdummy_instant_lock(tx.txid())would exercise the real contract.As per coding guidelines, "Write unit tests for new functionality".Suggested test update
- manager.process_mempool_transaction(&tx, Some(InstantLock::default())).await; + manager + .process_mempool_transaction(&tx, Some(dummy_instant_lock(tx.txid()))) + .await; - manager.process_mempool_transaction(&tx, Some(InstantLock::default())).await; + manager + .process_mempool_transaction(&tx, Some(dummy_instant_lock(tx.txid()))) + .await;Also applies to: 366-372
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-manager/src/event_tests.rs` around lines 210 - 216, The test uses InstantLock::default() which doesn't match the transaction under test and won't exercise the txid guard in process_mempool_transaction; update the test_instantsend_tx_emits_balance_updated_spendable (and the similar test at lines ~366-372) to pass a txid-matching lock by replacing InstantLock::default() with dummy_instant_lock(tx.txid()) so the InstantLock corresponds to the created tx and the assertions actually validate the txid check in process_mempool_transaction.key-wallet-ffi/FFI_API.md (1)
853-863:⚠️ Potential issue | 🟠 MajorRegenerate the safety docs for the new
islock_data/islock_lenABI.These signatures now expose raw lock-byte pointers, but the generated Description/Safety sections still describe the old parameter list and never say when
islock_datamay be null, howislock_lenrelates to it, or that the buffer must contain consensus-serializedInstantLockbytes for the duration of the call. For FFI callers, that is part of the contract, so the source docs/generator inputs need updating before regenerating this file.Also applies to: 1317-1327
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/FFI_API.md` around lines 853 - 863, Update the generated docs for the managed_wallet_check_transaction FFI signature to describe the new islock_data/islock_len ABI: state that islock_data may be NULL (meaning no InstantLock), islock_len is the byte length of the buffer when non-NULL, and the buffer must contain consensus-serialized InstantLock bytes for the duration of the call (caller retains ownership and must not mutate/free while the call runs). Also add the same clarifying Safety text for the other transaction-check signature variant that accepts islock_data/islock_len (referencing FFITransactionCheckResult and InstantLock) so the generator output includes the nullability, length relation, and lifetime/ownership contract before regenerating the docs.
🧹 Nitpick comments (2)
key-wallet/src/transaction_checking/wallet_checker.rs (1)
907-910: Use tx-bound locks in these InstantSend tests.
InstantLock::default()carries a zerotxid, so these assertions still pass even if the code stores a payload for the wrong transaction. Build the lock fromtxidin each case so the new payload plumbing is actually exercised.Example replacement
- let result2 = ctx - .check_transaction(&tx, TransactionContext::InstantSend(InstantLock::default())) - .await; + let result2 = ctx + .check_transaction( + &tx, + TransactionContext::InstantSend(InstantLock { txid, ..InstantLock::default() }), + ) + .await;As per coding guidelines "Write unit tests for new functionality".
Also applies to: 936-938, 952-954, 965-967
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet/src/transaction_checking/wallet_checker.rs` around lines 907 - 910, Tests use InstantLock::default() (which has a zero txid) so they don't validate tx-bound payload plumbing; replace InstantLock::default() with constructing an InstantLock tied to the current transaction id (use the tx.txid value) wherever TransactionContext::InstantSend(InstantLock::default()) is used (e.g., in calls to ctx.check_transaction(&tx, TransactionContext::InstantSend(...))). Update all occurrences (around the shown block and the other locations noted) so the InstantLock is created from tx.txid so the code actually exercises storing/reading payloads for the correct transaction.key-wallet-ffi/src/types.rs (1)
1036-1056: Please cover malformed and round-tripInstantSendcases in tests.These tests hit null and valid inbound bytes, but the valid case only checks the variant—not that the payload round-trips. The new
Err(_) => Nonebranch and the outboundFFITransactionContext::from(TransactionContext::InstantSend(_))+free_islock_data()path are still untested. As per coding guidelines, "Write unit tests for new functionality".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/types.rs` around lines 1036 - 1056, Add unit tests that cover malformed and full round-trip InstantSend paths: extend or add tests alongside transaction_context_from_ffi_instant_send_with_null_islock and transaction_context_from_ffi_instant_send_with_valid_islock to (1) pass deliberately corrupted/short bytes to transaction_context_from_ffi and assert it returns None (exercising the Err(_) => None branch) and (2) perform a full round-trip: create InstantLock::default(), serialize it, call transaction_context_from_ffi to get TransactionContext::InstantSend(islock), convert back using FFITransactionContext::from(TransactionContext::InstantSend(_)), extract the outbound islock bytes and assert they match the original serialized bytes, and then call free_islock_data() to exercise the outbound free path and avoid leaks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dash-spv-ffi/src/callbacks.rs`:
- Around line 797-806: The callback currently passes an FFITransactionContext
built from status (FFITransactionContext::from) then immediately calls unsafe {
ffi_ctx.free_islock_data() }, which can leave status.islock_data dangling for C
consumers that retain the pointer beyond the callback; either document that
status.islock_data is only valid for the duration of the cb call (make the
lifetime explicit in the callback typedef/docs) or change the ownership model:
stop freeing islock_data here and instead transfer ownership to the caller by
returning an owned FFITransactionContext (or a raw pointer) and expose a public
destructor (e.g. ffi_transaction_context_destroy /
FFITransactionContext::free_islock_data as a public FFI API) so C code must call
that destroy function when done; update usages of cb,
FFITransactionContext::from and free_islock_data accordingly.
In `@dash-spv/src/sync/mempool/manager.rs`:
- Around line 305-312: handle_tx() removes the pending IS lock early
(pending_is_locks.remove(&txid)) then awaits
wallet.process_mempool_transaction(&tx, pending_lock), which lets
process_instant_send() race and requeue a lock that will never be consumed; fix
by marking the txid as in-flight before removing the lock (e.g., insert txid
into an in_flight_txids set) or by re-checking pending_is_locks immediately
after the await and applying any newly queued lock to mempool_state/wallet
before inserting the tx into mempool_state; update references around
pending_is_locks, handle_tx(), process_mempool_transaction(),
process_instant_send(), and mempool_state insertion so late-arriving locks are
applied immediately rather than stranded.
In `@key-wallet-ffi/src/transaction_checking.rs`:
- Around line 146-161: After constructing context via
transaction_context_from_ffi and binding it to context, validate any InstantSend
lock's txid against the decoded transaction by checking lock.txid == tx.txid()
before proceeding into the wallet; if they differ, call FFIError::set_error with
FFIErrorCode::InvalidInput and return false. Locate the check immediately after
the match that yields context (the context variable) and perform the guard using
the InstantLock/lock fields from context and the transaction's txid() method,
rejecting mismatches rather than persisting a lock for a different tx.
In `@key-wallet-ffi/src/types.rs`:
- Around line 746-755: The struct FFITransactionContext currently derives Clone
but contains raw pointer fields (islock_data/islock_len) so the derived Clone
performs a shallow copy and leads to double-free/dangling pointers when
free_islock_data() is called; fix by removing Clone from FFITransactionContext
and either (a) create a separate non-owning view type (e.g.,
FFITransactionContextRef) used for callbacks and pass a reference to the
original instead of cloning, or (b) implement a manual Clone for
FFITransactionContext that performs a deep copy of the islock_data buffer
(allocating and copying bytes and ensuring Drop/free frees only its own
allocation); update call sites that do ffi_ctx.clone() (the callback at the
previous clone usage) to use the chosen safe approach and ensure
free_islock_data() semantics remain correct.
- Around line 57-75: Mark the FFI boundary as unsafe and document the pointer
safety contract: change the signature of transaction_context_from_ffi to be
unsafe fn transaction_context_from_ffi(...) and mark
FFITransactionContext::to_transaction_context() as unsafe as well; add a safety
doc comment on both (or on FFITransactionContext) stating that when islock_len >
0 the caller must guarantee islock_data is non-null, properly aligned, valid for
reads of islock_len bytes and initialized, and that deserialization of
InstantLock may run arbitrary code; alternatively (if you prefer to avoid
unsafe) change the FFI type to accept a pre-validated slice/Option<&[u8]>
instead of raw pointers and update to_transaction_context and
transaction_context_from_ffi to take that safe type. Ensure references to
islock_data/islock_len and InstantLock are mentioned in the docs so reviewers
can find the validation requirement.
In `@key-wallet-manager/src/process_block.rs`:
- Around line 47-55: In process_mempool_transaction, do not assume any
Some(InstantLock) belongs to tx: validate that lock.txid == tx.txid() before
constructing TransactionContext::InstantSend; if the txid mismatches, treat it
as TransactionContext::Mempool (or return an error) and log or warn about the
mismatched InstantLock. Update the match/branch that currently maps Some(lock)
=> TransactionContext::InstantSend to perform this equality check using
lock.txid and tx.txid(), and only use TransactionContext::InstantSend when they
match.
---
Outside diff comments:
In `@dash-spv/src/sync/mempool/sync_manager.rs`:
- Around line 95-100: The handler for SyncEvent::InstantLockReceived currently
calls process_instant_send unconditionally; change it to check the event's
validated flag and only call
self.process_instant_send(instant_lock.clone()).await when validated == true
(otherwise skip or log/track the unvalidated instant lock as appropriate).
Locate the match arm handling SyncEvent::InstantLockReceived and use the
validated field from the event to gate the call to process_instant_send so
unvalidated instant locks are not processed.
In `@key-wallet-ffi/FFI_API.md`:
- Around line 853-863: Update the generated docs for the
managed_wallet_check_transaction FFI signature to describe the new
islock_data/islock_len ABI: state that islock_data may be NULL (meaning no
InstantLock), islock_len is the byte length of the buffer when non-NULL, and the
buffer must contain consensus-serialized InstantLock bytes for the duration of
the call (caller retains ownership and must not mutate/free while the call
runs). Also add the same clarifying Safety text for the other transaction-check
signature variant that accepts islock_data/islock_len (referencing
FFITransactionCheckResult and InstantLock) so the generator output includes the
nullability, length relation, and lifetime/ownership contract before
regenerating the docs.
In `@key-wallet-ffi/src/transaction.rs`:
- Around line 428-443: transaction_context_from_ffi(...) can produce an
InstantLock context that deserializes but belongs to a different transaction;
before proceeding (after creating context variable in the match), verify that
for InstantLock contexts the embedded lock's txid equals the current tx.txid()
and if not set FFIError::set_error with FFIErrorCode::InvalidInput (or
appropriate code) and return false; locate this check immediately after
obtaining context in the block handling context (symbols:
transaction_context_from_ffi, context, InstantLock, lock.txid, tx.txid,
check_core_transaction) so check_core_transaction cannot be misled by a lock for
a different txid.
In `@key-wallet-manager/src/event_tests.rs`:
- Around line 210-216: The test uses InstantLock::default() which doesn't match
the transaction under test and won't exercise the txid guard in
process_mempool_transaction; update the
test_instantsend_tx_emits_balance_updated_spendable (and the similar test at
lines ~366-372) to pass a txid-matching lock by replacing InstantLock::default()
with dummy_instant_lock(tx.txid()) so the InstantLock corresponds to the created
tx and the assertions actually validate the txid check in
process_mempool_transaction.
In `@key-wallet/src/transaction_checking/wallet_checker.rs`:
- Around line 81-96: The code currently inserts txid into
self.instant_send_locks before checking whether the transaction is already
confirmed, which can poison the dedup set for stale locks; move the insert of
txid into self.instant_send_locks to after the already_confirmed check (i.e.,
first compute already_confirmed using result.affected_accounts and
self.accounts.get_by_account_type_match/... .is_confirmed(), return early if
true, and only then call self.instant_send_locks.insert(txid)) so that
stale/late IS locks do not suppress future valid transitions in
WalletChecker::... the branch handling context.is_instant_send().
In `@key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs`:
- Around line 245-261: In mark_instant_send_utxos, the method only sets
any_changed when account.mark_utxos_instant_send(txid) returns true, but it must
also consider mutations from
record.update_context(TransactionContext::InstantSend(lock.clone())); change the
loop so that the boolean returned by record.update_context(...) is OR'ed into
any_changed (e.g., let changed = account.mark_utxos_instant_send(txid) ||
record.update_context(...); any_changed |= changed), ensuring update_balance()
and the returned value reflect record context changes as well as UTXO changes.
---
Nitpick comments:
In `@key-wallet-ffi/src/types.rs`:
- Around line 1036-1056: Add unit tests that cover malformed and full round-trip
InstantSend paths: extend or add tests alongside
transaction_context_from_ffi_instant_send_with_null_islock and
transaction_context_from_ffi_instant_send_with_valid_islock to (1) pass
deliberately corrupted/short bytes to transaction_context_from_ffi and assert it
returns None (exercising the Err(_) => None branch) and (2) perform a full
round-trip: create InstantLock::default(), serialize it, call
transaction_context_from_ffi to get TransactionContext::InstantSend(islock),
convert back using
FFITransactionContext::from(TransactionContext::InstantSend(_)), extract the
outbound islock bytes and assert they match the original serialized bytes, and
then call free_islock_data() to exercise the outbound free path and avoid leaks.
In `@key-wallet/src/transaction_checking/wallet_checker.rs`:
- Around line 907-910: Tests use InstantLock::default() (which has a zero txid)
so they don't validate tx-bound payload plumbing; replace InstantLock::default()
with constructing an InstantLock tied to the current transaction id (use the
tx.txid value) wherever TransactionContext::InstantSend(InstantLock::default())
is used (e.g., in calls to ctx.check_transaction(&tx,
TransactionContext::InstantSend(...))). Update all occurrences (around the shown
block and the other locations noted) so the InstantLock is created from tx.txid
so the code actually exercises storing/reading payloads for the correct
transaction.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 925cdb1d-1df7-4fcc-b76d-08a03cd4fbbe
📒 Files selected for processing (20)
dash-spv-ffi/src/callbacks.rsdash-spv/src/sync/mempool/manager.rsdash-spv/src/sync/mempool/sync_manager.rskey-wallet-ffi/FFI_API.mdkey-wallet-ffi/src/managed_account.rskey-wallet-ffi/src/transaction.rskey-wallet-ffi/src/transaction_checking.rskey-wallet-ffi/src/types.rskey-wallet-manager/src/event_tests.rskey-wallet-manager/src/lib.rskey-wallet-manager/src/process_block.rskey-wallet-manager/src/test_helpers.rskey-wallet-manager/src/test_utils/mock_wallet.rskey-wallet-manager/src/wallet_interface.rskey-wallet/src/managed_account/mod.rskey-wallet/src/transaction_checking/transaction_context.rskey-wallet/src/transaction_checking/transaction_router/tests/coinbase.rskey-wallet/src/transaction_checking/transaction_router/tests/routing.rskey-wallet/src/transaction_checking/wallet_checker.rskey-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs
Base automatically changed from
feat/ffi-enrich-transaction-surface
to
v0.42-dev
April 1, 2026 21:10
github-actions
bot
added
the
merge-conflict
|
This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them. |
xdustinface
added a commit
that referenced
this pull request
Apr 1, 2026
Addresses CodeRabbit review comment on PR #615 #615 (comment)
xdustinface
added a commit
that referenced
this pull request
Apr 1, 2026
…nt shallow pointer copies Addresses CodeRabbit review comment on PR #615 #615 (comment)
xdustinface
added a commit
that referenced
this pull request
Apr 1, 2026
…ointer before move Addresses CodeRabbit review comment on PR #615 #615 (comment)
xdustinface
added a commit
that referenced
this pull request
Apr 1, 2026
…ool_transaction` Addresses CodeRabbit review comment on PR #615 #615 (comment)
xdustinface
force-pushed
the
feat/instantlock-in-context
branch
from
454790d to
6baaae6
Compare
github-actions
bot
removed
the
merge-conflict
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
key-wallet-manager/src/process_block.rs (1)
52-56:⚠️ Potential issue | 🟠 MajorValidate
instant_lock.txidin release builds.
debug_assert_eq!disappears in release, so a mismatchedSome(lock)still upgradestxtoTransactionContext::InstantSend(lock). That can persist the wrong context and make unrelated funds look spendable. Please enforce this at runtime and fall back toMempool(or return an error) when the txids do not match.Suggested guard
- let context = match instant_lock { - Some(lock) => { - debug_assert_eq!(lock.txid, tx.txid(), "InstantLock txid must match transaction"); - TransactionContext::InstantSend(lock) - } - None => TransactionContext::Mempool, - }; + let txid = tx.txid(); + let context = match instant_lock { + Some(lock) if lock.txid == txid => TransactionContext::InstantSend(lock), + Some(lock) => { + tracing::warn!( + actual_txid = %txid, + lock_txid = %lock.txid, + "ignoring InstantLock for mismatched transaction", + ); + TransactionContext::Mempool + } + None => TransactionContext::Mempool, + };As per coding guidelines, "Always validate inputs from untrusted sources".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-manager/src/process_block.rs` around lines 52 - 56, The debug-only check on instant_lock must become a runtime validation: in the match arm that currently returns TransactionContext::InstantSend(lock), compare lock.txid to tx.txid() and if they differ log an error/warning and instead return TransactionContext::Mempool (or propagate an error) rather than accepting the InstantSend; update the match for instant_lock, referencing instant_lock, TransactionContext::InstantSend, Mempool and tx.txid() to locate the code and ensure inputs from untrusted sources are validated in release builds.dash-spv-ffi/src/callbacks.rs (1)
806-824:⚠️ Potential issue | 🟠 MajorDocument
status.islock_dataas callback-scoped.The buffer behind
status.islock_datais reclaimed immediately aftercbreturns, butOnTransactionStatusChangedCallbackstill only documentswallet_idandtxidlifetimes. A C caller can reasonably retain a dangling pointer here unless the callback contract makesstatus.islock_dataexplicitly borrowed-for-the-callback only, or this path switches to an owned object with a destroy API.As per coding guidelines, "Ensure functions returning pointers transfer ownership (caller must destroy), while functions taking pointers borrow (caller retains ownership)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash-spv-ffi/src/callbacks.rs` around lines 806 - 824, The islock_data buffer in FFITransactionContext (derived from status) is freed immediately after invoking cb (the OnTransactionStatusChangedCallback), so update the callback contract: explicitly document in the OnTransactionStatusChangedCallback (and any public docs/comments near FFITransactionContext/islock_data and the cb invocation sites like where cb(c_wallet_id.as_ptr(), txid_bytes as *const [u8; 32], ffi_ctx, self.user_data) is called) that status.islock_data is callback-scoped and valid only for the duration of the callback; alternatively, if you prefer callers to own the buffer, change the API to return an owned pointer with a matching destroy function — but do not leave the current API undocumented. Ensure the docs mention that the caller must not retain or access islock_data after the callback returns (or must call the provided destroy function if ownership is transferred).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@dash-spv-ffi/src/callbacks.rs`:
- Around line 806-824: The islock_data buffer in FFITransactionContext (derived
from status) is freed immediately after invoking cb (the
OnTransactionStatusChangedCallback), so update the callback contract: explicitly
document in the OnTransactionStatusChangedCallback (and any public docs/comments
near FFITransactionContext/islock_data and the cb invocation sites like where
cb(c_wallet_id.as_ptr(), txid_bytes as *const [u8; 32], ffi_ctx, self.user_data)
is called) that status.islock_data is callback-scoped and valid only for the
duration of the callback; alternatively, if you prefer callers to own the
buffer, change the API to return an owned pointer with a matching destroy
function — but do not leave the current API undocumented. Ensure the docs
mention that the caller must not retain or access islock_data after the callback
returns (or must call the provided destroy function if ownership is
transferred).
In `@key-wallet-manager/src/process_block.rs`:
- Around line 52-56: The debug-only check on instant_lock must become a runtime
validation: in the match arm that currently returns
TransactionContext::InstantSend(lock), compare lock.txid to tx.txid() and if
they differ log an error/warning and instead return TransactionContext::Mempool
(or propagate an error) rather than accepting the InstantSend; update the match
for instant_lock, referencing instant_lock, TransactionContext::InstantSend,
Mempool and tx.txid() to locate the code and ensure inputs from untrusted
sources are validated in release builds.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 203085b3-1a19-439a-8081-043056c7faf3
📒 Files selected for processing (20)
dash-spv-ffi/src/callbacks.rsdash-spv/src/sync/mempool/manager.rsdash-spv/src/sync/mempool/sync_manager.rskey-wallet-ffi/FFI_API.mdkey-wallet-ffi/src/managed_account.rskey-wallet-ffi/src/transaction.rskey-wallet-ffi/src/transaction_checking.rskey-wallet-ffi/src/types.rskey-wallet-manager/src/event_tests.rskey-wallet-manager/src/lib.rskey-wallet-manager/src/process_block.rskey-wallet-manager/src/test_helpers.rskey-wallet-manager/src/test_utils/mock_wallet.rskey-wallet-manager/src/wallet_interface.rskey-wallet/src/managed_account/mod.rskey-wallet/src/transaction_checking/transaction_context.rskey-wallet/src/transaction_checking/transaction_router/tests/coinbase.rskey-wallet/src/transaction_checking/transaction_router/tests/routing.rskey-wallet/src/transaction_checking/wallet_checker.rskey-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs
✅ Files skipped from review due to trivial changes (2)
- key-wallet/src/managed_account/mod.rs
- key-wallet-manager/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (12)
- key-wallet-ffi/src/transaction.rs
- key-wallet/src/transaction_checking/transaction_router/tests/coinbase.rs
- dash-spv/src/sync/mempool/sync_manager.rs
- key-wallet/src/transaction_checking/transaction_router/tests/routing.rs
- key-wallet-ffi/src/managed_account.rs
- key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs
- key-wallet-manager/src/wallet_interface.rs
- key-wallet-ffi/src/transaction_checking.rs
- key-wallet/src/transaction_checking/transaction_context.rs
- key-wallet/src/transaction_checking/wallet_checker.rs
- key-wallet-ffi/src/types.rs
- key-wallet-ffi/FFI_API.md
xdustinface
added a commit
to xdustinface/rust-dashcore
that referenced
this pull request
Apr 2, 2026
commit d9ddcf2 Author: xdustinface <xdustinfacex@gmail.com> Date: Thu Apr 2 09:29:04 2026 +1100 test: fix is lock passed to `process_mempool_transaction` commit 6baaae6 Author: xdustinface <xdustinfacex@gmail.com> Date: Thu Apr 2 08:37:50 2026 +1100 fix: add `debug_assert` for `InstantLock` txid match in `process_mempool_transaction` Addresses CodeRabbit review comment on PR dashpay#615 dashpay#615 (comment) commit af6ae93 Author: xdustinface <xdustinfacex@gmail.com> Date: Thu Apr 2 00:16:53 2026 +1100 fix: avoid dangling `islock_data` pointer in FFI callback by saving pointer before move Addresses CodeRabbit review comment on PR dashpay#615 dashpay#615 (comment) commit 697de96 Author: xdustinface <xdustinfacex@gmail.com> Date: Thu Apr 2 00:16:49 2026 +1100 refactor: remove `Clone` derive from `FFITransactionContext` to prevent shallow pointer copies Addresses CodeRabbit review comment on PR dashpay#615 dashpay#615 (comment) commit 8dddb5c Author: xdustinface <xdustinfacex@gmail.com> Date: Thu Apr 2 00:15:23 2026 +1100 fix: validate `InstantLock` txid matches transaction in FFI boundary Addresses CodeRabbit review comment on PR dashpay#615 dashpay#615 (comment) commit 149649e Author: xdustinface <xdustinfacex@gmail.com> Date: Tue Mar 31 20:47:25 2026 +1100 refactor: rename `mark_instant_send` to `process_instant_send` commit caf0a2f Author: xdustinface <xdustinfacex@gmail.com> Date: Tue Mar 31 20:15:39 2026 +1100 feat: include `InstantLock` in `TransactionContext::InstantSend` variant Change `TransactionContext::InstantSend` from a unit variant to `InstantSend(InstantLock)` so the full IS lock payload is available throughout the wallet and SPV layers. - `WalletInterface::process_mempool_transaction` now takes `Option<InstantLock>` instead of `bool` - `WalletInterface::process_instant_send_lock` takes `InstantLock` instead of `Txid` - `MempoolManager::pending_is_locks` stores the full lock - `FFITransactionContext` struct gains `islock_data`/`islock_len` fields for consensus-serialized IS lock bytes across the FFI boundary - `transaction_context_from_ffi` accepts IS lock bytes and deserializes - `mark_instant_send_utxos` and `wallet_checker` update transaction record context with the IS lock payload
|
This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them. |
github-actions
bot
added
the
merge-conflict
Change `TransactionContext::InstantSend` from a unit variant to `InstantSend(InstantLock)` so the full IS lock payload is available throughout the wallet and SPV layers. - `WalletInterface::process_mempool_transaction` now takes `Option<InstantLock>` instead of `bool` - `WalletInterface::process_instant_send_lock` takes `InstantLock` instead of `Txid` - `MempoolManager::pending_is_locks` stores the full lock - `FFITransactionContext` struct gains `islock_data`/`islock_len` fields for consensus-serialized IS lock bytes across the FFI boundary - `transaction_context_from_ffi` accepts IS lock bytes and deserializes - `mark_instant_send_utxos` and `wallet_checker` update transaction record context with the IS lock payload
Addresses CodeRabbit review comment on PR #615 #615 (comment)
…nt shallow pointer copies Addresses CodeRabbit review comment on PR #615 #615 (comment)
…ointer before move Addresses CodeRabbit review comment on PR #615 #615 (comment)
…ool_transaction` Addresses CodeRabbit review comment on PR #615 #615 (comment)
xdustinface
force-pushed
the
feat/instantlock-in-context
branch
from
d9ddcf2 to
b1aaa08
Compare
github-actions
bot
removed
merge-conflict
ZocoLini
approved these changes
Apr 2, 2026
Collaborator
ZocoLini
left a comment
ZocoLini
left a comment
There was a problem hiding this comment.
While reviewing I noticed that you had to modify some staff that I think is not being used in the FFI side and doesn't make sense to use outside the crate along with some stuff that doesn't make sense. Would like to discuss what are your thought about this PR #528
About the comments, I only care about the functions you created for some tests, but not enough to request changes
About the memory management comment, I would like to discuss that in a call, I really think we need some rules and a way to enforce them
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Change
TransactionContext::InstantSendfrom a unit variant toInstantSend(InstantLock)so the full IS lock payload is available throughout the wallet and SPV layers.WalletInterface::process_mempool_transactionnow takesOption<InstantLock>instead ofboolWalletInterface::process_instant_send_locktakesInstantLockinstead ofTxidMempoolManager::pending_is_locksstores the full lockFFITransactionContextstruct gainsislock_data/islock_lenfields for consensus-serialized IS lock bytes across the FFI boundarytransaction_context_from_ffiaccepts IS lock bytes and deserializesmark_instant_send_utxosandwallet_checkerupdate transaction record context with the IS lock payloadBased on:
TransactionRecordthrough FFI and wallet events #614Summary by CodeRabbit
New Features
API Updates
Improvements
Tests