fix(wallet): address audit findings from PR #645 review#648
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>
…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>
# 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>
📝 WalkthroughWalkthroughWallet UTXO selection and asset-lock fee logic were refactored to add a retry path and DB-backed UTXO removal with balance persistence; wallet lock error handling was changed to propagate errors; a database helper's visibility was narrowed to crate scope; overflow-safe arithmetic and test updates were added. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/model/wallet/asset_lock_transaction.rs`:
- Around line 113-161: The retry currently in select_utxos_with_fee_retry only
loops twice and can still return Err when a second selection increases
num_inputs and raises the fee again; change the logic to iterate until the fee
estimate stabilizes or no new inputs are selected: repeatedly call
select_unspent_utxos_for with the current fee_estimate, compute
total_input_value and num_inputs from utxos, call calculate_asset_lock_fee, and
if it Err and the new estimated tx size (estimate_tx_size(num_inputs, 2) as u64)
produces a larger fee_estimate update fee_estimate and continue; stop and return
Ok((utxos, fee_result)) when calculate_asset_lock_fee returns Ok, or return Err
only when fee_estimate no longer changes and selection fails or no additional
UTXOs are added; keep references to MIN_ASSET_LOCK_FEE,
select_unspent_utxos_for, estimate_tx_size, calculate_asset_lock_fee, and utxos
to locate the changes.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/backend_task/identity/register_identity.rssrc/backend_task/identity/top_up_identity.rssrc/database/mod.rssrc/model/wallet/asset_lock_transaction.rssrc/model/wallet/utxos.rs
…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>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/model/wallet/asset_lock_transaction.rs (1)
113-161: Retry loop is bounded at 2 iterations — edge case with many tiny UTXOs.The retry only fires once (when
fee_estimate == MIN_ASSET_LOCK_FEE). If the second selection pulls in additional UTXOs whose marginal cost increases the fee beyond the second estimate, the function will returnErreven though more UTXOs might be available. In practice this requires a wallet composed almost entirely of dust-adjacent UTXOs, so the risk is low, but be aware of this limitation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/wallet/asset_lock_transaction.rs` around lines 113 - 161, The retry is currently limited to two iterations in select_utxos_with_fee_retry which can fail when successive selections add more inputs and raise the fee again; change the loop to iterate until the computed fee_result stabilizes or no new inputs are added (or until a higher reasonable max iteration count, e.g., 10) instead of hardcoding 2 tries: call select_unspent_utxos_for and calculate_asset_lock_fee repeatedly, track the last fee_estimate and last total_input_value/num_inputs (or a loop counter) and break/return once calculate_asset_lock_fee succeeds or when the estimate no longer increases (or max attempts reached), using the same MIN_ASSET_LOCK_FEE, estimate_tx_size, select_unspent_utxos_for, and calculate_asset_lock_fee symbols to locate and modify the logic.
🧹 Nitpick comments (1)
src/model/wallet/utxos.rs (1)
43-43:tx_out.value as i64cast is safe for Dash values but lacks a guard.While Dash's total supply ensures
tx_out.valuefits ini64in practice, this cast sits right next to the new overflow-safechecked_add/try_fromcode, creating an inconsistency in defensive posture. Consider whether aTryFromguard here would be worth the consistency.♻️ Optional: add overflow guard for consistency
- required -= tx_out.value as i64; + required -= i64::try_from(tx_out.value).ok()?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/wallet/utxos.rs` at line 43, The direct cast "tx_out.value as i64" next to other checked conversions is inconsistent and could silently overflow; replace the cast with a fallible conversion (use i64::try_from(tx_out.value) or the project's TryFrom helper) and propagate or handle the Err consistently (same error type/path used by the surrounding checked_add/try_from logic) before subtracting from required, i.e., obtain a validated i64 amount from tx_out.value and then do required -= amount.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/model/wallet/asset_lock_transaction.rs`:
- Around line 113-161: The retry is currently limited to two iterations in
select_utxos_with_fee_retry which can fail when successive selections add more
inputs and raise the fee again; change the loop to iterate until the computed
fee_result stabilizes or no new inputs are added (or until a higher reasonable
max iteration count, e.g., 10) instead of hardcoding 2 tries: call
select_unspent_utxos_for and calculate_asset_lock_fee repeatedly, track the last
fee_estimate and last total_input_value/num_inputs (or a loop counter) and
break/return once calculate_asset_lock_fee succeeds or when the estimate no
longer increases (or max attempts reached), using the same MIN_ASSET_LOCK_FEE,
estimate_tx_size, select_unspent_utxos_for, and calculate_asset_lock_fee symbols
to locate and modify the logic.
---
Nitpick comments:
In `@src/model/wallet/utxos.rs`:
- Line 43: The direct cast "tx_out.value as i64" next to other checked
conversions is inconsistent and could silently overflow; replace the cast with a
fallible conversion (use i64::try_from(tx_out.value) or the project's TryFrom
helper) and propagate or handle the Err consistently (same error type/path used
by the surrounding checked_add/try_from logic) before subtracting from required,
i.e., obtain a validated i64 amount from tx_out.value and then do required -=
amount.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/model/wallet/asset_lock_transaction.rssrc/model/wallet/mod.rssrc/model/wallet/utxos.rs
|
Good catch. The bounded In practice this is an extreme edge case — each additional P2PKH input only adds ~148 duffs to the fee, so it would only matter when UTXOs are barely above dust — but the suggested loop-until-stabilization approach is technically more correct and still guaranteed to terminate (finite UTXO pool + monotonically increasing fees). Since this PR is already merged, I'll open a follow-up issue to track this improvement. |
Summary
Addresses 6 findings from the code quality and security audit of PR #645:
(amount + fee) as i64replaced withchecked_add+i64::try_fromto prevent overflow/wrap on extreme valueslet fee = 3_000; value - fee(panic-prone) replaced withcalculate_asset_lock_fee(), matching the multi-input path with dust check and overflow safetyselect_utxos_with_fee_retry()retries selection once with the computed fee when the initial 3000-duff estimate was insufficient, preventing false "insufficient funds" errors on wallets with many small UTXOsselect_unspent_utxos_fordoc comment now warns that the caller must hold the wallet write lock continuously throughremove_selected_utxos.unwrap()on wallet write locks inregister_identityandtop_up_identityreplaced with.map_err(|e| e.to_string())?for consistent error handling (prevents cascading panics on lock poisoning)Database::shared_connection()narrowed frompubtopub(crate)Test plan
-D warnings)cargo +nightly fmtapplied🤖 Generated with Claude Code
🤖 Co-authored by Claudius the Magnificent AI Agent
Summary by CodeRabbit
Bug Fixes
New Features