feat(key-wallet-manager): single-map WalletManager with Arc<RwLock<T>> per wallet#634
Open
feat(key-wallet-manager): single-map WalletManager with Arc<RwLock<T>> per wallet#634
Conversation
Atomic changeset types for wallet state mutations. Every mutation produces a WalletChangeSet capturing what changed. Composable via Merge trait for batching. Sub-changesets: ChainChangeSet, UtxoChangeSet, TransactionChangeSet, AccountChangeSet, BalanceChangeSet. This is about atomicity and consistency, not persistence. Persistence is a separate layer that consumes changesets. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Each mutation method continues to mutate in place (existing behavior preserved) but also returns a changeset capturing what changed. The changeset is built alongside the mutation, not reconstructed after. Methods modified: - record_transaction -> (TransactionRecord, WalletChangeSet) - confirm_transaction -> (bool, WalletChangeSet) - mark_utxos_instant_send -> (bool, UtxoChangeSet) - mark_address_used -> (bool, AccountChangeSet) - maintain_gap_limit -> (Vec<Address>, Option<u32>) - update_balance -> BalanceChangeSet - check_core_transaction aggregates sub-changesets via Merge TransactionCheckResult gains a `changeset: WalletChangeSet` field that is merged from all sub-operations during transaction processing. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
mark_instant_send_utxos was discarding the UtxoChangeSet produced by each account's mark_utxos_instant_send call, losing the IS-lock deltas needed for changeset-based persistence. Change the trait signature and its ManagedWalletInfo implementation to return (bool, UtxoChangeSet) so callers can merge the delta into their persisted changesets. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ication Implements ManagedWalletInfo::apply(&mut self, changeset: &WalletChangeSet) which applies each sub-changeset to update wallet state in place: - ChainChangeSet: updates synced_height metadata - UtxoChangeSet: adds/removes/IS-locks UTXOs in owning accounts - TransactionChangeSet: inserts transaction records into owning accounts - AccountChangeSet: advances revealed address indices, marks addresses used - BalanceChangeSet: applies signed deltas to cached wallet balance Also adds WalletCoreBalance::apply_delta() for safe signed-delta application with saturating arithmetic. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Refactor all ManagedCoreAccount mutation methods to separate read-only computation from state mutation: - compute_mark_address_used(&self) -> AccountChangeSet - compute_utxo_changes(&self) -> UtxoChangeSet - compute_instant_send_lock(&self) -> UtxoChangeSet - compute_balance_update(&self) -> BalanceChangeSet - compute_record_transaction(&self) -> (TransactionRecord, WalletChangeSet) - compute_confirm_transaction(&self) -> (bool, WalletChangeSet) Extract build_transaction_record as a shared read-only helper. Add apply_utxo_changes for applying UTXO changesets to mutable state. Refactor check_core_transaction in wallet_checker.rs to use an explicit two-phase approach: Phase 1 computes all changesets without &mut self, Phase 2 applies them. This ensures no &mut self between compute and apply. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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. |
Contributor
📝 WalkthroughWalkthroughRefactors wallet APIs to async, replaces dual-map wallet storage with per-wallet Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
key-wallet/src/managed_account/address_pool.rs (1)
883-890:⚠️ Potential issue | 🟠 MajorGuard zero gap limits before subtracting 1.
Line 888 still underflows when
gap_limit == 0, which is constructible today. In debug builds that panics; in release builds it wraps and can turn gap-limit maintenance into an effectively unbounded address-generation loop. At minimum, reject zero here, and ideally validate it when constructing the pool.🛡️ Suggested guard
pub fn maintain_gap_limit( &mut self, key_source: &KeySource, ) -> Result<(Vec<Address>, Option<u32>)> { + if self.gap_limit == 0 { + return Err(Error::InvalidParameter("gap_limit must be greater than 0".into())); + } + let target = match self.highest_used { None => self.gap_limit - 1, Some(highest) => highest + self.gap_limit, };As per coding guidelines, "Never panic in library code; always use Result for fallible operations" and "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/src/managed_account/address_pool.rs` around lines 883 - 890, The maintain_gap_limit method can underflow when self.gap_limit == 0; add a guard returning an Err<Result::Err> when gap_limit is zero (check at start of maintain_gap_limit) instead of computing self.gap_limit - 1, and also add validation in the pool constructor (e.g. AddressPool::new or the struct builder) to reject or normalize a zero gap_limit at creation time so the invariant gap_limit > 0 is guaranteed everywhere; reference maintain_gap_limit, self.gap_limit, and highest_used when implementing these checks and returning a meaningful error variant.
🧹 Nitpick comments (10)
dash-spv/src/sync/mempool/manager.rs (1)
137-140: Consider a single wallet snapshot call for bloom-filter inputs.
monitored_addresses().awaitandwatched_outpoints().awaitnow each walk wallet state while the outerself.walletread guard is held. On rebuilds that doubles the per-wallet locking work and keeps write-side wallet operations blocked longer than necessary. A combined API that returns both collections in one pass would reduce that contention.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash-spv/src/sync/mempool/manager.rs` around lines 137 - 140, The two separate calls wallet.monitored_addresses().await and wallet.watched_outpoints().await each iterate wallet state under the same self.wallet read lock; implement a single wallet API (e.g., Wallet::snapshot_bloom_inputs or Wallet::bloom_inputs) that returns both monitored addresses and watched outpoints in one pass, update mempool manager to call that single method instead of monitored_addresses() and watched_outpoints(), and then drop the read guard; this reduces double traversal and shortens the duration of the self.wallet read lock in Manager::... where those calls occur.key-wallet/src/lib.rs (1)
29-29: Prefer narrow re-exports over a whole new public module.
pub mod changeset;makes the entirechangesettree part ofkey-wallet's public API. If only a small subset needs to cross crate boundaries, explicitpub uses keep the semver surface smaller. It also makes the currentfeat(key-wallet-manager)title/scope a better match for what this PR actually ships.As per coding guidelines, "Check whether the PR title prefix allowed in the pr-title.yml workflow accurately describes the changes" and "If a PR mixes unrelated concerns ... suggest splitting it into separate focused PRs."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet/src/lib.rs` at line 29, Replace the broad public re-export "pub mod changeset;" with a private module declaration ("mod changeset;") and then explicitly re-export only the specific items that must be part of key-wallet's public API by adding targeted "pub use" statements for those symbols from the changeset module (e.g., the specific structs, enums, traits or functions you intend to expose). Update references in the crate to import those items via their re-exports and remove any unintended public exposure of the entire changeset tree so the public surface remains minimal.key-wallet/src/managed_account/managed_platform_account.rs (1)
293-296: Don't droplast_revealedat this boundary without checking downstream needs.The lower-level pool API now computes a reveal marker, but this adapter immediately discards it. If Platform callers need that index for persistence or address publication, the information is lost before it reaches them. Please verify the pending Platform integration can safely ignore it; otherwise this method should propagate the tuple too.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet/src/managed_account/managed_platform_account.rs` around lines 293 - 296, The call to self.addresses.maintain_gap_limit(...) currently discards the returned last_revealed value; verify whether Platform callers need that reveal index and if so change this adapter to propagate the full (addresses, last_revealed) tuple instead of mapping to just addrs. Concretely, update the method signature that wraps this call to return the tuple (or Option-wrapped tuple) matching self.addresses.maintain_gap_limit, stop the .map(|(addrs, _last_revealed)| addrs) transformation, and update all callers to accept and persist or publish last_revealed; if you confirm callers do not need it, add an explicit comment explaining it’s intentionally discarded. Ensure error mapping still wraps failures as Error::InvalidParameter as before.key-wallet-manager/src/test_utils/mock_wallet.rs (1)
179-183: Consider using.awaitinstead oftry_lock()for consistency.The
process_instant_send_lockmethod usestry_lock().expect()while other methods in this impl use.lock().await. This inconsistency could cause unexpected panics if this test helper is used in concurrent test scenarios.♻️ Suggested fix
async fn process_instant_send_lock(&mut self, txid: Txid) { - let mut changes = - self.status_changes.try_lock().expect("status_changes lock contention in test helper"); + let mut changes = self.status_changes.lock().await; changes.push((txid, TransactionContext::InstantSend)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-manager/src/test_utils/mock_wallet.rs` around lines 179 - 183, The method process_instant_send_lock uses try_lock().expect which can panic under contention; replace that with the async mutex await pattern used elsewhere: await the lock via self.status_changes.lock().await (e.g., let mut changes = self.status_changes.lock().await) and then push the (txid, TransactionContext::InstantSend) into changes, removing the try_lock().expect usage to avoid unexpected panics in concurrent tests.key-wallet-manager/examples/wallet_creation.rs (1)
148-152: Simplify trait method calls.The fully-qualified
WalletInterface::synced_height(&manager)syntax is verbose. SinceWalletInterfaceis already imported, you can use method syntax directly.♻️ Suggested simplification
- println!(" Current height (Testnet): {:?}", WalletInterface::synced_height(&manager)); + println!(" Current height (Testnet): {:?}", manager.synced_height()); // Update height - WalletInterface::update_synced_height(&mut manager, 850_000).await; - println!(" Updated height to: {:?}", WalletInterface::synced_height(&manager)); + manager.update_synced_height(850_000).await; + println!(" Updated height to: {:?}", manager.synced_height());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-manager/examples/wallet_creation.rs` around lines 148 - 152, Replace the fully-qualified trait calls with direct method-call syntax on the manager: call manager.synced_height() instead of WalletInterface::synced_height(&manager), and call manager.update_synced_height(850_000).await instead of WalletInterface::update_synced_height(&mut manager, 850_000).await so the trait methods (synced_height and update_synced_height) are invoked via the manager instance.key-wallet/src/wallet/balance.rs (1)
65-70: Consider adding unit tests forapply_delta.The new
apply_deltamethod handles edge cases (negative deltas, underflow clamping) but lacks test coverage. Given the existingtest_balance_overflowtest demonstrates overflow behavior matters for this type, tests forapply_deltawould be valuable.💡 Suggested test cases
#[test] fn test_apply_delta_positive() { let mut balance = WalletCoreBalance::new(1000, 500, 100, 200); let delta = BalanceChangeSet { spendable_delta: 100, unconfirmed_delta: 50, immature_delta: 25, locked_delta: 10, }; balance.apply_delta(&delta); assert_eq!(balance.spendable(), 1100); assert_eq!(balance.unconfirmed(), 550); } #[test] fn test_apply_delta_negative_clamps_at_zero() { let mut balance = WalletCoreBalance::new(100, 0, 0, 0); let delta = BalanceChangeSet { spendable_delta: -200, // More than available unconfirmed_delta: 0, immature_delta: 0, locked_delta: 0, }; balance.apply_delta(&delta); assert_eq!(balance.spendable(), 0); // Clamped, not underflowed }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet/src/wallet/balance.rs` around lines 65 - 70, The new apply_delta method (in WalletCoreBalance) updates fields using apply_signed_delta but has no unit tests; add tests covering positive deltas and negative deltas that clamp at zero: create WalletCoreBalance instances, construct BalanceChangeSet values using spendable_delta, unconfirmed_delta, immature_delta, and locked_delta, call apply_delta(&delta) and assert the resulting getters (e.g., spendable(), unconfirmed(), immature(), locked()) match expected values (e.g., increased totals for positive deltas and zero for over-negative deltas) to verify apply_signed_delta behavior and underflow clamping.key-wallet-manager/src/event_tests.rs (1)
277-298: Consider usingread().awaitinstead oftry_read()for consistency.Lines 285 and 294 use
try_read().unwrap()which will panic if the lock is currently held. While this works in single-threaded test context, usingread().awaitwould be more consistent with the async patterns used elsewhere in the test suite.♻️ Suggested change for consistency
let balance_before = { let arc = manager.get_wallet_arc(&wallet_id).unwrap(); - let guard = arc.try_read().unwrap(); + let guard = arc.read().await; guard.balance() }; // ... let balance_after = { let arc = manager.get_wallet_arc(&wallet_id).unwrap(); - let guard = arc.try_read().unwrap(); + let guard = arc.read().await; guard.balance() };🤖 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 277 - 298, In test_process_instant_send_lock_for_unknown_txid replace the synchronous arc.try_read().unwrap() calls used to read the wallet state with the async .read().await on the RwLock returned by get_wallet_arc(&wallet_id), e.g. await the read lock, then call guard.balance() (no unwrap), so change both occurrences around balance_before and balance_after to use .read().await to avoid panics and follow async patterns used elsewhere (keep WalletInterface::process_instant_send_lock call unchanged).key-wallet/src/managed_account/mod.rs (2)
490-495:debug_assert_eq!may panic in debug builds.While
debug_assert!is acceptable for catching programming errors during development, this assertion could panic if a transaction is somehow re-processed with a differenttransaction_type. Consider either:
- Using a soft assertion that logs a warning instead, or
- Ensuring this is truly an invariant that can never be violated.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet/src/managed_account/mod.rs` around lines 490 - 495, The debug_assert_eq! on tx_record.transaction_type vs transaction_type (logged with tx.txid()) can panic in debug builds; replace it with a non-panicking check: verify the invariant inside the function that updates/records transactions (the code around tx_record.transaction_type and transaction_type) and if it can be violated, log a warning/error including tx.txid() and both values (or handle reconciliation) instead of using debug_assert_eq!; if it truly must never differ, add a comment documenting the invariant and keep a non-panicking fallback that logs the unexpected state and returns an Err or otherwise prevents inconsistent processing.
452-456: Unbounded growth ofspent_outpointsset.All transaction inputs are added to
spent_outpoints, even those not belonging to our wallet. Over time, this set can grow significantly, especially for wallets processing many transactions. The set is also#[serde(skip_serializing)], so it's rebuilt from transactions on deserialization, but during runtime it accumulates all seen inputs.Consider periodically pruning
spent_outpointsfor outpoints from transactions that are deeply confirmed (e.g., 100+ confirmations), as they're unlikely to be relevant for out-of-order processing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet/src/managed_account/mod.rs` around lines 452 - 456, The spent_outpoints set (self.spent_outpoints) is unbounded because every input.previous_output from every processed tx is inserted; add a pruning mechanism to remove stale outpoints for deeply confirmed transactions (e.g., >100 confirmations) to prevent memory growth. Implement a method like prune_spent_outpoints(threshold_confirmations: u32) that iterates over spent_outpoints, looks up the corresponding transaction confirmation height or confirmation count from your wallet's tx store (the same place used to rebuild spent_outpoints after deserialization), and removes outpoints whose transactions exceed the threshold; call this prune method periodically (or after inserting a block/transaction) in the transaction insertion/processing path (where the loop over tx.input currently inserts into self.spent_outpoints) so runtime growth is bounded while preserving out-of-order processing semantics.key-wallet-manager/src/accessors.rs (1)
109-118: Silent lock contention may return incomplete results.Using
try_read()means wallets with contended locks are silently skipped. While this is documented and prevents blocking, callers may not realize they're getting incomplete data.Consider either:
- Returning a tuple
(Vec<Utxo>, usize)where the second value indicates how many wallets were skipped, or- Making this method async like
wallet_utxosfor consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-manager/src/accessors.rs` around lines 109 - 118, The current get_all_utxos method silently skips wallets when arc.try_read() fails, leading to incomplete results; update get_all_utxos to either (A) return (Vec<Utxo>, usize) where the usize counts how many wallets were skipped (increment a skipped counter whenever try_read() is Err and continue collecting Utxo from successful reads via state.utxos()), or (B) convert get_all_utxos to an async version matching wallet_utxos so it can await a read lock (replace try_read() usage with an awaited read handle consistent with your async locking pattern); reference get_all_utxos, wallets, try_read, Utxo, and wallet_utxos to locate and apply the chosen change.
🤖 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-manager/src/lib.rs`:
- Around line 531-548: The create_account method currently only calls
state.wallet_mut().add_account(...) so ManagedWalletState's managed accounts are
never initialized; update create_account (the function create_account) to also
initialize or sync the managed account collection on the ManagedWalletState
after adding the account (e.g., call the state method that registers/initializes
managed accounts or push the new account into the state's managed accounts
structure), ensuring you hold the write lock on state while mutating both wallet
and managed state, propagate any errors as WalletError::AccountCreation, and
then call bump_structural_revision() as before so the Wallet and
ManagedWalletState remain in sync and address generation/matching works.
- Around line 355-358: The code currently sets the wallet birth height to
self.synced_height unconditionally (T::from_wallet + state.set_birth_height +
state.set_first_loaded_at), which assumes imported wallets started at tip;
instead detect imported wallets (e.g., wallet.imported or wallet.is_imported())
and, for imports, use an explicit rescan/start height from the wallet metadata
(e.g., wallet.first_key_birthday or wallet.import_height or 0 if unknown) when
calling state.set_birth_height; otherwise keep using self.synced_height; apply
the same conditional fix to the other occurrences around the T::from_wallet
blocks at the noted spots (lines ~402-405 and ~447-449).
In `@key-wallet-manager/src/managed_wallet_state.rs`:
- Around line 249-253: The code currently logs and continues when
self.persister.store(result.changeset.clone()) fails, leaving in-memory state
mutated without durable persistence; change this to propagate the error instead
of swallowing it: update the enclosing function (where result and
self.persister.store are used) to return a Result, and replace the if-let Err
branch with propagation using ? or map_err to convert the persister error into
your crate's custom Error variant (e.g., PersistError or
WalletPersistenceFailed) with contextual text like "failed to persist wallet
changeset" so callers receive a clear failure signal when
self.persister.store(result.changeset.clone()) fails.
- Around line 215-217: The mark_instant_send_utxos(txid) call returns a
UtxoChangeSet that is currently discarded in process_instant_send_lock(), so the
InstantSend lock mutation never gets persisted to WalletPersistence; capture the
returned (bool, UtxoChangeSet) from mark_instant_send_utxos in
process_instant_send_lock and forward the UtxoChangeSet to the persistence layer
(e.g., call the existing WalletPersistence method that persists UTXO changes) or
update mark_instant_send_utxos to persist internally; ensure you reference the
returned UtxoChangeSet from mark_instant_send_utxos (and the boolean result)
rather than dropping it so the change is recorded across restarts.
In `@key-wallet-manager/src/process_block.rs`:
- Around line 104-111: The update_synced_height loop currently calls
state.update_synced_height and state.update_balance but does not produce the
balance delta or notify subscribers; modify update_synced_height to mirror the
block/mempool paths by taking a pre-update balance snapshot for each wallet (use
the existing snapshot method used elsewhere), then call
state.update_synced_height(height) and state.update_balance(), compute the delta
from the snapshot, and call the same notification/report function used by the
block and mempool handlers to publish the balance change; reference the
update_synced_height and update_balance calls and the wallets map to locate
where to insert the snapshot/delta/notify logic.
In `@key-wallet/src/changeset/changeset.rs`:
- Around line 66-74: The merge implementation for ChainChangeSet updates height
and block_hash independently, which can leave an inconsistent tip; change the
impl Merge for ChainChangeSet::merge to update the tip atomically by only
assigning both self.height and self.block_hash together when the incoming other
has both values present (check other.height.is_some() &&
other.block_hash.is_some()), and do not individually overwrite one without the
other; perform a single assignment of both fields from other to ensure
atomicity.
In `@key-wallet/src/managed_account/address_pool.rs`:
- Around line 1231-1233: The tests call pool.maintain_gap_limit(&key_source) and
only assert new_addresses.len() and pool.highest_generated; update those
assertions to also check the returned last_revealed value from
maintain_gap_limit (rename the tuple binding from (_last_revealed) to
last_revealed) and assert the expected variants for each case:
assert_eq!(last_revealed, None) in the first scenario, assert_eq!(last_revealed,
Some(gap_limit)) in the second, and assert_eq!(last_revealed, Some(gap_limit +
2)) in the third; ensure you update all three similar blocks (currently around
the calls at the three locations) to validate the new API surface.
In `@key-wallet/src/wallet/managed_wallet_info/mod.rs`:
- Around line 350-363: The UTXO is being created with is_coinbase hardcoded to
false in Utxo::new (inside managed_wallet_info::mod), which breaks coinbase
maturity; update the changeset handling to obtain the correct coinbase flag:
extend the changeset's UtxoEntry to carry an is_coinbase boolean (or, if the
full transaction/txin data is available, detect coinbase from the transaction)
and pass that value into Utxo::new instead of false, then ensure the code that
constructs UtxoEntry and the consumer that applies the changeset (the block that
sets utxo.is_instantlocked and inserts into account.utxos) use the new
is_coinbase field.
- Around line 470-486: The changeet last_revealed currently maps account_index
-> new_index but the loop advances highest_generated on every pool returned by
account.account_type.address_pools_mut() (affecting both external and
internal/change pools); update the logic so you only advance the specific pool
that was revealed—either by changing last_revealed to carry pool identity (e.g.,
map (account_index, pool_type) -> new_index) or by keeping per-pool reveal
indices and applying new_index only to the matching pool.highest_generated;
modify the block that iterates acct_cs.last_revealed and the use of
address_pools_mut() so you select the correct pool before updating
pool.highest_generated.
- Around line 451-462: The current fallback that inserts a transaction into the
first standard_bip44_accounts entry when no account matched (the block using
inserted, self.accounts.standard_bip44_accounts.values_mut().next(),
account.transactions.insert(*txid, record)) can misattribute transactions;
change this so that when inserted is false you DO NOT insert into an arbitrary
account and instead either skip insertion or emit a warning. Locate the fallback
that checks the inserted flag and replace the forced insert with a warning/log
call that includes the txid and a brief context (e.g., "no matching account for
transaction"), or simply skip insertion; ensure you reference the same symbols
(inserted, txid, record, self.accounts.standard_bip44_accounts,
account.transactions.insert) so reviewers can find the modified logic.
---
Outside diff comments:
In `@key-wallet/src/managed_account/address_pool.rs`:
- Around line 883-890: The maintain_gap_limit method can underflow when
self.gap_limit == 0; add a guard returning an Err<Result::Err> when gap_limit is
zero (check at start of maintain_gap_limit) instead of computing self.gap_limit
- 1, and also add validation in the pool constructor (e.g. AddressPool::new or
the struct builder) to reject or normalize a zero gap_limit at creation time so
the invariant gap_limit > 0 is guaranteed everywhere; reference
maintain_gap_limit, self.gap_limit, and highest_used when implementing these
checks and returning a meaningful error variant.
---
Nitpick comments:
In `@dash-spv/src/sync/mempool/manager.rs`:
- Around line 137-140: The two separate calls wallet.monitored_addresses().await
and wallet.watched_outpoints().await each iterate wallet state under the same
self.wallet read lock; implement a single wallet API (e.g.,
Wallet::snapshot_bloom_inputs or Wallet::bloom_inputs) that returns both
monitored addresses and watched outpoints in one pass, update mempool manager to
call that single method instead of monitored_addresses() and
watched_outpoints(), and then drop the read guard; this reduces double traversal
and shortens the duration of the self.wallet read lock in Manager::... where
those calls occur.
In `@key-wallet-manager/examples/wallet_creation.rs`:
- Around line 148-152: Replace the fully-qualified trait calls with direct
method-call syntax on the manager: call manager.synced_height() instead of
WalletInterface::synced_height(&manager), and call
manager.update_synced_height(850_000).await instead of
WalletInterface::update_synced_height(&mut manager, 850_000).await so the trait
methods (synced_height and update_synced_height) are invoked via the manager
instance.
In `@key-wallet-manager/src/accessors.rs`:
- Around line 109-118: The current get_all_utxos method silently skips wallets
when arc.try_read() fails, leading to incomplete results; update get_all_utxos
to either (A) return (Vec<Utxo>, usize) where the usize counts how many wallets
were skipped (increment a skipped counter whenever try_read() is Err and
continue collecting Utxo from successful reads via state.utxos()), or (B)
convert get_all_utxos to an async version matching wallet_utxos so it can await
a read lock (replace try_read() usage with an awaited read handle consistent
with your async locking pattern); reference get_all_utxos, wallets, try_read,
Utxo, and wallet_utxos to locate and apply the chosen change.
In `@key-wallet-manager/src/event_tests.rs`:
- Around line 277-298: In test_process_instant_send_lock_for_unknown_txid
replace the synchronous arc.try_read().unwrap() calls used to read the wallet
state with the async .read().await on the RwLock returned by
get_wallet_arc(&wallet_id), e.g. await the read lock, then call guard.balance()
(no unwrap), so change both occurrences around balance_before and balance_after
to use .read().await to avoid panics and follow async patterns used elsewhere
(keep WalletInterface::process_instant_send_lock call unchanged).
In `@key-wallet-manager/src/test_utils/mock_wallet.rs`:
- Around line 179-183: The method process_instant_send_lock uses
try_lock().expect which can panic under contention; replace that with the async
mutex await pattern used elsewhere: await the lock via
self.status_changes.lock().await (e.g., let mut changes =
self.status_changes.lock().await) and then push the (txid,
TransactionContext::InstantSend) into changes, removing the try_lock().expect
usage to avoid unexpected panics in concurrent tests.
In `@key-wallet/src/lib.rs`:
- Line 29: Replace the broad public re-export "pub mod changeset;" with a
private module declaration ("mod changeset;") and then explicitly re-export only
the specific items that must be part of key-wallet's public API by adding
targeted "pub use" statements for those symbols from the changeset module (e.g.,
the specific structs, enums, traits or functions you intend to expose). Update
references in the crate to import those items via their re-exports and remove
any unintended public exposure of the entire changeset tree so the public
surface remains minimal.
In `@key-wallet/src/managed_account/managed_platform_account.rs`:
- Around line 293-296: The call to self.addresses.maintain_gap_limit(...)
currently discards the returned last_revealed value; verify whether Platform
callers need that reveal index and if so change this adapter to propagate the
full (addresses, last_revealed) tuple instead of mapping to just addrs.
Concretely, update the method signature that wraps this call to return the tuple
(or Option-wrapped tuple) matching self.addresses.maintain_gap_limit, stop the
.map(|(addrs, _last_revealed)| addrs) transformation, and update all callers to
accept and persist or publish last_revealed; if you confirm callers do not need
it, add an explicit comment explaining it’s intentionally discarded. Ensure
error mapping still wraps failures as Error::InvalidParameter as before.
In `@key-wallet/src/managed_account/mod.rs`:
- Around line 490-495: The debug_assert_eq! on tx_record.transaction_type vs
transaction_type (logged with tx.txid()) can panic in debug builds; replace it
with a non-panicking check: verify the invariant inside the function that
updates/records transactions (the code around tx_record.transaction_type and
transaction_type) and if it can be violated, log a warning/error including
tx.txid() and both values (or handle reconciliation) instead of using
debug_assert_eq!; if it truly must never differ, add a comment documenting the
invariant and keep a non-panicking fallback that logs the unexpected state and
returns an Err or otherwise prevents inconsistent processing.
- Around line 452-456: The spent_outpoints set (self.spent_outpoints) is
unbounded because every input.previous_output from every processed tx is
inserted; add a pruning mechanism to remove stale outpoints for deeply confirmed
transactions (e.g., >100 confirmations) to prevent memory growth. Implement a
method like prune_spent_outpoints(threshold_confirmations: u32) that iterates
over spent_outpoints, looks up the corresponding transaction confirmation height
or confirmation count from your wallet's tx store (the same place used to
rebuild spent_outpoints after deserialization), and removes outpoints whose
transactions exceed the threshold; call this prune method periodically (or after
inserting a block/transaction) in the transaction insertion/processing path
(where the loop over tx.input currently inserts into self.spent_outpoints) so
runtime growth is bounded while preserving out-of-order processing semantics.
In `@key-wallet/src/wallet/balance.rs`:
- Around line 65-70: The new apply_delta method (in WalletCoreBalance) updates
fields using apply_signed_delta but has no unit tests; add tests covering
positive deltas and negative deltas that clamp at zero: create WalletCoreBalance
instances, construct BalanceChangeSet values using spendable_delta,
unconfirmed_delta, immature_delta, and locked_delta, call apply_delta(&delta)
and assert the resulting getters (e.g., spendable(), unconfirmed(), immature(),
locked()) match expected values (e.g., increased totals for positive deltas and
zero for over-negative deltas) to verify apply_signed_delta behavior and
underflow clamping.
🪄 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: a9be7ed5-85b0-475f-b2c3-afe7c7fcdbe4
📒 Files selected for processing (47)
dash-spv/src/client/lifecycle.rsdash-spv/src/sync/filters/manager.rsdash-spv/src/sync/mempool/manager.rsdash-spv/src/sync/mempool/sync_manager.rskey-wallet-ffi/src/address_pool.rskey-wallet-ffi/src/wallet_manager_tests.rskey-wallet-manager/Cargo.tomlkey-wallet-manager/examples/wallet_creation.rskey-wallet-manager/src/accessors.rskey-wallet-manager/src/event_tests.rskey-wallet-manager/src/lib.rskey-wallet-manager/src/managed_wallet_state.rskey-wallet-manager/src/persistence.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-manager/tests/integration_test.rskey-wallet-manager/tests/spv_integration_tests.rskey-wallet-manager/tests/test_serialized_wallets.rskey-wallet/ACCOUNT_TYPE_REFACTOR.mdkey-wallet/BDK_ARCHITECTURE_COMPARISON.mdkey-wallet/KEY_DERIVATION_ISSUES.mdkey-wallet/PSBT_MIGRATION.mdkey-wallet/REORG_SAFETY.mdkey-wallet/src/changeset/changeset.rskey-wallet/src/changeset/merge.rskey-wallet/src/changeset/mod.rskey-wallet/src/lib.rskey-wallet/src/managed_account/address_pool.rskey-wallet/src/managed_account/managed_platform_account.rskey-wallet/src/managed_account/mod.rskey-wallet/src/test_utils/wallet.rskey-wallet/src/tests/balance_tests.rskey-wallet/src/tests/performance_tests.rskey-wallet/src/transaction_checking/account_checker.rskey-wallet/src/transaction_checking/transaction_router/tests/asset_unlock.rskey-wallet/src/transaction_checking/transaction_router/tests/coinbase.rskey-wallet/src/transaction_checking/transaction_router/tests/identity_transactions.rskey-wallet/src/transaction_checking/transaction_router/tests/provider.rskey-wallet/src/transaction_checking/transaction_router/tests/routing.rskey-wallet/src/transaction_checking/wallet_checker.rskey-wallet/src/wallet/balance.rskey-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rskey-wallet/src/wallet/managed_wallet_info/mod.rskey-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rskey-wallet/src/wallet/mod.rs
💤 Files with no reviewable changes (4)
- key-wallet/src/wallet/mod.rs
- key-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rs
- key-wallet/src/tests/balance_tests.rs
- key-wallet-manager/tests/spv_integration_tests.rs
Comment on lines
+355
to
+358
| // Create combined wallet state | ||
| let mut state = T::from_wallet(&wallet); | ||
| state.set_birth_height(self.synced_height); | ||
| state.set_first_loaded_at(current_timestamp()); |
Contributor
There was a problem hiding this comment.
Imported wallets need an explicit rescan starting point.
Using self.synced_height here assumes the imported wallet first existed at the current tip. Any earlier history is then skipped during scanning for xprv/xpub/bytes imports.
Also applies to: 402-405, 447-449
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@key-wallet-manager/src/lib.rs` around lines 355 - 358, The code currently
sets the wallet birth height to self.synced_height unconditionally
(T::from_wallet + state.set_birth_height + state.set_first_loaded_at), which
assumes imported wallets started at tip; instead detect imported wallets (e.g.,
wallet.imported or wallet.is_imported()) and, for imports, use an explicit
rescan/start height from the wallet metadata (e.g., wallet.first_key_birthday or
wallet.import_height or 0 if unknown) when calling state.set_birth_height;
otherwise keep using self.synced_height; apply the same conditional fix to the
other occurrences around the T::from_wallet blocks at the noted spots (lines
~402-405 and ~447-449).
Comment on lines
+215
to
+217
| fn mark_instant_send_utxos(&mut self, txid: &Txid) -> (bool, UtxoChangeSet) { | ||
| self.wallet_info.mark_instant_send_utxos(txid) | ||
| } |
Contributor
There was a problem hiding this comment.
Persist the dedicated InstantSend-lock path.
process_instant_send_lock() uses this method and drops the returned _changeset, so this mutation never reaches WalletPersistence. An IS lock applied after the original transaction processing will be lost on restart.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@key-wallet-manager/src/managed_wallet_state.rs` around lines 215 - 217, The
mark_instant_send_utxos(txid) call returns a UtxoChangeSet that is currently
discarded in process_instant_send_lock(), so the InstantSend lock mutation never
gets persisted to WalletPersistence; capture the returned (bool, UtxoChangeSet)
from mark_instant_send_utxos in process_instant_send_lock and forward the
UtxoChangeSet to the persistence layer (e.g., call the existing
WalletPersistence method that persists UTXO changes) or update
mark_instant_send_utxos to persist internally; ensure you reference the returned
UtxoChangeSet from mark_instant_send_utxos (and the boolean result) rather than
dropping it so the change is recorded across restarts.
Comment on lines
+249
to
+253
| // Persist non-empty changesets | ||
| if !result.changeset.is_empty() { | ||
| if let Err(e) = self.persister.store(result.changeset.clone()) { | ||
| tracing::warn!("Failed to persist wallet changeset: {}", e); | ||
| } |
Contributor
There was a problem hiding this comment.
Don’t continue after a failed store().
By this point the in-memory wallet state has already been mutated. Logging and returning success leaves durable state behind with no recovery signal to the caller.
As per coding guidelines, key-wallet/**/*.rs: "Never panic in library code; always use Result for fallible operations and use the ? operator for error propagation" and "Provide context in error messages and use custom Error type with specific variants for better error handling."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@key-wallet-manager/src/managed_wallet_state.rs` around lines 249 - 253, The
code currently logs and continues when
self.persister.store(result.changeset.clone()) fails, leaving in-memory state
mutated without durable persistence; change this to propagate the error instead
of swallowing it: update the enclosing function (where result and
self.persister.store are used) to return a Result, and replace the if-let Err
branch with propagation using ? or map_err to convert the persister error into
your crate's custom Error variant (e.g., PersistError or
WalletPersistenceFailed) with contextual text like "failed to persist wallet
changeset" so callers receive a clear failure signal when
self.persister.store(result.changeset.clone()) fails.
Comment on lines
+66
to
+74
| impl Merge for ChainChangeSet { | ||
| fn merge(&mut self, other: Self) { | ||
| if other.height.is_some() { | ||
| self.height = other.height; | ||
| } | ||
| if other.block_hash.is_some() { | ||
| self.block_hash = other.block_hash; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Merge the chain tip atomically.
height and block_hash describe the same tip, but this implementation overwrites them independently. A height-only delta can leave an older hash attached to a newer height, which corrupts the persisted tip.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@key-wallet/src/changeset/changeset.rs` around lines 66 - 74, The merge
implementation for ChainChangeSet updates height and block_hash independently,
which can leave an inconsistent tip; change the impl Merge for
ChainChangeSet::merge to update the tip atomically by only assigning both
self.height and self.block_hash together when the incoming other has both values
present (check other.height.is_some() && other.block_hash.is_some()), and do not
individually overwrite one without the other; perform a single assignment of
both fields from other to ensure atomicity.
Comment on lines
+1231
to
1233
| let (new_addresses, _last_revealed) = pool.maintain_gap_limit(&key_source).unwrap(); | ||
| assert_eq!(new_addresses.len(), 0); | ||
| assert_eq!(pool.highest_generated, Some(gap_limit - 1)); |
Contributor
There was a problem hiding this comment.
Please assert the new last_revealed return value in these tests.
These cases still only check new_addresses.len(), so the newly added part of the API can regress unnoticed. It would be useful to assert None, Some(gap_limit), and Some(gap_limit + 2) here.
🧪 Suggested assertions
- let (new_addresses, _last_revealed) = pool.maintain_gap_limit(&key_source).unwrap();
+ let (new_addresses, last_revealed) = pool.maintain_gap_limit(&key_source).unwrap();
assert_eq!(new_addresses.len(), 0);
+ assert_eq!(last_revealed, None);
assert_eq!(pool.highest_generated, Some(gap_limit - 1));
assert_eq!(pool.addresses.len(), gap_limit as usize);
@@
- let (new_addresses, _last_revealed) = pool.maintain_gap_limit(&key_source).unwrap();
+ let (new_addresses, last_revealed) = pool.maintain_gap_limit(&key_source).unwrap();
assert_eq!(new_addresses.len(), 1);
+ assert_eq!(last_revealed, Some(gap_limit));
assert_eq!(pool.highest_generated, Some(gap_limit));
assert_eq!(pool.addresses.len(), gap_limit as usize + 1);
@@
- let (new_addresses, _last_revealed) = pool.maintain_gap_limit(&key_source).unwrap();
+ let (new_addresses, last_revealed) = pool.maintain_gap_limit(&key_source).unwrap();
assert_eq!(new_addresses.len(), 2);
+ assert_eq!(last_revealed, Some(gap_limit + 2));
assert_eq!(pool.highest_generated, Some(gap_limit + 2));
assert_eq!(pool.addresses.len(), gap_limit as usize + 3);As per coding guidelines, "Write unit tests for new functionality".
Also applies to: 1241-1243, 1251-1253
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@key-wallet/src/managed_account/address_pool.rs` around lines 1231 - 1233, The
tests call pool.maintain_gap_limit(&key_source) and only assert
new_addresses.len() and pool.highest_generated; update those assertions to also
check the returned last_revealed value from maintain_gap_limit (rename the tuple
binding from (_last_revealed) to last_revealed) and assert the expected variants
for each case: assert_eq!(last_revealed, None) in the first scenario,
assert_eq!(last_revealed, Some(gap_limit)) in the second, and
assert_eq!(last_revealed, Some(gap_limit + 2)) in the third; ensure you update
all three similar blocks (currently around the calls at the three locations) to
validate the new API surface.
Comment on lines
+350
to
+363
| let utxo = Utxo::new( | ||
| *outpoint, | ||
| TxOut { | ||
| value: entry.value, | ||
| script_pubkey: entry.script_pubkey.clone(), | ||
| }, | ||
| entry.address.clone(), | ||
| self.metadata.synced_height, | ||
| false, // not coinbase – changeset doesn't carry this info | ||
| ); | ||
| // Propagate the IS-lock flag from the entry. | ||
| let mut utxo = utxo; | ||
| utxo.is_instantlocked = entry.is_instant_locked; | ||
| account.utxos.insert(*outpoint, utxo); |
Contributor
There was a problem hiding this comment.
Coinbase flag is always false, which may cause incorrect maturity calculations.
When adding UTXOs from the changeset, is_coinbase is hardcoded to false because the changeset doesn't carry this information. This will cause coinbase UTXOs to be treated as immediately spendable instead of requiring 100 confirmations for maturity.
Consider extending UtxoEntry in the changeset to include the is_coinbase flag, or detecting coinbase status from the transaction if available.
🤖 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/mod.rs` around lines 350 - 363, The
UTXO is being created with is_coinbase hardcoded to false in Utxo::new (inside
managed_wallet_info::mod), which breaks coinbase maturity; update the changeset
handling to obtain the correct coinbase flag: extend the changeset's UtxoEntry
to carry an is_coinbase boolean (or, if the full transaction/txin data is
available, detect coinbase from the transaction) and pass that value into
Utxo::new instead of false, then ensure the code that constructs UtxoEntry and
the consumer that applies the changeset (the block that sets
utxo.is_instantlocked and inserts into account.utxos) use the new is_coinbase
field.
Comment on lines
+451
to
+462
| // Fallback: if no account matched via outputs, try inputs or | ||
| // just insert into the first standard account. | ||
| if !inserted { | ||
| if let Some(account) = self | ||
| .accounts | ||
| .standard_bip44_accounts | ||
| .values_mut() | ||
| .next() | ||
| { | ||
| account.transactions.insert(*txid, record); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Fallback transaction insertion may misattribute transactions.
When no account matches via outputs, the transaction is inserted into the first standard_bip44_accounts entry. This could incorrectly attribute transactions that don't belong to any account (e.g., transactions only spending from our inputs with no outputs to us).
Consider logging a warning or skipping insertion when no matching account is found, rather than forcing insertion into an arbitrary account.
Suggested approach
// Fallback: if no account matched via outputs, try inputs or
// just insert into the first standard account.
if !inserted {
+ tracing::warn!(
+ txid = %txid,
+ "Transaction could not be matched to any account, inserting into default"
+ );
if let Some(account) = self
.accounts
.standard_bip44_accounts
.values_mut()
.next()
{
account.transactions.insert(*txid, record);
}
}🤖 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/mod.rs` around lines 451 - 462, The
current fallback that inserts a transaction into the first
standard_bip44_accounts entry when no account matched (the block using inserted,
self.accounts.standard_bip44_accounts.values_mut().next(),
account.transactions.insert(*txid, record)) can misattribute transactions;
change this so that when inserted is false you DO NOT insert into an arbitrary
account and instead either skip insertion or emit a warning. Locate the fallback
that checks the inserted flag and replace the forced insert with a warning/log
call that includes the txid and a brief context (e.g., "no matching account for
transaction"), or simply skip insertion; ensure you reference the same symbols
(inserted, txid, record, self.accounts.standard_bip44_accounts,
account.transactions.insert) so reviewers can find the modified logic.
Comment on lines
+470
to
+486
| for (&account_index, &new_index) in &acct_cs.last_revealed { | ||
| if let Some(account) = self | ||
| .accounts | ||
| .standard_bip44_accounts | ||
| .get_mut(&account_index) | ||
| { | ||
| for pool in account.account_type.address_pools_mut() { | ||
| if let Some(current) = pool.highest_generated { | ||
| if new_index > current { | ||
| pool.highest_generated = Some(new_index); | ||
| } | ||
| } else { | ||
| pool.highest_generated = Some(new_index); | ||
| } | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Address pool advancement may over-advance internal/change pools.
The last_revealed changeset maps account_index -> new_index, but the code advances highest_generated on all address pools for that account (both external and internal for Standard accounts). This could incorrectly advance the change address pool when only receive addresses were revealed.
Consider tracking per-pool reveal indices or only advancing the appropriate pool.
🤖 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/mod.rs` around lines 470 - 486, The
changeet last_revealed currently maps account_index -> new_index but the loop
advances highest_generated on every pool returned by
account.account_type.address_pools_mut() (affecting both external and
internal/change pools); update the logic so you only advance the specific pool
that was revealed—either by changing last_revealed to carry pool identity (e.g.,
map (account_index, pool_type) -> new_index) or by keeping per-pool reveal
indices and applying new_index only to the matching pool.highest_generated;
modify the block that iterates acct_cs.last_revealed and the use of
address_pools_mut() so you select the correct pool before updating
pool.highest_generated.
…<RwLock<T>> per wallet PR-30 Phase 1: Refactor WalletManager to use a single map with per-wallet Arc<RwLock<T>> instead of two separate maps (wallets + wallet_infos). Key changes: - New ManagedWalletState struct bundling Wallet + ManagedWalletInfo + Persister - New WalletPersistence trait (store returns Result, NoPersistence default) - WalletInfoInterface gains wallet()/wallet_mut() methods - WalletTransactionChecker::check_core_transaction no longer takes wallet param - ManagedWalletInfo gets check_core_transaction_with_wallet helper - WalletManager uses BTreeMap<WalletId, Arc<RwLock<T>>> for shared state - insert_wallet_state() for external callers to inject pre-built state - Sync methods (monitored_addresses, watched_outpoints, monitor_revision, update_synced_height, process_instant_send_lock) made async for proper lock acquisition; synced_height/filter_committed_height stay sync - ManagedWalletState fields pub(crate) with public accessor methods Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
shumkov
force-pushed
the
feat/platform-wallet
branch
from
0ff104d to
36daa7c
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
key-wallet/src/transaction_checking/wallet_checker.rs (1)
90-105:⚠️ Potential issue | 🟠 MajorDon't update the dedup set before the stale-lock early return.
On the already-confirmed path this branch now mutates
instant_send_locksand then returns as if nothing changed. That leavesstate_modified/changeset metadata out of sync with the applied state.Suggested change
if context == TransactionContext::InstantSend { - 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; + }🤖 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 90 - 105, The code currently inserts into instant_send_locks before checking the already_confirmed condition, causing a stale mutation when returning early; change the logic in the TransactionContext::InstantSend branch so you do not call self.instant_send_locks.insert(txid) until after you have computed already_confirmed and decided to accept the transition (i.e., only insert when !is_new and context == TransactionContext::InstantSend and already_confirmed is false and you will proceed), keeping the early return path from already_confirmed without mutating instant_send_locks so state_modified/changeset remain consistent; use the existing symbols (is_new, TransactionContext::InstantSend, instant_send_locks, already_confirmed, result, affected_accounts, accounts.get_by_account_type_match) to locate and update the code.
♻️ Duplicate comments (3)
key-wallet-manager/src/managed_wallet_state.rs (1)
249-253:⚠️ Potential issue | 🟠 MajorDon't treat a failed
store()as success.At this point the in-memory wallet has already been mutated. Logging and returning
TransactionCheckResultleaves durable state behind with no recovery signal, so the next restart can roll back user-visible history. Please surface this as a real error, or at least mark the state dirty and force the caller to reconcile.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-manager/src/managed_wallet_state.rs` around lines 249 - 253, The current code applies in-memory mutations and then ignores persistence failures (result.changeset + self.persister.store), which can leave durable state inconsistent; change the behavior so a failed self.persister.store(...) is treated as an error: either return an Err from the enclosing method (propagate the persistence error instead of returning a successful TransactionCheckResult) or set/return a explicit "dirty" state indicator on ManagedWalletState/TransactionCheckResult so callers must reconcile; update the branch that currently logs tracing::warn! to instead propagate the error (or flip a dirty flag on self and include that in the returned TransactionCheckResult) and ensure callers handle that failure path.key-wallet-manager/src/process_block.rs (2)
107-124:⚠️ Potential issue | 🟠 MajorKeep height-only balance changes observable.
update_synced_height()recomputes balances, but it never snapshots/emits the delta. When SPV advances height outsideprocess_block()—for example coinbase maturity or other height-driven transitions—subscribers see the balance change silently.update_filter_committed_height()inherits the same gap through its direct call into this method.Minimal fix
async fn update_synced_height(&mut self, height: CoreBlockHeight) { + let old_balances = self.snapshot_balances().await; self.synced_height = height; for arc in self.wallets.values() { let mut state = arc.write().await; state.update_synced_height(height); state.update_balance(); } + self.emit_balance_changes(&old_balances).await; }🤖 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 107 - 124, The balance recomputation in update_synced_height (and transitively update_filter_committed_height) updates wallets but does not produce the balance-change snapshot/event, so height-only transitions are silent; modify update_synced_height to, after calling state.update_synced_height(height) and state.update_balance(), also invoke the same snapshot/emission routine used in process_block (e.g., the wallet state's method that creates/emits balance deltas — call that method for each wallet) so subscribers receive delta events when height-driven changes occur. Ensure update_filter_committed_height continues to call update_synced_height so the emission path is preserved.
152-158:⚠️ Potential issue | 🟠 MajorPersist the InstantSend-only UTXO mutation.
mark_instant_send_utxos()returns aUtxoChangeSet, but this path drops it after mutating state. If an IS lock arrives after the original transaction processing, the lock bit is lost on restart because nothing reachesWalletPersistence.🤖 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 152 - 158, The code calls state.mark_instant_send_utxos(&txid) and discards the returned UtxoChangeSet, so the InstantSend bit mutation is never persisted; when changed is true, capture the returned changeset and send it to the wallet persistence API (WalletPersistence) so the UTXO mutation is recorded to storage before dropping the write lock—i.e., after mark_instant_send_utxos() and before or after state.update_balance(), call the appropriate persistence method with (wallet_id, changeset) or enqueue the UtxoChangeSet for WalletPersistence to apply, ensuring the change survives restarts.
🧹 Nitpick comments (1)
key-wallet/KEY_DERIVATION_ISSUES.md (1)
1-4: Please split these architecture/audit docs into a follow-up PR.This derivation audit, plus the PSBT/BDK planning docs added alongside it, makes a
feat(key-wallet-manager)changeset much broader than the title suggests and harder to review or roll back independently.As per coding guidelines "If a PR mixes unrelated concerns (e.g., a bug fix bundled with a refactor or new feature), suggest splitting it into separate focused PRs."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet/KEY_DERIVATION_ISSUES.md` around lines 1 - 4, This PR mixes an architecture/audit doc (KEY_DERIVATION_ISSUES.md) and PSBT/BDK planning notes with code changes; split the docs into a follow-up PR by removing or reverting the derivation audit and PSBT/BDK planning content from this branch and creating a new branch/PR that contains KEY_DERIVATION_ISSUES.md and the planning docs only, leaving this PR focused on the key-wallet-manager code changes (refer to the derivation audit mentions of src/derivation.rs and src/dip9.rs to identify which sections to extract).
🤖 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-manager/src/accessors.rs`:
- Around line 109-118: The current get_all_utxos and get_total_balance silently
skip wallets whose RwLock is contended because they use try_read(), producing
incomplete results; change their APIs to make contention explicit: either
convert them to async methods (e.g., async fn get_all_utxos(&self) -> Vec<Utxo>
and async fn get_total_balance(&self) -> Amount) and use arc.read().await to
wait for locks, or keep them synchronous but return a Result that reports which
wallets were locked (e.g., Result<Vec<Utxo>, LockedWalletsError> or
Result<(Vec<Utxo>, Vec<WalletId>), _>), replacing try_read() calls in the bodies
with read().await or propagating an Err when try_read() fails; update callers
accordingly so callers cannot silently receive partial aggregates.
- Around line 41-55: The current remove_wallet in remove_wallet uses
arc.try_read().map(...).unwrap_or(0) which loses the wallet's revision when the
lock is held and can make structural_revision non-monotonic; fix by actually
acquiring the read lock to get the revision (i.e., replace the non-blocking
try_read with a blocking read: call arc.read() or await arc.read().await
depending on whether the RwLock is sync or async, then call monitor_revision()
and drop the guard) before updating structural_revision, or alternatively
maintain a per-wallet stored revision (updated on every write) and read that
stored value here instead of using try_read; refer to remove_wallet, wallets,
monitor_revision, structural_revision, and WalletError::WalletNotFound when
making the change.
- Around line 22-37: insert_wallet_state currently inserts the provided
Arc<RwLock<T>> blindly; update it to validate the contained state before
inserting by attempting to acquire a read lock on the supplied state (the
Arc<RwLock<T>>) and comparing the inner state's wallet_id() and network()
against the provided wallet_id and the manager's expected network (or derived
expected value); if the lock cannot be acquired or the inner values disagree
return an Err (e.g., WalletError::InvalidState or
WalletError::WalletExists-style variant), only call self.wallets.insert(...) and
self.bump_structural_revision() after successful validation, and keep the
existing duplicate-key check (self.wallets.contains_key) as a pre-check.
In `@key-wallet-manager/src/event_tests.rs`:
- Around line 283-296: The test is using non-async try_read().unwrap() which can
panic under contention; replace both occurrences where you read the wallet state
(the blocks that call manager.get_wallet_arc(&wallet_id).unwrap() and then
arc.try_read().unwrap()) with async reads: get the Arc via
manager.get_wallet_arc(&wallet_id).unwrap(), then await the RwLock read with
arc.read().await to obtain the guard before calling guard.balance(); keep the
surrounding test async so WalletInterface::process_instant_send_lock(&mut
manager, unknown_txid).await still works and the
assertions/assert_no_events(&mut rx) remain unchanged.
In `@key-wallet-manager/src/managed_wallet_state.rs`:
- Around line 266-352: The methods like add_managed_account,
add_managed_account_with_passphrase, add_managed_account_from_xpub and the
feature-gated variants (add_managed_bls_account,
add_managed_bls_account_with_passphrase,
add_managed_bls_account_from_public_key, add_managed_eddsa_account,
add_managed_eddsa_account_with_passphrase,
add_managed_eddsa_account_from_public_key) currently forward the caller-supplied
wallet reference and can desynchronize the bundle; instead always use the
canonical bundle wallet owned by ManagedWalletState (i.e. call
self.wallet_info.* with &self.wallet) or first assert/validate that the passed
wallet matches &self.wallet, then forward—apply this change consistently to all
the listed methods so they no longer accept an arbitrary external Wallet
reference.
In `@key-wallet-manager/src/process_block.rs`:
- Around line 33-55: The block-processing path fails to update wallet balances
before emitting deltas because check_transaction_in_all_wallets is called with
update_balance=false; change the call to check_transaction_in_all_wallets(tx,
context, true, false, true) (i.e. pass true for the update_balance argument) or
alternatively ensure balances are recomputed on each wallet arc (via the wallet
state method that recalculates cached balances) after processing transactions
and before calling emit_balance_changes(&old_balances); update the call/site
referencing check_transaction_in_all_wallets, get_all_wallet_arcs,
update_synced_height, and emit_balance_changes so balances are up-to-date when
emit_balance_changes runs.
In `@key-wallet-manager/src/test_utils/mock_wallet.rs`:
- Around line 179-182: In process_instant_send_lock, replace the synchronous
try_lock().expect(...) on the status_changes mutex with the awaited async lock
pattern (use .lock().await) to avoid contention races and match the file's other
async mutex usage; acquire the lock with let mut changes =
self.status_changes.lock().await and then push (txid,
TransactionContext::InstantSend) into changes as before.
In `@key-wallet/BDK_ARCHITECTURE_COMPARISON.md`:
- Around line 17-25: The fenced code blocks that list modules (e.g., the block
containing "bdk_core — Shared data types", "bdk_chain — Pure blockchain data
structures (TxGraph, LocalChain) — zero I/O", "bdk_wallet — Wallet logic...",
etc.) need a language tag to satisfy markdownlint; update the opening fences
from ``` to ```text (or ```plaintext) for that block and the other similar
blocks that contain the same module lists (also the blocks around the other
occurrences of the same listing), so each fenced diagram begins with ```text (or
```plaintext) instead of an untagged ``` fence.
In `@key-wallet/KEY_DERIVATION_ISSUES.md`:
- Around line 132-141: The fenced code block containing the constant names
(COINJOIN_PATH_MAINNET, TESTNET, IDENTITY_REGISTRATION_PATH_MAINNET/TESTNET,
IDENTITY_TOPUP_PATH_MAINNET/TESTNET, IDENTITY_INVITATION_PATH_MAINNET/TESTNET,
ASSET_LOCK_ADDRESS_TOPUP_PATH_MAINNET/TESTNET,
ASSET_LOCK_SHIELDED_ADDRESS_TOPUP_PATH_MAINNET/TESTNET,
DASHPAY_ROOT_PATH_MAINNET/TESTNET, PLATFORM_PAYMENT_ROOT_PATH_MAINNET/TESTNET)
needs a language tag to satisfy markdownlint; update the fence from ``` to
include a tag such as ```text (or ```none) immediately after the opening
backticks so the block is recognized as a plain-text constants list.
In `@key-wallet/PSBT_MIGRATION.md`:
- Around line 7-19: Annotate the fenced code blocks that contain
directory-tree/graph snippets in PSBT_MIGRATION.md (e.g., the block starting
with "key-wallet/src/psbt/" and the similar block around lines 29-37) by adding
a language identifier like text (change ``` to ```text) so markdownlint
recognizes them as plain text; update each tree/graph fenced block accordingly.
In `@key-wallet/REORG_SAFETY.md`:
- Around line 9-20: Update the "current WalletInterface" code snippet in
REORG_SAFETY.md to match the real trait definition: change
process_mempool_transaction to the async signature that takes the
is_instant_send flag and returns MempoolTransactionResult, and mark
monitored_addresses and update_synced_height as async (and reflect their actual
return types), so the documented snippet matches the actual WalletInterface
trait (refer to WalletInterface, process_mempool_transaction,
monitored_addresses, update_synced_height).
- Around line 77-103: The “Current state” examples are outdated: update the
section to reflect the real struct definitions by inspecting the
TransactionRecord struct and the Utxo struct and listing their actual fields
(e.g., the current TransactionRecord fields as declared in the TransactionRecord
type and the Utxo fields including height and is_confirmed), then rewrite the
gap analysis and migration plan to be layered on those real fields (mention
TransactionRecord and Utxo by name so readers can locate the types) and clearly
call out what new reorg-state fields are needed and how they map onto the
existing fields.
---
Outside diff comments:
In `@key-wallet/src/transaction_checking/wallet_checker.rs`:
- Around line 90-105: The code currently inserts into instant_send_locks before
checking the already_confirmed condition, causing a stale mutation when
returning early; change the logic in the TransactionContext::InstantSend branch
so you do not call self.instant_send_locks.insert(txid) until after you have
computed already_confirmed and decided to accept the transition (i.e., only
insert when !is_new and context == TransactionContext::InstantSend and
already_confirmed is false and you will proceed), keeping the early return path
from already_confirmed without mutating instant_send_locks so
state_modified/changeset remain consistent; use the existing symbols (is_new,
TransactionContext::InstantSend, instant_send_locks, already_confirmed, result,
affected_accounts, accounts.get_by_account_type_match) to locate and update the
code.
---
Duplicate comments:
In `@key-wallet-manager/src/managed_wallet_state.rs`:
- Around line 249-253: The current code applies in-memory mutations and then
ignores persistence failures (result.changeset + self.persister.store), which
can leave durable state inconsistent; change the behavior so a failed
self.persister.store(...) is treated as an error: either return an Err from the
enclosing method (propagate the persistence error instead of returning a
successful TransactionCheckResult) or set/return a explicit "dirty" state
indicator on ManagedWalletState/TransactionCheckResult so callers must
reconcile; update the branch that currently logs tracing::warn! to instead
propagate the error (or flip a dirty flag on self and include that in the
returned TransactionCheckResult) and ensure callers handle that failure path.
In `@key-wallet-manager/src/process_block.rs`:
- Around line 107-124: The balance recomputation in update_synced_height (and
transitively update_filter_committed_height) updates wallets but does not
produce the balance-change snapshot/event, so height-only transitions are
silent; modify update_synced_height to, after calling
state.update_synced_height(height) and state.update_balance(), also invoke the
same snapshot/emission routine used in process_block (e.g., the wallet state's
method that creates/emits balance deltas — call that method for each wallet) so
subscribers receive delta events when height-driven changes occur. Ensure
update_filter_committed_height continues to call update_synced_height so the
emission path is preserved.
- Around line 152-158: The code calls state.mark_instant_send_utxos(&txid) and
discards the returned UtxoChangeSet, so the InstantSend bit mutation is never
persisted; when changed is true, capture the returned changeset and send it to
the wallet persistence API (WalletPersistence) so the UTXO mutation is recorded
to storage before dropping the write lock—i.e., after mark_instant_send_utxos()
and before or after state.update_balance(), call the appropriate persistence
method with (wallet_id, changeset) or enqueue the UtxoChangeSet for
WalletPersistence to apply, ensuring the change survives restarts.
---
Nitpick comments:
In `@key-wallet/KEY_DERIVATION_ISSUES.md`:
- Around line 1-4: This PR mixes an architecture/audit doc
(KEY_DERIVATION_ISSUES.md) and PSBT/BDK planning notes with code changes; split
the docs into a follow-up PR by removing or reverting the derivation audit and
PSBT/BDK planning content from this branch and creating a new branch/PR that
contains KEY_DERIVATION_ISSUES.md and the planning docs only, leaving this PR
focused on the key-wallet-manager code changes (refer to the derivation audit
mentions of src/derivation.rs and src/dip9.rs to identify which sections to
extract).
🪄 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: 3c579f76-6da5-4966-85dc-5def50fb1729
📒 Files selected for processing (36)
dash-spv/src/client/lifecycle.rsdash-spv/src/sync/filters/manager.rsdash-spv/src/sync/mempool/manager.rsdash-spv/src/sync/mempool/sync_manager.rskey-wallet-ffi/src/wallet_manager_tests.rskey-wallet-manager/Cargo.tomlkey-wallet-manager/examples/wallet_creation.rskey-wallet-manager/src/accessors.rskey-wallet-manager/src/event_tests.rskey-wallet-manager/src/lib.rskey-wallet-manager/src/managed_wallet_state.rskey-wallet-manager/src/persistence.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-manager/tests/integration_test.rskey-wallet-manager/tests/spv_integration_tests.rskey-wallet-manager/tests/test_serialized_wallets.rskey-wallet/ACCOUNT_TYPE_REFACTOR.mdkey-wallet/BDK_ARCHITECTURE_COMPARISON.mdkey-wallet/KEY_DERIVATION_ISSUES.mdkey-wallet/PSBT_MIGRATION.mdkey-wallet/REORG_SAFETY.mdkey-wallet/src/test_utils/wallet.rskey-wallet/src/tests/balance_tests.rskey-wallet/src/transaction_checking/transaction_router/tests/asset_unlock.rskey-wallet/src/transaction_checking/transaction_router/tests/coinbase.rskey-wallet/src/transaction_checking/transaction_router/tests/identity_transactions.rskey-wallet/src/transaction_checking/transaction_router/tests/provider.rskey-wallet/src/transaction_checking/transaction_router/tests/routing.rskey-wallet/src/transaction_checking/wallet_checker.rskey-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rskey-wallet/src/wallet/managed_wallet_info/mod.rskey-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rskey-wallet/src/wallet/mod.rs
💤 Files with no reviewable changes (4)
- key-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rs
- key-wallet/src/wallet/mod.rs
- key-wallet/src/tests/balance_tests.rs
- key-wallet-manager/tests/spv_integration_tests.rs
✅ Files skipped from review due to trivial changes (10)
- dash-spv/src/sync/mempool/sync_manager.rs
- key-wallet/src/transaction_checking/transaction_router/tests/coinbase.rs
- dash-spv/src/client/lifecycle.rs
- key-wallet-manager/tests/test_serialized_wallets.rs
- key-wallet/ACCOUNT_TYPE_REFACTOR.md
- key-wallet/src/transaction_checking/transaction_router/tests/routing.rs
- key-wallet-manager/src/persistence.rs
- dash-spv/src/sync/mempool/manager.rs
- key-wallet/src/transaction_checking/transaction_router/tests/provider.rs
- key-wallet-manager/src/wallet_interface.rs
🚧 Files skipped from review as they are similar to previous changes (10)
- key-wallet-manager/Cargo.toml
- dash-spv/src/sync/filters/manager.rs
- key-wallet/src/transaction_checking/transaction_router/tests/identity_transactions.rs
- key-wallet/src/transaction_checking/transaction_router/tests/asset_unlock.rs
- key-wallet-manager/examples/wallet_creation.rs
- key-wallet/src/test_utils/wallet.rs
- key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs
- key-wallet-manager/tests/integration_test.rs
- key-wallet-manager/src/lib.rs
- key-wallet/src/wallet/managed_wallet_info/mod.rs
Comment on lines
+22
to
+37
| /// Insert an externally-created `Arc<RwLock<T>>` into the manager. | ||
| /// | ||
| /// This allows a caller (e.g. platform-wallet) to create the state, | ||
| /// wrap it in `Arc::new(RwLock::new(state))`, keep a clone for its own | ||
| /// handle, and insert the same `Arc` into `WalletManager`. | ||
| pub fn insert_wallet_state( | ||
| &mut self, | ||
| wallet_id: WalletId, | ||
| state: Arc<RwLock<T>>, | ||
| ) -> Result<(), WalletError> { | ||
| if self.wallets.contains_key(&wallet_id) { | ||
| return Err(WalletError::WalletExists(wallet_id)); | ||
| } | ||
| self.wallets.insert(wallet_id, state); | ||
| self.bump_structural_revision(); | ||
| Ok(()) |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Validate the injected state before indexing it under a wallet ID.
insert_wallet_state() trusts the caller's wallet_id and state blindly. If the embedded state reports a different wallet_id() or network(), the manager's map key, emitted wallet_ids, and filter aggregation diverge immediately. Since this API exists specifically for external injection, it should verify those invariants before accepting the Arc—even if that means making insertion async or rejecting a currently locked state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@key-wallet-manager/src/accessors.rs` around lines 22 - 37,
insert_wallet_state currently inserts the provided Arc<RwLock<T>> blindly;
update it to validate the contained state before inserting by attempting to
acquire a read lock on the supplied state (the Arc<RwLock<T>>) and comparing the
inner state's wallet_id() and network() against the provided wallet_id and the
manager's expected network (or derived expected value); if the lock cannot be
acquired or the inner values disagree return an Err (e.g.,
WalletError::InvalidState or WalletError::WalletExists-style variant), only call
self.wallets.insert(...) and self.bump_structural_revision() after successful
validation, and keep the existing duplicate-key check
(self.wallets.contains_key) as a pre-check.
Comment on lines
+41
to
+55
| pub fn remove_wallet( | ||
| &mut self, | ||
| wallet_id: &WalletId, | ||
| ) -> Result<Arc<RwLock<T>>, WalletError> { | ||
| let arc = | ||
| self.wallets.remove(wallet_id).ok_or(WalletError::WalletNotFound(*wallet_id))?; | ||
| let info = | ||
| self.wallet_infos.remove(wallet_id).ok_or(WalletError::WalletNotFound(*wallet_id))?; | ||
| // Absorb the removed wallet's account-level revision so the total | ||
| // stays monotonically increasing even though we lost a contributor. | ||
| self.structural_revision += info.monitor_revision() + 1; | ||
| Ok((wallet, info)) | ||
| // Use try_read to get the revision without blocking. | ||
| let removed_rev = arc | ||
| .try_read() | ||
| .map(|s| s.monitor_revision()) | ||
| .unwrap_or(0); | ||
| self.structural_revision += removed_rev + 1; | ||
| Ok(arc) |
Contributor
There was a problem hiding this comment.
try_read() breaks the monotonic-revision guarantee on removal.
The comment says removal absorbs the wallet's revision so the total stays monotonic, but try_read().map(...).unwrap_or(0) drops that contribution whenever another holder has the state locked. The aggregate revision can then decrease, which is exactly the signal the mempool/filter side relies on for stale-filter detection. Either await the read here or track the per-wallet revision outside the lock.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@key-wallet-manager/src/accessors.rs` around lines 41 - 55, The current
remove_wallet in remove_wallet uses arc.try_read().map(...).unwrap_or(0) which
loses the wallet's revision when the lock is held and can make
structural_revision non-monotonic; fix by actually acquiring the read lock to
get the revision (i.e., replace the non-blocking try_read with a blocking read:
call arc.read() or await arc.read().await depending on whether the RwLock is
sync or async, then call monitor_revision() and drop the guard) before updating
structural_revision, or alternatively maintain a per-wallet stored revision
(updated on every write) and read that stored value here instead of using
try_read; refer to remove_wallet, wallets, monitor_revision,
structural_revision, and WalletError::WalletNotFound when making the change.
Comment on lines
+109
to
118
| /// Get UTXOs for all wallets (sync, uses try_read). | ||
| pub fn get_all_utxos(&self) -> Vec<Utxo> { | ||
| let mut all_utxos = Vec::new(); | ||
| for info in self.wallet_infos.values() { | ||
| all_utxos.extend(info.utxos().iter()); | ||
| for arc in self.wallets.values() { | ||
| if let Ok(state) = arc.try_read() { | ||
| all_utxos.extend(state.utxos().into_iter().cloned()); | ||
| } | ||
| } | ||
| all_utxos | ||
| } |
Contributor
There was a problem hiding this comment.
Don't silently return partial aggregates when a wallet is locked.
Both get_all_utxos() and get_total_balance() skip any wallet whose RwLock is contended. That turns ordinary concurrent access into incomplete UTXO sets / undercounted balances with no error path. These should be async like the other per-wallet aggregations, or return a status that makes the best-effort semantics explicit.
Also applies to: 131-138
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@key-wallet-manager/src/accessors.rs` around lines 109 - 118, The current
get_all_utxos and get_total_balance silently skip wallets whose RwLock is
contended because they use try_read(), producing incomplete results; change
their APIs to make contention explicit: either convert them to async methods
(e.g., async fn get_all_utxos(&self) -> Vec<Utxo> and async fn
get_total_balance(&self) -> Amount) and use arc.read().await to wait for locks,
or keep them synchronous but return a Result that reports which wallets were
locked (e.g., Result<Vec<Utxo>, LockedWalletsError> or Result<(Vec<Utxo>,
Vec<WalletId>), _>), replacing try_read() calls in the bodies with read().await
or propagating an Err when try_read() fails; update callers accordingly so
callers cannot silently receive partial aggregates.
Comment on lines
+283
to
+296
| let balance_before = { | ||
| let arc = manager.get_wallet_arc(&wallet_id).unwrap(); | ||
| let guard = arc.try_read().unwrap(); | ||
| guard.balance() | ||
| }; | ||
|
|
||
| manager.process_instant_send_lock(unknown_txid); | ||
| WalletInterface::process_instant_send_lock(&mut manager, unknown_txid).await; | ||
|
|
||
| assert_no_events(&mut rx); | ||
| let balance_after = manager.wallet_infos.get(&wallet_id).unwrap().balance(); | ||
| let balance_after = { | ||
| let arc = manager.get_wallet_arc(&wallet_id).unwrap(); | ||
| let guard = arc.try_read().unwrap(); | ||
| guard.balance() | ||
| }; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check if the file exists and view the relevant section
head -n 300 key-wallet-manager/src/event_tests.rs | tail -n 50Repository: dashpay/rust-dashcore
Length of output: 1696
🏁 Script executed:
# Also check the broader context of the test function
grep -n "test_process_instant_send_lock_for_unknown_txid" key-wallet-manager/src/event_tests.rs -A 30Repository: dashpay/rust-dashcore
Length of output: 1270
🏁 Script executed:
# Check the get_wallet_arc function and RwLock usage
rg "get_wallet_arc|tokio::sync::RwLock" key-wallet-manager/src/ -A 2Repository: dashpay/rust-dashcore
Length of output: 804
🏁 Script executed:
# Check what type of lock is used in the manager
rg "struct.*Manager|RwLock" key-wallet-manager/src/ -B 2 -A 5 | head -n 100Repository: dashpay/rust-dashcore
Length of output: 6561
🏁 Script executed:
# Search for read() usage in test files
rg "read\(\)\.await" key-wallet-manager/src/ --type rustRepository: dashpay/rust-dashcore
Length of output: 910
🏁 Script executed:
# Check other tests using RwLock in event_tests.rs to see patterns
grep -n "try_read\|\.read()" key-wallet-manager/src/event_tests.rsRepository: dashpay/rust-dashcore
Length of output: 162
🏁 Script executed:
# Check the process_instant_send_lock implementation to understand what locks it holds
rg -A 15 "fn process_instant_send_lock" key-wallet-manager/src/Repository: dashpay/rust-dashcore
Length of output: 3540
🏁 Script executed:
# Search broadly for read().await patterns in the codebase
rg "\.read\(\)\.await" --type rust key-wallet-manager/Repository: dashpay/rust-dashcore
Length of output: 1586
Use read().await instead of try_read().unwrap() for lock acquisition in async code.
Throughout the codebase (process_block.rs, accessors.rs, integration_tests.rs), lock reads use the async read().await pattern. The try_read().unwrap() here is non-idiomatic for tokio and will panic if a write lock is held by process_instant_send_lock. Since that function acquires write locks on all wallets, the test can experience flakiness when read attempts race with lock acquisition.
Suggested change
let balance_before = {
let arc = manager.get_wallet_arc(&wallet_id).unwrap();
- let guard = arc.try_read().unwrap();
+ let guard = arc.read().await;
guard.balance()
};
@@
assert_no_events(&mut rx);
let balance_after = {
let arc = manager.get_wallet_arc(&wallet_id).unwrap();
- let guard = arc.try_read().unwrap();
+ let guard = arc.read().await;
guard.balance()
};🤖 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 283 - 296, The test is
using non-async try_read().unwrap() which can panic under contention; replace
both occurrences where you read the wallet state (the blocks that call
manager.get_wallet_arc(&wallet_id).unwrap() and then arc.try_read().unwrap())
with async reads: get the Arc via manager.get_wallet_arc(&wallet_id).unwrap(),
then await the RwLock read with arc.read().await to obtain the guard before
calling guard.balance(); keep the surrounding test async so
WalletInterface::process_instant_send_lock(&mut manager, unknown_txid).await
still works and the assertions/assert_no_events(&mut rx) remain unchanged.
Comment on lines
+266
to
+352
| fn add_managed_account( | ||
| &mut self, | ||
| wallet: &Wallet, | ||
| account_type: AccountType, | ||
| ) -> key_wallet::Result<()> { | ||
| self.wallet_info.add_managed_account(wallet, account_type) | ||
| } | ||
|
|
||
| fn add_managed_account_with_passphrase( | ||
| &mut self, | ||
| wallet: &Wallet, | ||
| account_type: AccountType, | ||
| passphrase: &str, | ||
| ) -> key_wallet::Result<()> { | ||
| self.wallet_info | ||
| .add_managed_account_with_passphrase(wallet, account_type, passphrase) | ||
| } | ||
|
|
||
| fn add_managed_account_from_xpub( | ||
| &mut self, | ||
| account_type: AccountType, | ||
| account_xpub: ExtendedPubKey, | ||
| ) -> key_wallet::Result<()> { | ||
| self.wallet_info | ||
| .add_managed_account_from_xpub(account_type, account_xpub) | ||
| } | ||
|
|
||
| #[cfg(feature = "bls")] | ||
| fn add_managed_bls_account( | ||
| &mut self, | ||
| wallet: &Wallet, | ||
| account_type: AccountType, | ||
| ) -> key_wallet::Result<()> { | ||
| self.wallet_info | ||
| .add_managed_bls_account(wallet, account_type) | ||
| } | ||
|
|
||
| #[cfg(feature = "bls")] | ||
| fn add_managed_bls_account_with_passphrase( | ||
| &mut self, | ||
| wallet: &Wallet, | ||
| account_type: AccountType, | ||
| passphrase: &str, | ||
| ) -> key_wallet::Result<()> { | ||
| self.wallet_info | ||
| .add_managed_bls_account_with_passphrase(wallet, account_type, passphrase) | ||
| } | ||
|
|
||
| #[cfg(feature = "bls")] | ||
| fn add_managed_bls_account_from_public_key( | ||
| &mut self, | ||
| account_type: AccountType, | ||
| bls_public_key: [u8; 48], | ||
| ) -> key_wallet::Result<()> { | ||
| self.wallet_info | ||
| .add_managed_bls_account_from_public_key(account_type, bls_public_key) | ||
| } | ||
|
|
||
| #[cfg(feature = "eddsa")] | ||
| fn add_managed_eddsa_account( | ||
| &mut self, | ||
| wallet: &Wallet, | ||
| account_type: AccountType, | ||
| ) -> key_wallet::Result<()> { | ||
| self.wallet_info | ||
| .add_managed_eddsa_account(wallet, account_type) | ||
| } | ||
|
|
||
| #[cfg(feature = "eddsa")] | ||
| fn add_managed_eddsa_account_with_passphrase( | ||
| &mut self, | ||
| wallet: &Wallet, | ||
| account_type: AccountType, | ||
| passphrase: &str, | ||
| ) -> key_wallet::Result<()> { | ||
| self.wallet_info | ||
| .add_managed_eddsa_account_with_passphrase(wallet, account_type, passphrase) | ||
| } | ||
|
|
||
| #[cfg(feature = "eddsa")] | ||
| fn add_managed_eddsa_account_from_public_key( | ||
| &mut self, | ||
| account_type: AccountType, | ||
| ed25519_public_key: [u8; 32], | ||
| ) -> key_wallet::Result<()> { | ||
| self.wallet_info | ||
| .add_managed_eddsa_account_from_public_key(account_type, ed25519_public_key) |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use self.wallet for the bundled account operations.
These delegations forward an arbitrary wallet: &Wallet supplied by the caller, even though ManagedWalletState already owns the canonical wallet. Passing the wrong wallet here can desynchronize wallet_info from the bundled keys and reintroduce the mismatch this refactor is supposed to remove. Either ignore the parameter and always pass &self.wallet, or validate that it matches before forwarding.
One way to keep the bundle invariant
fn add_managed_account(
&mut self,
- wallet: &Wallet,
+ _wallet: &Wallet,
account_type: AccountType,
) -> key_wallet::Result<()> {
- self.wallet_info.add_managed_account(wallet, account_type)
+ self.wallet_info.add_managed_account(&self.wallet, account_type)
}Apply the same pattern to the other ManagedAccountOperations methods.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@key-wallet-manager/src/managed_wallet_state.rs` around lines 266 - 352, The
methods like add_managed_account, add_managed_account_with_passphrase,
add_managed_account_from_xpub and the feature-gated variants
(add_managed_bls_account, add_managed_bls_account_with_passphrase,
add_managed_bls_account_from_public_key, add_managed_eddsa_account,
add_managed_eddsa_account_with_passphrase,
add_managed_eddsa_account_from_public_key) currently forward the caller-supplied
wallet reference and can desynchronize the bundle; instead always use the
canonical bundle wallet owned by ManagedWalletState (i.e. call
self.wallet_info.* with &self.wallet) or first assert/validate that the passed
wallet matches &self.wallet, then forward—apply this change consistently to all
the listed methods so they no longer accept an arbitrary external Wallet
reference.
Comment on lines
+17
to
+25
| ``` | ||
| bdk_core — Shared data types | ||
| bdk_chain — Pure blockchain data structures (TxGraph, LocalChain) — zero I/O | ||
| bdk_wallet — Wallet logic (address generation, tx building, signing) | ||
| bdk_electrum — Electrum sync backend | ||
| bdk_esplora — Esplora HTTP sync backend | ||
| bdk_bitcoind_rpc — Bitcoin Core RPC sync backend | ||
| bdk_file_store — Append-only flat-file persistence (dev/test only) | ||
| ``` |
Contributor
There was a problem hiding this comment.
Add language tags to these fenced diagrams.
text/plaintext is enough here and will clear the markdownlint warnings.
Also applies to: 37-43, 228-255
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 17-17: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@key-wallet/BDK_ARCHITECTURE_COMPARISON.md` around lines 17 - 25, The fenced
code blocks that list modules (e.g., the block containing "bdk_core — Shared
data types", "bdk_chain — Pure blockchain data structures (TxGraph, LocalChain)
— zero I/O", "bdk_wallet — Wallet logic...", etc.) need a language tag to
satisfy markdownlint; update the opening fences from ``` to ```text (or
```plaintext) for that block and the other similar blocks that contain the same
module lists (also the blocks around the other occurrences of the same listing),
so each fenced diagram begins with ```text (or ```plaintext) instead of an
untagged ``` fence.
Comment on lines
+132
to
+141
| ``` | ||
| COINJOIN_PATH_MAINNET / TESTNET | ||
| IDENTITY_REGISTRATION_PATH_MAINNET / TESTNET | ||
| IDENTITY_TOPUP_PATH_MAINNET / TESTNET | ||
| IDENTITY_INVITATION_PATH_MAINNET / TESTNET | ||
| ASSET_LOCK_ADDRESS_TOPUP_PATH_MAINNET / TESTNET | ||
| ASSET_LOCK_SHIELDED_ADDRESS_TOPUP_PATH_MAINNET / TESTNET | ||
| DASHPAY_ROOT_PATH_MAINNET / TESTNET | ||
| PLATFORM_PAYMENT_ROOT_PATH_MAINNET / TESTNET | ||
| ``` |
Contributor
There was a problem hiding this comment.
Add a language tag to this fenced block.
markdownlint is already flagging this fence. text is enough if you want to keep it as a constants list.
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 132-132: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@key-wallet/KEY_DERIVATION_ISSUES.md` around lines 132 - 141, The fenced code
block containing the constant names (COINJOIN_PATH_MAINNET, TESTNET,
IDENTITY_REGISTRATION_PATH_MAINNET/TESTNET, IDENTITY_TOPUP_PATH_MAINNET/TESTNET,
IDENTITY_INVITATION_PATH_MAINNET/TESTNET,
ASSET_LOCK_ADDRESS_TOPUP_PATH_MAINNET/TESTNET,
ASSET_LOCK_SHIELDED_ADDRESS_TOPUP_PATH_MAINNET/TESTNET,
DASHPAY_ROOT_PATH_MAINNET/TESTNET, PLATFORM_PAYMENT_ROOT_PATH_MAINNET/TESTNET)
needs a language tag to satisfy markdownlint; update the fence from ``` to
include a tag such as ```text (or ```none) immediately after the opening
backticks so the block is recognized as a plain-text constants list.
Comment on lines
+7
to
+19
| ``` | ||
| key-wallet/src/psbt/ | ||
| ├── mod.rs — PartiallySignedTransaction, signing logic (1894 lines) | ||
| ├── map/ | ||
| │ ├── input.rs — PSBT Input map (616 lines) | ||
| │ ├── output.rs — PSBT Output map (187 lines) | ||
| │ ├── global.rs — PSBT Global map (251 lines) | ||
| │ └── mod.rs — Map trait (34 lines) | ||
| ├── serialize.rs — BIP174 binary serialization (486 lines) | ||
| ├── raw.rs — Raw key/value encoding (228 lines) | ||
| ├── error.rs — Error types (240 lines) | ||
| └── macros.rs — Internal macros (179 lines) | ||
| ``` |
Contributor
There was a problem hiding this comment.
Annotate these fenced blocks with a language.
markdownlint will keep flagging the tree/graph snippets until they use something like text.
Also applies to: 29-37
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@key-wallet/PSBT_MIGRATION.md` around lines 7 - 19, Annotate the fenced code
blocks that contain directory-tree/graph snippets in PSBT_MIGRATION.md (e.g.,
the block starting with "key-wallet/src/psbt/" and the similar block around
lines 29-37) by adding a language identifier like text (change ``` to ```text)
so markdownlint recognizes them as plain text; update each tree/graph fenced
block accordingly.
Comment on lines
+9
to
+20
| The `WalletInterface` trait (`key-wallet-manager/src/wallet_interface.rs`) — the contract between dash-spv and the wallet — has no reorg-related method at all: | ||
|
|
||
| ```rust | ||
| pub trait WalletInterface: Send + Sync + 'static { | ||
| async fn process_block(&mut self, block: &Block, height: CoreBlockHeight) -> BlockProcessingResult; | ||
| async fn process_mempool_transaction(&mut self, tx: &Transaction); | ||
| fn monitored_addresses(&self) -> Vec<Address>; | ||
| fn synced_height(&self) -> CoreBlockHeight; | ||
| fn update_synced_height(&mut self, height: CoreBlockHeight); | ||
| // ... no process_reorg, no on_block_disconnected, nothing | ||
| } | ||
| ``` |
Contributor
There was a problem hiding this comment.
Update the “current WalletInterface” snippet to match the code.
This block still shows the pre-refactor contract. In key-wallet-manager/src/wallet_interface.rs:49-137, process_mempool_transaction now takes is_instant_send and returns MempoolTransactionResult, while monitored_addresses and update_synced_height are async. Since this note is meant to guide follow-up work, the current-state section should reflect the real trait before proposing process_reorg().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@key-wallet/REORG_SAFETY.md` around lines 9 - 20, Update the "current
WalletInterface" code snippet in REORG_SAFETY.md to match the real trait
definition: change process_mempool_transaction to the async signature that takes
the is_instant_send flag and returns MempoolTransactionResult, and mark
monitored_addresses and update_synced_height as async (and reflect their actual
return types), so the documented snippet matches the actual WalletInterface
trait (refer to WalletInterface, process_mempool_transaction,
monitored_addresses, update_synced_height).
Comment on lines
+77
to
+103
| ## Current state in key-wallet | ||
|
|
||
| ### `TransactionRecord` has no chain state | ||
|
|
||
| ```rust | ||
| pub struct TransactionRecord { | ||
| pub transaction: Transaction, | ||
| pub block_hash: Option<BlockHash>, | ||
| pub block_height: Option<u32>, | ||
| pub timestamp: u64, | ||
| // no: is_evicted, confirmations_at_tip, competing_txids | ||
| } | ||
| ``` | ||
|
|
||
| `block_height: Option<u32>` is set once when a block is processed and never cleared. There is no field to express "this transaction was confirmed at height 1000 but that block was reorganized away." | ||
|
|
||
| ### `Utxo` has no chain state | ||
|
|
||
| ```rust | ||
| pub struct Utxo { | ||
| pub outpoint: OutPoint, | ||
| pub tx_out: TxOut, | ||
| pub address: Address, | ||
| pub is_instantlocked: bool, | ||
| // no: confirmed_at_height, is_from_evicted_tx | ||
| } | ||
| ``` |
Contributor
There was a problem hiding this comment.
Rebase the “current state” section on the actual structs.
These examples no longer match key-wallet today: TransactionRecord in key-wallet/src/managed_account/transaction_record.rs:69-91 does not have block_hash / block_height / timestamp, and Utxo in key-wallet/src/utxo.rs:18-35 already has height and is_confirmed. That makes the gap analysis and migration plan misleading. Update this section to describe the real fields, then layer the reorg proposal on top of that.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@key-wallet/REORG_SAFETY.md` around lines 77 - 103, The “Current state”
examples are outdated: update the section to reflect the real struct definitions
by inspecting the TransactionRecord struct and the Utxo struct and listing their
actual fields (e.g., the current TransactionRecord fields as declared in the
TransactionRecord type and the Utxo fields including height and is_confirmed),
then rewrite the gap analysis and migration plan to be layered on those real
fields (mention TransactionRecord and Utxo by name so readers can locate the
types) and clearly call out what new reorg-state fields are needed and how they
map onto the existing fields.
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
1 participant
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.
Summary
Refactors
WalletManagerto use a single map with per-walletArc<RwLock<T>>instead of two separate maps (wallets+wallet_infos). This enablesplatform-walletto share wallet state betweenWalletManager(for SPV processing) andPlatformWallethandles (for sub-wallet operations) through the sameArc.Key changes:
ManagedWalletStatestruct bundlingWallet+ManagedWalletInfo+PersisterWalletPersistencetrait (storereturnsResult,NoPersistencedefault)WalletInfoInterfacegainswallet()/wallet_mut()methodsWalletTransactionChecker::check_core_transactionno longer takes separatewalletparamManagedWalletInfo::check_core_transaction_with_wallet()helper for composite typesWalletManagerusesBTreeMap<WalletId, Arc<RwLock<T>>>— single map, per-wallet locksinsert_wallet_state()for external callers to inject pre-builtArc<RwLock<T>>monitored_addresses,watched_outpoints,monitor_revision,update_synced_height,process_instant_send_lock) made asyncsynced_height/filter_committed_heightstay sync (plain field reads)ManagedWalletStatefieldspub(crate)with public accessor methodsMotivation
platform-wallet'sSpvWalletAdapter(~330 lines) reimplementedWalletInterfaceby iterating wallets manually — exactly whatWalletManageralready does. With this change,platform-walletcan useWalletManager<PlatformWalletInfo>directly viaDashSpvClient<WalletManager<PlatformWalletInfo>, ...>, eliminating the adapter entirely.Test plan
cargo test -p key-wallet-manager— all tests passcargo test -p key-wallet— all tests passcargo check -p dash-spv— SPV compiles with async WalletInterface methods🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Refactor
Documentation