Skip to content

fix: prevent cumulative change-address index leak in dry-run tx builds#72

Merged
ovitrif merged 1 commit intofix/dsym-debug-symbolsfrom
fix/change-address-index-leak
Mar 4, 2026
Merged

fix: prevent cumulative change-address index leak in dry-run tx builds#72
ovitrif merged 1 commit intofix/dsym-debug-symbolsfrom
fix/change-address-index-leak

Conversation

@ben-kaufman
Copy link

Summary

  • Fix cumulative change-address derivation index leak caused by BDK's TxBuilder::finish() being called for fee estimation / dry-run purposes without cancelling the resulting PSBT. Each uncancelled finish() permanently burns a change address index.
  • Add AggregateWallet::cancel_dry_run_tx() to unmark change addresses on the primary wallet without persisting, and call it on all dry-run and error-after-finish() paths.
  • Add 4 unit tests covering the bug, the fix, error-path handling, and real-tx-after-dry-run interaction.

Detail

BDK's TxBuilder::finish() has two side-effects on the internal (change) keychain: it reveals a new derivation index and marks it as "used". When the PSBT is only used for fee estimation (never signed/broadcast), the marked address is permanently consumed. Repeated fee estimations steadily burn through the change address keyspace — a problem for wallet recovery (gap-limit), resource efficiency, and address hygiene.

cancel_dry_run_tx calls BDK's cancel_tx on the primary wallet to reverse the "used" marking so the next finish() reuses the same index. The reveal itself persists harmlessly in the staged changeset.

Call sites fixed

Call site What changed
build_transaction_psbt — AllRetainingReserve temp tx cancel_tx moved before the ? on calculate_fee so it fires unconditionally
build_transaction_psbt — reserve checks Every early-return error path now calls cancel_dry_run_tx before returning
calculate_transaction_fee cancel_dry_run_tx fires unconditionally before fee result is unwrapped
select_confirmed_utxos cancel_dry_run_tx fires after UTXO collection, before return

Safety

No change-address reuse between broadcast transactions is possible:

  • Mutex<AggregateWallet> serializes all wallet access
  • cancel_dry_run_tx is never called on the broadcast path (send_to_address)
  • Real finish() re-marks the address as "used" and persists it

Test plan

  • test_finish_without_cancel_leaks_change_index — demonstrates the bug
  • test_cancel_dry_run_prevents_cumulative_index_leak — proves the fix across 5 iterations
  • test_cancel_after_failed_intermediate_prevents_leak — error-path coverage
  • test_dry_run_cancel_then_real_tx_reuses_change_address — real tx after dry-run
  • cargo build / cargo fmt / cargo clippy clean

Copy link
Collaborator

@ovitrif ovitrif left a comment

Choose a reason for hiding this comment

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

LGTM.
I will rebuild bindings. Probably can revert to rc.31, since my original release needs to be recreated, as it missed a point.

@ovitrif ovitrif merged commit ee72371 into fix/dsym-debug-symbols Mar 4, 2026
2 checks passed
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.

2 participants