fix(key-wallet): track self-send change in confirmed balance - #707
Conversation
When a mempool transaction spends one of our own UTXOs and pays change back to one of our internal addresses, the change UTXO is just our previously-tracked funds returning. Marking it `is_confirmed = false` sent the entire change amount into the unconfirmed bucket, making the user's confirmed balance appear to drop by the full input value while the change waited in the mempool. `ManagedCoreAccount::update_utxos` now flags such change outputs as confirmed regardless of context, using `account_match.sent > 0` as the "we own an input" signal and `involved_change_addresses` for the internal-pool match. Outputs to receive (external) addresses, and mempool payments where we don't own any inputs, keep the existing unconfirmed treatment. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR introduces a ChangesTrusted Mempool UTXO Marking
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v0.42-dev #707 +/- ##
=============================================
- Coverage 70.59% 70.59% -0.01%
=============================================
Files 320 320
Lines 68028 68243 +215
=============================================
+ Hits 48026 48177 +151
- Misses 20002 20066 +64
|
Replace the overloaded `is_confirmed = context.confirmed() || is_self_send_change` written in the previous commit with a dedicated `Utxo::is_change` flag. `is_confirmed` now means strictly "the parent transaction is in a block (or chain-locked block)"; the new `is_change` flag captures "this output is the change of a transaction we created (we own at least one input and the output pays one of our internal addresses)". `update_balance` credits a UTXO to the `confirmed` bucket when any of `is_confirmed`, `is_instantlocked`, or `is_change` is set, preserving the behavior introduced for self-sends in the mempool while keeping the two properties orthogonal in the data model. The new field deserializes to `false` (`#[serde(default)]`) so existing serialized wallet state stays compatible. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
xdustinface
left a comment
There was a problem hiding this comment.
It feels a little bit wrong to make it look confirmed even though it isn't but i guess sits alright.
There is just one think we should maybe consider: Calling it is_trusted instead of is_change makes maybe more sense as we then can also let actual self sends which are no change fall into this category?
Address review feedback: the new flag mirrors Bitcoin Core's `CWalletTx::IsTrusted()` concept, and the name `is_trusted` reads better than `is_change` for the role it plays in `update_balance`. The predicate is unchanged (output is to one of our internal addresses on a transaction that also spends one of our own UTXOs); only the field, locals, comments, doc references, and tests are renamed. Also drop `#[cfg_attr(feature = "serde", serde(default))]` on the field for consistency with the other `Utxo` flags — none of which carry that attribute. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
key-wallet/src/utxo.rs (1)
17-18:⚠️ Potential issue | 🟠 MajorPreserve backward compatibility for persisted
Utxos.With derived
Deserialize, older wallet snapshots that predateis_trustedwill fail to deserialize when the codebase is upgraded. TheBTreeMap<OutPoint, Utxo>inManagedCoreAccountwill be unable to parse snapshots lacking this field. Addserde(default)to default the new field tofalseduring deserialization, matching the constructor behavior.Suggested fix
pub struct Utxo { pub is_locked: bool, - pub is_trusted: bool, + #[cfg_attr(feature = "serde", serde(default))] + pub is_trusted: bool, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet/src/utxo.rs` around lines 17 - 18, The new Utxo field is_trusted will break deserialization of old wallet snapshots; update the Utxo definition so deserializing missing is_trusted defaults to false by annotating the field with serde default (e.g., add #[serde(default)] on the is_trusted field in the Utxo struct), ensuring the existing derived Deserialize stays in place and ManagedCoreAccount's BTreeMap<OutPoint, Utxo> can load older snapshots without errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@key-wallet/src/utxo.rs`:
- Around line 17-18: The new Utxo field is_trusted will break deserialization of
old wallet snapshots; update the Utxo definition so deserializing missing
is_trusted defaults to false by annotating the field with serde default (e.g.,
add #[serde(default)] on the is_trusted field in the Utxo struct), ensuring the
existing derived Deserialize stays in place and ManagedCoreAccount's
BTreeMap<OutPoint, Utxo> can load older snapshots without errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0a0c993e-cb27-4a61-9aac-9076e0b96fa5
📒 Files selected for processing (4)
key-wallet/src/managed_account/mod.rskey-wallet/src/transaction_checking/wallet_checker.rskey-wallet/src/utxo.rskey-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rs
|
Going to merge this in. |
Summary
When a mempool transaction spends one of our own UTXOs and pays change back to an internal address, the resulting change UTXO was being placed in the unconfirmed balance bucket. From a user's perspective the confirmed balance appeared to drop by the entire input value as soon as the spend was broadcast — even though the change is just previously-tracked funds returning.
This PR teaches
ManagedCoreAccount::update_utxosto recognize that situation and mark the change UTXO as confirmed even while the parent transaction is still in the mempool. The condition is:account_match.sent > 0)The transaction record itself still reports
TransactionContext::Mempoolandis_confirmed() == false; only the change UTXO's confirmation flag (and therefore the balance bucket it lands in) is adjusted.External / receive outputs are unchanged. Incoming mempool payments where we do not own an input keep the prior unconfirmed treatment.
Test plan
cargo test -p key-wallet --lib(461 passing, including 2 new tests)test_self_send_change_in_mempool_lands_in_confirmed_balanceexercises the new path: a confirmed funding UTXO is spent in mempool with change to our internal pool; the change UTXO isis_confirmed = trueand the wallet balance reportsconfirmed = change_amount,unconfirmed = 0.test_external_mempool_payment_remains_unconfirmedis the regression guard: a fresh mempool payment we receive (no owned input) stays in the unconfirmed bucket.cargo test -p key-wallet-manager --libcargo test -p dash-spv --libcargo clippy -p key-wallet --all-features --all-targets -- -D warningscargo fmt --check -p key-wallet🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests