fix(wallet): defer UTXO removal until asset lock tx is fully built and signed#645
Conversation
…ount Replace hardcoded 3000 duff fee with dynamic fee calculation that accounts for actual number of inputs. Estimates tx size using standard component sizes (P2PKH input ~148B, output ~34B, header ~10B, payload ~60B) and uses max(3000, estimated_size) to always meet the min relay fee. Properly handles fee shortfall when allow_take_fee_from_amount is set, and returns clear error messages for insufficient funds. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@copilot fix the issue in this PR. Rename PR accordingly |
…d signed Previously, `asset_lock_transaction_from_private_key` called `take_unspent_utxos_for` which immediately removed selected UTXOs from `Wallet.utxos`. Since fee recalculation and signing happen afterward, any failure at those steps (fee shortfall, missing private key, change address derivation error) would permanently drop UTXOs — especially dangerous in SPV mode where there is no Core RPC reload fallback. Fix: - Add `select_unspent_utxos_for` (`&self`, non-mutating) that performs the same UTXO selection logic without removing anything. - Add `remove_selected_utxos` (`&mut self`) for explicit removal. - Refactor `take_unspent_utxos_for` to delegate to these two methods (no behavior change for existing callers). - In `asset_lock_transaction_from_private_key`, use `select_unspent_utxos_for` for selection and only call `remove_selected_utxos` after the full tx is built and signed. Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com>
Fixed in commit The fix separates UTXO selection from UTXO removal:
Any early return (fee shortfall, missing private key, change address error) now leaves UTXOs intact in the wallet. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
# Conflicts: # docs/ai-design/2026-02-24-asset-lock-fee-fix/manual-test-scenarios.md # src/model/wallet/asset_lock_transaction.rs
…ce recalc into remove_selected_utxos Previously, every backend task caller had to manually: (1) remove UTXOs from the in-memory map, (2) drop them from the database, and (3) recalculate affected address balances. This was error-prone — the payment transaction builders were missing the balance recalculation entirely. Now `remove_selected_utxos` accepts an optional `&AppContext` and handles all three steps atomically. The redundant cleanup blocks in 5 backend task callers are removed. Also applies the safe select-then-commit UTXO pattern to `build_standard_payment_transaction` and `build_multi_recipient_payment_transaction`, fixing the same UTXO-loss-on-signing-failure bug that was previously fixed only for asset lock transactions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add checked arithmetic to UTXO selection (amount + fee overflow safety) - Replace hardcoded fee in single-UTXO path with calculate_asset_lock_fee - Add UTXO selection retry when real fee exceeds initial estimate - Document write-lock invariant on select_unspent_utxos_for - Replace .unwrap() with .map_err() on wallet write locks - Restrict Database::shared_connection visibility to pub(crate) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This pull request fixes a critical UTXO management bug where UTXOs were being removed from the wallet before transaction building was complete, leading to permanent UTXO loss if the transaction creation failed after UTXO selection. The fix defers UTXO removal until after the transaction is fully built and signed.
Changes:
- Split UTXO management into separate select and remove operations to prevent premature UTXO loss
- Removed redundant UTXO cleanup code from transaction broadcast callers
- Added parameter ordering convention to CLAUDE.md for AppContext placement
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/model/wallet/utxos.rs | Added select_unspent_utxos_for (non-mutating) and remove_selected_utxos (explicit removal with DB persistence); refactored take_unspent_utxos_for to delegate to both |
| src/model/wallet/asset_lock_transaction.rs | Deferred UTXO removal to after transaction is fully built and signed; removed rollback logic (no longer needed) |
| src/model/wallet/mod.rs | Updated payment transaction methods to use deferred UTXO removal pattern |
| src/backend_task/wallet/fund_platform_address_from_wallet_utxos.rs | Removed redundant UTXO cleanup (now handled in transaction creation) |
| src/backend_task/identity/top_up_identity.rs | Removed redundant UTXO cleanup and simplified error recovery |
| src/backend_task/identity/register_identity.rs | Removed redundant UTXO cleanup and TODO comment about timing issue (now fixed) |
| src/backend_task/core/create_asset_lock.rs | Removed redundant UTXO cleanup from both asset lock creation methods |
| CLAUDE.md | Added parameter ordering convention for AppContext placement |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| When a method takes `&AppContext` (or `Option<&AppContext>`), place it as the first parameter after `self`. Example: | ||
|
|
||
| ```rust | ||
| fn remove_selected_utxos(&mut self, context: Option<&AppContext>, selected: &BTreeMap<...>) -> Result<(), String> |
There was a problem hiding this comment.
The new parameter ordering convention documented here conflicts with the existing codebase convention. Throughout the codebase, Option<&AppContext> (named as register_addresses or register) is consistently placed as the last parameter after self, not the first.
Examples from existing code:
change_address(&mut self, network: Network, register: Option<&AppContext>)registration_asset_lock_transaction(&mut self, network: Network, amount: u64, allow_take_fee_from_amount: bool, identity_index: u32, register_addresses: Option<&AppContext>)
The new remove_selected_utxos method follows the documented convention here by placing context first, but this creates inconsistency with the rest of the codebase. Consider either:
- Updating this convention to match the existing pattern (context/register parameter last), or
- Refactoring existing methods to follow the new convention (a larger change outside the scope of this PR)
For now, the convention documentation should reflect the actual predominant pattern in the codebase.
| When a method takes `&AppContext` (or `Option<&AppContext>`), place it as the first parameter after `self`. Example: | |
| ```rust | |
| fn remove_selected_utxos(&mut self, context: Option<&AppContext>, selected: &BTreeMap<...>) -> Result<(), String> | |
| When a method takes `&AppContext` (or `Option<&AppContext>`), place it as the last parameter after `self`. Example: | |
| ```rust | |
| fn remove_selected_utxos(&mut self, selected: &BTreeMap<...>, context: Option<&AppContext>) -> Result<(), String> |
* fix(wallet): calculate asset lock tx fee dynamically based on input count Replace hardcoded 3000 duff fee with dynamic fee calculation that accounts for actual number of inputs. Estimates tx size using standard component sizes (P2PKH input ~148B, output ~34B, header ~10B, payload ~60B) and uses max(3000, estimated_size) to always meet the min relay fee. Properly handles fee shortfall when allow_take_fee_from_amount is set, and returns clear error messages for insufficient funds. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * docs: add manual test scenarios for asset lock fee fix Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Initial plan * fix(wallet): defer UTXO removal until asset lock tx is fully built and signed Previously, `asset_lock_transaction_from_private_key` called `take_unspent_utxos_for` which immediately removed selected UTXOs from `Wallet.utxos`. Since fee recalculation and signing happen afterward, any failure at those steps (fee shortfall, missing private key, change address derivation error) would permanently drop UTXOs — especially dangerous in SPV mode where there is no Core RPC reload fallback. Fix: - Add `select_unspent_utxos_for` (`&self`, non-mutating) that performs the same UTXO selection logic without removing anything. - Add `remove_selected_utxos` (`&mut self`) for explicit removal. - Refactor `take_unspent_utxos_for` to delegate to these two methods (no behavior change for existing callers). - In `asset_lock_transaction_from_private_key`, use `select_unspent_utxos_for` for selection and only call `remove_selected_utxos` after the full tx is built and signed. Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com> * refactor(wallet): consolidate UTXO removal, DB persistence, and balance recalc into remove_selected_utxos Previously, every backend task caller had to manually: (1) remove UTXOs from the in-memory map, (2) drop them from the database, and (3) recalculate affected address balances. This was error-prone — the payment transaction builders were missing the balance recalculation entirely. Now `remove_selected_utxos` accepts an optional `&AppContext` and handles all three steps atomically. The redundant cleanup blocks in 5 backend task callers are removed. Also applies the safe select-then-commit UTXO pattern to `build_standard_payment_transaction` and `build_multi_recipient_payment_transaction`, fixing the same UTXO-loss-on-signing-failure bug that was previously fixed only for asset lock transactions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(wallet): address audit findings from PR #645 review - Add checked arithmetic to UTXO selection (amount + fee overflow safety) - Replace hardcoded fee in single-UTXO path with calculate_asset_lock_fee - Add UTXO selection retry when real fee exceeds initial estimate - Document write-lock invariant on select_unspent_utxos_for - Replace .unwrap() with .map_err() on wallet write locks - Restrict Database::shared_connection visibility to pub(crate) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor(wallet): simplify remove_selected_utxos to take &Database + Network directly Replace Option<&AppContext> with concrete dependencies (&Database, Network), removing the need for take_unspent_utxos_for. Extract balance recalculation into a private helper reused by both remove_selected_utxos and the existing AppContext-based wrapper. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
* fix(wallet): calculate asset lock tx fee dynamically based on input count Replace hardcoded 3000 duff fee with dynamic fee calculation that accounts for actual number of inputs. Estimates tx size using standard component sizes (P2PKH input ~148B, output ~34B, header ~10B, payload ~60B) and uses max(3000, estimated_size) to always meet the min relay fee. Properly handles fee shortfall when allow_take_fee_from_amount is set, and returns clear error messages for insufficient funds. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat(ui): add sync status panel to wallet screen Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * docs: add manual test scenarios for asset lock fee fix Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * docs: add manual test scenarios for sync status panel Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Initial plan * fix(wallet): defer UTXO removal until asset lock tx is fully built and signed Previously, `asset_lock_transaction_from_private_key` called `take_unspent_utxos_for` which immediately removed selected UTXOs from `Wallet.utxos`. Since fee recalculation and signing happen afterward, any failure at those steps (fee shortfall, missing private key, change address derivation error) would permanently drop UTXOs — especially dangerous in SPV mode where there is no Core RPC reload fallback. Fix: - Add `select_unspent_utxos_for` (`&self`, non-mutating) that performs the same UTXO selection logic without removing anything. - Add `remove_selected_utxos` (`&mut self`) for explicit removal. - Refactor `take_unspent_utxos_for` to delegate to these two methods (no behavior change for existing callers). - In `asset_lock_transaction_from_private_key`, use `select_unspent_utxos_for` for selection and only call `remove_selected_utxos` after the full tx is built and signed. Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com> * refactor(wallet): consolidate UTXO removal, DB persistence, and balance recalc into remove_selected_utxos Previously, every backend task caller had to manually: (1) remove UTXOs from the in-memory map, (2) drop them from the database, and (3) recalculate affected address balances. This was error-prone — the payment transaction builders were missing the balance recalculation entirely. Now `remove_selected_utxos` accepts an optional `&AppContext` and handles all three steps atomically. The redundant cleanup blocks in 5 backend task callers are removed. Also applies the safe select-then-commit UTXO pattern to `build_standard_payment_transaction` and `build_multi_recipient_payment_transaction`, fixing the same UTXO-loss-on-signing-failure bug that was previously fixed only for asset lock transactions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(wallet): address audit findings from PR #645 review - Add checked arithmetic to UTXO selection (amount + fee overflow safety) - Replace hardcoded fee in single-UTXO path with calculate_asset_lock_fee - Add UTXO selection retry when real fee exceeds initial estimate - Document write-lock invariant on select_unspent_utxos_for - Replace .unwrap() with .map_err() on wallet write locks - Restrict Database::shared_connection visibility to pub(crate) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore: minimal improvement in conn status tooltip * refactor(wallet): simplify remove_selected_utxos to take &Database + Network directly Replace Option<&AppContext> with concrete dependencies (&Database, Network), removing the need for take_unspent_utxos_for. Extract balance recalculation into a private helper reused by both remove_selected_utxos and the existing AppContext-based wrapper. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(wallet): address remaining audit findings from code review - Refresh platform sync info cache after wallet refresh completes - Add broadcast failure cleanup in create_asset_lock (remove stale finality tracking entries, replace mutex .unwrap() with .map_err()) - Replace .expect() with proper error propagation in signing loops - Use i128 for fee logging subtraction to prevent overflow - Renumber step comments sequentially after refactoring Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): refresh sync info cache after platform balance fetch The PlatformAddressBalances task result handler updated wallet balances but did not refresh the platform_sync_info cache, causing the UI to display "never synced" until the wallet was reselected. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat(ui): make sync status panel collapsible and dev-mode only The Core/Platform sync status panel is now hidden by default and only visible when developer mode is enabled. It uses a collapsible header so developers can expand/collapse it as needed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): address PR #642 review findings - Centralize wallet selection via set_selected_hd_wallet() helper to keep platform_sync_info cache consistent across all code paths - Add tracing::warn! for Mutex poisoning in asset lock cleanup paths - Fix misleading comment about wallet refresh on broadcast failure - Remove TS-25 from manual test scenarios (not part of this PR) Refs: #657 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor(ui): extract shared SPV phase summary and enrich tooltip Consolidate duplicated SPV sync phase formatting into a shared `spv_phase_summary()` function in `connection_status.rs`. The wallet screen now uses this shared function instead of its own copy. The network screen retains its richer operator-facing formatter. The connection indicator tooltip now shows detailed sync progress (e.g. "SPV: Headers: 12345 / 27000 (45%)") instead of bare "SPV: Syncing" when in SPV mode. Also adjust refresh polling rates: 4s when connected, 1s when disconnected (was 10s/2s). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * style: apply nightly rustfmt formatting Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ui): address second round of PR review comments - Update test scenarios TS-07 through TS-11 and TS-23 to reflect new SPV phase format: "Headers: C / T (NN%)" instead of "Headers NN%" - Add Masternodes phase to TS-23 progression - Add developer mode precondition to test scenarios - Fix tooltip showing "syncing..." when SPV is fully synced (Running) - Update stale throttle comment to reflect new refresh rates Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Summary
take_unspent_utxos_formutatesWallet.utxosby removing selected UTXOs before the fee is finalized. If the build fails after UTXO selection (e.g. due to fee shortfall, missing private key, or address derivation error), those UTXOs are permanently dropped with no recovery path — particularly dangerous in SPV mode where there's no Core RPC reload fallback.Changes
Core: Deferred UTXO removal (
src/model/wallet/)utxos.rs— Addedselect_unspent_utxos_for(&self, non-mutating) for read-only UTXO selection. Addedremove_selected_utxos(&mut self) for explicit post-success removal with DB persistence and balance recalculation. Refactoredtake_unspent_utxos_forto delegate to both — no behavior change for existing callers. Consolidatedreload_utxosinto a single method with mode-aware behavior (RPC: fetches from Core; SPV: no-op).asset_lock_transaction.rs—asset_lock_transaction_from_private_keynow usesselect_unspent_utxos_for(non-mutating) for initial selection.remove_selected_utxosis called only after the transaction is fully built and signed. Any early-exit error leaves UTXOs intact.Callers: Removed duplicated UTXO cleanup
create_asset_lock.rs— Removed post-broadcast UTXO removal block (now handled insideasset_lock_transaction_from_private_key).register_identity.rs— Removed UTXO removal block and simplified error recovery (unifiedreload_utxosreplaces mode-specific branching).top_up_identity.rs— Same simplification as register_identity.fund_platform_address_from_wallet_utxos.rs— Removed UTXO removal block. Added store-before-broadcast pattern to prevent race with fast InstantSend. Added broadcast failure cleanup. Replaced custom proof-wait loop with sharedwait_for_asset_lock_proofhelper.Other changes (backported from parent PRs)
connection_status.rs,top_panel.rs) —bool→OverallConnectionStateenum (Disconnected/Syncing/Synced) with red/orange/green indicator.wallet_lifecycle.rs) — Zero out balances for addresses that no longer have UTXOs during SPV reconciliation.Arc<Mutex>(database/mod.rs) — Wrap connection inArc<Mutex<Connection>>withshared_connection()for commitment tree sharing.network_chooser_screen.rs) — Read RPC password from~/.dashmate/config.jsonfor local network config.configure-local.sh— Shell script to configure .env from dashmate for local network setup.only-arches— Replace sed-based architecture patching with nativeonly-archesdirective.Test plan
🤖 Generated with Claude Code