]> Untitled Git - bdk/commitdiff
Merge bitcoindevkit/bdk#1579: fix(wallet): only mark change address used if `create_t...
authorSteve Myers <steve@notmandatory.org>
Mon, 9 Sep 2024 16:31:42 +0000 (11:31 -0500)
committerSteve Myers <steve@notmandatory.org>
Mon, 9 Sep 2024 16:32:34 +0000 (11:32 -0500)
606fa0874db0f10cd1c64de0f1f097b12db3a16d ci: bump actions/upload-artifact to v4 (valued mammal)
75989d8cde3902f226bfa89aae05803b93a7cf1d test(wallet): Add `test_create_tx_increment_change_index` (valued mammal)
b60d1d29cb8908c354b43c49237acbea373c3dc7 fix(wallet): only mark change address used if `create_tx` succeeds (valued mammal)

Pull request description:

  If no drain script is specified in tx params then we get it from the change keychain by looking at the next unused address. Before this PR we marked the index used regardless of whether a change output is finally added. Then if creating a psbt failed, we never restored the unused status of the change address, so creating the next tx would have revealed an extra one.

  We want to mark the change address used so that other callers won't attempt to use the same address between the time we create the tx and when it appears on chain. With this PR we only mark the change address used if we successfully create a psbt and the drain script is used in the change output.

  fixes #1578

  ### Notes to the reviewers

  An early idea was to unmark the change address used if we fail to create a tx due to `InsufficientFunds`, but after looking into it I figure it doesn't totally make sense to mark the address used before we've determined that a change output is necessary. Further, `create_tx` can fail in other ways besides running coin selection, so I moved the `mark_used` logic to the end of the function.

  ### Changelog notice

  Fixed an issue that caused an unused internal address to be skipped when creating transactions (#1578)

  ### Checklists

  #### All Submissions:

  * [x] I've signed all my commits
  * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md)
  * [x] I ran `cargo fmt` and `cargo clippy` before committing

  #### Bugfixes:

  * [x] I've added tests to reproduce the issue which are now passing
  * [x] I'm linking the issue being fixed by this PR

ACKs for top commit:
  notmandatory:
    ACK 606fa0874db0f10cd1c64de0f1f097b12db3a16d

Tree-SHA512: 4715494d901efccff38d636f0538f193ff32db1de44f8d56a98bb0136483f3a8ce1315901bb98117d6870d5b7e4a3bdf3d208f005e2adc0b29625f84a9e8974e


Trivial merge