Skip to content

fix(wallet): include fees when selecting inactive proofs - #2564

Merged
thesimplekid merged 1 commit into
cashubtc:mainfrom
mmalmi:fix/inactive-proof-fees
Sep 22, 2026
Merged

thesimplekid merged 1 commit into
cashubtc:mainfrom
mmalmi:fix/inactive-proof-fees

Conversation

@mmalmi

@mmalmi mmalmi commented Sep 17, 2026 •

Copy link
Copy Markdown
Contributor

Description

When inactive-keyset proofs cover the requested amount before fees, selection returns early without honoring include_fees. An 8-sat proof at 500 ppk leaves only 7 sats after redemption. Apply the existing fee-coverage helper while retaining keyset and coin-age preferences.

Notes to the reviewers

Four regressions fail before the fix; all 51 proof-selection tests pass afterward, including fee opt-out and age ordering. Targeted Clippy, formatting and whitespace checks pass. Independent of #2499; no public API or database changes.

Full just quick-check passes on the current PR head (88469ae6) with Rust 1.98.0, nixpkgs-fmt 1.3.0 and typos 1.46.3. Changed Rust files also pass the repository’s nightly rustfmt import rules.

Suggested CHANGELOG Updates

FIXED

  • Include redemption fees when selecting inactive-keyset proofs.

Checklist

  • Followed the code style guidelines.
  • Ran just quick-check on the current PR head.
  • Wallet API unchanged; no FFI update needed.

@github-project-automation github-project-automation Bot moved this to Backlog in CDK Sep 17, 2026

@j-kon j-kon left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • No critical, warning, or nit-level issues found.
    • Corrects an early return in select_proofs where inactive proofs bypassed include_fees: true, leading to insufficient fund calculations when keysets were rotated.
    • Appropriately routes selected inactive proofs through Self::include_fees.
    • Added comprehensive test coverage in inactive_fee_tests.rs verifying fee coverage, shortfall handling with active proofs, and derivation index age preference.

Summary

Reviewed proof selection logic in crates/cdk/src/wallet/proofs.rs and the new test suite in crates/cdk/src/wallet/proofs/inactive_fee_tests.rs. The fix correctly ensures fee requirements are fulfilled when spending inactive keyset proofs.

Verdict

APPROVE

@thesimplekid

Copy link
Copy Markdown
Collaborator

@cdk-bot review

@cdk-bot cdk-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified findings approved for disclosure:

  • Fee top-up on inactive-proofs branch never reconsiders equal-nominal lower-fee subsets, causing avoidable over-selection (low) - see inline comment
    Unanchored locations included in summary:
    • crates/cdk/src/wallet/proofs.rs:318

@thesimplekid thesimplekid left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants