Skip to content

fix(key-wallet): return the fee the tx actually pays from build/build_signed#872

Merged
QuantumExplorer merged 2 commits into
devfrom
claude/rust-dashcore-871-e5e978
Jul 12, 2026
Merged

fix(key-wallet): return the fee the tx actually pays from build/build_signed#872
QuantumExplorer merged 2 commits into
devfrom
claude/rust-dashcore-871-e5e978

Conversation

@QuantumExplorer

@QuantumExplorer QuantumExplorer commented Jul 12, 2026

Copy link
Copy Markdown
Member

Fixes #871

Problem

TransactionBuilder::build_unsigned() / build_signed() returned a fee computed from the encoded size of the built transaction (fee_rate.calculate_fee(encoded_size(tx))). When the builder drops a dust change remainder (0 < change ≤ 546 duffs), those duffs go to miners — the transaction's real fee is the size fee plus the dust, so the returned value under-reported by up to 546 duffs exactly in the case where the fee got larger. Callers surfacing this value as "the network fee" (e.g. send_payment via FFI/Swift, see dashpay/platform#4049) showed a wrong number.

Fix

Return Σ(selected input values) − Σ(output values) — the fee the transaction actually pays — from both build_unsigned() and build_signed(). Both totals are already in scope, so this removes code rather than adding lookups:

  • build_unsigned(): fee computed from the assembled tx's inputs/outputs.
  • build_signed(): input total captured before signing consumes the UTXO list; output total read from the signed tx.
  • The now-unused encoded_size helper, its error paths (including the reservation-release-on-encode-failure branches), and the Encodable import are removed.
  • Doc comments updated to state the new semantics.

The semantics are unchanged in the normal cases: with emitted change or a drain (SelectionStrategy::All), inputs − outputs equals the size-based estimate. Only the dust-drop case changes, where the new value is the correct one.

Consumer audit

Internal consumers pass the fee through unchanged: key-wallet-ffi (transaction.rs fee_out), AssetLockResult::fee, and transaction_building.rs wrappers. The dash-spv integration tests assert fees via balance deltas, which already match the corrected semantics.

Testing

  • New regression test test_dropped_dust_change_counts_toward_returned_fee: 150 526-duff input, 150 000-duff output → 300-duff dust dropped, returned fee is 526 (not the 226 size fee).
  • cargo test -p key-wallet — 526 passed
  • cargo test -p key-wallet-ffi — all passed
  • cargo test -p dash-spv --test dashd_sync transaction against a real dashd regtest node — 9 passed
  • cargo clippy -p key-wallet --all-features --all-targets and cargo fmt --check — clean

Follow-up

dashpay/platform can now delete the send_payment caller-side recomputation block and take build_signed's fee directly (pinned by the regression tests added in dashpay/platform#4049).

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Improved transaction fee reporting to reflect the actual fee deducted from selected inputs and outputs.
    • Included dropped dust change amounts in the reported transaction fee.
  • Reliability
    • Added a limit of 500 inputs during transaction assembly to support safer transaction building.

…_signed

TransactionBuilder::build_unsigned()/build_signed() returned a fee computed
from the encoded size of the built transaction. When the builder drops a
dust change remainder (0 < change <= 546 duffs), those duffs go to miners,
so the real fee is the size fee plus the dust — the returned value
under-reported by up to 546 duffs exactly when the fee got larger.

Compute the returned fee as sum(selected input values) - sum(output values)
instead, which is what the transaction actually pays in every case
(normal change, exact change, dust drop, and drain). Remove the now-unused
encoded_size helper and its error paths, and update the doc comments.

Fixes #871

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@QuantumExplorer, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 47 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 9e7d1a2d-e9e2-4012-b7f3-62ceb271f854

📥 Commits

Reviewing files that changed from the base of the PR and between bfb02ad and 2e4c1c4.

📒 Files selected for processing (1)
  • key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs
📝 Walkthrough

Walkthrough

TransactionBuilder now reports actual transaction fees as total selected inputs minus final outputs. Both unsigned and signed builds use this calculation, and a regression test verifies that dropped dust change is included in the returned fee.

Changes

Actual transaction fee reporting

Layer / File(s) Summary
Input-output fee calculation
key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs
Unsigned and signed transaction builds return total inputs minus final outputs instead of size-based fee estimates; encoding errors are mapped to BuilderError::InvalidData.
Dropped dust fee regression
key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs
A unit test verifies that omitted dust change contributes to the returned fee.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly states the main change: returning the actual fee from build/build_signed.
Linked Issues check ✅ Passed The fee calculation now matches the issue by using selected inputs minus final outputs, with a regression test for dust-drop behavior.
Out of Scope Changes check ✅ Passed The changes stay focused on fee semantics, documentation, and a regression test, with only minor supporting cleanup.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/rust-dashcore-871-e5e978

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

coderabbitai[bot]
coderabbitai Bot previously approved these changes Jul 12, 2026
@codecov

codecov Bot commented Jul 12, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.74%. Comparing base (d4692bb) to head (2e4c1c4).

Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #872      +/-   ##
==========================================
+ Coverage   73.67%   73.74%   +0.07%     
==========================================
  Files         325      325              
  Lines       73775    73799      +24     
==========================================
+ Hits        54353    54424      +71     
+ Misses      19422    19375      -47     
Flag Coverage Δ
core 77.08% <ø> (ø)
ffi 47.36% <ø> (ø)
rpc 20.00% <ø> (ø)
spv 91.05% <ø> (+0.21%) ⬆️
wallet 73.33% <100.00%> (+0.08%) ⬆️
Files with missing lines Coverage Δ
.../wallet/managed_wallet_info/transaction_builder.rs 87.59% <100.00%> (+1.93%) ⬆️

... and 4 files with indirect coverage changes

@github-actions github-actions Bot added the ready-for-review CodeRabbit has approved this PR label Jul 12, 2026
…ot fee

An asset lock burns the locked amount into an on-chain OP_RETURN output
mirroring the payload's credit outputs, so it is part of the output sum
and inputs - outputs yields the miner fee only. Add a regression test
proving the returned fee excludes the locked credits.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@github-actions github-actions Bot removed the ready-for-review CodeRabbit has approved this PR label Jul 12, 2026
@QuantumExplorer

Copy link
Copy Markdown
Member Author

Reviewed

@QuantumExplorer
QuantumExplorer merged commit 234e3c4 into dev Jul 12, 2026
36 checks passed
@QuantumExplorer
QuantumExplorer deleted the claude/rust-dashcore-871-e5e978 branch July 12, 2026 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

key-wallet: TransactionBuilder build/build_signed return the size-based fee, not the fee the tx actually pays (dust-drop under-report)

1 participant