fix(fees): consolidate Core and Platform fee estimation#651
Open
fix(fees): consolidate Core and Platform fee estimation#651
Conversation
…0 duffs fee The RPC wallet payment path used a hardcoded DEFAULT_TX_FEE of 1000 duffs, which was rejected by Dash Core when the transaction size required a higher minimum relay fee (e.g. "min relay fee not met, 1000 < 1405"). Changes: - Add Core (L1) transaction fee utilities to fee_estimation.rs: estimate_p2pkh_tx_size, estimate_asset_lock_tx_size, calculate_relay_fee, DUST_THRESHOLD, MIN_ASSET_LOCK_FEE, AssetLockFeeResult, and calculate_asset_lock_fee - Fix send_wallet_payment_via_rpc to use size-based fee calculation with iterative estimation instead of hardcoded 1000 duffs - Replace 3 duplicate estimate_p2pkh_tx_size implementations (mod.rs, send_single_key_wallet_payment.rs, single_key_send_screen.rs) with the consolidated function - Move asset lock fee logic from asset_lock_transaction.rs to fee_estimation.rs - Add regression test: relay_fee_exceeds_1000_for_large_tx Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… max() Add unified fee methods to PlatformFeeEstimator that compute max(legacy, transition-based) internally, so callers don't need to know about the dual estimation strategy. Consolidates changes from PR #521 and PR #514 into centralized estimator methods. New public methods: - estimate_platform_to_core_fee() - estimate_platform_to_platform_fee() - estimate_core_to_platform_fee() - estimate_identity_create_from_addresses_fee() - estimate_identity_topup_from_addresses_fee() Reduce public API surface: - Make internal helpers private (calculate_processing_fee, calculate_seek_fee, calculate_storage_fee, etc.) - Remove dead code (estimate_masternode_vote, storage_fees(), estimate_address_credit_withdrawal, estimate_contract_create_detailed) Callers updated: send_screen, register_identity, identity/mod, add_new_identity_screen, top_up_identity_screen. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add 9 tests covering the new public unified methods that compute max(legacy, transition-based) fees: - platform-to-core fee (positive, scales with inputs) - platform-to-platform fee (positive) - core-to-platform fee (positive) - identity-create-from-addresses fee (positive, scales with keys) - identity-topup-from-addresses fee (positive) - unified fees >= legacy (invariant check for max() contract) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCentralized fee estimation in a new fee_estimation module; replaced local size/fee logic across core, asset-lock, platform, identity, wallet and UI flows. Send flows perform iterative relay-fee reconciliation (rebuild when actual fee > estimate). Identity flows use per-address input maps; UTXO removal consolidated via wallet.remove_selected_utxos. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Wallet Client
participant TxBuilder as Transaction Builder
participant FeeEstimator as Fee Estimator
participant UTXOManager as UTXO Manager
Client->>TxBuilder: Request send (recipients, override_fee?)
TxBuilder->>FeeEstimator: Estimate fee (inputs?, outputs)
FeeEstimator-->>TxBuilder: estimated_fee
TxBuilder->>TxBuilder: Build transaction using estimated_fee
TxBuilder->>FeeEstimator: calculate_relay_fee(built_tx_size)
FeeEstimator-->>TxBuilder: actual_fee
alt actual_fee > estimated_fee and no override
TxBuilder->>UTXOManager: reload/refresh UTXOs
UTXOManager-->>TxBuilder: updated_utxos
TxBuilder->>FeeEstimator: re-estimate with updated inputs
FeeEstimator-->>TxBuilder: new_estimate
TxBuilder->>TxBuilder: Rebuild transaction with new_estimate/actual_fee
TxBuilder-->>Client: Broadcast rebuilt transaction
else actual_fee ≤ estimated_fee or override provided
TxBuilder-->>Client: Broadcast initial transaction
end
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 |
This was referenced Feb 24, 2026
Contributor
Author
🔍 Audit SummaryReviewed by: Claude Code with a 3-agent team:
Overall AssessmentSolid consolidation PR. The UTXO safety refactoring (select → build → sign → remove) is a significant improvement over the old eager-removal pattern. Fee estimation deduplication is clean and well-tested. No critical or high-severity issues found. Findings Summary
Pre-existing / Outside-diff IssuesThese are in code modified by earlier commits already on
Redundancy Report
Positive Observations
🤖 Co-authored by Claudius the Magnificent AI Agent |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/backend_task/identity/mod.rs (1)
753-759:⚠️ Potential issue | 🟡 MinorPotential overflow in fee mismatch logging.
The cast on line 758 (
actual_fee as i64 - estimated_fee as i64) could overflow if eitherCreditsvalue exceedsi64::MAX. While unlikely in practice, this is a logging path that shouldn't panic.Safer alternative
- estimated_fee, - actual_fee, - actual_fee as i64 - estimated_fee as i64 + estimated_fee, + actual_fee, + (actual_fee as i128) - (estimated_fee as i128)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend_task/identity/mod.rs` around lines 753 - 759, The current tracing::warn call computes the fee difference by casting to i64 (actual_fee as i64 - estimated_fee as i64), which can overflow; change the logging to compute the signed difference safely—for example cast both actual_fee and estimated_fee to a wider signed type like i128 before subtracting, or compute an unsigned delta plus an explicit sign (check actual_fee >= estimated_fee and subtract in that branch) — update the tracing::warn call that references actual_fee and estimated_fee to use the safe diff value/format instead of the direct i64 casts.
🧹 Nitpick comments (8)
src/backend_task/core/mod.rs (1)
375-424: Iterative fee estimation: single retry may theoretically under-estimate on the second pass.After the first build, the fee is recalculated from actual tx dimensions (line 406) and a rebuild is done. However, the rebuild itself isn't re-checked — if the higher fee causes selection of additional UTXOs (increasing input count beyond the recalculated estimate), the fee could still fall short. This is unlikely in practice since the first build's input count is typically an upper bound, but worth noting.
Consider adding a debug assertion or log to confirm
actual_feestill covers the rebuilt tx:Possible defensive check
tx = wallet_guard.build_multi_recipient_payment_transaction( self.network, &parsed_recipients, actual_fee, request.subtract_fee_from_amount, Some(self), )?; + // Sanity check: rebuilt tx should not need a higher fee + let rebuilt_fee = calculate_relay_fee( + estimate_p2pkh_tx_size(tx.input.len(), tx.output.len()), + ); + debug_assert!( + actual_fee >= rebuilt_fee, + "Fee still insufficient after rebuild: {} < {}", + actual_fee, + rebuilt_fee + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend_task/core/mod.rs` around lines 375 - 424, The iterative fee logic can still under-estimate after the rebuild; after calling wallet_guard.build_multi_recipient_payment_transaction the second time, recompute the relay fee using calculate_relay_fee(estimate_p2pkh_tx_size(tx.input.len(), tx.output.len())) and compare it against the fee passed into build_multi_recipient_payment_transaction (actual_fee used for rebuild); if the recomputed fee is greater, either loop once more (reloading UTXOs again) or return an explicit error/log message indicating fee under-estimation. Add a debug log or debug_assert in the rebuild branch referencing build_multi_recipient_payment_transaction, estimate_p2pkh_tx_size, and calculate_relay_fee so we detect and surface the rare case where the rebuilt tx requires an even higher fee.src/backend_task/core/send_single_key_wallet_payment.rs (1)
119-149: Fee is recalculated withnum_outputs_with_changebut change might not be added.On line 120-124, the fee is computed assuming a change output (
outputs.len() + 1). Ifchange_amount <= DUST_THRESHOLD(line 144), no change output is created, so the fee is slightly over-estimated (the tx is smaller than assumed). This is safe — the excess becomes an implicit miner fee — but worth noting as a minor inefficiency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend_task/core/send_single_key_wallet_payment.rs` around lines 119 - 149, The fee is initially estimated using estimate_p2pkh_tx_size with num_outputs_with_change (outputs.len() + 1) but change may not be added if change_amount <= DUST_THRESHOLD, causing over-estimation; fix by making fee calculation iterative: compute a fee assuming no change, calculate change_amount (respecting request.subtract_fee_from_amount and outputs[0] adjustment), if change_amount > DUST_THRESHOLD then recompute the fee using estimate_p2pkh_tx_size(selected_utxos.len(), outputs.len() + 1), recalc change_amount and then push the TxOut; if change_amount <= DUST_THRESHOLD keep the no-change fee and do not add a change output. Reference symbols: estimate_p2pkh_tx_size, calculate_relay_fee, num_outputs_with_change, change_amount, DUST_THRESHOLD, outputs, request.subtract_fee_from_amount.src/ui/identities/add_new_identity_screen/by_platform_address.rs (1)
156-177: Verbose type annotation could use existing imports.The fully-qualified types on lines 156-161 (
std::collections::BTreeMap,dash_sdk::dpp::prelude::AddressNonce,dash_sdk::dpp::balances::credits::Credits) could be simplified using theBTreeMapalready available viastd::collections::BTreeMapand by adding imports forAddressNonceandCredits, similar to howPlatformAddressis imported at line 10.Suggested simplification
Add at file top:
use dash_sdk::dpp::prelude::AddressNonce; use dash_sdk::dpp::balances::credits::Credits;Then simplify:
- let inputs: std::collections::BTreeMap< - PlatformAddress, - ( - dash_sdk::dpp::prelude::AddressNonce, - dash_sdk::dpp::balances::credits::Credits, - ), - > = self + let inputs: BTreeMap<PlatformAddress, (AddressNonce, Credits)> = self🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/identities/add_new_identity_screen/by_platform_address.rs` around lines 156 - 177, The type annotations for `inputs` use fully‑qualified paths; add `use dash_sdk::dpp::prelude::AddressNonce;` and `use dash_sdk::dpp::balances::credits::Credits;` at the top of the file and then simplify the declaration to use `BTreeMap`, `AddressNonce`, and `Credits` directly (i.e. change `std::collections::BTreeMap<PlatformAddress, (dash_sdk::dpp::prelude::AddressNonce, dash_sdk::dpp::balances::credits::Credits)>` to `BTreeMap<PlatformAddress, (AddressNonce, Credits)>`) for the `inputs` variable used with `selected_platform_address_for_funding` and passed into `estimate_identity_create_from_addresses_fee`.src/ui/wallets/send_screen.rs (2)
1529-1570: Duplicated fee-estimation dispatch logic across three call sites.The pattern of branching on
dest_typeto choose betweenestimate_platform_to_platform_feeandestimate_platform_to_core_feeis repeated inrender_platform_source_breakdown(here),estimate_max_fee_for_platform_send(lines 334–367), and thesend_platform_to_*methods. Consider extracting a helper that takesdest_type,inputs, and destination info, and returns the appropriate fee closure or fee value. This would reduce the maintenance burden if the fee API changes again.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/wallets/send_screen.rs` around lines 1529 - 1570, Extract the repeated dispatch that chooses between estimate_platform_to_platform_fee and estimate_platform_to_core_fee into a single helper (e.g., build_platform_fee_closure or select_fee_estimator) that accepts dest_type, destination_address (or parsed CoreScript), platform_version and a reference to fee_estimator and returns a closure or function which, given inputs, returns the appropriate fee Result; then replace the branching in render_platform_source_breakdown, estimate_max_fee_for_platform_send, and all send_platform_to_* methods to call allocate_platform_addresses_with_fee with that helper’s returned closure, and use detect_address_type, allocate_platform_addresses_with_fee, estimate_platform_to_platform_fee, estimate_platform_to_core_fee, and CoreScript parsing inside the new helper to centralize logic.
51-58: TheResultreturn types are unused in practice — all callers pass infallible closures.The
fee_for_inputsclosure signature was changed toFn(&BTreeMap<...>) -> Result<u64, String>, andallocate_platform_addresses_with_feenow returnsResult<..., String>. However, every call site wraps infallible estimator methods inOk(...)(e.g., lines 754, 894, 1539, 1559). TheResultpath is never actually exercised.This is fine as defensive future-proofing if fee estimation may become fallible later, but worth noting that the error paths are currently dead code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/wallets/send_screen.rs` around lines 51 - 58, The function allocate_platform_addresses_with_fee currently takes fee_for_inputs: F where F returns Result<u64, String> and the function returns Result<AddressAllocationResult, String>, but all call sites pass infallible estimators wrapped in Ok(...), so the error path is dead; fix by simplifying the API: change allocate_platform_addresses_with_fee to accept F: Fn(&BTreeMap<PlatformAddress, u64>) -> u64 (remove the Result return) and make the function return AddressAllocationResult (not Result<..., String>), updating the internal usage to call fee_for_inputs(...) directly; alternatively, if you want to keep fallible fees, update each caller (e.g., the places now wrapping estimators in Ok(...)) to propagate real errors instead of wrapping them—pick one approach for consistency and update signatures and call sites accordingly.src/model/fee_estimation.rs (3)
864-867: Emptypublic_keysondefault_versionedfailure will underestimate transition-based fee.If
IdentityPublicKeyInCreation::default_versioned(platform_version)returnsErr,public_keysbecomes an emptyVec, so the transition-based fee excludes per-key costs. Themax(legacy, transition)pattern ensures the legacy estimate (which does include keys) still covers this, so it's safe in practice.Consider logging the error for visibility:
Suggested fix
let public_keys = match IdentityPublicKeyInCreation::default_versioned(platform_version) { Ok(key) => std::iter::repeat_n(key, key_count).collect(), - Err(_) => Vec::new(), + Err(e) => { + tracing::debug!("Failed to create default key for fee estimation: {e}"); + Vec::new() + } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/fee_estimation.rs` around lines 864 - 867, If IdentityPublicKeyInCreation::default_versioned(platform_version) fails, public_keys is silently set to an empty Vec which causes transition-based fee to miss per-key costs; change the match around IdentityPublicKeyInCreation::default_versioned(platform_version) to capture the Err and log the error (using the module's logger or tracing) with context before falling back to the current empty Vec (or a safe default), referencing IdentityPublicKeyInCreation::default_versioned and the public_keys variable so future debugging can see why key creation failed and why the transition fee may be underestimated.
721-728: Silent error swallowing inestimate_fee_from_transitionis intentional but opaque.
unwrap_or(0)means any SDK estimation failure is silently ignored and the legacy path always dominates. This is safe given themax(legacy, transition)pattern, but adding atracing::debug!on the error branch would help diagnose cases where the transition-based path is unexpectedly failing.Suggested enhancement
fn estimate_fee_from_transition<T: StateTransitionEstimatedFeeValidation>( transition: &T, platform_version: &PlatformVersion, ) -> u64 { - transition - .calculate_min_required_fee(platform_version) - .unwrap_or(0) + match transition.calculate_min_required_fee(platform_version) { + Ok(fee) => fee, + Err(e) => { + tracing::debug!("Transition-based fee estimation failed: {e}"); + 0 + } + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/fee_estimation.rs` around lines 721 - 728, The function estimate_fee_from_transition currently swallows errors via unwrap_or(0); change it to call transition.calculate_min_required_fee(platform_version) and match the Result so that on Err(e) you log the error with tracing::debug! (include the error and context such as the transition type via std::any::type_name::<T>() and the platform_version id or version if available) and then return 0, otherwise return the Ok value; update the body of estimate_fee_from_transition to perform this match/logging instead of using unwrap_or(0).
1246-1287:test_unified_fees_at_least_as_high_as_legacydoesn't cover all unified methods.This excellent invariant test validates
estimate_platform_to_platform_fee,estimate_identity_create_from_addresses_fee, andestimate_identity_topup_from_addresses_fee, but omitsestimate_platform_to_core_feeandestimate_core_to_platform_fee. Consider adding those for completeness.Additional test cases
+ // Platform → Core + let output_script = CoreScript::from_bytes(vec![0x76, 0xa9, 0x14]); + let legacy_withdrawal = estimator.legacy_platform_transfer_fee(1); + let unified_withdrawal = + estimator.estimate_platform_to_core_fee(pv, &inputs, &output_script); + assert!( + unified_withdrawal >= legacy_withdrawal, + "unified withdrawal ({unified_withdrawal}) must be >= legacy ({legacy_withdrawal})" + ); + + // Core → Platform + let legacy_funding = estimator.estimate_address_funding_from_asset_lock_duffs(1); + let unified_funding = + estimator.estimate_core_to_platform_fee(pv, &test_platform_address(1)); + assert!( + unified_funding >= legacy_funding, + "unified funding ({unified_funding}) must be >= legacy ({legacy_funding})" + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/fee_estimation.rs` around lines 1246 - 1287, Add two assertions mirroring the existing transfer check to cover the missing unified methods: call estimator.estimate_platform_to_core_fee(pv, &inputs, &test_platform_address(2)) and assert it is >= estimator.legacy_platform_to_core_fee(1), and call estimator.estimate_core_to_platform_fee(pv, &inputs, &test_platform_address(2)) and assert it is >= estimator.legacy_core_to_platform_fee(1); reuse the same inputs/BTreeMap and test_platform_address(2) setup and the existing estimator/test_platform_version variables so the new checks match the style of test_unified_fees_at_least_as_high_as_legacy.
🤖 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 53-56: The fee_estimate is using raw byte size from
estimate_asset_lock_tx_size(num_inputs, 2) cast to u64 instead of converting
bytes to duffs; change the assignment so you pass the estimated size into
calculate_relay_fee(...) and use its return value (but still enforce
MIN_ASSET_LOCK_FEE with std::cmp::max), i.e. compute
calculate_relay_fee(estimate_asset_lock_tx_size(num_inputs, 2) as u64) and use
that in the max with MIN_ASSET_LOCK_FEE for fee_estimate.
---
Outside diff comments:
In `@src/backend_task/identity/mod.rs`:
- Around line 753-759: The current tracing::warn call computes the fee
difference by casting to i64 (actual_fee as i64 - estimated_fee as i64), which
can overflow; change the logging to compute the signed difference safely—for
example cast both actual_fee and estimated_fee to a wider signed type like i128
before subtracting, or compute an unsigned delta plus an explicit sign (check
actual_fee >= estimated_fee and subtract in that branch) — update the
tracing::warn call that references actual_fee and estimated_fee to use the safe
diff value/format instead of the direct i64 casts.
---
Nitpick comments:
In `@src/backend_task/core/mod.rs`:
- Around line 375-424: The iterative fee logic can still under-estimate after
the rebuild; after calling
wallet_guard.build_multi_recipient_payment_transaction the second time,
recompute the relay fee using
calculate_relay_fee(estimate_p2pkh_tx_size(tx.input.len(), tx.output.len())) and
compare it against the fee passed into build_multi_recipient_payment_transaction
(actual_fee used for rebuild); if the recomputed fee is greater, either loop
once more (reloading UTXOs again) or return an explicit error/log message
indicating fee under-estimation. Add a debug log or debug_assert in the rebuild
branch referencing build_multi_recipient_payment_transaction,
estimate_p2pkh_tx_size, and calculate_relay_fee so we detect and surface the
rare case where the rebuilt tx requires an even higher fee.
In `@src/backend_task/core/send_single_key_wallet_payment.rs`:
- Around line 119-149: The fee is initially estimated using
estimate_p2pkh_tx_size with num_outputs_with_change (outputs.len() + 1) but
change may not be added if change_amount <= DUST_THRESHOLD, causing
over-estimation; fix by making fee calculation iterative: compute a fee assuming
no change, calculate change_amount (respecting request.subtract_fee_from_amount
and outputs[0] adjustment), if change_amount > DUST_THRESHOLD then recompute the
fee using estimate_p2pkh_tx_size(selected_utxos.len(), outputs.len() + 1),
recalc change_amount and then push the TxOut; if change_amount <= DUST_THRESHOLD
keep the no-change fee and do not add a change output. Reference symbols:
estimate_p2pkh_tx_size, calculate_relay_fee, num_outputs_with_change,
change_amount, DUST_THRESHOLD, outputs, request.subtract_fee_from_amount.
In `@src/model/fee_estimation.rs`:
- Around line 864-867: If
IdentityPublicKeyInCreation::default_versioned(platform_version) fails,
public_keys is silently set to an empty Vec which causes transition-based fee to
miss per-key costs; change the match around
IdentityPublicKeyInCreation::default_versioned(platform_version) to capture the
Err and log the error (using the module's logger or tracing) with context before
falling back to the current empty Vec (or a safe default), referencing
IdentityPublicKeyInCreation::default_versioned and the public_keys variable so
future debugging can see why key creation failed and why the transition fee may
be underestimated.
- Around line 721-728: The function estimate_fee_from_transition currently
swallows errors via unwrap_or(0); change it to call
transition.calculate_min_required_fee(platform_version) and match the Result so
that on Err(e) you log the error with tracing::debug! (include the error and
context such as the transition type via std::any::type_name::<T>() and the
platform_version id or version if available) and then return 0, otherwise return
the Ok value; update the body of estimate_fee_from_transition to perform this
match/logging instead of using unwrap_or(0).
- Around line 1246-1287: Add two assertions mirroring the existing transfer
check to cover the missing unified methods: call
estimator.estimate_platform_to_core_fee(pv, &inputs, &test_platform_address(2))
and assert it is >= estimator.legacy_platform_to_core_fee(1), and call
estimator.estimate_core_to_platform_fee(pv, &inputs, &test_platform_address(2))
and assert it is >= estimator.legacy_core_to_platform_fee(1); reuse the same
inputs/BTreeMap and test_platform_address(2) setup and the existing
estimator/test_platform_version variables so the new checks match the style of
test_unified_fees_at_least_as_high_as_legacy.
In `@src/ui/identities/add_new_identity_screen/by_platform_address.rs`:
- Around line 156-177: The type annotations for `inputs` use fully‑qualified
paths; add `use dash_sdk::dpp::prelude::AddressNonce;` and `use
dash_sdk::dpp::balances::credits::Credits;` at the top of the file and then
simplify the declaration to use `BTreeMap`, `AddressNonce`, and `Credits`
directly (i.e. change `std::collections::BTreeMap<PlatformAddress,
(dash_sdk::dpp::prelude::AddressNonce,
dash_sdk::dpp::balances::credits::Credits)>` to `BTreeMap<PlatformAddress,
(AddressNonce, Credits)>`) for the `inputs` variable used with
`selected_platform_address_for_funding` and passed into
`estimate_identity_create_from_addresses_fee`.
In `@src/ui/wallets/send_screen.rs`:
- Around line 1529-1570: Extract the repeated dispatch that chooses between
estimate_platform_to_platform_fee and estimate_platform_to_core_fee into a
single helper (e.g., build_platform_fee_closure or select_fee_estimator) that
accepts dest_type, destination_address (or parsed CoreScript), platform_version
and a reference to fee_estimator and returns a closure or function which, given
inputs, returns the appropriate fee Result; then replace the branching in
render_platform_source_breakdown, estimate_max_fee_for_platform_send, and all
send_platform_to_* methods to call allocate_platform_addresses_with_fee with
that helper’s returned closure, and use detect_address_type,
allocate_platform_addresses_with_fee, estimate_platform_to_platform_fee,
estimate_platform_to_core_fee, and CoreScript parsing inside the new helper to
centralize logic.
- Around line 51-58: The function allocate_platform_addresses_with_fee currently
takes fee_for_inputs: F where F returns Result<u64, String> and the function
returns Result<AddressAllocationResult, String>, but all call sites pass
infallible estimators wrapped in Ok(...), so the error path is dead; fix by
simplifying the API: change allocate_platform_addresses_with_fee to accept F:
Fn(&BTreeMap<PlatformAddress, u64>) -> u64 (remove the Result return) and make
the function return AddressAllocationResult (not Result<..., String>), updating
the internal usage to call fee_for_inputs(...) directly; alternatively, if you
want to keep fallible fees, update each caller (e.g., the places now wrapping
estimators in Ok(...)) to propagate real errors instead of wrapping them—pick
one approach for consistency and update signatures and call sites accordingly.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/backend_task/core/mod.rssrc/backend_task/core/send_single_key_wallet_payment.rssrc/backend_task/identity/mod.rssrc/backend_task/identity/register_identity.rssrc/model/fee_estimation.rssrc/model/wallet/asset_lock_transaction.rssrc/ui/identities/add_new_identity_screen/by_platform_address.rssrc/ui/identities/top_up_identity_screen/by_platform_address.rssrc/ui/wallets/send_screen.rssrc/ui/wallets/single_key_send_screen.rs
lklimek
commented
Feb 24, 2026
- FEE-001: guard reload_utxos() return in SPV mode to prevent silent UTXO loss during fee re-estimation - FEE-002: use calculate_relay_fee() on asset lock tx size instead of raw byte count as fee amount - FEE-003: replace manual utxos.retain()/drop_utxo() in FundWithUtxo paths with consolidated remove_selected_utxos() - FEE-004: remove unnecessary Result wrapping from allocate_platform_addresses_with_fee (infallible fee estimation) - FEE-005: replace expect() on Core client lock with map_err for graceful error handling Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add tracing::warn sanity check after fee rebuild to detect under-estimation edge case - Simplify verbose type annotations in by_platform_address.rs - Log errors in estimate_fee_from_transition instead of silent unwrap_or(0) - Log default_versioned key creation failures in fee estimation - Add platform-to-core and core-to-platform to unified fee invariant test - Use i128 cast in fee mismatch logging to prevent i64 overflow Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace the old asset-lock-fee-fix test scenarios (from PR #636) with comprehensive scenarios covering the full PR #651 scope: Core wallet payment fees, asset lock fees, Platform fee consolidation, UTXO removal consistency, and edge cases (SPV mode, dust threshold, large input count). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ent funds The "subtract fee from amount" checkbox in core-to-core sends was silently ignored when the wallet had enough funds to cover amount + fee. The fee subtraction logic only activated in the edge case where UTXOs covered the amount but not amount + fee (change_option.is_none()). Now when the flag is set, UTXOs are selected for just the amount (fee=0) and the fee is always deducted proportionally from recipient outputs, regardless of wallet balance. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…tion Resolve conflict in send_wallet_payment_via_rpc where upstream added AppContext as first param to build_multi_recipient_payment_transaction while this branch added iterative fee re-estimation logic. Extract private _inner method accepting Option<&AppContext> + &Database so unit tests can run without a full AppContext (which requires live Dash node, SDK, config files, etc.). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/model/wallet/mod.rs (1)
1722-1739:⚠️ Potential issue | 🟡 MinorAvoid fee over-collection from per-output truncation.
The proportional split can round each output down, so the outputs may sum to less than
total_amount - fee, implicitly increasing the fee. Consider integer math with a deterministic remainder assignment to preserve the exact fee.♻️ Suggested fix (distribute remainder deterministically)
- let reduction_ratio = available_after_fee as f64 / total_amount as f64; - - recipients - .iter() - .map(|(recipient, amount)| { - let adjusted_amount = (*amount as f64 * reduction_ratio) as u64; - TxOut { - value: adjusted_amount, - script_pubkey: recipient.script_pubkey(), - } - }) - .collect() + let mut outputs = Vec::with_capacity(recipients.len()); + let mut remaining = available_after_fee; + for (idx, (recipient, amount)) in recipients.iter().enumerate() { + let adjusted_amount = if idx + 1 == recipients.len() { + remaining + } else { + let portion = ((*amount as u128) * (available_after_fee as u128) + / (total_amount as u128)) as u64; + remaining = remaining + .checked_sub(portion) + .ok_or_else(|| "Fee distribution underflow".to_string())?; + portion + }; + outputs.push(TxOut { + value: adjusted_amount, + script_pubkey: recipient.script_pubkey(), + }); + } + outputs🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/wallet/mod.rs` around lines 1722 - 1739, The proportional reduction uses floating math and truncation which can under-allocate outputs and over-collect fee; instead compute adjusted_amounts with integer arithmetic: compute available_after_fee = total_amount - fee, for each recipient compute base = amount * available_after_fee / total_amount and remainder = amount * available_after_fee % total_amount, collect base values and the remainders, then deterministically distribute the summed remainders (e.g., sort recipients by a stable key or use original index order) by adding 1 sat to the first k outputs until the total of outputs equals available_after_fee; update the TxOut construction in the subtract_fee_from_amount branch (references: outputs, subtract_fee_from_amount, available_after_fee, total_amount, fee, reduction_ratio, TxOut, recipients) to use this integer division + remainder distribution.
🧹 Nitpick comments (1)
src/model/wallet/mod.rs (1)
2790-2905: Tests cover the new fee-subtraction path.Consider adding a multi-recipient case to assert the proportional split sums to
total_amount - feeonce remainder handling is tightened.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/wallet/mod.rs` around lines 2790 - 2905, Add a new test that exercises build_multi_recipient_payment_transaction_inner with multiple recipients and subtract_fee_from_amount=true to verify the proportional split sums to total_amount - fee and that the implied fee matches; use test_wallet_with_bip44_utxo to create a wallet with enough UTXO, register the address via register_test_address, construct multiple distinct recipient addresses and requested shares, call wallet.build_multi_recipient_payment_transaction_inner(None, &db, Network::Testnet, &[(r1, a1), (r2, a2), ...], fee, true), then assert that the sum of the outputs whose script_pubkey matches the recipients equals (sum of requested amounts) - fee (and that total_input - total_output == fee), and add assertions that the per-recipient outputs follow the expected proportional distribution and deterministic remainder handling (e.g., floor division plus remainder assigned predictably) so the split logic is covered.
🤖 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/backend_task/core/mod.rs`:
- Around line 394-438: The fee reconciliation can still underpay after one
rebuild; update the logic around
build_multi_recipient_payment_transaction/calculate_relay_fee/estimate_p2pkh_tx_size
so that when request.override_fee.is_none() you perform a small bounded retry
loop (e.g. max 3 attempts) that: compute actual_fee from tx size, if actual_fee
> current_fee then reload_utxos(self)?, rebuild tx with actual_fee, recompute
rebuilt_fee and set current_fee = rebuilt_fee (or actual_fee) and continue until
actual_fee <= rebuilt_fee (stable) or attempts exhausted; if attempts exhausted
return an error instead of just warning and broadcasting. Ensure SPV/no-reload
still returns the existing error path and keep the override_fee short-circuit
behavior.
---
Outside diff comments:
In `@src/model/wallet/mod.rs`:
- Around line 1722-1739: The proportional reduction uses floating math and
truncation which can under-allocate outputs and over-collect fee; instead
compute adjusted_amounts with integer arithmetic: compute available_after_fee =
total_amount - fee, for each recipient compute base = amount *
available_after_fee / total_amount and remainder = amount * available_after_fee
% total_amount, collect base values and the remainders, then deterministically
distribute the summed remainders (e.g., sort recipients by a stable key or use
original index order) by adding 1 sat to the first k outputs until the total of
outputs equals available_after_fee; update the TxOut construction in the
subtract_fee_from_amount branch (references: outputs, subtract_fee_from_amount,
available_after_fee, total_amount, fee, reduction_ratio, TxOut, recipients) to
use this integer division + remainder distribution.
---
Nitpick comments:
In `@src/model/wallet/mod.rs`:
- Around line 2790-2905: Add a new test that exercises
build_multi_recipient_payment_transaction_inner with multiple recipients and
subtract_fee_from_amount=true to verify the proportional split sums to
total_amount - fee and that the implied fee matches; use
test_wallet_with_bip44_utxo to create a wallet with enough UTXO, register the
address via register_test_address, construct multiple distinct recipient
addresses and requested shares, call
wallet.build_multi_recipient_payment_transaction_inner(None, &db,
Network::Testnet, &[(r1, a1), (r2, a2), ...], fee, true), then assert that the
sum of the outputs whose script_pubkey matches the recipients equals (sum of
requested amounts) - fee (and that total_input - total_output == fee), and add
assertions that the per-recipient outputs follow the expected proportional
distribution and deterministic remainder handling (e.g., floor division plus
remainder assigned predictably) so the split logic is covered.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
docs/ai-design/2026-02-24-fee-consolidation/manual-test-scenarios.mdsrc/backend_task/core/mod.rssrc/backend_task/identity/register_identity.rssrc/backend_task/identity/top_up_identity.rssrc/model/wallet/asset_lock_transaction.rssrc/model/wallet/mod.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- src/backend_task/identity/top_up_identity.rs
- docs/ai-design/2026-02-24-fee-consolidation/manual-test-scenarios.md
- src/backend_task/identity/register_identity.rs
Comment on lines
+394
to
+438
| // First attempt with estimated fee. | ||
| let mut tx = wallet_guard.build_multi_recipient_payment_transaction( | ||
| self, | ||
| self.network, | ||
| &parsed_recipients, | ||
| DEFAULT_TX_FEE, | ||
| initial_fee, | ||
| request.subtract_fee_from_amount, | ||
| )? | ||
| )?; | ||
|
|
||
| // If override_fee was supplied, trust it — skip recalculation. | ||
| if request.override_fee.is_none() { | ||
| let actual_fee = | ||
| calculate_relay_fee(estimate_p2pkh_tx_size(tx.input.len(), tx.output.len())); | ||
| if actual_fee > initial_fee { | ||
| // The real fee is higher; rebuild with the correct fee. | ||
| // The first build already removed UTXOs, so reload the wallet | ||
| // state and rebuild. In practice this rarely triggers because | ||
| // the initial 5-input estimate is usually sufficient. | ||
| if !wallet_guard.reload_utxos(self)? { | ||
| // SPV mode: reload is a no-op, cannot recover removed UTXOs. | ||
| // This path should only be reached in RPC mode. | ||
| return Err( | ||
| "Fee re-estimation failed: cannot reload UTXOs in SPV mode".to_string() | ||
| ); | ||
| } | ||
| tx = wallet_guard.build_multi_recipient_payment_transaction( | ||
| self, | ||
| self.network, | ||
| &parsed_recipients, | ||
| actual_fee, | ||
| request.subtract_fee_from_amount, | ||
| )?; | ||
| // Sanity check: rebuilt tx should not need a higher fee | ||
| let rebuilt_fee = calculate_relay_fee(estimate_p2pkh_tx_size( | ||
| tx.input.len(), | ||
| tx.output.len(), | ||
| )); | ||
| if actual_fee < rebuilt_fee { | ||
| tracing::warn!( | ||
| "Fee still insufficient after rebuild: {} < {}", | ||
| actual_fee, | ||
| rebuilt_fee | ||
| ); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Fee reconciliation can still underpay after a rebuild.
If the rebuild selects additional inputs, the required relay fee can increase again. The current code only warns and still broadcasts, which can reintroduce “min relay fee not met.” Consider a small bounded retry (or fail fast) until the fee stabilizes.
🔧 Suggested bounded retry to converge on a stable fee
- // First attempt with estimated fee.
- let mut tx = wallet_guard.build_multi_recipient_payment_transaction(
+ // First attempt with estimated fee.
+ let mut tx = wallet_guard.build_multi_recipient_payment_transaction(
self,
self.network,
&parsed_recipients,
initial_fee,
request.subtract_fee_from_amount,
)?;
// If override_fee was supplied, trust it — skip recalculation.
if request.override_fee.is_none() {
- let actual_fee =
- calculate_relay_fee(estimate_p2pkh_tx_size(tx.input.len(), tx.output.len()));
- if actual_fee > initial_fee {
- // The real fee is higher; rebuild with the correct fee.
- // The first build already removed UTXOs, so reload the wallet
- // state and rebuild. In practice this rarely triggers because
- // the initial 5-input estimate is usually sufficient.
- if !wallet_guard.reload_utxos(self)? {
- // SPV mode: reload is a no-op, cannot recover removed UTXOs.
- // This path should only be reached in RPC mode.
- return Err(
- "Fee re-estimation failed: cannot reload UTXOs in SPV mode".to_string()
- );
- }
- tx = wallet_guard.build_multi_recipient_payment_transaction(
- self,
- self.network,
- &parsed_recipients,
- actual_fee,
- request.subtract_fee_from_amount,
- )?;
- // Sanity check: rebuilt tx should not need a higher fee
- let rebuilt_fee = calculate_relay_fee(estimate_p2pkh_tx_size(
- tx.input.len(),
- tx.output.len(),
- ));
- if actual_fee < rebuilt_fee {
- tracing::warn!(
- "Fee still insufficient after rebuild: {} < {}",
- actual_fee,
- rebuilt_fee
- );
- }
- }
+ const MAX_FEE_REBUILDS: usize = 3;
+ let mut target_fee = initial_fee;
+ for _ in 0..MAX_FEE_REBUILDS {
+ let required_fee = calculate_relay_fee(
+ estimate_p2pkh_tx_size(tx.input.len(), tx.output.len()),
+ );
+ if required_fee <= target_fee {
+ break;
+ }
+ if !wallet_guard.reload_utxos(self)? {
+ return Err(
+ "Fee re-estimation failed: cannot reload UTXOs in SPV mode".to_string()
+ );
+ }
+ target_fee = required_fee;
+ tx = wallet_guard.build_multi_recipient_payment_transaction(
+ self,
+ self.network,
+ &parsed_recipients,
+ target_fee,
+ request.subtract_fee_from_amount,
+ )?;
+ }
+ let final_required = calculate_relay_fee(
+ estimate_p2pkh_tx_size(tx.input.len(), tx.output.len()),
+ );
+ if final_required > target_fee {
+ return Err(format!(
+ "Fee re-estimation failed: required {}, used {}",
+ final_required, target_fee
+ ));
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/backend_task/core/mod.rs` around lines 394 - 438, The fee reconciliation
can still underpay after one rebuild; update the logic around
build_multi_recipient_payment_transaction/calculate_relay_fee/estimate_p2pkh_tx_size
so that when request.override_fee.is_none() you perform a small bounded retry
loop (e.g. max 3 attempts) that: compute actual_fee from tx size, if actual_fee
> current_fee then reload_utxos(self)?, rebuild tx with actual_fee, recompute
rebuilt_fee and set current_fee = rebuilt_fee (or actual_fee) and continue until
actual_fee <= rebuilt_fee (stable) or attempts exhausted; if attempts exhausted
return an error instead of just warning and broadcasting. Ensure SPV/no-reload
still returns the existing error path and keep the override_fee short-circuit
behavior.
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
DEFAULT_TX_FEEof 1000 duffs with size-based fee calculation. Fixes "min relay fee not met" errors for larger transactions (e.g., multi-input asset locks). Deduplicates 3 copies ofestimate_p2pkh_tx_sizeinto a single utility infee_estimation.rs.PlatformFeeEstimatormethods that computemax(legacy, transition-based)fees internally, so callers don't need dual-estimation logic. Reduces public API surface by making internal helpers private and removing dead code.FundWithUtxopaths in identity registration/top-up now useremove_selected_utxos()instead of manualretain().Test plan
cargo test --all-features --workspace— includes new fee estimation tests🤖 Generated with Claude Code
🤖 Co-authored by Claudius the Magnificent AI Agent
Summary by CodeRabbit
New Features
Bug Fixes
Documentation