refactor(key-wallet): wire ManagedCoreKeysAccount into the collection#716
Draft
QuantumExplorer wants to merge 11 commits intov0.42-devfrom
Draft
refactor(key-wallet): wire ManagedCoreKeysAccount into the collection#716QuantumExplorer wants to merge 11 commits intov0.42-devfrom
QuantumExplorer wants to merge 11 commits intov0.42-devfrom
Conversation
Lets callers opt out of in-memory transaction history retention while still tracking UTXOs and balance. Defaults to true to preserve existing behavior; serde deserialization of legacy payloads also defaults to true. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replaces the runtime `keep_txs_in_memory: bool` field with a Cargo feature of the same name (enabled by default). When the feature is off, `ManagedCoreAccount::transactions` is removed at compile time so memory isn't reserved for per-tx history; UTXOs and balance continue to be tracked normally. - Cargo.toml: add `keep_txs_in_memory` to default features; mark the dev-dep self-reference as `default-features = false` so the feature can actually be turned off in tests. - ManagedCoreAccount: gate the `transactions` field; expose `has_transaction` / `get_transaction` helpers that always work and return false / None when the feature is off. - ManagedAccountTrait: gate `transactions()` / `transactions_mut()`. - wallet_checker / wallet_info_interface: route field access through the helpers or `#[cfg]` blocks; wallet-level history queries return empty when the feature is off. - Tests: gate sub-modules that exercise the field; rewrite the feature-specific test module to cover both compile-time configurations. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Make in-memory transaction history opt-in. Crates that need the `ManagedCoreAccount::transactions` map enable the feature explicitly; key-wallet-ffi already does so to keep its FFI surface unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
key-wallet-manager's events depend on per-account transaction history (matured coinbase records, mempool→block transitions), so it pulls in the `keep_txs_in_memory` feature on its key-wallet dependency. This also fixes downstream crates (dash-spv) whose tests inspect `account.transactions` directly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CodeRabbit review caught that confirm_transaction's dedup guard (self.transactions.contains_key(&txid)) is always false with the feature off, so re-events (mempool→block, IS-lock arrivals after a block) are treated as brand new every time — replaying record_transaction, re-bumping monitor_revision, and spuriously flipping state_modified. Add a `processed_txids: HashSet<Txid>` field that's always populated regardless of the feature, and use it as the dedup signal. has_transaction now reads it (so call sites still work in both configs); record_transaction inserts into it; Deserialize rebuilds it from `transactions` keys. Also adds an idempotency regression test (mempool→block on the same tx must not change utxo count or balance and must bump monitor_revision at most once) that runs in both feature configurations. Existing wallet_checker tests that simulated a missing record by removing from `transactions` directly are updated to clear `processed_txids` too (since the dedup signal now lives there). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ants Move the existing struct out of `managed_account/mod.rs` into a dedicated `managed_core_funds_account.rs` and rename it to `ManagedCoreFundsAccount` to make room for a sibling `ManagedCoreKeysAccount` that omits funds-tracking state (`balance`, `utxos`, `spent_outpoints`). The keys variant is intended for accounts that derive special-purpose keys (identity registration, asset locks, masternode provider keys) but is not yet wired into `ManagedAccountCollection` — that migration follows in a subsequent PR. This change is purely structural: `ManagedAccountCollection` continues to store `ManagedCoreFundsAccount` for every account type and behavior is unchanged. The C ABI is preserved — `FFIManagedCoreAccount` and the `managed_core_account_*` C functions keep their original names and now wrap the renamed Rust struct internally. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…Account - Sort imports and rewrap long signatures (rustfmt fixes flagged by pre-commit) - Qualify the `[\`ManagedCoreFundsAccount\`]` intra-doc links in `managed_core_keys_account.rs` to `[\`crate::managed_account::ManagedCoreFundsAccount\`]` so rustdoc can resolve them under `-D warnings` Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…N_SUMMARY The example called `get_next_receive_address()`, which is not a method on this struct. Replace with the real `next_receive_address(...)` signature so the snippet compiles. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Promotes the dead-code `ManagedCoreKeysAccount` introduced in the previous
refactor into the active type for accounts that derive special-purpose keys
but don't track funds. Identity, asset-lock, and provider account fields on
`ManagedAccountCollection` switch from `ManagedCoreFundsAccount` to
`ManagedCoreKeysAccount`; standard, CoinJoin, and DashPay fields stay on the
funds variant.
The keys account gains the methods it needs to be a first-class participant
in transaction matching and recording: `record_transaction` /
`confirm_transaction` (simplified, no UTXO/balance work), `processed_txids`
dedup set, feature-gated `transactions` mirroring the funds-account pattern,
plus `classify_address`, `check_transaction_for_match`,
`check_asset_lock_transaction_for_match`, and the four
`check_provider_*_key_in_transaction_for_match` methods that previously
lived on `ManagedCoreFundsAccount`.
Spanning collection accessors (`get_by_account_type_match`, `all_accounts`,
`get`, `remove`, …) return a new `ManagedAccountRef<'a> { Funds, Keys }`
enum (with a mutable counterpart). The enum carries delegating helpers so
callers that don't care about the variant — most of `wallet_info_interface`,
`wallet_checker`, FFI address-pool lookup — keep working without explicit
match dispatch. `insert` now takes an owned `OwnedManagedCoreAccount` enum;
direct-typed `insert_funds` / `insert_keys` are also exposed for callers
that statically know the variant.
`wallet/managed_wallet_info` accessors that returned `&mut
ManagedCoreFundsAccount` for identity / asset-lock / provider accounts now
return `&mut ManagedCoreKeysAccount`. `account_balances`,
`transaction_history`, `update_balance`, `mark_instant_send_utxos`, and the
matured-coinbase / immature-tx surfaces filter to the funds variant, since
those concepts only apply there.
`ManagedAccountTrait` stays funds-only; only `ManagedCoreFundsAccount`
implements it. The keys variant uses inherent methods plus the enum.
C ABI is preserved on the FFI side: `FFIManagedCoreAccount` now wraps an
internal `Funds | Keys` enum carrying an `Arc<…>`, with new `as_funds` /
`as_keys` accessors and a `new_keys` constructor. The legacy `inner()`
helper panics on a keys-only handle (it's only reachable from funds-only
FFI functions; the variant-aware entry points create the right kind of
handle).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 (10)
key-wallet/README.md (1)
204-213:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing
keep_txs_in_memoryfrom the Feature Flags section.The new feature added in this PR has no entry in the Feature Flags section, leaving users unaware of the opt-in behavior.
📝 Suggested addition
- `test-utils`: Testing helpers and fixtures (for use in dev-dependencies) +- `keep_txs_in_memory`: Retain per-account transaction history in memory; disable when only UTXOs and balances are needed🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet/README.md` around lines 204 - 213, Add the missing feature flag entry for keep_txs_in_memory to the "Feature Flags" section so users know it's opt-in; update the README's Feature Flags list to include a bullet describing `keep_txs_in_memory` (what it enables, e.g., keeping transactions in memory for performance/behavior, and whether it's enabled by default) near the other flags (`default`, `std`, `serde`, etc.) so it appears alongside those entries.key-wallet/src/managed_account/managed_account_collection.rs (1)
635-769:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDon't silently drop accounts during
from_account_collection.Every
if let Ok(...)here turns construction failures into missing accounts, so the caller gets a partially populatedManagedAccountCollectionwith no signal that conversion failed. This is especially risky on the feature-gated provider paths, where key-source/derivation setup can fail and the account just disappears. Please return aResult<Self, Error>here, or at least accumulate and surface conversion errors instead of skipping them.As per coding guidelines "Return
Result<T>for all fallible operations and provide context in error messages".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet/src/managed_account/managed_account_collection.rs` around lines 635 - 769, The conversion loop silently drops failed account conversions in from_account_collection; change the constructor to return Result<ManagedAccountCollection, Error> (or a suitable error type) and propagate errors from create_managed_account_from_account, create_managed_account_from_bls_account, create_managed_account_from_eddsa_account, and create_managed_platform_account_from_account instead of using if let Ok(...). Update the from_account_collection signature to return Result<Self, Error>, replace each if let Ok(...) branch with matching that maps Err(e) to an early return (using ? or map_err) that augments context (include the account index/key and account kind like "standard_bip44", "provider_operator_keys", etc.), and ensure feature-gated paths also propagate their errors rather than omitting accounts.key-wallet-ffi/src/managed_account.rs (8)
694-704:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winUse the variant-aware accessor to avoid panic on keys-only accounts.
This function calls
account.inner().is_watch_onlywhich will panic on keys-only accounts. TheFFIManagedCoreAccount::is_watch_only()method (lines 98-103) works on both variants.Proposed fix
pub unsafe extern "C" fn managed_core_account_get_is_watch_only( account: *const FFIManagedCoreAccount, ) -> bool { if account.is_null() { return false; } let account = &*account; - account.inner().is_watch_only + account.is_watch_only() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/managed_account.rs` around lines 694 - 704, The function managed_core_account_get_is_watch_only uses account.inner().is_watch_only which panics for keys-only accounts; change it to call the variant-aware accessor FFIManagedCoreAccount::is_watch_only() on the dereferenced account pointer (after the null check) so both variants are handled safely and no direct access to inner() is performed.
713-733:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReturn zero balance for keys-only accounts instead of panicking.
This function calls
account.inner().balancewhich will panic on keys-only accounts. Keys accounts don't track balances, so returning a zero balance is the appropriate behavior.Proposed fix
pub unsafe extern "C" fn managed_core_account_get_balance( account: *const FFIManagedCoreAccount, balance_out: *mut crate::types::FFIBalance, ) -> bool { if account.is_null() || balance_out.is_null() { return false; } let account = &*account; - let balance = &account.inner().balance; - - *balance_out = crate::types::FFIBalance { - confirmed: balance.confirmed(), - unconfirmed: balance.unconfirmed(), - immature: balance.immature(), - locked: balance.locked(), - total: balance.total(), - }; + *balance_out = match account.as_funds() { + Some(funds) => { + let balance = &funds.balance; + crate::types::FFIBalance { + confirmed: balance.confirmed(), + unconfirmed: balance.unconfirmed(), + immature: balance.immature(), + locked: balance.locked(), + total: balance.total(), + } + } + None => crate::types::FFIBalance { + confirmed: 0, + unconfirmed: 0, + immature: 0, + locked: 0, + total: 0, + }, + }; true }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/managed_account.rs` around lines 713 - 733, managed_core_account_get_balance currently dereferences account.inner().balance which panics for keys-only accounts; update the function to detect keys-only accounts (e.g., via a method like account.inner().is_keys_only() or by using an Option-returning accessor such as account.inner().balance().or_else / balance_opt()) and, when no balance is available, populate balance_out with a zero FFIBalance (all fields zero) instead of accessing balance; otherwise fill FFIBalance from the real balance as before. Ensure you reference FFIManagedCoreAccount, managed_core_account_get_balance, account.inner(), and crate::types::FFIBalance in the change.
594-603:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winUse the variant-aware accessor to avoid panic on keys-only accounts.
This function calls
account.inner().networkwhich will panic if a keys-only account handle is passed. TheFFIManagedCoreAccount::network()method (lines 90-95) works on both variants and should be used instead.Proposed fix
pub unsafe extern "C" fn managed_core_account_get_network( account: *const FFIManagedCoreAccount, ) -> FFINetwork { if account.is_null() { return FFINetwork::Mainnet; } let account = &*account; - account.inner().network.into() + account.network().into() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/managed_account.rs` around lines 594 - 603, The function managed_core_account_get_network currently dereferences account.inner().network which will panic for keys-only variants; replace that direct access with the variant-aware accessor by calling FFIManagedCoreAccount::network() on the account reference (i.e., use account.network() or FFIManagedCoreAccount::network(&*account)) after the null check so it safely returns the correct FFINetwork for both account variants instead of panicking on keys-only accounts.
631-647:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winUse the variant-aware accessor to avoid panic on keys-only accounts.
This function calls
account.inner()which will panic on keys-only accounts. TheFFIManagedCoreAccount::managed_account_type()method (lines 82-87) works on both variants.Proposed fix
pub unsafe extern "C" fn managed_core_account_get_account_type( account: *const FFIManagedCoreAccount, index_out: *mut c_uint, ) -> FFIAccountKind { if account.is_null() { return FFIAccountKind::StandardBIP44; // Default type } let account = &*account; - let managed_account = account.inner(); - let account_type_rust = managed_account.managed_account_type.to_account_type(); + let account_type_rust = account.managed_account_type().to_account_type(); // Set the index if output pointer is provided if !index_out.is_null() { *index_out = account_type_rust.index().unwrap_or(0); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/managed_account.rs` around lines 631 - 647, The function managed_core_account_get_account_type currently calls account.inner(), which can panic for keys-only accounts; instead use the variant-aware accessor FFIManagedCoreAccount::managed_account_type() to obtain the managed account type (e.g., call account.managed_account_type().to_account_type()) and derive the index and kind from that result; remove the inner() usage so keys-only variants won't panic and keep the same logic for writing to index_out and returning the FFIAccountKind.
741-767:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReturn zero counts for keys-only accounts instead of panicking.
Both
managed_core_account_get_transaction_countandmanaged_core_account_get_utxo_countcallaccount.inner()which will panic on keys-only accounts. Keys accounts don't track UTXOs, so returning 0 is appropriate.Proposed fix for transaction count
pub unsafe extern "C" fn managed_core_account_get_transaction_count( account: *const FFIManagedCoreAccount, ) -> c_uint { if account.is_null() { return 0; } let account = &*account; - account.inner().transactions.len() as c_uint + match account.as_funds() { + Some(funds) => funds.transactions.len() as c_uint, + None => 0, + } }Proposed fix for UTXO count
pub unsafe extern "C" fn managed_core_account_get_utxo_count( account: *const FFIManagedCoreAccount, ) -> c_uint { if account.is_null() { return 0; } let account = &*account; - account.inner().utxos.len() as c_uint + match account.as_funds() { + Some(funds) => funds.utxos.len() as c_uint, + None => 0, + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/managed_account.rs` around lines 741 - 767, Both managed_core_account_get_transaction_count and managed_core_account_get_utxo_count call account.inner() which panics for keys-only accounts; change them to safely handle missing inner state and return 0 for keys-only accounts. Replace the direct account.inner() call with a safe check such as if let Some(inner) = account.inner_opt() { inner.transactions.len() as c_uint } else { 0 } (or check an is_keys_only/is_watch_only flag on FFIManagedCoreAccount and return 0), and apply the same pattern for utxos in managed_core_account_get_utxo_count so keys-only accounts return 0 instead of panicking.
1046-1070:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReturn empty transaction list for keys-only accounts instead of panicking.
This function calls
account.inner().transactionswhich will panic on keys-only accounts.Proposed fix
pub unsafe extern "C" fn managed_core_account_get_transactions( account: *const FFIManagedCoreAccount, transactions_out: *mut *mut FFITransactionRecord, count_out: *mut usize, ) -> bool { if account.is_null() || transactions_out.is_null() || count_out.is_null() { return false; } let account = &*account; - let transactions = &account.inner().transactions; + let Some(funds) = account.as_funds() else { + // Keys-only accounts don't expose transactions through this API + *transactions_out = std::ptr::null_mut(); + *count_out = 0; + return true; + }; + let transactions = &funds.transactions; if transactions.is_empty() { *transactions_out = std::ptr::null_mut(); *count_out = 0; return true; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/managed_account.rs` around lines 1046 - 1070, The function managed_core_account_get_transactions currently dereferences account.inner() which panics for keys-only accounts; update it to safely handle the absence of an inner account by checking for the inner account before accessing transactions (e.g., if there is an is_keys_only()/inner_opt()/inner_ref() accessor, use that or match on Option/Result), and when no inner exists set *transactions_out = std::ptr::null_mut(), *count_out = 0 and return true; otherwise proceed to collect transactions from inner().transactions and fill the outputs as before.
1174-1184:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the variant-aware accessor to avoid panic on keys-only accounts.
This function calls
account.inner().managed_account_typewhich will panic on keys-only accounts.Proposed fix
pub unsafe extern "C" fn managed_core_account_get_index( account: *const FFIManagedCoreAccount, ) -> c_uint { if account.is_null() { return 0; } let account = &*account; - account.inner().managed_account_type.index_or_default() + account.managed_account_type().index_or_default() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/managed_account.rs` around lines 1174 - 1184, managed_core_account_get_index currently dereferences account.inner().managed_account_type directly which panics for keys-only accounts; replace that direct field access with the enum-aware accessor on the inner account (use the provided variant-aware getter for managed_account_type that safely returns an index or a default) so managed_core_account_get_index returns the safe index_or_default value for all variants (including keys-only) instead of causing a panic.
1196-1256:⚠️ Potential issue | 🟠 MajorAddress pool functions panic on keys-only accounts.
managed_core_account_get_external_address_pool,managed_core_account_get_internal_address_pool, andmanaged_core_account_get_address_poolall callaccount.inner()which panics on keys-only accounts with the message "FFIManagedCoreAccount::inner() called on a keys-only account; use as_keys() or the variant-aware accessors". These functions should either useas_funds()and return null for keys-only accounts, or implement keys account handling if applicable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/managed_account.rs` around lines 1196 - 1256, The three address-pool getters (managed_core_account_get_external_address_pool, managed_core_account_get_internal_address_pool, and managed_core_account_get_address_pool) currently call account.inner() which panics for keys-only accounts; change them to avoid inner() by using the variant-aware accessor (call account.as_funds() or equivalent) and only proceed to read managed_account.managed_account_type for fund-bearing accounts, returning std::ptr::null_mut() for keys-only accounts; ensure you replace the account.inner() usage in each function with the safe variant-aware check so no panic occurs for keys-only FFIManagedCoreAccount values.
🧹 Nitpick comments (1)
key-wallet-ffi/Cargo.toml (1)
23-23:keep_txs_in_memoryis hardcoded without a passthrough feature.The
key-wallet-ffi[features]table has nokeep_txs_in_memoryentry, so consumers of this crate (particularly thelibcrate type) cannot opt out of in-memory transaction history. Downstream builds will always compile with this feature on, regardless of whether they need it.Given the PR description already notes "Follow-ups suggested for FFI surface," consider adding a passthrough feature at that point:
# In [features]: keep_txs_in_memory = ["key-wallet/keep_txs_in_memory"]and removing the hard-wired feature from the dependency line.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/Cargo.toml` at line 23, The dependency on key-wallet currently hardcodes the keep_txs_in_memory feature in the key-wallet entry; remove the features = ["keep_txs_in_memory"] fragment from the key-wallet dependency and add a passthrough feature in this crate's [features] table named keep_txs_in_memory that enables the upstream feature via keep_txs_in_memory = ["key-wallet/keep_txs_in_memory"] so downstream consumers can opt in/out; update Cargo.toml to rely on the new feature flag rather than the hardwired dependency feature.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dash-spv/tests/dashd_sync/setup.rs`:
- Around line 124-129: The current computation using
wallet_info.accounts().all_accounts().into_iter().map(|a|
a.transactions_iter().count()).sum() overcounts when the same txid appears in
multiple accounts; change the logic in the transaction_count helper to collect
txids from each account via a transactions_iter(), insert them into a HashSet to
deduplicate, and return the set.len() instead of summing per-account counts (see
count_wallet_transactions in helpers.rs for the intended pattern and use of
HashSet to count unique txids).
In `@key-wallet/Cargo.toml`:
- Around line 19-21: The PR is labeled "refactor" but adds new public
functionality (new types ManagedAccountRef / ManagedAccountRefMut, a new Cargo
feature keep_txs_in_memory, expanded ManagedCoreKeysAccount methods, new FFI
constructors/accessors, and tests); change the commit/PR labeling to "feat" to
reflect new functionality: update the PR title and any conventional-commit
prefixes on the commits (or squash/amend commits) to use "feat:" and ensure the
description mentions the new symbols (ManagedAccountRef, ManagedAccountRefMut,
keep_txs_in_memory, ManagedCoreKeysAccount, FFI constructors/accessors) so
release tooling and changelogs pick up the feature addition.
- Line 52: The dev-dependency self-reference in Cargo.toml omits the
"keep_txs_in_memory" feature, so running cargo test in key-wallet doesn't enable
the coinbase test module; update the key-wallet dependency entry (the key-wallet
= { path = ".", default-features = false, features = [...] } line) to include
"keep_txs_in_memory" in the features array so the coinbase tests are compiled
and run during a plain cargo test.
In `@key-wallet/src/managed_account/managed_account_collection.rs`:
- Around line 823-829: create_managed_account_from_account_type() currently maps
AccountType::PlatformPayment into OwnedManagedCoreAccount::Funds which cannot be
round-tripped because insert_funds() rejects ManagedAccountType::PlatformPayment
and expects a ManagedPlatformAccount; change
create_managed_account_from_account_type() to special-case
AccountType::PlatformPayment so it does not produce a Funds variant — either
return an Err with a clear context-bearing error (convert the function to return
Result<OwnedManagedCoreAccount, YourError>) or provide a dedicated platform
constructor that returns a ManagedPlatformAccount/OwnedManagedCoreAccount
variant acceptable to insert_funds(); update callers accordingly so all fallible
behavior uses Result and include ManagedAccountType::PlatformPayment and
ManagedPlatformAccount and OwnedManagedCoreAccount::Funds in your changes for
discoverability.
In `@key-wallet/src/managed_account/managed_core_funds_account.rs`:
- Around line 768-804: The batch generators next_receive_addresses and
next_change_addresses currently add addresses to the internal/external address
pools but do not advance monitor_revision; update both functions so that if
add_to_state is true and the pool returned more new addresses (addresses.len() >
0) you increment self.monitor_revision (e.g., self.monitor_revision =
self.monitor_revision.wrapping_add(1) or ++) immediately after the pool mutation
and before returning Ok, so callers that rely on monitor_revision to refresh
filters will see the change; locate the logic around
managed_account_type::{external_addresses,internal_addresses} and
address_pool::KeySource to insert this increment.
- Around line 1417-1437: The code currently always builds processed_txids and
spent_outpoints from helper.transactions which is empty when the
keep_txs_in_memory feature is disabled; change the construction so that if
helper.transactions is non-empty you keep the existing logic, but when it's
empty you reconstruct these guards from helper.utxos (and any spent markers
contained therein) to preserve deduping and spent-output protection across
deserialization; update the creation of processed_txids and spent_outpoints used
to initialize ManagedCoreFundsAccount (referencing processed_txids,
spent_outpoints, helper.transactions, helper.utxos, and the
ManagedCoreFundsAccount initializer) to branch accordingly.
In `@key-wallet/src/managed_account/managed_core_keys_account.rs`:
- Around line 1298-1314: The deserializer currently resets processed_txids to
empty when the keep_txs_in_memory feature is off, losing deduplication state;
update the Helper struct to always include a processed_txids field, deserialize
it in Helper::deserialize, and in this function assign
ManagedCoreKeysAccount.processed_txids = helper.processed_txids (instead of
creating a new empty HashSet under #[cfg(not(feature = "keep_txs_in_memory"))]);
keep the existing #[cfg(feature = "keep_txs_in_memory")] path that sets
transactions = helper.transactions but ensure processed_txids is restored from
helper for both feature-on and -off builds.
- Around line 336-415: The helpers that call addresses.next_unused (e.g.,
ManagedCoreKeysAccount::next_address) must bump the account's monitor_revision
whenever the address pool is grown via add_to_state so bloom filters get
rebuilt; after a successful addresses.next_unused(..., add_to_state) return
(i.e., Ok(address)) and only when add_to_state is true, increment
self.monitor_revision (e.g., self.monitor_revision =
self.monitor_revision.wrapping_add(1) or += 1) before returning the Address;
apply the same change to the other similar helpers referenced in the comment
(the other next_* / generator methods around lines 418-495 and 500-571) so all
pool-growth paths update monitor_revision.
In `@key-wallet/src/tests/keep_txs_in_memory_tests.rs`:
- Around line 1-8: Update the module doc comment to use the new type name:
replace mentions of ManagedCoreAccount with ManagedCoreFundsAccount so the
documentation reflects the renamed/split type used in this PR (look for the
top-of-file comment in keep_txs_in_memory_tests.rs that describes the behavior
and change the type reference accordingly).
In `@key-wallet/src/transaction_checking/wallet_checker.rs`:
- Around line 89-94: The InstantSend misclassification occurs because
already_confirmed uses get_transaction() which returns None when
keep_txs_in_memory is false, causing known txs to be treated as new; change the
affected_accounts iteration in WalletChecker (the already_confirmed computation)
to test presence the same way is_new does (use has_transaction() and/or
contains_transaction_record(&txid) on the account rather than
get_transaction()), so you never trigger record_transaction() or rebuild records
when the wallet is configured without in-memory txs; make the same fix to the
other occurrence in the 104-128 block so both paths use the presence checks
(has_transaction/contains_transaction_record) and only call get_transaction()
when those checks indicate the record exists.
---
Outside diff comments:
In `@key-wallet-ffi/src/managed_account.rs`:
- Around line 694-704: The function managed_core_account_get_is_watch_only uses
account.inner().is_watch_only which panics for keys-only accounts; change it to
call the variant-aware accessor FFIManagedCoreAccount::is_watch_only() on the
dereferenced account pointer (after the null check) so both variants are handled
safely and no direct access to inner() is performed.
- Around line 713-733: managed_core_account_get_balance currently dereferences
account.inner().balance which panics for keys-only accounts; update the function
to detect keys-only accounts (e.g., via a method like
account.inner().is_keys_only() or by using an Option-returning accessor such as
account.inner().balance().or_else / balance_opt()) and, when no balance is
available, populate balance_out with a zero FFIBalance (all fields zero) instead
of accessing balance; otherwise fill FFIBalance from the real balance as before.
Ensure you reference FFIManagedCoreAccount, managed_core_account_get_balance,
account.inner(), and crate::types::FFIBalance in the change.
- Around line 594-603: The function managed_core_account_get_network currently
dereferences account.inner().network which will panic for keys-only variants;
replace that direct access with the variant-aware accessor by calling
FFIManagedCoreAccount::network() on the account reference (i.e., use
account.network() or FFIManagedCoreAccount::network(&*account)) after the null
check so it safely returns the correct FFINetwork for both account variants
instead of panicking on keys-only accounts.
- Around line 631-647: The function managed_core_account_get_account_type
currently calls account.inner(), which can panic for keys-only accounts; instead
use the variant-aware accessor FFIManagedCoreAccount::managed_account_type() to
obtain the managed account type (e.g., call
account.managed_account_type().to_account_type()) and derive the index and kind
from that result; remove the inner() usage so keys-only variants won't panic and
keep the same logic for writing to index_out and returning the FFIAccountKind.
- Around line 741-767: Both managed_core_account_get_transaction_count and
managed_core_account_get_utxo_count call account.inner() which panics for
keys-only accounts; change them to safely handle missing inner state and return
0 for keys-only accounts. Replace the direct account.inner() call with a safe
check such as if let Some(inner) = account.inner_opt() {
inner.transactions.len() as c_uint } else { 0 } (or check an
is_keys_only/is_watch_only flag on FFIManagedCoreAccount and return 0), and
apply the same pattern for utxos in managed_core_account_get_utxo_count so
keys-only accounts return 0 instead of panicking.
- Around line 1046-1070: The function managed_core_account_get_transactions
currently dereferences account.inner() which panics for keys-only accounts;
update it to safely handle the absence of an inner account by checking for the
inner account before accessing transactions (e.g., if there is an
is_keys_only()/inner_opt()/inner_ref() accessor, use that or match on
Option/Result), and when no inner exists set *transactions_out =
std::ptr::null_mut(), *count_out = 0 and return true; otherwise proceed to
collect transactions from inner().transactions and fill the outputs as before.
- Around line 1174-1184: managed_core_account_get_index currently dereferences
account.inner().managed_account_type directly which panics for keys-only
accounts; replace that direct field access with the enum-aware accessor on the
inner account (use the provided variant-aware getter for managed_account_type
that safely returns an index or a default) so managed_core_account_get_index
returns the safe index_or_default value for all variants (including keys-only)
instead of causing a panic.
- Around line 1196-1256: The three address-pool getters
(managed_core_account_get_external_address_pool,
managed_core_account_get_internal_address_pool, and
managed_core_account_get_address_pool) currently call account.inner() which
panics for keys-only accounts; change them to avoid inner() by using the
variant-aware accessor (call account.as_funds() or equivalent) and only proceed
to read managed_account.managed_account_type for fund-bearing accounts,
returning std::ptr::null_mut() for keys-only accounts; ensure you replace the
account.inner() usage in each function with the safe variant-aware check so no
panic occurs for keys-only FFIManagedCoreAccount values.
In `@key-wallet/README.md`:
- Around line 204-213: Add the missing feature flag entry for keep_txs_in_memory
to the "Feature Flags" section so users know it's opt-in; update the README's
Feature Flags list to include a bullet describing `keep_txs_in_memory` (what it
enables, e.g., keeping transactions in memory for performance/behavior, and
whether it's enabled by default) near the other flags (`default`, `std`,
`serde`, etc.) so it appears alongside those entries.
In `@key-wallet/src/managed_account/managed_account_collection.rs`:
- Around line 635-769: The conversion loop silently drops failed account
conversions in from_account_collection; change the constructor to return
Result<ManagedAccountCollection, Error> (or a suitable error type) and propagate
errors from create_managed_account_from_account,
create_managed_account_from_bls_account,
create_managed_account_from_eddsa_account, and
create_managed_platform_account_from_account instead of using if let Ok(...).
Update the from_account_collection signature to return Result<Self, Error>,
replace each if let Ok(...) branch with matching that maps Err(e) to an early
return (using ? or map_err) that augments context (include the account index/key
and account kind like "standard_bip44", "provider_operator_keys", etc.), and
ensure feature-gated paths also propagate their errors rather than omitting
accounts.
---
Nitpick comments:
In `@key-wallet-ffi/Cargo.toml`:
- Line 23: The dependency on key-wallet currently hardcodes the
keep_txs_in_memory feature in the key-wallet entry; remove the features =
["keep_txs_in_memory"] fragment from the key-wallet dependency and add a
passthrough feature in this crate's [features] table named keep_txs_in_memory
that enables the upstream feature via keep_txs_in_memory =
["key-wallet/keep_txs_in_memory"] so downstream consumers can opt in/out; update
Cargo.toml to rely on the new feature flag rather than the hardwired dependency
feature.
🪄 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: c4ee71c0-cc86-4af9-88ba-3250b24b096f
📒 Files selected for processing (35)
dash-spv/src/sync/mempool/sync_manager.rsdash-spv/tests/dashd_sync/helpers.rsdash-spv/tests/dashd_sync/setup.rskey-wallet-ffi/Cargo.tomlkey-wallet-ffi/src/address_pool.rskey-wallet-ffi/src/managed_account.rskey-wallet-ffi/src/managed_account_collection.rskey-wallet-ffi/src/managed_wallet.rskey-wallet-ffi/src/utxo_tests.rskey-wallet-manager/Cargo.tomlkey-wallet/CLAUDE.mdkey-wallet/Cargo.tomlkey-wallet/IMPLEMENTATION_SUMMARY.mdkey-wallet/README.mdkey-wallet/src/account/mod.rskey-wallet/src/managed_account/managed_account_collection.rskey-wallet/src/managed_account/managed_account_ref.rskey-wallet/src/managed_account/managed_account_trait.rskey-wallet/src/managed_account/managed_core_funds_account.rskey-wallet/src/managed_account/managed_core_keys_account.rskey-wallet/src/managed_account/managed_platform_account.rskey-wallet/src/managed_account/mod.rskey-wallet/src/test_utils/account.rskey-wallet/src/test_utils/wallet.rskey-wallet/src/tests/balance_tests.rskey-wallet/src/tests/keep_txs_in_memory_tests.rskey-wallet/src/tests/mod.rskey-wallet/src/tests/spent_outpoints_tests.rskey-wallet/src/transaction_checking/account_checker.rskey-wallet/src/transaction_checking/transaction_router/tests/mod.rskey-wallet/src/transaction_checking/wallet_checker.rskey-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rskey-wallet/src/wallet/managed_wallet_info/helpers.rskey-wallet/src/wallet/managed_wallet_info/managed_accounts.rskey-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs
Comment on lines
+124
to
+129
| wallet_info | ||
| .accounts() | ||
| .all_accounts() | ||
| .into_iter() | ||
| .map(|a| a.transactions_iter().count()) | ||
| .sum() |
Contributor
There was a problem hiding this comment.
transaction_count overcounts if a txid appears in multiple accounts.
transaction_count sums per-account transactions_iter().count() while the conceptually equivalent count_wallet_transactions in helpers.rs (lines 65–70) builds a HashSet to count unique txids, and assert_wallet_synced also uses a HashSet. If any transaction is recorded under more than one account, transaction_count will return a higher value, silently diverging from the other two helpers and potentially causing assertion failures.
🔧 Proposed fix
- wallet_info
- .accounts()
- .all_accounts()
- .into_iter()
- .map(|a| a.transactions_iter().count())
- .sum()
+ let txids: std::collections::HashSet<_> = wallet_info
+ .accounts()
+ .all_accounts()
+ .into_iter()
+ .flat_map(|a| a.transactions_iter().map(|(txid, _)| txid))
+ .collect();
+ txids.len()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| wallet_info | |
| .accounts() | |
| .all_accounts() | |
| .into_iter() | |
| .map(|a| a.transactions_iter().count()) | |
| .sum() | |
| let txids: std::collections::HashSet<_> = wallet_info | |
| .accounts() | |
| .all_accounts() | |
| .into_iter() | |
| .flat_map(|a| a.transactions_iter().map(|(txid, _)| txid)) | |
| .collect(); | |
| txids.len() |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dash-spv/tests/dashd_sync/setup.rs` around lines 124 - 129, The current
computation using wallet_info.accounts().all_accounts().into_iter().map(|a|
a.transactions_iter().count()).sum() overcounts when the same txid appears in
multiple accounts; change the logic in the transaction_count helper to collect
txids from each account via a transactions_iter(), insert them into a HashSet to
deduplicate, and return the set.len() instead of summing per-account counts (see
count_wallet_transactions in helpers.rs for the intended pattern and use of
HashSet to count unique txids).
Comment on lines
+19
to
+21
| # Keep per-account transaction history (`ManagedCoreAccount::transactions`) in memory. | ||
| # Disable to skip storing transaction records when only UTXOs and balances matter. | ||
| keep_txs_in_memory = [] |
Contributor
There was a problem hiding this comment.
PR title uses refactor but this PR adds substantial new functionality.
The changes include new types (ManagedAccountRef/ManagedAccountRefMut), a new keep_txs_in_memory Cargo feature, expanded ManagedCoreKeysAccount methods, new FFI constructors/accessors, and new tests — all net-new additions rather than internal restructuring. The conventional commit prefix should be feat, not refactor.
As per coding guidelines: "Flag e.g. if a PR labeled 'fix' appears to add new functionality, if a 'feat' PR only refactors existing code."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@key-wallet/Cargo.toml` around lines 19 - 21, The PR is labeled "refactor" but
adds new public functionality (new types ManagedAccountRef /
ManagedAccountRefMut, a new Cargo feature keep_txs_in_memory, expanded
ManagedCoreKeysAccount methods, new FFI constructors/accessors, and tests);
change the commit/PR labeling to "feat" to reflect new functionality: update the
PR title and any conventional-commit prefixes on the commits (or squash/amend
commits) to use "feat:" and ensure the description mentions the new symbols
(ManagedAccountRef, ManagedAccountRefMut, keep_txs_in_memory,
ManagedCoreKeysAccount, FFI constructors/accessors) so release tooling and
changelogs pick up the feature addition.
| dashcore = { path="../dash", features = ["test-utils"] } | ||
| hex = "0.4" | ||
| key-wallet = { path = ".", features = ["test-utils", "bip38", "serde", "bincode", "eddsa", "bls"] } | ||
| key-wallet = { path = ".", default-features = false, features = ["test-utils", "bip38", "serde", "bincode", "eddsa", "bls", "getrandom"] } |
Contributor
There was a problem hiding this comment.
keep_txs_in_memory absent from the dev-dep self-reference — coinbase tests silently excluded from cargo test.
With Cargo's resolver v2 (the default for edition = "2021"), dev-dependencies do not activate features unless building a Cargo target that needs them (like tests or examples). Since the self-referential dev-dep lists default-features = false and explicit features that omit keep_txs_in_memory, running cargo test directly inside key-wallet will not compile the coinbase test module. The tests will only execute when building downstream crates (key-wallet-ffi, key-wallet-manager) that do enable the feature.
Add "keep_txs_in_memory" to the dev-dep feature list so the tests run during a plain cargo test on this crate:
🔧 Proposed fix
-key-wallet = { path = ".", default-features = false, features = ["test-utils", "bip38", "serde", "bincode", "eddsa", "bls", "getrandom"] }
+key-wallet = { path = ".", default-features = false, features = ["test-utils", "bip38", "serde", "bincode", "eddsa", "bls", "getrandom", "keep_txs_in_memory"] }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| key-wallet = { path = ".", default-features = false, features = ["test-utils", "bip38", "serde", "bincode", "eddsa", "bls", "getrandom"] } | |
| key-wallet = { path = ".", default-features = false, features = ["test-utils", "bip38", "serde", "bincode", "eddsa", "bls", "getrandom", "keep_txs_in_memory"] } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@key-wallet/Cargo.toml` at line 52, The dev-dependency self-reference in
Cargo.toml omits the "keep_txs_in_memory" feature, so running cargo test in
key-wallet doesn't enable the coinbase test module; update the key-wallet
dependency entry (the key-wallet = { path = ".", default-features = false,
features = [...] } line) to include "keep_txs_in_memory" in the features array
so the coinbase tests are compiled and run during a plain cargo test.
Comment on lines
+823
to
+829
| /// Create an [`OwnedManagedCoreAccount`] from an [`AccountType`] plus | ||
| /// network and watch-only status. | ||
| /// | ||
| /// The variant of the returned [`OwnedManagedCoreAccount`] is determined | ||
| /// by `account_type`: identity / asset-lock / provider types build a | ||
| /// [`ManagedCoreKeysAccount`], everything else (Standard, CoinJoin, DashPay, | ||
| /// PlatformPayment) builds a [`ManagedCoreFundsAccount`]. |
Contributor
There was a problem hiding this comment.
PlatformPayment still produces an account variant that insert() cannot accept.
create_managed_account_from_account_type() wraps AccountType::PlatformPayment in OwnedManagedCoreAccount::Funds, but insert_funds() rejects ManagedAccountType::PlatformPayment and requires ManagedPlatformAccount instead. That leaves callers with a value they cannot legally round-trip through the collection API. Please special-case PlatformPayment here and return an error (or split it into a dedicated platform constructor) so this helper only produces insertable variants.
As per coding guidelines "Return Result<T> for all fallible operations and provide context in error messages".
Also applies to: 1063-1075
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@key-wallet/src/managed_account/managed_account_collection.rs` around lines
823 - 829, create_managed_account_from_account_type() currently maps
AccountType::PlatformPayment into OwnedManagedCoreAccount::Funds which cannot be
round-tripped because insert_funds() rejects ManagedAccountType::PlatformPayment
and expects a ManagedPlatformAccount; change
create_managed_account_from_account_type() to special-case
AccountType::PlatformPayment so it does not produce a Funds variant — either
return an Err with a clear context-bearing error (convert the function to return
Result<OwnedManagedCoreAccount, YourError>) or provide a dedicated platform
constructor that returns a ManagedPlatformAccount/OwnedManagedCoreAccount
variant acceptable to insert_funds(); update callers accordingly so all fallible
behavior uses Result and include ManagedAccountType::PlatformPayment and
ManagedPlatformAccount and OwnedManagedCoreAccount::Funds in your changes for
discoverability.
Comment on lines
+768
to
+804
| pub fn next_receive_addresses( | ||
| &mut self, | ||
| account_xpub: Option<&ExtendedPubKey>, | ||
| count: usize, | ||
| add_to_state: bool, | ||
| ) -> Result<Vec<Address>, String> { | ||
| // For standard accounts, use the address pool to get multiple unused addresses | ||
| if let ManagedAccountType::Standard { | ||
| external_addresses, | ||
| .. | ||
| } = &mut self.managed_account_type | ||
| { | ||
| // Create appropriate key source based on whether xpub is provided | ||
| let key_source = match account_xpub { | ||
| Some(xpub) => address_pool::KeySource::Public(*xpub), | ||
| None => address_pool::KeySource::NoKeySource, | ||
| }; | ||
|
|
||
| let addresses = | ||
| external_addresses.next_unused_multiple(count, &key_source, add_to_state); | ||
| if addresses.is_empty() && count > 0 { | ||
| Err("Failed to generate any receive addresses".to_string()) | ||
| } else if addresses.len() < count | ||
| && matches!(key_source, address_pool::KeySource::NoKeySource) | ||
| { | ||
| Err(format!( | ||
| "Could only generate {} out of {} requested addresses (no key source)", | ||
| addresses.len(), | ||
| count | ||
| )) | ||
| } else { | ||
| Ok(addresses) | ||
| } | ||
| } else { | ||
| Err("Cannot generate receive addresses for non-standard account type".to_string()) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Batch address generation should advance monitor_revision.
next_receive_addresses and next_change_addresses can grow the monitored address set, but unlike the single-address helpers they never bump monitor_revision. Any caller that refreshes filters off that counter can keep scanning the old address set and miss payments to the newly generated batch.
Suggested direction
pub fn next_receive_addresses(
&mut self,
account_xpub: Option<&ExtendedPubKey>,
count: usize,
add_to_state: bool,
) -> Result<Vec<Address>, String> {
// For standard accounts, use the address pool to get multiple unused addresses
if let ManagedAccountType::Standard {
external_addresses,
..
} = &mut self.managed_account_type
{
+ let generated_before = external_addresses.stats().total_generated;
let key_source = match account_xpub {
Some(xpub) => address_pool::KeySource::Public(*xpub),
None => address_pool::KeySource::NoKeySource,
};
let addresses =
external_addresses.next_unused_multiple(count, &key_source, add_to_state);
+ if external_addresses.stats().total_generated != generated_before {
+ self.bump_monitor_revision();
+ }
if addresses.is_empty() && count > 0 {Also applies to: 811-847
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@key-wallet/src/managed_account/managed_core_funds_account.rs` around lines
768 - 804, The batch generators next_receive_addresses and next_change_addresses
currently add addresses to the internal/external address pools but do not
advance monitor_revision; update both functions so that if add_to_state is true
and the pool returned more new addresses (addresses.len() > 0) you increment
self.monitor_revision (e.g., self.monitor_revision =
self.monitor_revision.wrapping_add(1) or ++) immediately after the pool mutation
and before returning Ok, so callers that rely on monitor_revision to refresh
filters will see the change; locate the logic around
managed_account_type::{external_addresses,internal_addresses} and
address_pool::KeySource to insert this increment.
Comment on lines
+1417
to
+1437
| let processed_txids: HashSet<Txid> = helper.transactions.keys().copied().collect(); | ||
| let spent_outpoints = helper | ||
| .transactions | ||
| .values() | ||
| .flat_map(|record| &record.transaction.input) | ||
| .map(|input| input.previous_output) | ||
| .collect(); | ||
|
|
||
| Ok(ManagedCoreFundsAccount { | ||
| managed_account_type: helper.managed_account_type, | ||
| network: helper.network, | ||
| metadata: helper.metadata, | ||
| is_watch_only: helper.is_watch_only, | ||
| balance: helper.balance, | ||
| #[cfg(feature = "keep_txs_in_memory")] | ||
| transactions: helper.transactions, | ||
| utxos: helper.utxos, | ||
| processed_txids, | ||
| spent_outpoints, | ||
| monitor_revision: 0, | ||
| }) |
Contributor
There was a problem hiding this comment.
Feature-off deserialization drops the dedup and spent-output guards.
When keep_txs_in_memory is disabled, helper.transactions comes back empty here, so both processed_txids and spent_outpoints are rebuilt as empty even if utxos already reflect previously processed state. After a reload, has_transaction() stops deduping historical txs, and the out-of-order rescan protection can reintroduce outputs that were already spent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@key-wallet/src/managed_account/managed_core_funds_account.rs` around lines
1417 - 1437, The code currently always builds processed_txids and
spent_outpoints from helper.transactions which is empty when the
keep_txs_in_memory feature is disabled; change the construction so that if
helper.transactions is non-empty you keep the existing logic, but when it's
empty you reconstruct these guards from helper.utxos (and any spent markers
contained therein) to preserve deduping and spent-output protection across
deserialization; update the creation of processed_txids and spent_outpoints used
to initialize ManagedCoreFundsAccount (referencing processed_txids,
spent_outpoints, helper.transactions, helper.utxos, and the
ManagedCoreFundsAccount initializer) to branch accordingly.
Comment on lines
+336
to
+415
| /// Generate the next address for non-standard accounts | ||
| pub fn next_address( | ||
| &mut self, | ||
| account_xpub: Option<&ExtendedPubKey>, | ||
| add_to_state: bool, | ||
| ) -> Result<Address, &'static str> { | ||
| match &mut self.managed_account_type { | ||
| ManagedAccountType::Standard { | ||
| .. | ||
| } => Err("Standard accounts must use next_receive_address or next_change_address"), | ||
| ManagedAccountType::CoinJoin { | ||
| addresses, | ||
| .. | ||
| } | ||
| | ManagedAccountType::IdentityRegistration { | ||
| addresses, | ||
| .. | ||
| } | ||
| | ManagedAccountType::IdentityTopUpNotBoundToIdentity { | ||
| addresses, | ||
| .. | ||
| } | ||
| | ManagedAccountType::IdentityInvitation { | ||
| addresses, | ||
| .. | ||
| } | ||
| | ManagedAccountType::AssetLockAddressTopUp { | ||
| addresses, | ||
| .. | ||
| } | ||
| | ManagedAccountType::AssetLockShieldedAddressTopUp { | ||
| addresses, | ||
| .. | ||
| } | ||
| | ManagedAccountType::ProviderVotingKeys { | ||
| addresses, | ||
| .. | ||
| } | ||
| | ManagedAccountType::ProviderOwnerKeys { | ||
| addresses, | ||
| .. | ||
| } | ||
| | ManagedAccountType::ProviderOperatorKeys { | ||
| addresses, | ||
| .. | ||
| } | ||
| | ManagedAccountType::ProviderPlatformKeys { | ||
| addresses, | ||
| .. | ||
| } | ||
| | ManagedAccountType::DashpayReceivingFunds { | ||
| addresses, | ||
| .. | ||
| } | ||
| | ManagedAccountType::DashpayExternalAccount { | ||
| addresses, | ||
| .. | ||
| } | ||
| | ManagedAccountType::PlatformPayment { | ||
| addresses, | ||
| .. | ||
| } | ||
| | ManagedAccountType::IdentityTopUp { | ||
| addresses, | ||
| .. | ||
| } => { | ||
| let key_source = match account_xpub { | ||
| Some(xpub) => address_pool::KeySource::Public(*xpub), | ||
| None => address_pool::KeySource::NoKeySource, | ||
| }; | ||
|
|
||
| addresses.next_unused(&key_source, add_to_state).map_err(|e| match e { | ||
| crate::error::Error::NoKeySource => { | ||
| "No unused addresses available and no key source provided" | ||
| } | ||
| _ => "Failed to generate address", | ||
| }) | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Keys-account generators need to bump monitor_revision on pool growth.
These helpers can add fresh identity/provider addresses into the monitored set, but none of them update monitor_revision. That leaves bloom-filter staleness detection blind right after a caller provisions a new key or address.
Suggested direction
} => {
+ let generated_before = addresses.stats().total_generated;
let key_source = match account_xpub {
Some(xpub) => address_pool::KeySource::Public(*xpub),
None => address_pool::KeySource::NoKeySource,
};
- addresses.next_unused(&key_source, add_to_state).map_err(|e| match e {
+ let address = addresses.next_unused(&key_source, add_to_state).map_err(|e| match e {
crate::error::Error::NoKeySource => {
"No unused addresses available and no key source provided"
}
_ => "Failed to generate address",
- })
+ })?;
+ if addresses.stats().total_generated != generated_before {
+ self.bump_monitor_revision();
+ }
+ Ok(address)
}Also applies to: 418-495, 500-571
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@key-wallet/src/managed_account/managed_core_keys_account.rs` around lines 336
- 415, The helpers that call addresses.next_unused (e.g.,
ManagedCoreKeysAccount::next_address) must bump the account's monitor_revision
whenever the address pool is grown via add_to_state so bloom filters get
rebuilt; after a successful addresses.next_unused(..., add_to_state) return
(i.e., Ok(address)) and only when add_to_state is true, increment
self.monitor_revision (e.g., self.monitor_revision =
self.monitor_revision.wrapping_add(1) or += 1) before returning the Address;
apply the same change to the other similar helpers referenced in the comment
(the other next_* / generator methods around lines 418-495 and 500-571) so all
pool-growth paths update monitor_revision.
Comment on lines
+1298
to
+1314
| let helper = Helper::deserialize(deserializer)?; | ||
|
|
||
| #[cfg(feature = "keep_txs_in_memory")] | ||
| let processed_txids: HashSet<Txid> = helper.transactions.keys().copied().collect(); | ||
| #[cfg(not(feature = "keep_txs_in_memory"))] | ||
| let processed_txids: HashSet<Txid> = HashSet::new(); | ||
|
|
||
| Ok(ManagedCoreKeysAccount { | ||
| managed_account_type: helper.managed_account_type, | ||
| network: helper.network, | ||
| metadata: helper.metadata, | ||
| is_watch_only: helper.is_watch_only, | ||
| #[cfg(feature = "keep_txs_in_memory")] | ||
| transactions: helper.transactions, | ||
| processed_txids, | ||
| monitor_revision: 0, | ||
| }) |
Contributor
There was a problem hiding this comment.
Reloading a keys account forgets every processed txid in feature-off builds.
With keep_txs_in_memory off, deserialization hard-resets processed_txids to an empty set. After restoring wallet state, every previously seen identity/provider transaction looks new again, so deduplication and is_new_transaction classification stop working across restarts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@key-wallet/src/managed_account/managed_core_keys_account.rs` around lines
1298 - 1314, The deserializer currently resets processed_txids to empty when the
keep_txs_in_memory feature is off, losing deduplication state; update the Helper
struct to always include a processed_txids field, deserialize it in
Helper::deserialize, and in this function assign
ManagedCoreKeysAccount.processed_txids = helper.processed_txids (instead of
creating a new empty HashSet under #[cfg(not(feature = "keep_txs_in_memory"))]);
keep the existing #[cfg(feature = "keep_txs_in_memory")] path that sets
transactions = helper.transactions but ensure processed_txids is restored from
helper for both feature-on and -off builds.
Comment on lines
+1
to
+8
| //! Tests for the `keep_txs_in_memory` Cargo feature. | ||
| //! | ||
| //! When the feature is enabled (the default), `ManagedCoreAccount` keeps a | ||
| //! per-account `transactions` map populated as transactions are processed. | ||
| //! When the feature is disabled, the field does not exist and processing a | ||
| //! transaction only updates UTXOs and balance — but `processed_txids` | ||
| //! still tracks which txids have been seen so `confirm_transaction` | ||
| //! continues to deduplicate re-events. |
Contributor
There was a problem hiding this comment.
Stale type name in module doc comment.
Line 3 refers to ManagedCoreAccount, which was split and renamed to ManagedCoreFundsAccount in this PR.
📝 Proposed fix
-//! When the feature is enabled (the default), `ManagedCoreAccount` keeps a
-//! per-account `transactions` map populated as transactions are processed.
+//! When the feature is enabled (the default), `ManagedCoreFundsAccount` keeps a
+//! per-account `transactions` map populated as transactions are processed.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| //! Tests for the `keep_txs_in_memory` Cargo feature. | |
| //! | |
| //! When the feature is enabled (the default), `ManagedCoreAccount` keeps a | |
| //! per-account `transactions` map populated as transactions are processed. | |
| //! When the feature is disabled, the field does not exist and processing a | |
| //! transaction only updates UTXOs and balance — but `processed_txids` | |
| //! still tracks which txids have been seen so `confirm_transaction` | |
| //! continues to deduplicate re-events. | |
| //! Tests for the `keep_txs_in_memory` Cargo feature. | |
| //! | |
| //! When the feature is enabled (the default), `ManagedCoreFundsAccount` keeps a | |
| //! per-account `transactions` map populated as transactions are processed. | |
| //! When the feature is disabled, the field does not exist and processing a | |
| //! transaction only updates UTXOs and balance — but `processed_txids` | |
| //! still tracks which txids have been seen so `confirm_transaction` | |
| //! continues to deduplicate re-events. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@key-wallet/src/tests/keep_txs_in_memory_tests.rs` around lines 1 - 8, Update
the module doc comment to use the new type name: replace mentions of
ManagedCoreAccount with ManagedCoreFundsAccount so the documentation reflects
the renamed/split type used in this PR (look for the top-of-file comment in
keep_txs_in_memory_tests.rs that describes the behavior and change the type
reference accordingly).
Comment on lines
89
to
94
| 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)) | ||
| .and_then(|a| a.get_transaction(&txid)) | ||
| .map_or(false, |r| r.is_confirmed()) | ||
| }); |
Contributor
There was a problem hiding this comment.
The InstantSend replay path misclassifies existing txs when tx history is off.
is_new is based on has_transaction(), but this branch switches to get_transaction() and contains_transaction_record(), both of which collapse when keep_txs_in_memory is disabled. A known tx then falls back through record_transaction() again, which can emit spurious new_records, rebuild record details from post-update state, and stops the stale-confirmed IS guard from working.
Suggested direction
- let has_record = account.contains_transaction_record(&txid);
-
- if has_record {
+ let already_processed = account.has_transaction(&txid);
+
+ if already_processed {
account.mark_utxos_instant_send(&txid);
#[cfg(feature = "keep_txs_in_memory")]
if let Some(record) =
account.update_transaction_record_context(&txid, context.clone())
{
result.updated_records.push(record);
}
} else {Also applies to: 104-128
🤖 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 89 - 94,
The InstantSend misclassification occurs because already_confirmed uses
get_transaction() which returns None when keep_txs_in_memory is false, causing
known txs to be treated as new; change the affected_accounts iteration in
WalletChecker (the already_confirmed computation) to test presence the same way
is_new does (use has_transaction() and/or contains_transaction_record(&txid) on
the account rather than get_transaction()), so you never trigger
record_transaction() or rebuild records when the wallet is configured without
in-memory txs; make the same fix to the other occurrence in the 104-128 block so
both paths use the presence checks (has_transaction/contains_transaction_record)
and only call get_transaction() when those checks indicate the record exists.
github-actions
Bot
added
the
merge-conflict
Contributor
|
This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them. |
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
PR 2 of the two-part
ManagedCoreAccountsplit (PR 1 was #711). Promotes the dead-codeManagedCoreKeysAccountintroduced in #711 into the active type for accounts that derive special-purpose keys but don't track funds.ManagedAccountCollectionmove fromManagedCoreFundsAccount→ManagedCoreKeysAccount.ManagedCoreFundsAccount.get_by_account_type_match,all_accounts, …) return a newManagedAccountRef<'a> { Funds, Keys }enum (mutable counterpartManagedAccountRefMut<'a>) with delegating helpers, so callers that don't care about the variant don't need to match.wallet/managed_wallet_infoidentity/asset-lock/provider accessors now return&mut ManagedCoreKeysAccount. Funds-only surfaces (account_balances,transaction_history,update_balance,mark_instant_send_utxos, matured-coinbase/immature) filter to the funds variant.Scope
key-wallet/src/managed_account/managed_account_ref.rs(~516 lines) — the borrowed enum + delegating impls.ManagedCoreKeysAccount(~645 lines added) withrecord_transaction/confirm_transaction(simplified — no UTXO/balance work),processed_txidsdedup set, feature-gatedtransactionsmirroring the funds-account pattern, and thecheck_*_for_match/classify_addressmethods previously on the funds account.ManagedAccountTraitstays funds-only; onlyManagedCoreFundsAccountimpls it.ManagedAccountCollection::inserttakes an ownedOwnedManagedCoreAccount { Funds, Keys }enum;insert_funds/insert_keysare exposed for callers that statically know the variant.C ABI preserved
FFIManagedCoreAccountnow internally wraps eitherArc<ManagedCoreFundsAccount>orArc<ManagedCoreKeysAccount>via a private enum, with newas_funds/as_keysaccessors and anew_keysconstructor. The legacyinner()helper panics on a keys-only handle, but it's only reachable from funds-only FFI functions today; the variant-aware entry points (managed_wallet_get_managed_accountand friends) create the right kind of handle.A follow-up should either migrate every funds-only FFI function to call
as_funds()and return a structured "not applicable" error code, or add a parallelFFIManagedCoreKeysAccountC surface for keys-account-only methods (next_address,next_path, etc.). Filing as a follow-up issue.Test plan
cargo check --workspace --all-targets— cleancargo check --workspace --all-targets --features key-wallet/keep_txs_in_memory— cleancargo clippy --workspace --all-targets --all-features -- -D warnings— clean (one auto-deref hint fixed in a follow-on commit before push)cargo fmt --all -- --check— cleanRUSTDOCFLAGS="-D warnings" cargo doc --workspace --all-features --no-deps— cleancargo test -p key-wallet --lib— 434 passed, 1 failedThe single failing test is
tests::performance_tests::test_wallet_serialization_performance. It's a known-flaky debug-build timing assertion (assert!(metrics.avg_time < Duration::from_millis(50))) that fails under parallel-test load and passes in isolation. Same threshold history that prompted #663. Not a regression from this PR — confirmed by running it standalone (1 passed, 0 failed) and by the agent observing the same failure before any PR-2 changes were applied. May want to file a separate ticket to relax further or reduce test parallelism.Follow-ups (not addressed here)
check_asset_lock_transaction_for_matchand the fourcheck_provider_*_key_in_transaction_for_matchimpls remain onManagedCoreFundsAccountbut are now only invoked onManagedCoreKeysAccount. Removable in a cleanup pass; left in to keep this PR additive on the funds side.monitor_revisionbumping (key-wallet: bumpmonitor_revisionin all pool-mutating helpers onManagedCoreFundsAccount#713) andconfirm_transactionrescan metadata (key-wallet:confirm_transactionshould refreshTransactionRecordderived metadata on gap-limit rescan #714) carry over unchanged.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
keep_txs_in_memoryfeature flag to control whether per-account transaction history is retained in memory, enabling reduced memory usage when transaction history is not needed.Documentation
Refactor