feat(key-wallet): add keep_txs_in_memory Cargo feature#709
Open
QuantumExplorer wants to merge 5 commits intov0.42-devfrom
Open
feat(key-wallet): add keep_txs_in_memory Cargo feature#709QuantumExplorer wants to merge 5 commits intov0.42-devfrom
QuantumExplorer wants to merge 5 commits intov0.42-devfrom
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
✅ Files skipped from review due to trivial changes (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughPer-account in-memory transaction retention for ManagedCoreAccount is now controlled by a new Cargo feature ChangesIn-memory transaction feature gating (single cohort)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 👉 Get your free trial and get 200 agent minutes per Slack user (a $50 value). 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v0.42-dev #709 +/- ##
=============================================
+ Coverage 70.57% 70.69% +0.12%
=============================================
Files 319 319
Lines 68211 68225 +14
=============================================
+ Hits 48142 48235 +93
+ Misses 20069 19990 -79
|
![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: 1
🧹 Nitpick comments (1)
key-wallet/src/tests/keep_txs_in_memory_tests.rs (1)
52-75: ⚡ Quick winAdd a re-confirmation idempotency test to lock in correct behavior.
The existing test verifies single-pass behavior well, but there's no coverage for calling
confirm_transaction(or the equivalent through the wallet pipeline) a second time for the same transaction whenkeep_txs_in_memoryisfalse. Such a test would catch the deduplication regression identified above and serve as a regression guard once it's fixed.📝 Suggested additional test
#[tokio::test] async fn confirm_transaction_is_idempotent_when_disabled() { let mut ctx = TestWalletContext::new_random(); ctx.managed_wallet .first_bip44_managed_account_mut() .expect("Should have BIP44 account") .set_keep_txs_in_memory(false); let tx = Transaction::dummy(&ctx.receive_address, 0..1, &[200_000]); // First pass: mempool let r1 = ctx.check_transaction(&tx, TransactionContext::Mempool).await; assert!(r1.is_relevant); // Second pass: block confirmation of the same tx let r2 = ctx.check_transaction(&tx, TransactionContext::InBlock(/* block info */)).await; let account = ctx.bip44_account(); // UTXO count must not double assert_eq!(account.utxos.len(), 1); // Balance must remain the same assert_eq!(account.balance.spendable(), 200_000); // monitor_revision must not be bumped on the second pass if no UTXO actually changed // (once the deduplication bug is fixed) }🤖 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 52 - 75, Add an idempotency test that confirms calling ctx.check_transaction twice for the same Transaction (first with TransactionContext::Mempool, then with TransactionContext::InBlock) does not double-count UTXOs or change balance when keep_txs_in_memory is false: create a TestWalletContext, call .first_bip44_managed_account_mut().set_keep_txs_in_memory(false), build the same Transaction::dummy, call ctx.check_transaction(&tx, TransactionContext::Mempool).await then ctx.check_transaction(&tx, TransactionContext::InBlock(...)).await, then assert on bip44_account().utxos.len() == 1 and bip44_account().balance.spendable() == 200_000 and (if available) that bip44_account().monitor_revision was not incremented on the second pass; name the test confirm_transaction_is_idempotent_when_disabled and add it to keep_txs_in_memory_tests.rs.
🤖 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/src/managed_account/mod.rs`:
- Around line 594-596: The deduplication bug occurs because confirm_transaction
currently checks self.transactions.contains_key(&tx.txid()) which is empty when
keep_txs_in_memory is false; add a new field processed_txids: HashSet<Txid>
(annotated like spent_outpoints with serde(skip_serializing)) to track seen
txids independent of stored transactions, always insert tx.txid() into
processed_txids inside record_transaction (even when keep_txs_in_memory is
false), change confirm_transaction to use processed_txids.contains(&tx.txid())
as the early-return guard, initialize processed_txids = HashSet::new() in new()
and reconstruct it from helper.transactions during Deserialize (same place
transactions and spent_outpoints are rebuilt) so behavior is consistent after
load.
---
Nitpick comments:
In `@key-wallet/src/tests/keep_txs_in_memory_tests.rs`:
- Around line 52-75: Add an idempotency test that confirms calling
ctx.check_transaction twice for the same Transaction (first with
TransactionContext::Mempool, then with TransactionContext::InBlock) does not
double-count UTXOs or change balance when keep_txs_in_memory is false: create a
TestWalletContext, call
.first_bip44_managed_account_mut().set_keep_txs_in_memory(false), build the same
Transaction::dummy, call ctx.check_transaction(&tx,
TransactionContext::Mempool).await then ctx.check_transaction(&tx,
TransactionContext::InBlock(...)).await, then assert on
bip44_account().utxos.len() == 1 and bip44_account().balance.spendable() ==
200_000 and (if available) that bip44_account().monitor_revision was not
incremented on the second pass; name the test
confirm_transaction_is_idempotent_when_disabled and add it to
keep_txs_in_memory_tests.rs.
🪄 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: 31ffa800-a57f-42b1-886b-975dd1a4bcff
📒 Files selected for processing (3)
key-wallet/src/managed_account/mod.rskey-wallet/src/tests/keep_txs_in_memory_tests.rskey-wallet/src/tests/mod.rs
QuantumExplorer
changed the title
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
key-wallet/src/managed_account/mod.rs (1)
103-117:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDeduplication is still broken when
keep_txs_in_memoryis disabled.Line 114-Line 115 makes
has_transactionalwaysfalse, and Line 506-Line 507 makesconfirm_transactionalways re-record and report a change. The same tx replay (e.g., mempool → block, rescans) is treated as new repeatedly, which can inflate wallet-level state-change signaling and transaction counters downstream.🔧 Fix direction (dedupe independent of tx record retention)
pub struct ManagedCoreAccount { + /// Txids processed by this account regardless of record retention policy. + #[cfg_attr(feature = "serde", serde(skip_serializing))] + processed_txids: HashSet<Txid>, #[cfg(feature = "keep_txs_in_memory")] pub transactions: BTreeMap<Txid, TransactionRecord>, ... } impl ManagedCoreAccount { pub fn has_transaction(&self, txid: &Txid) -> bool { - #[cfg(feature = "keep_txs_in_memory")] - { self.transactions.contains_key(txid) } - #[cfg(not(feature = "keep_txs_in_memory"))] - { let _ = txid; false } + self.processed_txids.contains(txid) } pub(crate) fn confirm_transaction(...) -> bool { - #[cfg(feature = "keep_txs_in_memory")] - { - if !self.transactions.contains_key(&tx.txid()) { ... } - ... - } - #[cfg(not(feature = "keep_txs_in_memory"))] - { - self.record_transaction(...); - true - } + if !self.processed_txids.contains(&tx.txid()) { + self.record_transaction(...); + return true; + } + // existing context/utxo update logic } pub(crate) fn record_transaction(...) -> TransactionRecord { + self.processed_txids.insert(tx.txid()); #[cfg(feature = "keep_txs_in_memory")] self.transactions.insert(tx.txid(), tx_record); ... } }Also applies to: 466-508
🤖 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 103 - 117, has_transaction currently returns false when keep_txs_in_memory is disabled, causing confirm_transaction to always treat repeats as new; add a lightweight persistent "seen txid" pathway so deduplication works regardless of tx record retention: introduce a HashSet<FieldName like seen_txids> on ManagedAccount (or reuse an existing persistent index/map), update has_transaction to check seen_txids under the #[cfg(not(feature = "keep_txs_in_memory"))] branch, and modify confirm_transaction to consult seen_txids before re-recording/reporting a change and to insert the txid into seen_txids when first processed so subsequent mempool→block or rescan events are deduplicated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@key-wallet/src/managed_account/mod.rs`:
- Around line 103-117: has_transaction currently returns false when
keep_txs_in_memory is disabled, causing confirm_transaction to always treat
repeats as new; add a lightweight persistent "seen txid" pathway so
deduplication works regardless of tx record retention: introduce a
HashSet<FieldName like seen_txids> on ManagedAccount (or reuse an existing
persistent index/map), update has_transaction to check seen_txids under the
#[cfg(not(feature = "keep_txs_in_memory"))] branch, and modify
confirm_transaction to consult seen_txids before re-recording/reporting a change
and to insert the txid into seen_txids when first processed so subsequent
mempool→block or rescan events are deduplicated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5c09014e-2a07-44d5-85c9-24a384785a1b
📒 Files selected for processing (9)
key-wallet/Cargo.tomlkey-wallet/src/managed_account/managed_account_trait.rskey-wallet/src/managed_account/mod.rskey-wallet/src/test_utils/wallet.rskey-wallet/src/tests/keep_txs_in_memory_tests.rskey-wallet/src/tests/mod.rskey-wallet/src/transaction_checking/transaction_router/tests/mod.rskey-wallet/src/transaction_checking/wallet_checker.rskey-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- key-wallet/src/tests/keep_txs_in_memory_tests.rs
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>
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. |
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>
QuantumExplorer
force-pushed
the
claude/gracious-dirac-44a53b
branch
from
2d38624 to
ce2ba4b
Compare
github-actions
Bot
removed
the
merge-conflict
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
keep_txs_in_memory(NOT enabled by default) that gates theManagedCoreAccount::transactionsfield at compile time. With the feature off, the field doesn't exist and processing transactions only updates UTXOs and balance.key-wallet-ffienables the feature explicitly to keep its FFI surface unchanged. Other crates can opt in the same way.ManagedCoreAccount::has_transaction(&Txid)/get_transaction(&Txid)helpers that work in both configurations (returningfalse/Nonewhen the feature is off) so call sites stay readable.wallet_checkerandwallet_info_interfacethrough the helpers or#[cfg]blocks. Wallet-level history queries (transaction_history,immature_transactions,matured_coinbase_records) return emptyVecs when the feature is off.ManagedAccountTrait::transactions()/transactions_mut()are also feature-gated.key-walletdev-dep self-reference asdefault-features = falseso the feature can actually be disabled when running tests.Test plan
cargo build --workspace(default features)cargo test --workspace --no-runbuilds all tests across the workspacecargo test -p key-wallet --lib(feature off) — 434 passedcargo test -p key-wallet --features keep_txs_in_memory --lib— 464 passedcargo test -p key-wallet --all-features --lib— 469 passedcargo clippy -p key-wallet --testsand--features keep_txs_in_memory --tests— cleancargo clippy --workspace --tests— clean🤖 Generated with Claude Code
Summary by CodeRabbit