feat: make wallet events atomic#696
Merged
QuantumExplorer merged 8 commits intov0.42-devfrom Apr 28, 2026
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRefactors wallet events and FFI callbacks to carry post-change balances and richer account metadata: introduces TransactionDetected, TransactionInstantLocked, BlockProcessed, SyncHeightAdvanced; renames FFIAccountType → FFIAccountKind across the FFI; embeds account_type into TransactionRecord; and updates dispatch, callbacks, and tests. Changes
sequenceDiagram
autonumber
participant Node as Dash Node
participant Manager as WalletManager (core)
participant FFI as dash-spv-ffi (FFI)
participant Client as External Client
Node->>Manager: mempool tx / instant-lock / block
Note right of Manager: process_mempool_transaction / process_instant_send / process_block
Manager->>Manager: update records & balances\ncollect per-wallet inserted/updated/matured
Manager->>FFI: emit TransactionDetected (record + balance)
Manager->>FFI: emit TransactionInstantLocked (txid + islock + balance) when applicable
Manager->>FFI: emit BlockProcessed (height + inserted/updated/matured + balance)
FFI->>Client: invoke registered callbacks\n(on_transaction_detected, on_transaction_instant_locked, on_wallet_block_processed, on_sync_height_advanced)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Collaborator
Author
|
@CodeRabbit review |
Contributor
✅ Actions performedReview triggered.
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v0.42-dev #696 +/- ##
=============================================
- Coverage 70.47% 70.22% -0.25%
=============================================
Files 319 319
Lines 66613 66899 +286
=============================================
+ Hits 46946 46983 +37
- Misses 19667 19916 +249
|
![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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
key-wallet-ffi/src/types.rs (1)
241-312:⚠️ Potential issue | 🔴 CriticalMake
FFIAccountKind::to_account_typefallible instead of panicking.This enum is exposed to FFI callers, and
special_account_typesfeeds it back into Rust. Unsupported variants currently panic here, so bad external input can abort the process instead of returningInvalidInput.Suggested direction
- pub fn to_account_type(self, index: u32) -> key_wallet::AccountType { + pub fn to_account_type( + self, + index: u32, + ) -> Result<key_wallet::AccountType, crate::error::FFIErrorCode> { use key_wallet::account::account_type::StandardAccountType; match self { - FFIAccountKind::StandardBIP44 => key_wallet::AccountType::Standard { + FFIAccountKind::StandardBIP44 => Ok(key_wallet::AccountType::Standard { index, standard_account_type: StandardAccountType::BIP44Account, - }, + }), // ... - FFIAccountKind::DashpayReceivingFunds => { - panic!(...) - } + FFIAccountKind::DashpayReceivingFunds + | FFIAccountKind::DashpayExternalAccount + | FFIAccountKind::PlatformPayment => { + Err(crate::error::FFIErrorCode::InvalidInput) + } } }Then have
to_wallet_options()/ the exported entrypoint turn that error into the usualFFIError.🤖 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 241 - 312, Change FFIAccountKind::to_account_type to be fallible: return Result<key_wallet::AccountType, FFIError/InvalidInput> instead of panicking for unsupported variants (e.g., DashpayReceivingFunds, DashpayExternalAccount, PlatformPayment); for those branches return an Err with a clear InvalidInput message. Update callers (e.g., to_wallet_options and the exported FFI entrypoint that consumes special_account_types) to propagate the Result and convert the Err into the usual FFIError return path so external invalid input produces an FFI error rather than aborting the process.key-wallet-ffi/src/account.rs (1)
83-94:⚠️ Potential issue | 🔴 Critical
wallet_get_accountpanics across FFI boundary for unsupported account kinds.Line 93 immediately calls
to_account_type(account_index)on theFFIAccountKindparameter without validation. Three variants—DashpayReceivingFunds,DashpayExternalAccount, andPlatformPayment—intentionally panic in that method (perkey-wallet-ffi/src/types.rs:288-310) because they require additional context (32-byte identity IDs or account/key_class indices) not available through the current FFI API.Panicking across FFI boundaries is undefined behavior. This
extern "C"function must either (1) reject unsupported kinds and return an error, or (2) extend the API signature to accept the required context before attempting conversion. Callers from C code cannot catch panics and have no safe recovery path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/account.rs` around lines 83 - 94, wallet_get_account currently calls FFIAccountKind::to_account_type(account_index) unconditionally, which can panic for variants that need extra context (DashpayReceivingFunds, DashpayExternalAccount, PlatformPayment); change wallet_get_account to first inspect the incoming FFIAccountKind and if it is one of those unsupported variants return an FFIAccountResult::error(FFIErrorCode::InvalidInput, "...") with a clear message instead of calling to_account_type, or alternatively extend the extern signature to accept the required extra context and only call to_account_type after verifying the extra parameters are present; locate references to wallet_get_account and to_account_type to implement the chosen fix.key-wallet-ffi/src/address_pool.rs (1)
286-299:⚠️ Potential issue | 🔴 CriticalAdd validation to reject unsupported
FFIAccountKindvariants before callingto_account_type().The variants
DashpayReceivingFunds,DashpayExternalAccount, andPlatformPaymentcannot be reconstructed from(kind, account_index)alone and will cause panics into_account_type(). These exportedextern "C"entry points should validate and reject unsupported kinds with proper error returns instead of allowing caller-triggered panics across the FFI boundary.Affects
managed_wallet_get_address_pool_info(line 298),managed_wallet_set_gap_limit(line 383), andmanaged_wallet_generate_addresses_to_index(line 459).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/address_pool.rs` around lines 286 - 299, Before calling to_account_type() guard against unsupported FFIAccountKind variants by explicitly checking the incoming account kind and returning a proper FFIError instead of letting to_account_type() panic; in managed_wallet_get_address_pool_info, managed_wallet_set_gap_limit, and managed_wallet_generate_addresses_to_index validate the account kind and if it is DashpayReceivingFunds, DashpayExternalAccount, or PlatformPayment set an appropriate error message on the provided error pointer and return false, otherwise proceed to call account_type.to_account_type(account_index) as before.key-wallet-ffi/src/managed_account.rs (1)
184-223:⚠️ Potential issue | 🔴 CriticalAdd input validation to reject unsupported account kinds in
managed_wallet_get_account.The function at line 184 accepts any
FFIAccountKindbut callsto_account_type(account_index)on line 223 without validating the variant. Three account kinds—DashpayReceivingFunds,DashpayExternalAccount, andPlatformPayment—intentionally panic into_account_type()because they require additional data not available in this API (32-byte identity IDs or account/key_class indices). When a caller passes these unsupported variants, the function panics and aborts the entire program instead of returning an error.Check
account_typeand returnFFIManagedCoreAccountResult::error(...)withFFIErrorCode::InvalidInputfor unsupported variants, or document which kinds are valid and redirect callers to the specialized getters (managed_wallet_get_dashpay_receiving_account,managed_wallet_get_dashpay_external_account,managed_wallet_get_platform_payment_account).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/managed_account.rs` around lines 184 - 223, managed_wallet_get_account must reject FFIAccountKind variants that to_account_type(account_index) cannot handle; before calling account_type.to_account_type(...) check the incoming account_type (FFIAccountKind) and if it is DashpayReceivingFunds, DashpayExternalAccount, or PlatformPayment return FFIManagedCoreAccountResult::error(FFIErrorCode::InvalidInput, "<clear message>") pointing callers to the specialized getters (managed_wallet_get_dashpay_receiving_account, managed_wallet_get_dashpay_external_account, managed_wallet_get_platform_payment_account); keep the existing flow for supported variants and only call account_type.to_account_type(account_index) after this validation.
🧹 Nitpick comments (6)
key-wallet/src/transaction_checking/account_checker.rs (1)
49-58: Field redesign LGTM — consider clarifying that population happens at the wallet layer.The doc rationale ("each record carries its owning
AccountTypeonrecord.account_type") is good. Note that withinManagedAccountCollection::check_transaction(Lines 371‑402) these two vectors are always returned empty; actual population occurs inwallet_checker.rs. A one-line note on the struct or onManagedAccountCollection::check_transactionindicating that new/updated record collection is the wallet checker's responsibility would prevent caller confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet/src/transaction_checking/account_checker.rs` around lines 49 - 58, Add a one-line clarification that although TransactionCheckingResult.{new_records, updated_records} carry AccountType on each TransactionRecord, they are not populated by ManagedAccountCollection::check_transaction; instead population is performed at the wallet layer (see wallet_checker.rs). Update the doc comment on the struct TransactionCheckingResult (or the doc on ManagedAccountCollection::check_transaction) to state that managed-account checks currently return empty vectors and the wallet checker is responsible for filling new_records and updated_records so callers are not misled.key-wallet/src/tests/spent_outpoints_tests.rs (1)
39-53: LGTM — explicit BIP44AccountTypein the test helper is correct.Optional: a similar
test_account_type()helper exists inkey-wallet/src/managed_account/transaction_record.rs(module-private). If you expose apub(crate) fn test_account_type()from a shared test util, both call sites can share it; not necessary for this PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet/src/tests/spent_outpoints_tests.rs` around lines 39 - 53, The test defines an explicit BIP44 AccountType in record_from_tx; optionally factor this into a shared test helper by exposing the existing module-private test_account_type() from key-wallet/src/managed_account/transaction_record.rs as pub(crate) and replace the inline AccountType construction in record_from_tx (and the other test call site) to call that shared test_account_type(); update imports/usages accordingly so both tests reuse the same helper.key-wallet-ffi/src/transaction_checking.rs (1)
29-30: Doc rename toFFIAccountKindis consistent with the broader FFI surface change.The hard-coded discriminants (0..15) below this comment must continue to match
FFIAccountKindexactly; consider replacing the magic numbers withFFIAccountKind::… as c_uintin a follow-up to make this drift-proof, but no action needed for this PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/transaction_checking.rs` around lines 29 - 30, Update the documentation comment for the struct field `account_type` to reference the renamed enum `FFIAccountKind` (e.g., "Account type ID (matches FFIAccountKind enum values)"), ensuring the doc text uses `FFIAccountKind`; leave the existing numeric discriminants alone for this PR but add a note to replace the hard-coded values with `FFIAccountKind::Variant as c_uint` in a follow-up to prevent drift.key-wallet/src/managed_account/transaction_record.rs (1)
78-132: LGTM —account_typeownership tagging onTransactionRecordis clean.Carrying
AccountTypeon the record itself nicely replaces the prior(u32, TransactionRecord)tuples and makes downstream FFI/event delivery self-describing. The constructor argument count is now 8 — reasonable for now with#[allow(clippy::too_many_arguments)], though aTransactionRecordBuilderwould scale better if more fields land later.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet/src/managed_account/transaction_record.rs` around lines 78 - 132, Constructor argument count and account ownership tagging are fine—no code changes required; keep TransactionRecord::new as is with #[allow(clippy::too_many_arguments)] and the account_type field to preserve self-describing records. If more fields are added later, replace the long constructor by implementing a TransactionRecordBuilder (e.g., TransactionRecordBuilder::new(), chainable setters for transaction, account_type, context, transaction_type, direction, input_details, output_details, net_amount, fee, label, and a build() -> TransactionRecord), remove the clippy allow, and update call sites to use the builder.key-wallet-manager/src/events.rs (1)
20-77: Event reshape looks coherent.The new variant set (
TransactionReceived/TransactionStatusChanged/BlockUpdate/SyncHeightUpdate) cleanly carries everything a consumer needs to persist atomically, and thewallet_id()helper plus updateddescription()cover all four arms. One minor consideration for later: sinceWalletEventisCloneand goes throughtokio::sync::broadcast, each subscriber clones the threeVec<TransactionRecord>inBlockUpdate; if you ever expect many subscribers and dense blocks, wrapping the record vectors inArc<[TransactionRecord]>would make fan-out cheap. Not blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-manager/src/events.rs` around lines 20 - 77, Change the BlockUpdate variant's inserted, updated and matured fields from Vec<TransactionRecord> to Arc<[TransactionRecord]> to avoid expensive per-subscriber cloning; update the enum variant definition (BlockUpdate) and any construction sites that build those fields to wrap the Vec into Arc::from(slice) or Arc::from(vec) and update any places that assume Vec (iteration still works via slice); add use std::sync::Arc and adjust tests/consumers to clone the Arc cheaply instead of cloning the full vectors.key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs (1)
274-298: Import and reuse the existingCOINBASE_MATURITYconstant instead of hardcoding100.The constant
COINBASE_MATURITYis already defined indashcore::constants(value 100) and is used in test code. Replace the hardcoded100here with the constant to match the pattern already established inkey-wallet-manager/tests/spv_integration_tests.rs. The same applies tokey-wallet/src/utxo.rs::is_maturewhich also hardcodes100.♻️ Refactor sketch
Add to imports:
+use dashcore::constants::COINBASE_MATURITY;Then update the code:
- let maturity_height = record_height.saturating_add(100); + let maturity_height = record_height.saturating_add(COINBASE_MATURITY as CoreBlockHeight);Apply the same change to
key-wallet/src/utxo.rs:80.🤖 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 274 - 298, Replace the hardcoded 100 with the shared constant COINBASE_MATURITY from dashcore::constants: add use dashcore::constants::COINBASE_MATURITY to the imports and change the maturity computation in matured_coinbase_records (in wallet_info_interface.rs) to use record_height.saturating_add(COINBASE_MATURITY as CoreBlockHeight) (or cast to CoreBlockHeight as required by types) instead of adding 100; make the identical replacement in is_mature in key-wallet/src/utxo.rs so both locations reuse the same constant.
🤖 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/tests/dashd_sync/callbacks.rs`:
- Around line 492-496: Move the call to record_balance(tracker, balance) so it
executes before incrementing tracker.block_processed_wallet_count; specifically,
after optionally updating tracker.block_processed_wallet_record_count (if
records_added > 0) call record_balance(tracker, balance) and only then call
tracker.block_processed_wallet_count.fetch_add(1, Ordering::SeqCst) to ensure
the balance snapshot is stored before tests waiting on
block_processed_wallet_count read last_confirmed/last_unconfirmed.
In `@key-wallet-ffi/src/managed_account.rs`:
- Around line 717-719: The AccountType::IdentityTopUp arm is placing
registration_index into the primary index field but should populate the
secondary index per the struct docs; update the match arm for
AccountType::IdentityTopUp (currently returning (FFIAccountKind::IdentityTopUp,
registration_index, -1)) so it returns (FFIAccountKind::IdentityTopUp, -1,
registration_index) — i.e., set the primary index to -1 and move
registration_index into index_secondary to match the documented contract.
In `@key-wallet/src/transaction_checking/wallet_checker.rs`:
- Around line 135-139: Before calling account.confirm_transaction, record
whether a TransactionRecord already existed (e.g., existed =
account.transactions.contains_key(&tx.txid())); call confirm_transaction as
before; after the call, if existed is true push the post-call record into
result.updated_records, otherwise if the record was created by
confirm_transaction push it into result.inserted. Update the logic around
account.confirm_transaction, result.updated_records, result.inserted, and
account.transactions.get(&tx.txid()) so backfilled confirmations are classified
as inserted instead of updated.
---
Outside diff comments:
In `@key-wallet-ffi/src/account.rs`:
- Around line 83-94: wallet_get_account currently calls
FFIAccountKind::to_account_type(account_index) unconditionally, which can panic
for variants that need extra context (DashpayReceivingFunds,
DashpayExternalAccount, PlatformPayment); change wallet_get_account to first
inspect the incoming FFIAccountKind and if it is one of those unsupported
variants return an FFIAccountResult::error(FFIErrorCode::InvalidInput, "...")
with a clear message instead of calling to_account_type, or alternatively extend
the extern signature to accept the required extra context and only call
to_account_type after verifying the extra parameters are present; locate
references to wallet_get_account and to_account_type to implement the chosen
fix.
In `@key-wallet-ffi/src/address_pool.rs`:
- Around line 286-299: Before calling to_account_type() guard against
unsupported FFIAccountKind variants by explicitly checking the incoming account
kind and returning a proper FFIError instead of letting to_account_type() panic;
in managed_wallet_get_address_pool_info, managed_wallet_set_gap_limit, and
managed_wallet_generate_addresses_to_index validate the account kind and if it
is DashpayReceivingFunds, DashpayExternalAccount, or PlatformPayment set an
appropriate error message on the provided error pointer and return false,
otherwise proceed to call account_type.to_account_type(account_index) as before.
In `@key-wallet-ffi/src/managed_account.rs`:
- Around line 184-223: managed_wallet_get_account must reject FFIAccountKind
variants that to_account_type(account_index) cannot handle; before calling
account_type.to_account_type(...) check the incoming account_type
(FFIAccountKind) and if it is DashpayReceivingFunds, DashpayExternalAccount, or
PlatformPayment return
FFIManagedCoreAccountResult::error(FFIErrorCode::InvalidInput, "<clear
message>") pointing callers to the specialized getters
(managed_wallet_get_dashpay_receiving_account,
managed_wallet_get_dashpay_external_account,
managed_wallet_get_platform_payment_account); keep the existing flow for
supported variants and only call account_type.to_account_type(account_index)
after this validation.
In `@key-wallet-ffi/src/types.rs`:
- Around line 241-312: Change FFIAccountKind::to_account_type to be fallible:
return Result<key_wallet::AccountType, FFIError/InvalidInput> instead of
panicking for unsupported variants (e.g., DashpayReceivingFunds,
DashpayExternalAccount, PlatformPayment); for those branches return an Err with
a clear InvalidInput message. Update callers (e.g., to_wallet_options and the
exported FFI entrypoint that consumes special_account_types) to propagate the
Result and convert the Err into the usual FFIError return path so external
invalid input produces an FFI error rather than aborting the process.
---
Nitpick comments:
In `@key-wallet-ffi/src/transaction_checking.rs`:
- Around line 29-30: Update the documentation comment for the struct field
`account_type` to reference the renamed enum `FFIAccountKind` (e.g., "Account
type ID (matches FFIAccountKind enum values)"), ensuring the doc text uses
`FFIAccountKind`; leave the existing numeric discriminants alone for this PR but
add a note to replace the hard-coded values with `FFIAccountKind::Variant as
c_uint` in a follow-up to prevent drift.
In `@key-wallet-manager/src/events.rs`:
- Around line 20-77: Change the BlockUpdate variant's inserted, updated and
matured fields from Vec<TransactionRecord> to Arc<[TransactionRecord]> to avoid
expensive per-subscriber cloning; update the enum variant definition
(BlockUpdate) and any construction sites that build those fields to wrap the Vec
into Arc::from(slice) or Arc::from(vec) and update any places that assume Vec
(iteration still works via slice); add use std::sync::Arc and adjust
tests/consumers to clone the Arc cheaply instead of cloning the full vectors.
In `@key-wallet/src/managed_account/transaction_record.rs`:
- Around line 78-132: Constructor argument count and account ownership tagging
are fine—no code changes required; keep TransactionRecord::new as is with
#[allow(clippy::too_many_arguments)] and the account_type field to preserve
self-describing records. If more fields are added later, replace the long
constructor by implementing a TransactionRecordBuilder (e.g.,
TransactionRecordBuilder::new(), chainable setters for transaction,
account_type, context, transaction_type, direction, input_details,
output_details, net_amount, fee, label, and a build() -> TransactionRecord),
remove the clippy allow, and update call sites to use the builder.
In `@key-wallet/src/tests/spent_outpoints_tests.rs`:
- Around line 39-53: The test defines an explicit BIP44 AccountType in
record_from_tx; optionally factor this into a shared test helper by exposing the
existing module-private test_account_type() from
key-wallet/src/managed_account/transaction_record.rs as pub(crate) and replace
the inline AccountType construction in record_from_tx (and the other test call
site) to call that shared test_account_type(); update imports/usages accordingly
so both tests reuse the same helper.
In `@key-wallet/src/transaction_checking/account_checker.rs`:
- Around line 49-58: Add a one-line clarification that although
TransactionCheckingResult.{new_records, updated_records} carry AccountType on
each TransactionRecord, they are not populated by
ManagedAccountCollection::check_transaction; instead population is performed at
the wallet layer (see wallet_checker.rs). Update the doc comment on the struct
TransactionCheckingResult (or the doc on
ManagedAccountCollection::check_transaction) to state that managed-account
checks currently return empty vectors and the wallet checker is responsible for
filling new_records and updated_records so callers are not misled.
In `@key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs`:
- Around line 274-298: Replace the hardcoded 100 with the shared constant
COINBASE_MATURITY from dashcore::constants: add use
dashcore::constants::COINBASE_MATURITY to the imports and change the maturity
computation in matured_coinbase_records (in wallet_info_interface.rs) to use
record_height.saturating_add(COINBASE_MATURITY as CoreBlockHeight) (or cast to
CoreBlockHeight as required by types) instead of adding 100; make the identical
replacement in is_mature in key-wallet/src/utxo.rs so both locations reuse the
same constant.
🪄 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: 69f6b604-09d6-45ca-ab00-1df6ddc4a754
📒 Files selected for processing (34)
dash-spv-ffi/src/bin/ffi_cli.rsdash-spv-ffi/src/callbacks.rsdash-spv-ffi/src/lib.rsdash-spv-ffi/tests/dashd_sync/callbacks.rsdash-spv-ffi/tests/dashd_sync/context.rsdash-spv-ffi/tests/dashd_sync/tests_callback.rsdash-spv-ffi/tests/dashd_sync/tests_transaction.rsdash-spv/tests/dashd_sync/helpers.rsdash-spv/tests/dashd_sync/tests_mempool.rskey-wallet-ffi/FFI_API.mdkey-wallet-ffi/src/account.rskey-wallet-ffi/src/account_collection.rskey-wallet-ffi/src/account_derivation_tests.rskey-wallet-ffi/src/account_tests.rskey-wallet-ffi/src/address_pool.rskey-wallet-ffi/src/managed_account.rskey-wallet-ffi/src/transaction_checking.rskey-wallet-ffi/src/types.rskey-wallet-ffi/src/wallet.rskey-wallet-ffi/src/wallet_tests.rskey-wallet-ffi/tests/test_managed_account_collection.rskey-wallet-manager/Cargo.tomlkey-wallet-manager/src/accessors.rskey-wallet-manager/src/event_tests.rskey-wallet-manager/src/events.rskey-wallet-manager/src/lib.rskey-wallet-manager/src/process_block.rskey-wallet-manager/src/test_helpers.rskey-wallet/src/managed_account/mod.rskey-wallet/src/managed_account/transaction_record.rskey-wallet/src/tests/spent_outpoints_tests.rskey-wallet/src/transaction_checking/account_checker.rskey-wallet/src/transaction_checking/wallet_checker.rskey-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs
💤 Files with no reviewable changes (1)
- key-wallet-manager/src/test_helpers.rs
github-actions
Bot
added
the
merge-conflict
Contributor
|
This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them. |
Reshape `WalletEvent` so each variant carries the records or context needed to persist a wallet update atomically off a single event, alongside the post-change balance.
The variant set is now:
- `TransactionReceived { wallet_id, record, balance }`. Fires when the wallet first sees an off-chain transaction.
- `TransactionStatusChanged { wallet_id, txid, context, balance }`. Fires when a known off-chain transaction has its state change. Currently fires only for InstantSend locks.
- `BlockUpdate { wallet_id, height, inserted, updated, matured, balance }`. Carries records bucketed by what happened to them in the block, plus the post-block balance.
- `SyncHeightUpdate { wallet_id, height }`. Marks a filter-batch checkpoint.
`TransactionRecord` carries `account_type` directly, identifying the owning account. `WalletInfoInterface` gains a `matured_coinbase_records` method that enumerates coinbase records crossing the maturity threshold during a height advance, populating `BlockUpdate.matured`.
The FFI groups the flattened account-discriminator fields into an `FFIAccountType` struct and renames the prior discriminant enum to `FFIAccountKind`.
Tests wait on `block_processed_wallet_count` and then read `last_confirmed`/`last_unconfirmed`. Bumping the counter before storing the balance snapshot left those reads racey. Reorder so the balance is recorded first. Addresses CodeRabbit review comment on PR #696 #696 (comment)
The `FFIAccountType` doc states `index_secondary` carries `registration_index` for `IdentityTopUp` and `index = 0` for variants without a meaningful primary index, matching the parallel encoding in `FFIAccountKind::from_account_type`. The `From<&AccountType>` impl wrote `registration_index` into `index` instead, breaking the documented FFI contract. Addresses CodeRabbit review comment on PR #696 #696 (comment)
`is_new` is wallet-wide (set on the first matching account, then breaks), so the per-account `else` branch can run for an account that did not previously hold the record. `confirm_transaction` backfills via `record_transaction` in that case, but the post-call record was always pushed onto `updated_records`, breaking the atomic `inserted`/`updated` contract consumed by `WalletEvent::BlockUpdate`. Capture `existed_before` per account and route to `new_records` when the record was just created. Addresses CodeRabbit review comment on PR #696 #696 (comment)
`process_block` and `update_last_processed_height` duplicated the entire balance-snapshot, prior-heights collection, matured-coinbase window, height advance, and per-wallet `BlockUpdate` emission. Extract the shared tail into a private `WalletManager::finalize_block_advance` helper that takes the inserted/updated maps. `update_last_processed_height` becomes a one-line call with empty maps; `process_block` keeps only its txdata loop before delegating.
xdustinface
force-pushed
the
feat/wallet-events-atomic
branch
from
14ef598 to
14da758
Compare
github-actions
Bot
removed
the
merge-conflict
Rename `WalletEvent` variants and the matching FFI callbacks to past-participle names that say what happened, replacing vague "Update" suffixes: - `TransactionReceived` -> `TransactionDetected`. "Received" implied incoming funds, but the event fires for any first-time off-chain sighting (incoming or outgoing). - `TransactionStatusChanged` -> `TransactionInstantLocked`. The event only ever fires for an InstantSend lock applied to a known mempool tx, so name it for what it actually is. Drop the `status: TransactionContext` field and carry the `InstantLock` directly. - `BlockUpdate` -> `BlockProcessed`. Mirrors `process_block` and matches the past-participle pattern. - `SyncHeightUpdate` -> `SyncHeightAdvanced`. Conveys monotonic forward motion. FFI rename mirrors the Rust side: the IS callback now takes `islock_data: *const u8` + `islock_len: usize` instead of an `FFITransactionContext`, removing a discriminant that was always `InstantSend`. The wallet-side `OnBlockProcessedCallback` becomes `OnWalletBlockProcessedCallback` to disambiguate from the existing sync-event type with the same name.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
key-wallet/src/transaction_checking/wallet_checker.rs (1)
83-107:⚠️ Potential issue | 🟠 MajorBackfill newly affected accounts in the InstantSend path.
is_newis computed at the wallet level, but this branch only updates accounts that already havetxid. If one account has the record and another newly matched account does not, the latter is skipped entirely here. The confirmation path below already handles that per-account backfill; the InstantSend path needs the same treatment or the account never gets its record/UTXO state.🤖 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 83 - 107, InstantSend branch only updates accounts that already contain the tx record, skipping newly matched accounts; instead, when iterating result.affected_accounts after acquiring account via self.get_by_account_type_match_mut(&account_match.account_type_match), ensure you backfill a missing transaction record the same way the confirmation path does: if account.transactions.get(&txid) is None, create/insert a proper transaction record (preserving/setting per-account is_new as computed at wallet level or recompute per-account), then call account.mark_utxos_instant_send(&txid), update the record context via record.update_context(context.clone()), and push the record.clone() into result.updated_records; use the same helpers used elsewhere (account.mark_utxos_instant_send, account.transactions, result.updated_records) so newly-affected accounts receive UTXO and record state for InstantSend.dash-spv-ffi/tests/dashd_sync/tests_transaction.rs (1)
14-24:⚠️ Potential issue | 🟠 MajorMark these dashd-backed tests as ignored.
These cases spin up
DashdTestContext, wait on sync, and mine blocks, so they are network-dependent integration tests. Running them by default makes the normal test target depend on local dashd/mining availability instead of opt-in ignored coverage.As per coding guidelines, "Mark network-dependent or long-running tests with
#[ignore]attribute; run with-- --ignored".Also applies to: 183-193, 267-277
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash-spv-ffi/tests/dashd_sync/tests_transaction.rs` around lines 14 - 24, Mark these dashd-backed integration tests as ignored by adding the #[ignore] attribute to each test function that spins up DashdTestContext (e.g., test_ffi_sync_then_generate_blocks) and the other two dashd-dependent tests referenced around the other diff blocks; leave the existing runtime/runtime-check code (rt.build, DashdTestContext::new, supports_mining checks) intact so the tests still early-return when dashd is unavailable, but require explicit `cargo test -- --ignored` to run them.dash-spv-ffi/tests/dashd_sync/tests_callback.rs (1)
11-18:⚠️ Potential issue | 🟠 MajorMark these dashd-backed callback tests as ignored.
Both tests depend on a live dashd instance, network callbacks, and block generation. They should be opt-in integration coverage rather than part of the default test run.
As per coding guidelines, "Mark network-dependent or long-running tests with
#[ignore]attribute; run with-- --ignored".Also applies to: 311-320
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash-spv-ffi/tests/dashd_sync/tests_callback.rs` around lines 11 - 18, The test(s) that require a live dashd instance (e.g., the test function test_all_callbacks_during_sync in tests/dashd_sync/tests_callback.rs and the other network-dependent test around lines 311-320) should be marked as opt-in integration tests by adding the #[ignore] attribute to their test functions; locate the test functions (test_all_callbacks_during_sync and the other dashd-backed callback test) and prepend #[ignore] above the existing #[test] so they are excluded from default cargo test runs and only executed when running cargo test -- --ignored.
♻️ Duplicate comments (1)
dash-spv-ffi/tests/dashd_sync/callbacks.rs (1)
431-437:⚠️ Potential issue | 🟡 MinorWrite the balance before bumping
transaction_instant_send_locked_count.This callback increments the counter on Line 435 and only then updates
last_confirmed/last_unconfirmedon Line 436. Any test that waits ontransaction_instant_send_locked_countcan still observe the previous balance snapshot.Suggested fix
- tracker.transaction_instant_send_locked_count.fetch_add(1, Ordering::SeqCst); - record_balance(tracker, balance); + record_balance(tracker, balance); + tracker.transaction_instant_send_locked_count.fetch_add(1, Ordering::SeqCst);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash-spv-ffi/tests/dashd_sync/callbacks.rs` around lines 431 - 437, The callback currently increments transaction_instant_send_locked_count before updating the tracker balance, which can let waiters observe the old snapshot; move the call to record_balance(tracker, balance) (which updates last_confirmed/last_unconfirmed via tracker) so it runs before tracker.transaction_instant_send_locked_count.fetch_add(1, Ordering::SeqCst), keeping the increment after the balance update and preserving correct ordering for any threads waiting on the counter.
🧹 Nitpick comments (1)
dash-spv-ffi/tests/dashd_sync/callbacks.rs (1)
441-494: Don't drop thematuredbucket from the tracker.
on_wallet_block_processedrecordsinsertedandupdated, but_maturedis ignored completely. That means a callback carrying only matured coinbase records incrementsblock_processed_wallet_countwhile leavingblock_processed_wallet_record_countand the per-record vectors unchanged, so this new event bucket is never validated end-to-end through the FFI test harness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash-spv-ffi/tests/dashd_sync/callbacks.rs` around lines 441 - 494, on_wallet_block_processed currently ignores the _matured parameter which causes matured-only callbacks to increment block_processed_wallet_count without recording per-record state; update the function to consume the matured pointer (rename _matured -> matured) and handle it like inserted/updated: create a slice from_raw_parts(matured, matured_count), push each record's (txid, net_amount) into tracker.block_received_transactions and account kind/index into tracker.block_account_types/block_account_indices, increment records_added, and add a distinct marker for matured records (either extend the existing bucket representation to encode inserted/updated/matured or add a new tracker vector such as block_record_matured) so tests observe matured records; ensure block_processed_wallet_record_count.fetch_add includes matured records and keep the existing calls to record_balance and block_processed_wallet_count unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@key-wallet-ffi/src/managed_account.rs`:
- Around line 686-698: The FFI currently casts hardened unsigned indices to i32
causing sign-bit flips; update the FFI struct fields that represent unsigned
hardened indices (e.g., index_secondary and key_class) to use u32 (or another
unsigned type that preserves 0..0xFFFF_FFFF) instead of i32, and adjust any
places that construct these fields from PlatformPayment.key_class and
IdentityTopUp.registration_index so they pass through as u32 rather than being
downcast to i32; ensure consumers and any conversion logic are updated to expect
u32 to preserve hardened indices >= 0x8000_0000.
In `@key-wallet-manager/src/process_block.rs`:
- Around line 263-286: The code advances per-wallet heights via
info.update_last_processed_height(height) but reads the cached balance later via
info.balance() without recomputing it, so BlockProcessed may carry a stale
balance; fix by recomputing/refreshing each WalletInfo's cached balance
immediately after update_last_processed_height (e.g., call the WalletInfo method
that recalculates balances such as info.recompute_balance() or
info.refresh_balance() / the existing balance-update API) and then use
info.balance() when building WalletEvent::BlockProcessed so the emitted balance
reflects post-height-change state.
---
Outside diff comments:
In `@dash-spv-ffi/tests/dashd_sync/tests_callback.rs`:
- Around line 11-18: The test(s) that require a live dashd instance (e.g., the
test function test_all_callbacks_during_sync in
tests/dashd_sync/tests_callback.rs and the other network-dependent test around
lines 311-320) should be marked as opt-in integration tests by adding the
#[ignore] attribute to their test functions; locate the test functions
(test_all_callbacks_during_sync and the other dashd-backed callback test) and
prepend #[ignore] above the existing #[test] so they are excluded from default
cargo test runs and only executed when running cargo test -- --ignored.
In `@dash-spv-ffi/tests/dashd_sync/tests_transaction.rs`:
- Around line 14-24: Mark these dashd-backed integration tests as ignored by
adding the #[ignore] attribute to each test function that spins up
DashdTestContext (e.g., test_ffi_sync_then_generate_blocks) and the other two
dashd-dependent tests referenced around the other diff blocks; leave the
existing runtime/runtime-check code (rt.build, DashdTestContext::new,
supports_mining checks) intact so the tests still early-return when dashd is
unavailable, but require explicit `cargo test -- --ignored` to run them.
In `@key-wallet/src/transaction_checking/wallet_checker.rs`:
- Around line 83-107: InstantSend branch only updates accounts that already
contain the tx record, skipping newly matched accounts; instead, when iterating
result.affected_accounts after acquiring account via
self.get_by_account_type_match_mut(&account_match.account_type_match), ensure
you backfill a missing transaction record the same way the confirmation path
does: if account.transactions.get(&txid) is None, create/insert a proper
transaction record (preserving/setting per-account is_new as computed at wallet
level or recompute per-account), then call
account.mark_utxos_instant_send(&txid), update the record context via
record.update_context(context.clone()), and push the record.clone() into
result.updated_records; use the same helpers used elsewhere
(account.mark_utxos_instant_send, account.transactions, result.updated_records)
so newly-affected accounts receive UTXO and record state for InstantSend.
---
Duplicate comments:
In `@dash-spv-ffi/tests/dashd_sync/callbacks.rs`:
- Around line 431-437: The callback currently increments
transaction_instant_send_locked_count before updating the tracker balance, which
can let waiters observe the old snapshot; move the call to
record_balance(tracker, balance) (which updates last_confirmed/last_unconfirmed
via tracker) so it runs before
tracker.transaction_instant_send_locked_count.fetch_add(1, Ordering::SeqCst),
keeping the increment after the balance update and preserving correct ordering
for any threads waiting on the counter.
---
Nitpick comments:
In `@dash-spv-ffi/tests/dashd_sync/callbacks.rs`:
- Around line 441-494: on_wallet_block_processed currently ignores the _matured
parameter which causes matured-only callbacks to increment
block_processed_wallet_count without recording per-record state; update the
function to consume the matured pointer (rename _matured -> matured) and handle
it like inserted/updated: create a slice from_raw_parts(matured, matured_count),
push each record's (txid, net_amount) into tracker.block_received_transactions
and account kind/index into tracker.block_account_types/block_account_indices,
increment records_added, and add a distinct marker for matured records (either
extend the existing bucket representation to encode inserted/updated/matured or
add a new tracker vector such as block_record_matured) so tests observe matured
records; ensure block_processed_wallet_record_count.fetch_add includes matured
records and keep the existing calls to record_balance and
block_processed_wallet_count unchanged.
🪄 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: 61c80880-5ffa-4eee-8584-b0cb60cf6125
📒 Files selected for processing (34)
dash-spv-ffi/src/bin/ffi_cli.rsdash-spv-ffi/src/callbacks.rsdash-spv-ffi/src/lib.rsdash-spv-ffi/tests/dashd_sync/callbacks.rsdash-spv-ffi/tests/dashd_sync/context.rsdash-spv-ffi/tests/dashd_sync/tests_callback.rsdash-spv-ffi/tests/dashd_sync/tests_transaction.rsdash-spv/tests/dashd_sync/helpers.rsdash-spv/tests/dashd_sync/tests_mempool.rskey-wallet-ffi/FFI_API.mdkey-wallet-ffi/src/account.rskey-wallet-ffi/src/account_collection.rskey-wallet-ffi/src/account_derivation_tests.rskey-wallet-ffi/src/account_tests.rskey-wallet-ffi/src/address_pool.rskey-wallet-ffi/src/managed_account.rskey-wallet-ffi/src/transaction_checking.rskey-wallet-ffi/src/types.rskey-wallet-ffi/src/wallet.rskey-wallet-ffi/src/wallet_tests.rskey-wallet-ffi/tests/test_managed_account_collection.rskey-wallet-manager/Cargo.tomlkey-wallet-manager/src/accessors.rskey-wallet-manager/src/event_tests.rskey-wallet-manager/src/events.rskey-wallet-manager/src/lib.rskey-wallet-manager/src/process_block.rskey-wallet-manager/src/test_helpers.rskey-wallet/src/managed_account/mod.rskey-wallet/src/managed_account/transaction_record.rskey-wallet/src/tests/spent_outpoints_tests.rskey-wallet/src/transaction_checking/account_checker.rskey-wallet/src/transaction_checking/wallet_checker.rskey-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs
💤 Files with no reviewable changes (1)
- key-wallet-manager/src/test_helpers.rs
✅ Files skipped from review due to trivial changes (8)
- key-wallet-ffi/src/transaction_checking.rs
- dash-spv/tests/dashd_sync/tests_mempool.rs
- key-wallet-ffi/src/wallet_tests.rs
- key-wallet-ffi/src/account_derivation_tests.rs
- key-wallet-ffi/src/account_collection.rs
- key-wallet-ffi/src/account_tests.rs
- key-wallet-ffi/src/account.rs
- key-wallet-ffi/FFI_API.md
🚧 Files skipped from review as they are similar to previous changes (12)
- key-wallet/src/managed_account/mod.rs
- key-wallet-manager/Cargo.toml
- dash-spv-ffi/src/lib.rs
- key-wallet-manager/src/accessors.rs
- key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs
- key-wallet-ffi/tests/test_managed_account_collection.rs
- key-wallet/src/transaction_checking/account_checker.rs
- dash-spv/tests/dashd_sync/helpers.rs
- key-wallet-manager/src/lib.rs
- key-wallet-ffi/src/types.rs
- key-wallet-ffi/src/address_pool.rs
- key-wallet-ffi/src/wallet.rs
Addresses CodeRabbit review comment on PR #696 #696 (review) The instant_locked callback bumped `transaction_instant_send_locked_count` before calling `record_balance`. Tests that wait on the counter and then read `last_confirmed`/`last_unconfirmed` could observe the previous balance snapshot. Match the ordering used by the other callbacks: store the balance first, then bump the counter.
Addresses CodeRabbit review comment on PR #696 #696 (review) The IS-lock branch in `WalletTransactionChecker::check_core_transaction` only updated accounts that already held a `TransactionRecord` for the txid. When wallet-level `is_new` was `false` (because at least one account had the record) but another matched account did not, the latter was silently skipped: no record was created and `mark_utxos_instant_send` ran against an empty UTXO set on that account. Mirror the confirmation path: when the affected account lacks the record, call `record_transaction` to register the record and its UTXOs, then mark them IS-locked. This ordering ensures the freshly registered UTXOs receive the IS-lock flag too. The backfilled record is pushed into `new_records` to match the existing convention from commit 659a6d5. Add `test_instantsend_backfills_missing_record_in_other_account` covering the multi-account scenario.
Collaborator
Author
|
@CodeRabbit review |
Contributor
✅ Actions performedReview triggered.
|
QuantumExplorer
approved these changes
Apr 28, 2026
QuantumExplorer
added a commit
that referenced
this pull request
Apr 28, 2026
* chore(dash-spv): refresh masternode seed files (#695) Co-authored-by: QuantumExplorer <11468583+QuantumExplorer@users.noreply.github.com> * feat: make wallet events atomic (#696) * feat: make wallet events atomic Reshape `WalletEvent` so each variant carries the records or context needed to persist a wallet update atomically off a single event, alongside the post-change balance. The variant set is now: - `TransactionReceived { wallet_id, record, balance }`. Fires when the wallet first sees an off-chain transaction. - `TransactionStatusChanged { wallet_id, txid, context, balance }`. Fires when a known off-chain transaction has its state change. Currently fires only for InstantSend locks. - `BlockUpdate { wallet_id, height, inserted, updated, matured, balance }`. Carries records bucketed by what happened to them in the block, plus the post-block balance. - `SyncHeightUpdate { wallet_id, height }`. Marks a filter-batch checkpoint. `TransactionRecord` carries `account_type` directly, identifying the owning account. `WalletInfoInterface` gains a `matured_coinbase_records` method that enumerates coinbase records crossing the maturity threshold during a height advance, populating `BlockUpdate.matured`. The FFI groups the flattened account-discriminator fields into an `FFIAccountType` struct and renames the prior discriminant enum to `FFIAccountKind`. * fix: record balance before bumping `block_processed_wallet_count` Tests wait on `block_processed_wallet_count` and then read `last_confirmed`/`last_unconfirmed`. Bumping the counter before storing the balance snapshot left those reads racey. Reorder so the balance is recorded first. Addresses CodeRabbit review comment on PR #696 #696 (comment) * fix: place `IdentityTopUp.registration_index` in `index_secondary` The `FFIAccountType` doc states `index_secondary` carries `registration_index` for `IdentityTopUp` and `index = 0` for variants without a meaningful primary index, matching the parallel encoding in `FFIAccountKind::from_account_type`. The `From<&AccountType>` impl wrote `registration_index` into `index` instead, breaking the documented FFI contract. Addresses CodeRabbit review comment on PR #696 #696 (comment) * fix: route confirmation backfills to `new_records` `is_new` is wallet-wide (set on the first matching account, then breaks), so the per-account `else` branch can run for an account that did not previously hold the record. `confirm_transaction` backfills via `record_transaction` in that case, but the post-call record was always pushed onto `updated_records`, breaking the atomic `inserted`/`updated` contract consumed by `WalletEvent::BlockUpdate`. Capture `existed_before` per account and route to `new_records` when the record was just created. Addresses CodeRabbit review comment on PR #696 #696 (comment) * refactor(key-wallet-manager): extract `finalize_block_advance` helper `process_block` and `update_last_processed_height` duplicated the entire balance-snapshot, prior-heights collection, matured-coinbase window, height advance, and per-wallet `BlockUpdate` emission. Extract the shared tail into a private `WalletManager::finalize_block_advance` helper that takes the inserted/updated maps. `update_last_processed_height` becomes a one-line call with empty maps; `process_block` keeps only its txdata loop before delegating. * refactor: rename wallet events for clearer semantics Rename `WalletEvent` variants and the matching FFI callbacks to past-participle names that say what happened, replacing vague "Update" suffixes: - `TransactionReceived` -> `TransactionDetected`. "Received" implied incoming funds, but the event fires for any first-time off-chain sighting (incoming or outgoing). - `TransactionStatusChanged` -> `TransactionInstantLocked`. The event only ever fires for an InstantSend lock applied to a known mempool tx, so name it for what it actually is. Drop the `status: TransactionContext` field and carry the `InstantLock` directly. - `BlockUpdate` -> `BlockProcessed`. Mirrors `process_block` and matches the past-participle pattern. - `SyncHeightUpdate` -> `SyncHeightAdvanced`. Conveys monotonic forward motion. FFI rename mirrors the Rust side: the IS callback now takes `islock_data: *const u8` + `islock_len: usize` instead of an `FFITransactionContext`, removing a discriminant that was always `InstantSend`. The wallet-side `OnBlockProcessedCallback` becomes `OnWalletBlockProcessedCallback` to disambiguate from the existing sync-event type with the same name. * fix: record balance before bumping IS-locked counter in test callback Addresses CodeRabbit review comment on PR #696 #696 (review) The instant_locked callback bumped `transaction_instant_send_locked_count` before calling `record_balance`. Tests that wait on the counter and then read `last_confirmed`/`last_unconfirmed` could observe the previous balance snapshot. Match the ordering used by the other callbacks: store the balance first, then bump the counter. * fix: backfill missing transaction record in InstantSend path Addresses CodeRabbit review comment on PR #696 #696 (review) The IS-lock branch in `WalletTransactionChecker::check_core_transaction` only updated accounts that already held a `TransactionRecord` for the txid. When wallet-level `is_new` was `false` (because at least one account had the record) but another matched account did not, the latter was silently skipped: no record was created and `mark_utxos_instant_send` ran against an empty UTXO set on that account. Mirror the confirmation path: when the affected account lacks the record, call `record_transaction` to register the record and its UTXOs, then mark them IS-locked. This ordering ensures the freshly registered UTXOs receive the IS-lock flag too. The backfilled record is pushed into `new_records` to match the existing convention from commit 659a6d5. Add `test_instantsend_backfills_missing_record_in_other_account` covering the multi-account scenario. --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: QuantumExplorer <11468583+QuantumExplorer@users.noreply.github.com> Co-authored-by: Kevin Rombach <35775977+xdustinface@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
QuantumExplorer
added a commit
that referenced
this pull request
Apr 28, 2026
* feat: per-wallet filter scan and runtime wallet catch-up Filter matching and block processing now operate per wallet, so a wallet added at runtime catches up without forcing the already-synced wallets to reprocess anything. - `WalletInterface` restructured around per-wallet ops: `process_block_for_wallets`, `wallets_behind`, `monitored_addresses_for`, `wallet_synced_height`, and monotonic per-wallet height updates. Aggregate heights are derived (min of `synced_height`, max of `last_processed_height`) rather than stored. - `FiltersManager::scan_batch` matches each behind wallet's addresses only against filter heights it hasn't yet covered; already-synced wallets are skipped entirely. Matched blocks flow through `BlocksNeeded` carrying the per-block wallet set so `BlocksManager` processes each block only against the wallets whose filters matched. `FiltersBatch` records the scanned-wallet set so commit advances only their `synced_height`. - `FiltersManager::tick` detects when a wallet's `synced_height` sits below the current `committed_height` (a runtime add behind scan progress), clears in-flight pipeline state, lowers `committed_height` to the new aggregate floor, and re-enters `start_download` on the next 100ms tick. Runs in `Syncing`, `Synced`, and `WaitForEvents`. Based on: - #689 * chore: merge v0.42-dev into feat/per-wallet-filter-scan (#698) * chore(dash-spv): refresh masternode seed files (#695) Co-authored-by: QuantumExplorer <11468583+QuantumExplorer@users.noreply.github.com> * feat: make wallet events atomic (#696) * feat: make wallet events atomic Reshape `WalletEvent` so each variant carries the records or context needed to persist a wallet update atomically off a single event, alongside the post-change balance. The variant set is now: - `TransactionReceived { wallet_id, record, balance }`. Fires when the wallet first sees an off-chain transaction. - `TransactionStatusChanged { wallet_id, txid, context, balance }`. Fires when a known off-chain transaction has its state change. Currently fires only for InstantSend locks. - `BlockUpdate { wallet_id, height, inserted, updated, matured, balance }`. Carries records bucketed by what happened to them in the block, plus the post-block balance. - `SyncHeightUpdate { wallet_id, height }`. Marks a filter-batch checkpoint. `TransactionRecord` carries `account_type` directly, identifying the owning account. `WalletInfoInterface` gains a `matured_coinbase_records` method that enumerates coinbase records crossing the maturity threshold during a height advance, populating `BlockUpdate.matured`. The FFI groups the flattened account-discriminator fields into an `FFIAccountType` struct and renames the prior discriminant enum to `FFIAccountKind`. * fix: record balance before bumping `block_processed_wallet_count` Tests wait on `block_processed_wallet_count` and then read `last_confirmed`/`last_unconfirmed`. Bumping the counter before storing the balance snapshot left those reads racey. Reorder so the balance is recorded first. Addresses CodeRabbit review comment on PR #696 #696 (comment) * fix: place `IdentityTopUp.registration_index` in `index_secondary` The `FFIAccountType` doc states `index_secondary` carries `registration_index` for `IdentityTopUp` and `index = 0` for variants without a meaningful primary index, matching the parallel encoding in `FFIAccountKind::from_account_type`. The `From<&AccountType>` impl wrote `registration_index` into `index` instead, breaking the documented FFI contract. Addresses CodeRabbit review comment on PR #696 #696 (comment) * fix: route confirmation backfills to `new_records` `is_new` is wallet-wide (set on the first matching account, then breaks), so the per-account `else` branch can run for an account that did not previously hold the record. `confirm_transaction` backfills via `record_transaction` in that case, but the post-call record was always pushed onto `updated_records`, breaking the atomic `inserted`/`updated` contract consumed by `WalletEvent::BlockUpdate`. Capture `existed_before` per account and route to `new_records` when the record was just created. Addresses CodeRabbit review comment on PR #696 #696 (comment) * refactor(key-wallet-manager): extract `finalize_block_advance` helper `process_block` and `update_last_processed_height` duplicated the entire balance-snapshot, prior-heights collection, matured-coinbase window, height advance, and per-wallet `BlockUpdate` emission. Extract the shared tail into a private `WalletManager::finalize_block_advance` helper that takes the inserted/updated maps. `update_last_processed_height` becomes a one-line call with empty maps; `process_block` keeps only its txdata loop before delegating. * refactor: rename wallet events for clearer semantics Rename `WalletEvent` variants and the matching FFI callbacks to past-participle names that say what happened, replacing vague "Update" suffixes: - `TransactionReceived` -> `TransactionDetected`. "Received" implied incoming funds, but the event fires for any first-time off-chain sighting (incoming or outgoing). - `TransactionStatusChanged` -> `TransactionInstantLocked`. The event only ever fires for an InstantSend lock applied to a known mempool tx, so name it for what it actually is. Drop the `status: TransactionContext` field and carry the `InstantLock` directly. - `BlockUpdate` -> `BlockProcessed`. Mirrors `process_block` and matches the past-participle pattern. - `SyncHeightUpdate` -> `SyncHeightAdvanced`. Conveys monotonic forward motion. FFI rename mirrors the Rust side: the IS callback now takes `islock_data: *const u8` + `islock_len: usize` instead of an `FFITransactionContext`, removing a discriminant that was always `InstantSend`. The wallet-side `OnBlockProcessedCallback` becomes `OnWalletBlockProcessedCallback` to disambiguate from the existing sync-event type with the same name. * fix: record balance before bumping IS-locked counter in test callback Addresses CodeRabbit review comment on PR #696 #696 (review) The instant_locked callback bumped `transaction_instant_send_locked_count` before calling `record_balance`. Tests that wait on the counter and then read `last_confirmed`/`last_unconfirmed` could observe the previous balance snapshot. Match the ordering used by the other callbacks: store the balance first, then bump the counter. * fix: backfill missing transaction record in InstantSend path Addresses CodeRabbit review comment on PR #696 #696 (review) The IS-lock branch in `WalletTransactionChecker::check_core_transaction` only updated accounts that already held a `TransactionRecord` for the txid. When wallet-level `is_new` was `false` (because at least one account had the record) but another matched account did not, the latter was silently skipped: no record was created and `mark_utxos_instant_send` ran against an empty UTXO set on that account. Mirror the confirmation path: when the affected account lacks the record, call `record_transaction` to register the record and its UTXOs, then mark them IS-locked. This ordering ensures the freshly registered UTXOs receive the IS-lock flag too. The backfilled record is pushed into `new_records` to match the existing convention from commit 659a6d5. Add `test_instantsend_backfills_missing_record_in_other_account` covering the multi-account scenario. --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: QuantumExplorer <11468583+QuantumExplorer@users.noreply.github.com> Co-authored-by: Kevin Rombach <35775977+xdustinface@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: QuantumExplorer <quantum@dash.org> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: QuantumExplorer <11468583+QuantumExplorer@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
5 tasks
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.
Reshape
WalletEventso each variant carries the records or context needed to persist a wallet update atomically off a single event, alongside the post-change balance.The variant set is now:
TransactionReceived { wallet_id, record, balance }. Fires when the wallet first sees an off-chain transaction.TransactionStatusChanged { wallet_id, txid, context, balance }. Fires when a known off-chain transaction has its state change. Currently fires only for InstantSend locks.BlockUpdate { wallet_id, height, inserted, updated, matured, balance }. Carries records bucketed by what happened to them in the block, plus the post-block balance.SyncHeightUpdate { wallet_id, height }. Marks a filter-batch checkpoint.TransactionRecordcarriesaccount_typedirectly, identifying the owning account.WalletInfoInterfacegains amatured_coinbase_recordsmethod that enumerates coinbase records crossing the maturity threshold during a height advance, populatingBlockUpdate.matured.The FFI groups the flattened account-discriminator fields into an
FFIAccountTypestruct and renames the prior discriminant enum toFFIAccountKind.Based on:
Summary by CodeRabbit
Improvements
New Features
API
Tests