feat(key-wallet): add keep-finalized-transactions Cargo feature#733
feat(key-wallet): add keep-finalized-transactions Cargo feature#733QuantumExplorer merged 4 commits intov0.42-devfrom
Conversation
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR introduces a ChangesKeep-Finalized-Transactions Feature and Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
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 unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v0.42-dev #733 +/- ##
=============================================
- Coverage 71.43% 71.26% -0.18%
=============================================
Files 320 320
Lines 68770 68798 +28
=============================================
- Hits 49128 49028 -100
- Misses 19642 19770 +128
|
|
This replaces #709 |
|
This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them. |
github-actions
Bot
added
the
merge-conflict
xdustinface
left a comment
xdustinface
left a comment
There was a problem hiding this comment.
This will lead to missing out on block confirmations for transactions. We should not see transactions as "finalized" or however you wanna call it until they are confirmed fully confirmed via a chainlock (either at the height they are mined or a later height if the confiming block didnt receive a chainlock) i think.
| // path so e.g. an IS-locked record can transition to ChainLock | ||
| // when the surrounding block confirmation arrives. | ||
| #[cfg(not(feature = "keep-finalized-transactions"))] | ||
| if self.keys.transaction_is_finalized(&txid) { |
There was a problem hiding this comment.
So you will never know when transaction gets confirmed in chain now?
| // the `#[allow(unused_variables)]` keeps clippy happy when the | ||
| // `keep-finalized-transactions` feature is on (no `let _ = ...` | ||
| // cfg shim required). | ||
| #[allow(unused_variables)] |
There was a problem hiding this comment.
Why not use the feature gate here too?
| // the `#[allow(unused_variables)]` keeps clippy happy when the | ||
| // `keep-finalized-transactions` feature is on (no `let _ = ...` | ||
| // cfg shim required). | ||
| #[allow(unused_variables)] |
| { | ||
| return self.finalized_txids.contains(txid); | ||
| } | ||
| #[allow(unreachable_code)] |
There was a problem hiding this comment.
This seems a bit strange.. should use the feature gate properly?
QuantumExplorer
force-pushed
the
claude/keep-finalized-transactions
branch
from
afe76d9 to
a7fca52
Compare
github-actions
Bot
removed
the
merge-conflict
xdustinface
left a comment
xdustinface
left a comment
There was a problem hiding this comment.
Just a side note, we can still get this PR still in for now i guess but at the moment this will not work really because chainlocks are not propagated/processed in the wallet at this point. There is some work required first to wire them up and some thoughts need to be put in how we deal with historical transactions and if we want to emit events for all of them on after initial sync after the first received chainlock.
Also left few more comments, unused code should be dropped, some renaming we should do i think and the proper feature usage probably makes sense to do too.
| /// Use [`Self::transaction_is_finalized_in_block`] for the stricter | ||
| /// "fully confirmed in a chainlocked block" answer that drives | ||
| /// memory-pruning decisions. | ||
| fn transaction_is_finalized(&self, txid: &Txid) -> bool; |
| /// height / block hash before the chainlock catches up). Only | ||
| /// `InChainLockedBlock` qualifies. This is the trigger for dropping | ||
| /// the full record under the default feature configuration. | ||
| fn transaction_is_finalized_in_block(&self, txid: &Txid) -> bool; |
There was a problem hiding this comment.
| fn transaction_is_finalized_in_block(&self, txid: &Txid) -> bool; | |
| fn transaction_is_finalized(&self, txid: &Txid) -> bool; |
This should probably be just called transaction_is_finalized then now when the original one gets dropped to align with the naming of the feature.
| /// Returns `true` if this account has already processed `txid`, | ||
| /// whether it's still mutable in `transactions` or has been | ||
| /// finalized-and-pruned (under the default feature configuration). | ||
| /// Used as the dedup signal in `confirm_transaction`. |
There was a problem hiding this comment.
| /// Used as the dedup signal in `confirm_transaction`. |
This seems off? How does it dedup there?
| // the `#[allow(unused_variables)]` keeps clippy happy when the | ||
| // `keep-finalized-transactions` feature is on (no `let _ = ...` | ||
| // cfg shim required). | ||
| #[allow(unused_variables)] |
| // the `#[allow(unused_variables)]` keeps clippy happy when the | ||
| // `keep-finalized-transactions` feature is on (no `let _ = ...` | ||
| // cfg shim required). | ||
| #[allow(unused_variables)] |
| { | ||
| return self.finalized_txids.contains(txid); | ||
| } | ||
| #[allow(unreachable_code)] |
| { | ||
| return self.finalized_txids.contains(txid); | ||
| } | ||
| #[allow(unreachable_code)] |
There was a problem hiding this comment.
Proper use of the feature gate?
| /// ChainLock as final. Use [`Self::is_finalized_in_block`] for the | ||
| /// stricter "fully confirmed in a chainlocked block" answer that | ||
| /// drives memory-pruning decisions. | ||
| pub fn is_finalized(&self) -> bool { |
There was a problem hiding this comment.
This is unused, should be dropped.
| /// block confirmation may still arrive and write the height / | ||
| /// block hash before the chainlock catches up). Only | ||
| /// `InChainLockedBlock` qualifies. | ||
| pub fn is_finalized_in_block(&self) -> bool { |
There was a problem hiding this comment.
| pub fn is_finalized_in_block(&self) -> bool { | |
| pub fn is_chain_locked(&self) -> bool { |
We have is_instant_send above so i think we should go with ^ here.
By default, records of chainlocked transactions are now dropped from each managed account's in-memory `transactions` map; only their txids are retained (in a new `finalized_txids` set on `ManagedCoreKeysAccount`) to keep dedup, `has_transaction`, and finality queries working. The opt-in `keep-finalized-transactions` Cargo feature reverts to the old behavior — every processed transaction stays in the map for the wallet's lifetime. The drop is driven off `TransactionContext::is_finalized_in_block` (chainlock only), not `is_finalized` (chainlock or IS-lock), so IS-locked records survive long enough to absorb the surrounding block-confirmation event (xdustinface review on #709). `confirm_transaction` now returns `Option<TransactionRecord>` so callers always observe the record even when it's about to be dropped. The feature is forwarded through `key-wallet-manager` and `key-wallet-ffi`. Three FFI accessors that walk the full `transactions` map (`managed_core_account_get_transaction_count`, `managed_core_account_get_transactions`, `managed_core_account_free_transactions`) are gated to the feature because they would otherwise return a partial history. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Integration tests under `dash-spv-ffi/tests/dashd_sync/` walk the full per-account transaction history (`managed_core_account_get_transactions`, `managed_core_account_get_transaction_count`, `managed_core_account_free_transactions`) to verify end-to-end wallet sync against a regtest dashd. These accessors are gated behind the `keep-finalized-transactions` feature on `key-wallet-ffi`, so under the default feature set they aren't compiled in and the test build fails to resolve the imports. Add `key-wallet-ffi` as a dev-dependency with the feature enabled. Cargo unifies features across the build graph at test time, which makes the gated accessors available in the test binaries without expanding the lib crate's default feature surface. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Address xdustinface review feedback on PR #733: - Drop `TransactionContext::is_finalized()` (the soft IS-or-chainlock check). The wallet now treats only a chainlock as finality. - Rename `TransactionContext::is_finalized_in_block()` to `is_chain_locked()` so the predicate name describes the variant rather than overloading "finalized" — same naming style as the existing `is_instant_send()` helper. - Drop `ManagedAccountTrait::transaction_is_finalized()` (the unused soft-finality variant) and rename `transaction_is_finalized_in_block()` to `transaction_is_finalized()`. Now that there's a single finalization concept, the trait method name aligns with the feature name. - Replace runtime `if cfg!(...) { ... } else { ... }` branches in `ManagedCoreKeysAccount::has_transaction` and `transaction_is_finalized` with two `#[cfg]`-gated function bodies — one per feature configuration. Same for the chainlock-driven drop call in `confirm_transaction` / `record_transaction`: the `let drop_now = …` binding now only exists when the feature is off, removing the `#[allow(unused_variables)]` workaround. - Rewrite the doc on `has_transaction` so it's clear what it actually reports (live record OR finalized-txid marker) and how callers use it (distinguish brand-new sightings from re-processings) instead of the misleading "dedup signal in `confirm_transaction`" wording. Drop logic still triggers on the strict `is_chain_locked()` check — IS-locked-but-not-yet-chainlocked records still survive so the surrounding block-confirmation event can populate height / block hash before the chainlock catches up. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
QuantumExplorer
force-pushed
the
claude/keep-finalized-transactions
branch
from
2f0f2e9 to
eb5afa2
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
key-wallet/src/tests/keep_finalized_transactions_tests.rs (1)
1-155: ⚡ Quick winAlign this new test module with the repository’s required test-module layout.
Please move/merge these cases into one of the prescribed
key-wallet/src/testsmodules (e.g., transaction-focused module) instead of introducing a new top-level test filename.As per coding guidelines,
key-wallet/src/tests/**/*.rs: "Organize unit tests by functionality into separate test modules: account_tests.rs, address_pool_tests.rs, transaction_tests.rs, wallet_tests.rs, integration_tests.rs, balance_tests.rs, spent_outpoints_tests.rs, special_transaction_tests.rs, advanced_transaction_tests.rs, managed_account_collection_tests.rs, backup_restore_tests.rs, edge_case_tests.rs, performance_tests.rs".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@key-wallet/src/tests/keep_finalized_transactions_tests.rs` around lines 1 - 155, You created a new top-level test file with several transaction-focused tests (test_chainlocked_record_kept_when_feature_on, test_chainlocked_record_dropped_when_feature_off, test_islocked_record_kept_when_feature_off, test_islocked_then_chainlocked_drops_at_chainlock) which violates the repo test-module layout; move/merge these tests into the existing transaction test module (e.g., key-wallet/src/tests/transaction_tests.rs). To fix: remove this new file and copy each test function (preserving their cfg attributes and tokio signatures) into transaction_tests.rs, add or reconcile required imports (ManagedAccountTrait, TestWalletContext, BlockInfo, TransactionContext, InstantLock where cfg(not(feature = "keep-finalized-transactions"))), and run cargo test to ensure no duplicate symbols or missing imports; adjust any module-level docs to match surrounding tests and keep the tests grouped by functionality in transaction_tests.rs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@key-wallet/Cargo.toml`:
- Around line 27-28: Update the feature documentation comment that references
the old method name `transaction_is_finalized_in_block` to the current API
method `transaction_is_finalized`; specifically, replace the stale symbol in the
comment so it matches the implemented API name (e.g., update any occurrence of
`transaction_is_finalized_in_block` to `transaction_is_finalized` near the
feature docs mentioning `has_transaction` / `transaction_is_finalized`).
---
Nitpick comments:
In `@key-wallet/src/tests/keep_finalized_transactions_tests.rs`:
- Around line 1-155: You created a new top-level test file with several
transaction-focused tests (test_chainlocked_record_kept_when_feature_on,
test_chainlocked_record_dropped_when_feature_off,
test_islocked_record_kept_when_feature_off,
test_islocked_then_chainlocked_drops_at_chainlock) which violates the repo
test-module layout; move/merge these tests into the existing transaction test
module (e.g., key-wallet/src/tests/transaction_tests.rs). To fix: remove this
new file and copy each test function (preserving their cfg attributes and tokio
signatures) into transaction_tests.rs, add or reconcile required imports
(ManagedAccountTrait, TestWalletContext, BlockInfo, TransactionContext,
InstantLock where cfg(not(feature = "keep-finalized-transactions"))), and run
cargo test to ensure no duplicate symbols or missing imports; adjust any
module-level docs to match surrounding tests and keep the tests grouped by
functionality in transaction_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: 3b9d0bdb-919d-4d45-b9ad-6fd64a818a6a
📒 Files selected for processing (13)
dash-spv-ffi/Cargo.tomlkey-wallet-ffi/Cargo.tomlkey-wallet-ffi/FFI_API.mdkey-wallet-ffi/src/managed_account.rskey-wallet-manager/Cargo.tomlkey-wallet/Cargo.tomlkey-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/tests/keep_finalized_transactions_tests.rskey-wallet/src/tests/mod.rskey-wallet/src/transaction_checking/transaction_context.rskey-wallet/src/transaction_checking/wallet_checker.rs
The previous commit renamed the trait method to `transaction_is_finalized` but missed this Cargo.toml feature-doc reference. Caught by CodeRabbit on PR #733. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2 participants
Summary
Opt-in
keep-finalized-transactionsCargo feature that drops the fullTransactionRecordfor transactions that have reached a finalized state (InstantSend lock or ChainLock — both signal the transaction can never change state again) and keeps only the txid in a per-accountfinalized_txids: HashSet<Txid>. Bounds memory growth in long-lived wallets without losing dedup.Public API
Added to
ManagedAccountTrait(always available, both feature configs):fn has_transaction(&self, txid: &Txid) -> bool— true if the tx is either still intransactions(active) or has been dropped intofinalized_txids(finalized). Used as the dedup signal inconfirm_transaction.fn transaction_is_finalized(&self, txid: &Txid) -> bool— true only when the txid is infinalized_txids. Alwaysfalsewith the feature off (no finalization tracking).ManagedAccountTransactionsTrait::transactions()/transactions_mut()remain as before. With the feature on the map only contains non-finalized records — finalized ones are gone, so iterating the map will not see them.confirm_transactionnow returnsOption<TransactionRecord>instead ofbool. The record is cloned BEFORE any finalization so callers can still emit events even when the record has just been dropped from the map.TransactionContext::is_finalized()returns true forInstantSend(_)orInChainLockedBlock(_).Behavior
record_transaction(.., context, ..)— ifcontext.is_finalized(), the record is inserted then immediately dropped, with the txid added tofinalized_txids. Returns the cloned record.confirm_transaction(.., context, ..)— short-circuits when the txid is already finalized (no record left to update). Otherwise updates the existing context, captures the record, then finalizes if the new context is finalized.mark_instant_send_utxos(wallet-level) — also callsfinalize_transactionso IS-lock arrivals through that path drop the record consistently.Defaults
key-walletenables it, socargo test -p key-walletruns the feature-on path. Existing tests that explicitly asserted record content after IS-lock/chainlock are gated to#[cfg(not(feature = \"keep-finalized-transactions\"))].key-wallet-managerandkey-wallet-ffiexpose akeep-finalized-transactionsfeature that forwards tokey-wallet.Test plan
cargo build --workspace(default — feature off) buildscargo build --workspace --features key-wallet-ffi/keep-finalized-transactionsbuildscargo test -p key-wallet --lib --test-threads=1— 463 passed (feature on via dev-dep, includes 4 new feature tests)cargo test -p key-wallet --no-default-features --features test-utils,serde,bincode,getrandom,bls,eddsa,bip38 --lib --test-threads=1— 463 passed (feature off; gates the new tests, exercises legacy ones)cargo test -p key-wallet-manager --all-features— 31/7/5/2/0 ✅cargo clippy --workspace --testscleancargo fmt --checkcleanNew focused tests in
key-wallet/src/tests/keep_finalized_transactions_tests.rs:instantsend_drops_record_and_records_finalization— mempool → IS-lock drops the record;has_transactionstill true;transaction_is_finalizedtrue.chainlock_drops_record_and_records_finalization— InBlock alone is NOT finalization; chainlock drops the record.first_sighting_already_finalized_drops_record_immediately— record + finalize in a single call when the first event is IS-lock.replays_after_finalize_dont_double_count_or_resurrect_record— stale block events for already-finalized txs don't resurrect the record or change UTXOs/balance.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
keep-finalized-transactionsfeature flag to enable full transaction history retention in wallets (by default, finalized transactions are pruned).Documentation
Tests