This fixes a problem with `bdk_electrum` making bogus `transaction_get_merkle` calls to the Electrum server.
In electrum, heights returned alongside txs may be 0 or -1. 0 means the tx is unconfirmed. -1 means the tx is unconfirmed and spends an unconfirmed tx.
Previously, the codebase assumed that heights cannot be negative and used a `i32 as usize` cast (which would lead to the casted height being 18446744073709551615). Then the codebase will try to call `transaction_get_merkle` with the overflowed height.
### Notes to the reviewers
The test introduced in this PR does not fail with the old codebase. What ends up happening is that `transaction_get_merke` will be called with the overflow height and the Electrum server will return with "merkle not found".
To see the failure, you can change the `height as usize` casts to use `.try_into().expect()` then you will see a panic.
These changes should be applied as `1.0.1` and `1.1.1` fixes.
### Changelog notice
* Fix `bdk_electrum` handling of negative spk-history height.
### 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
~* [ ] This pull request breaks the existing API~
~* [ ] I'm linking the issue being fixed by this PR~
志宇 [Sun, 16 Feb 2025 04:14:41 +0000 (15:14 +1100)]
test(electrum): Chained mempool sync
Is an spk history contains a tx that spends another unconfirmed tx, the
Electrum API will return the tx with a negative height. This should
succeed and not panic.
志宇 [Sat, 15 Feb 2025 14:47:20 +0000 (01:47 +1100)]
fix(electrum): Handle negative heights properly
In electrum, heights returned alongside txs may be 0 or -1. 0 means the
tx is unconfirmed. -1 means the tx is unconfirmed and spends an
unconfirmed tx.
Previously, the codebase assumed that heights cannot be negative and
used a `i32 as usize` cast (which would lead to panic if the i32 is
negative).
* CI complained about docs here. https://github.com/bitcoindevkit/bdk/actions/runs/13420654916/job/37492201157
* CI complained about MSRV not being satisfied. https://github.com/bitcoindevkit/bdk/actions/runs/13420654913/job/37492254119
### 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
Automated update to Github CI workflow `cont_integration.yml` by [create-pull-request](https://github.com/peter-evans/create-pull-request) GitHub action
As I was developing the changes in #1798 I discover issue #1810. So I introduced the fixes in that PR but later I split them in two to ease the review by suggestion of @oleonardolima .
The `preselect_utxos` method has an off-by-one error that is making the selection of optional UTxOs too restrictive, by
requiring the coinbase outputs to surpass or equal coinbase maturity time at the current height of the selection, and
not in the block in which the transaction may be included in the blockchain.
The changes in this commit fix it by considering the maturity of the coinbase output at the spending height and not
the transaction creation height, this means, a +1 at the considered height at the moment of building the
transaction.
Fixes #1810.
### Notes to the reviewers
Tests for issue #1810 have not been explicitly added, as there already was a `text_spend_coinbase` test which was corrected to ensure coinbase maturation is considered in alignment with the new logic.
Changes are not breaking but I'm modifying slightly the documentation for the public method `TxBuilder::current_height` to adjust to the fixed code. Does this merit an entry in the CHANGELOG?
### Changelog notice
`Wallet` now considers a utxo originated from a coinbase transaction (`coinbase utxo`) as available for selection if it will mature in the next block after the height provided to the selection, the current height by default. The previous behavior allowed selecting a `coinbase utxo` only when the height at the moment of selection was equal to maturity height or greater.
### Checklists
* [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
* [x] I've updated existing tests to match the fix
* [x] I've updated docs to match the fix logic
* [x] This pull request DOES NOT break the existing API
* [x] I'm linking the issue being fixed by this PR
nymius [Wed, 12 Feb 2025 19:44:15 +0000 (16:44 -0300)]
fix(wallet): off-by-one error checking coinbase maturity in optional UTxOs
The `preselect_utxos` method has an off-by-one error that is making the
selection of optional UTxOs too restrictive, by requiring the coinbase
outputs to surpass or equal coinbase maturity time at the current height
of the selection, and not in the block in which the transaction may be
included in the blockchain.
The changes in this commit fix it by considering the maturity of the
coinbase output at the spending height and not the transaction creation
height, this means, a +1 at the considered height at the moment of
building the transaction.
### Description
`cc` dependency was unpinned to fix a clash with `cmake` that was causing CI errors.
### Changelog notice
* Unpinned cc dependency.
### 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
Steve Myers [Tue, 4 Feb 2025 17:11:51 +0000 (11:11 -0600)]
Merge bitcoindevkit/bdk#1738: test(core): add unit tests for `merge` trait
fcc0a3409aa390e9f32a3308dd5fe0e2e33b9b44 test: add tests for trait of operations on various data structures, including conflict scenarios and checking for empty structures (pluveto)
Pull request description:
### Description
This update adds unit tests for the `Merge` trait implementations on `BTreeMap`, `BTreeSet`, `Vec`, and tuples. It also includes tests for the `is_empty` and `take` methods to ensure their correct functionality.
### Notes to the Reviewers
- The tests cover basic scenarios for merging collections and checking their emptiness.
- Consider the potential method name conflict with `BTreeSet`'s existing `take` method.
### Changelog Notice
- Added unit tests for merging functionality and utility methods (`is_empty`, `take`) on various collection types.
### 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.
<!-- You can erase any parts of this template not applicable to your Pull Request. -->
### Description
I used `zizmor` on all current CI workflows, it's a tool that helps detecting possible vulnerabilities in our CI jobs, see https://woodruffw.github.io/zizmor/.
It can run against most of it's audit rules, however the ones that require the GitHub API Token would require some with access to it in order to test against it. So this PR does not cover for impostor-commit, ref-confusion known-vulnerable-actions audit rules.
<!-- Describe the purpose of this PR, what's being adding and/or fixed -->
### Notes to the reviewers
<!-- In this section you can include notes directed to the reviewers, like explaining why some parts
of the PR were done in a specific way -->
### Changelog notice
- Do not persist credentials on GitHub Actions.
<!-- Notice the release manager should include in the release tag message changelog -->
<!-- See https://keepachangelog.com/en/1.0.0/ for examples -->
### 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
Leonardo Lima [Tue, 28 Jan 2025 14:06:55 +0000 (11:06 -0300)]
fix(cont-integration): template injection audit
- fixes the `template_injection` audit failure due to
`matrix.rust.version` usage, use an environement var instead
see: https://woodruffw.github.io/zizmor/audits/#template-injection
The PR adds a `bip158` module to the `bdk_bitcoind_rpc` crate along with a new type `FilterIter` that can be used for retrieving blocks from a full node which contain transactions relevant to a list of script pubkeys.
### Notes to the reviewers
### Changelog notice
`bdk_bitcoind_rpc`: Added `bip158` module as a means of updating `bdk_chain` structures
### 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
#### New Features:
* [x] I've added tests for the new feature
* [x] I've added docs for the new feature
Previously we failed to remove the change output if the wallet has no internal keychain which caused tx building to fail at the new higher feerate. Fix this by mapping the internal keychain to the de-facto change keychain so that the drain output can be recalculated.
fixes #1807
### Changelog notice
Fixed
- wallet: Fixed an issue preventing `build_fee_bump` from re-targeting the drain value for some wallets
### 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
Bumps [Swatinem/rust-cache](https://github.com/swatinem/rust-cache) from 2.7.5 to 2.7.7.
<details>
<summary>Release notes</summary>
<p><em>Sourced from <a href="https://github.com/swatinem/rust-cache/releases">Swatinem/rust-cache's releases</a>.</em></p>
<blockquote>
<h2>v2.7.7</h2>
<p><strong>Full Changelog</strong>: <a href="https://github.com/Swatinem/rust-cache/compare/v2.7.6...v2.7.7">https://github.com/Swatinem/rust-cache/compare/v2.7.6...v2.7.7</a></p>
<h2>v2.7.6</h2>
<h2>What's Changed</h2>
<ul>
<li>Updated artifact upload action to v4 by <a href="https://github.com/guylamar2006"><code>@guylamar2006</code></a> in <a href="https://redirect.github.com/Swatinem/rust-cache/pull/212">Swatinem/rust-cache#212</a></li>
<li>Adds an option to do lookup-only of the cache by <a href="https://github.com/danlec"><code>@danlec</code></a> in <a href="https://redirect.github.com/Swatinem/rust-cache/pull/217">Swatinem/rust-cache#217</a></li>
<li>add runner OS in cache key by <a href="https://github.com/rnbguy"><code>@rnbguy</code></a> in <a href="https://redirect.github.com/Swatinem/rust-cache/pull/220">Swatinem/rust-cache#220</a></li>
<li>Allow opting out of caching $CARGO_HOME/bin. by <a href="https://github.com/benjyw"><code>@benjyw</code></a> in <a href="https://redirect.github.com/Swatinem/rust-cache/pull/216">Swatinem/rust-cache#216</a></li>
</ul>
<h2>New Contributors</h2>
<ul>
<li><a href="https://github.com/guylamar2006"><code>@guylamar2006</code></a> made their first contribution in <a href="https://redirect.github.com/Swatinem/rust-cache/pull/212">Swatinem/rust-cache#212</a></li>
<li><a href="https://github.com/danlec"><code>@danlec</code></a> made their first contribution in <a href="https://redirect.github.com/Swatinem/rust-cache/pull/217">Swatinem/rust-cache#217</a></li>
<li><a href="https://github.com/rnbguy"><code>@rnbguy</code></a> made their first contribution in <a href="https://redirect.github.com/Swatinem/rust-cache/pull/220">Swatinem/rust-cache#220</a></li>
<li><a href="https://github.com/benjyw"><code>@benjyw</code></a> made their first contribution in <a href="https://redirect.github.com/Swatinem/rust-cache/pull/216">Swatinem/rust-cache#216</a></li>
</ul>
<p><strong>Full Changelog</strong>: <a href="https://github.com/Swatinem/rust-cache/compare/v2.7.5...v2.7.6">https://github.com/Swatinem/rust-cache/compare/v2.7.5...v2.7.6</a></p>
</blockquote>
</details>
<details>
<summary>Commits</summary>
<ul>
<li><a href="https://github.com/Swatinem/rust-cache/commit/f0deed1e0edfc6a9be95417288c0e1099b1eeec3"><code>f0deed1</code></a> 2.7.7</li>
<li><a href="https://github.com/Swatinem/rust-cache/commit/008623fb834cadde1d7ccee1a26dc84acb660ec3"><code>008623f</code></a> also cache <code>cargo install</code> metadata</li>
<li><a href="https://github.com/Swatinem/rust-cache/commit/720f7e45ccee46c12a7b1d7bed2ab733be9be5a1"><code>720f7e4</code></a> 2.7.6</li>
<li><a href="https://github.com/Swatinem/rust-cache/commit/4b1f006ad2112a11d66969e219444096a98af937"><code>4b1f006</code></a> update dependencies, in particular <code>@actions/cache</code></li>
<li><a href="https://github.com/Swatinem/rust-cache/commit/e8e63cdbf2788df3801e6f9a81516b2ca8391886"><code>e8e63cd</code></a> Allow opting out of caching $CARGO_HOME/bin. (<a href="https://redirect.github.com/swatinem/rust-cache/issues/216">#216</a>)</li>
<li><a href="https://github.com/Swatinem/rust-cache/commit/9a2e0d32122f6883cb48fad7a1ac5c49f25b7661"><code>9a2e0d3</code></a> add runner OS in cache key (<a href="https://redirect.github.com/swatinem/rust-cache/issues/220">#220</a>)</li>
<li><a href="https://github.com/Swatinem/rust-cache/commit/c00f3025caeee0e9c78c18c43de11ab15fd3b486"><code>c00f302</code></a> Adds an option to do lookup-only of the cache (<a href="https://redirect.github.com/swatinem/rust-cache/issues/217">#217</a>)</li>
<li><a href="https://github.com/Swatinem/rust-cache/commit/68b3cb7503c78e67dae8373749990a220eb65352"><code>68b3cb7</code></a> Updated artifact upload action to v4 (<a href="https://redirect.github.com/swatinem/rust-cache/issues/212">#212</a>)</li>
<li>See full diff in <a href="https://github.com/swatinem/rust-cache/compare/v2.7.5...v2.7.7">compare view</a></li>
</ul>
</details>
<br />
Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`.
<details>
<summary>Dependabot commands and options</summary>
<br />
You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot squash and merge` will squash and merge this PR after your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge and block automerging
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually
- `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency
- `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
The `Testnet4` variant is included in `keys::any_network` and `keys::test_networks`. Tests are updated accordingly and a new assertion is added to `test_descriptor_from_str_with_keys_network` checking the result of `into_wallet_descriptor` when the specified network is `Network::Testnet4`.
### Changelog notice
Added:
- `Wallet` can now be constructed using `Network::Testnet4`
### 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
valued mammal [Fri, 24 Jan 2025 14:13:08 +0000 (09:13 -0500)]
fix(wallet): use `map_keychain` in `Wallet::build_fee_bump`
Previously we failed to remove the change output if the wallet has
no internal keychain which caused tx building to fail at the new
higher feerate. Fix this by mapping the internal keychain to the
de-facto change keychain so that the drain output can be
recalculated.
<!-- You can erase any parts of this template not applicable to your Pull Request. -->
### Description
This PR adds a unit test that checks the satisfaction of timestamp-based timelocks. The goal is to test the absolute time therefore the variable passed to the miniscript fragment `after` has to be equal to or greater that 500_000_000 otherwise it would be checking the block height.
### Notes to the reviewers
<!-- In this section you can include notes directed to the reviewers, like explaining why some parts
of the PR were done in a specific way -->
This unit test tries to check if #642 is still an issue.
### 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
<!-- You can erase any parts of this template not applicable to your Pull Request. -->
### Description
<!-- Describe the purpose of this PR, what's being adding and/or fixed -->
It adds a secondary MSRV version of `1.75.0` for the `bdk_electrum` crate, excluding it from the `1.63.0` MSRV dependency pinning and CI step.
As it's the new CI will run without `bdk_electrum` for `1.63.0` MSRV step, will run another step for `1.75.0` MSRV and the latest stable.
Currently,, the only affected crate is `electrum-client`, therefore `bdk-electrum`, as it relies directly on `rustls` which migrated to `1.71.0` MSRV. The `esplora-client` relies on `rustls` as a dependency of `minreq` or `reqwest`, but those crates didn't upgraded it's `rustls` versions yet.
In a further PR it should also bump the `electrum-client` crate to it's latest version, which relies on `1.75.0` MSRV, see https://github.com/bitcoindevkit/rust-electrum-client/pull/159.
### Notes to the reviewers
<!-- In this section you can include notes directed to the reviewers, like explaining why some parts
of the PR were done in a specific way -->
It's open for discussion if this approach is the expected one, or another one would be better.
### Changelog notice
<!-- Notice the release manager should include in the release tag message changelog -->
<!-- See https://keepachangelog.com/en/1.0.0/ for examples -->
- Add a secondary MSRV of `1.75.0` for `bdk_electrum`.
### 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
#### New Features:
* [ ] I've added tests for the new feature
* [ ] I've added docs for the new feature
#### Bugfixes:
* [ ] This pull request breaks the existing API
* [ ] I've added tests to reproduce the issue which are now passing
* [ ] I'm linking the issue being fixed by this PR
Automated update to Github CI workflow `cont_integration.yml` by [create-pull-request](https://github.com/peter-evans/create-pull-request) GitHub action
This PR adds a unit test for checking that the fee calculation of `bdk_electrum` is correct.
> a "preliminary" tx to an address in Core's wallet such that the output created becomes the input of the next tx. This preliminary tx is the prev_tx that needs to be fetched in order to calculate the fee of the tx sent to the receiver. This way is better because the required outpoint is less determined. Then we assert we have the right (outpoint, txout) in the update TxGraph https://github.com/bitcoindevkit/bdk/issues/1444#issuecomment-2112751282
### Notes to the reviewers
fixes: #1444
### Changelog notice
### 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
<!-- You can erase any parts of this template not applicable to your Pull Request. -->
### Description
Changes the default version number to version 2 to enchance privacy for the type of wallet used
Fixes #1676
### Changelog notice
- Changed the default transaction version number in the transaction builder.
- Updated the test to test for the default transaction number.
### 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
<!-- You can erase any parts of this template not applicable to your Pull Request. -->
fix #1711
### Description
This PR replaces how currently potentially bug-prone PSBT's input access is done using a more carefully approach.
### Notes to the reviewers
<!-- In this section you can include notes directed to the reviewers, like explaining why some parts
of the PR were done in a specific way -->
### Changelog notice
<!-- Notice the release manager should include in the release tag message changelog -->
<!-- See https://keepachangelog.com/en/1.0.0/ for examples -->
### 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
benalleng [Thu, 2 Jan 2025 17:09:15 +0000 (12:09 -0500)]
fix(tx_builder): change default tx to version 2
This change improves privacy since >85% of transactions on the network are version 2. Version 2 is also necessary to eventually implement BIP 326 nSequence-based anti-fee-sniping.
Opening this up as a record of past decisions. The template is inspired by the one used by uniffi. Whether or not we decide to add these to the repo, it's an opportunity to learn, discuss, and collab when it comes to design and architecture.
The first 2 ADRs pertain to bdk's approach to data persistence. Another recent suggestion was to document how we're thinking about single vs multiple descriptor wallets. Feedback welcome.
Cleanup and remove unused code in `Wallet::create_tx`, this was noticed during review of #1763. See: https://github.com/bitcoindevkit/bdk/pull/1763#discussion_r1876740140
fixes #1710
### Notes to the reviewers
In addition to removing the unneeded assignments to `fee_amount` and `received` I also refactored creation of the change output to be an `if let` instead of `match` statement since it only needs to do something if there is `Excess::Change`.
I should have done this cleanup as part of #1048.
### Changelog notice
None, only internal cleanup.
### 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
This cleans up the `test_wallet_transactions_relevant` test based on suggestions that didn't make it into #1779.
### Notes to the reviewers
Also included a small use cleanup in wallet/mod.
### Changelog notice
None.
### 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
Steve Myers [Thu, 19 Dec 2024 03:55:18 +0000 (21:55 -0600)]
Bump bdk_wallet version to 1.0.0
bdk_core to 0.4.1
bdk_chain to 0.21.1
bdk_bitcoind_rpc to 0.17.1
bdk_electrum to 0.20.1
bdk_esplora to 0.20.1
bdk_file_store to 0.18.1
bdk_testenv to 0.11.1
Currently the behavior of `Wallet::transactions` is not well defined and unintuitive.
The approach taken in this PR is to make `Wallet::transactions` return what most wallets would like the caller to see (i.e. transactions that are part of the canonical history and spend from/to a tracked spk). A.k.a make the method more restrictive.
Documentation is updated to refer the caller to the underlying `bdk_chain` structures for any over usecase.
After #1765 gets merged, the behavior of `Wallet::transactions` will become even more unintuitive. Refer to https://github.com/bitcoindevkit/bdk/pull/1765#pullrequestreview-2503968540.
### Notes to the reviewers
**Why not have multiple methods in `Wallet` that return different sets of transactions?**
I think it's better to only provide common usecase histories from `Wallet` and I can only think of one.
### Changelog notice
* Change `Wallet::transactions` to only include "relevant" transactions.
### 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
Automated update to Github CI workflow `cont_integration.yml` by [create-pull-request](https://github.com/peter-evans/create-pull-request) GitHub action
I manually fixed new clippy warnings.
I also changed CI to:
~~1. not build or run tests or test dependencies when building with MSRV. This reduced the number of dependencies we need to pin for MSRV and fixes 1398.~~ I removed these changes.
2. use the same version or rust as in the `rust-version` file for all jobs instead of just for clippy. I made this change to prevent CI breaks when stable changes before we have a chance to fix the auto created PR that bumps the `rust-version` file.
I noticed these warnings while publishing beta.6 and would like to get them fixed before the final `bdk_wallet` 1.0.0. With default features we should have any build warnings. To reproduce:
* [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
<!-- You can erase any parts of this template not applicable to your Pull Request. -->
### Description
Adds an example on what `used` stands for, and make it explicit that it
has the same behavior as `Wallet::reveal_next_address` in the scenario
where all previously revealed addresses have been used.
<!-- Describe the purpose of this PR, what's being adding and/or fixed -->
### Notes to the reviewers
Is there any other behavior of `next_unused_address` we'd need to make clear through documentation ?
<!-- In this section you can include notes directed to the reviewers, like explaining why some parts
of the PR were done in a specific way -->
### Changelog notice
<!-- Notice the release manager should include in the release tag message changelog -->
<!-- See https://keepachangelog.com/en/1.0.0/ for examples -->
- Improve the `Wallet::next_unused_address` documentation to better describe expected behavior/usage.
### 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
#### New Features:
* [ ] I've added tests for the new feature
* [ ] I've added docs for the new feature
#### Bugfixes:
* [ ] This pull request breaks the existing API
* [ ] I've added tests to reproduce the issue which are now passing
* [ ] I'm linking the issue being fixed by this PR
Leonardo Lima [Wed, 4 Dec 2024 18:33:45 +0000 (15:33 -0300)]
docs(wallet): reword the `next_unused_address` doc
docs(wallet): reword the `next_unused_address` doc
Adds an example on what `used` stands for, and make it explicit that it
has the same behavior as `Wallet::reveal_next_address` in the scenario
where all previously revealed addresses have been used.
Steve Myers [Thu, 12 Dec 2024 00:45:01 +0000 (18:45 -0600)]
Bump bdk_wallet version to 1.0.0-beta.6
bdk_core to 0.4.0
bdk_chain to 0.21.0
bdk_bitcoind_rpc to 0.17.0
bdk_electrum to 0.20.0
bdk_esplora to 0.20.0
bdk_file_store to 0.18.0
bdk_testenv to 0.11.0
refactor(coin_selection)!: use Amount and SignedAmount for API and internally
refactor(chain)!: use Amount for DescriptorExt::dust_value()
Using named types make the API and internal code easier to read and reason about since it makes it clear that the values are bitcoin amounts. Also to create these types the units (ie .from_sat()) must be specified.
### Notes to the reviewers
For coin_selection using Amount and SignedAmount makes internal code safer against overflow errors. In particular because these types will panic if an amount overflow occurs. Using u64/i64 on the other hand can silently rollover. See: https://doc.rust-lang.org/book/ch03-02-data-types.html#integer-overflow
This is a continuation of the work done in #1595. Since this is an API breaking change I would like to include it in the 1.0.0-beta milestone if possible.
### Changelog notice
- Change coin_selection to use Amount instead of u64 for all bitcoin amounts.
- Change DescriptorExt::dust_value() to return an Amount instead of u64.
### 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] This pull request breaks the existing API
* [ ] I've added tests to reproduce the issue which are now passing
* [ ] I'm linking the issue being fixed by this PR
Steve Myers [Sun, 8 Dec 2024 01:28:24 +0000 (19:28 -0600)]
refactor(coin_selection)!: use Amount and SignedAmount for API and internally
Using named types make the API and internal code easier to read and reason
about since it makes it clear that the values are bitcoin amounts. Also to
create these types the units (ie .from_sat()) must be specified.
Using Amount and SignedAmount also makes internal code safer against overflow
errors. In particular because these types will panic if an amount overflow
occurs. Using u64/i64 on the otherhand can silently rollover. See:
https://doc.rust-lang.org/book/ch03-02-data-types.html#integer-overflow
Previously, getting the canonical history of transactions/UTXOs required calling `TxGraph::get_chain_position` on each transaction. This was highly inefficient and resulted in an `O(n^2)` algorithm. The situation is especially problematic when we have many unconfirmed conflicts.
This PR introduces an `O(n)` algorithm to determine the canonical set of transactions in `TxGraph`. The algorithm's premise is as follows:
1. If transaction `A` is determined to be canonical, all of `A`'s ancestors must also be canonical.
2. If transaction `B` is determined to be NOT canonical, all of `B`'s descendants must also be NOT canonical.
3. If a transaction is anchored in the best chain, it is canonical.
4. If a transaction conflicts with a canonical transaction, it is NOT canonical.
5. A transaction with a higher last-seen has precedence.
6. Last-seen values are transitive. A transaction's collective last-seen value is the max of it's last-seen value and all of it's descendants.
We maintain two mutually-exclusive `txid` sets: `canoncial` and `not_canonical`.
Imagine a method `mark_canonical(A)` that is based on premise 1 and 2. This method will mark transaction `A` and all of it's ancestors as canonical. For each transaction that is marked canonical, we can iterate all of it's conflicts and mark those as `non_canonical`. If a transaction already exists in `canoncial` or `not_canonical`, we can break early, avoiding duplicate work.
This algorithm iterates transactions in 3 runs.
1. Iterate over all transactions with anchors in descending anchor-height order. For any transaction that has an anchor pointing to the best chain, we call `mark_canonical` on it. We iterate in descending-height order to reduce the number of anchors we need to check against the `ChainOracle` (premise 1). The purpose of this run is to populate `non_canonical` with all transactions that directly conflict with anchored transactions and populate `canonical` with all anchored transactions and ancestors of anchors transactions (transitive anchors).
2. Iterate over all transactions with last-seen values, in descending last-seen order. We can call `mark_canonical` on all of these that do not already exist in `canonical` or `not_canonical`.
3. Iterate over remaining transactions that contains anchors (but not in the best chain) and have no last-seen value. We treat these transactions in the same way as we do in run 2.
```
many_conflicting_unconfirmed::list_canonical_txs
time: [709.46 us 710.36 us 711.35 us]
many_conflicting_unconfirmed::filter_chain_txouts
time: [712.59 us 713.23 us 713.90 us]
many_conflicting_unconfirmed::filter_chain_unspents
time: [709.95 us 711.16 us 712.45 us]
many_chained_unconfirmed::list_canonical_txs
time: [2.2604 ms 2.2641 ms 2.2680 ms]
many_chained_unconfirmed::filter_chain_txouts
time: [3.5763 ms 3.5869 ms 3.5979 ms]
many_chained_unconfirmed::filter_chain_unspents
time: [3.5540 ms 3.5596 ms 3.5652 ms]
nested_conflicts_unconfirmed::list_canonical_txs
time: [660.06 us 661.75 us 663.60 us]
nested_conflicts_unconfirmed::filter_chain_txouts
time: [650.15 us 651.36 us 652.71 us]
nested_conflicts_unconfirmed::filter_chain_unspents
time: [658.37 us 661.54 us 664.81 us]
```
```
many_conflicting_unconfirmed::list_canonical_txs
time: [94.618 ms 94.966 ms 95.338 ms]
many_conflicting_unconfirmed::filter_chain_txouts
time: [159.31 ms 159.76 ms 160.22 ms]
many_conflicting_unconfirmed::filter_chain_unspents
time: [163.29 ms 163.61 ms 163.96 ms]
# I gave up running the rest of the benchmarks since they were taking too long.
```
### Notes to the reviewers
* ***PLEASE MERGE #1733 BEFORE THIS PR!*** We had to change the signature of `ChainPosition` to account for transitive anchors and unconfirmed transactions with no `last-seen` value.
* The canonicalization algorithm is contained in `/crates/chain/src/canonical_iter.rs`.
* Since the algorithm requires traversing transactions ordered by anchor height, and then last-seen values, we introduce two index fields in `TxGraph`; `txs_by_anchor` and `txs_by_last_seen`. Methods `insert_anchor` and `insert_seen_at` are changed to populate these index fields.
* An ADR is added: `docs/adr/0003_canonicalization_algorithm.md`. This is based on the work in #1592.
### Changelog notice
* Added: Introduce an `O(n)` canonicalization algorithm. This logic is contained in `/crates/chain/src/canonical_iter.rs`.
* Added: Indexing fields in `TxGraph`; `txs_by_anchor_height` and `txs_by_last_seen`. Pre-indexing allows us to construct the canonical history more efficiently.
* Removed: `TxGraph` methods: `try_get_chain_position` and `get_chain_position`. This is superseded by the new canonicalization algorithm.
### 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
#### New Features:
* [x] I've added tests for the new feature
* [x] I've added docs for the new feature
#### Bugfixes:
* [x] This pull request breaks the existing API
* [x] I've added tests to reproduce the issue which are now passing
* [x] I'm linking the issue being fixed by this PR
Jiri Jakes [Thu, 5 Dec 2024 08:51:37 +0000 (16:51 +0800)]
refactor(wallet): Reuse chain position instead of obtaining new one
In `Wallet::preselect_utxos()`, the code used to obtain chain position
of the UTXO's transaction from the graph, however the chain position
is already recorded within the UTXO's representation (`LocalOutput`).
This patch reuses the existing chain position instead of obtaining a
fresh one.
perf(chain): add benchmarks for canonicalization logic
This is mostly taken from #1735 except we inline many of the functions
and test `list_canonical_txs`, `filter_chain_unspents` and
`filter_chain_txouts` on all scenarios.
This is an O(n) algorithm to determine the canonical set of txids.
* Run 1: Iterate txs with anchors, starting from highest anchor height
txs.
* Run 2: Iterate txs with last-seen values, starting from highest
last-seen values.
* Run 3: Iterate txs that are remaining from run 1 which are not
anchored in the best chain.
Since all transitively-anchored txs are added to the `canonical` set in
run 1, and anything that conflicts to anchored txs are already added to
`not_canonial`, we can guarantee that run 2 will not traverse anything
that directly or indirectly conflicts anything that is anchored.
Run 3 is needed in case a tx does not have a last-seen value, but is
seen in a conflicting chain.
`TxGraph` is updated to include indexes `txids_by_anchor_height` and
`txids_by_last_seen`. These are populated by the `insert_anchor` and
`insert_seen_at` methods. Generic constaints needed to be tightened as
these methods need to be aware of the anchor height to create
`LastSeenIn`.
志宇 [Wed, 16 Oct 2024 14:28:23 +0000 (14:28 +0000)]
feat(chain)!: `TxGraph` contain anchors in one field
Previously, the field `TxGraph::anchors` existed because we assumed
there was use in having a partially-chronological order of transactions
there. Turns out it wasn't used at all.
This commit removes anchors from the `txs` field and reworks `anchors`
field to be a map of `txid -> set<anchors>`.
This is a breaking change since the signature of `all_anchors()` is
changed.
Update `TxGraph` field docs for `empty_outspends` and `empty_anchors`.
As expressed by @LLFourn _We want to not depend on serde_json. If we keep it around for serializing anchors we won't be able to remove it in the future because it will always be needed to do migrations_
Currently there is only one widely used anchor, `ConfirmationBlockTime`.
The decision was to constrain support to just be for a single anchor type ConfirmationBlockTime. The anchor table will represent all fields of `ConfirmationBlockTime`, each one in its own column.
The reasons:
- No one is using rusqlite with any other anchor type, and if they are, they can do something custom anyway.
- The anchor representation may change in the future, supporting for multiple Anchor types here will cause more problems for migration later on.
Resolves #1695
### Notes to the reviewers
<details>
<summary>Why the type of the confirmation_time column is INTEGER?</summary>
By [sqlite3 docs](https://www.sqlite.org/datatype3.html):
> Each value stored in an SQLite database (or manipulated by the database engine) has one of the following storage classes:
> ...
> INTEGER. The value is a *signed integer*, stored in 0, 1, 2, 3, 4, 6, or 8 bytes depending on the magnitude of the value.
(Remember confirmation time is `u64`)
> REAL. The value is a floating point value, stored as an 8-byte IEEE floating point number.
> ...
> SQLite uses a more general dynamic type system. In SQLite, the datatype of a value is associated with the value itself, not with its container.
> ...
> In order to maximize compatibility between SQLite and other database engines, ..., SQLite supports the concept of "type affinity" on columns. The type affinity of a column is the recommended type for data stored in that column. The important idea here is that the type is recommended, not required. Any column can still store any type of data. It is just that some columns, given the choice, will prefer to use one storage class over another. The preferred storage class for a column is called its "affinity".
> ...
> A column with NUMERIC affinity may contain values using all five storage classes. When text data is inserted into a NUMERIC column, **the storage class of the text is converted to INTEGER or REAL (in order of preference)** if the text is a well-formed integer or real literal, respectively.
> A column that uses INTEGER affinity behaves the same as a column with NUMERIC affinity.
But anchor table was defined using the STRICT keyword, again, by sqlite docs:
> ... The STRICT keyword causes the following differences:
> ...
> SQLite attempts to coerce the data into the appropriate type using the usual affinity rules, as PostgreSQL, MySQL, SQL Server, and Oracle all do. *If the value cannot be losslessly converted* in the specified datatype, then an SQLITE_CONSTRAINT_DATATYPE error is raised.
So, the *TLDR*, with some help of this [blog post](https://jakegoulding.com/blog/2011/02/06/sqlite-64-bit-integers/) is:
- SQLite INTEGER supported values range from `-2**63 to (2**63-1)`
- If the number is bigger it will treat the number as REAL.
- As the SQLite table is defined with the STRICT keyword, it should enforce a losslessly conversion or fail with SQLITE_CONSTRAINT_DATATYPE.
</details>
<details>
<summary>Why not setting confirmation_time as BLOB or NUMERIC?</summary>
I don't have a strong opinion on this. INTEGER was the first numeric type I found, then later I found NUMERIC and they seemed to behave in the same way, so I didn't change the type.
I discarded BLOB and ANY first because they didn't provide a clear idea of what was being stored in the column, but in retrospective they seem to remove all the considerations that I had to do above, so maybe they are better fitted for the type.
</details>
<details>
<summary>Why adding a default value to confirmation_time column if the anchor column constraint is to be NOT NULL so all copied values will be filled?</summary>
`confirmation_time` should have the same constraint as `anchor`, to be `NOT NULL`, but as UPDATE statements might be executed in already filled tables, you must provide a default value for all the rows you are going to add the new column to. As the `confirmation_time` extraction of the json blob in anchor cannot be performed in the same step, I had to add this default value.
This is flexibilizing the schema of the tables and extending the bug surface it may have, but I'm assuming the application layer will enforce the addition of a valid `confirmation_time` always.
</details>
<details>
<summary>Why the default value of confirmation_time column is -1?</summary>
Considering the other alternatives were to use the max value, min value or zero and confirmation time should always be positive, I considered `-1` just to be computer and human readable enough to perceive there must be something wrong if the `ConfirmationBlockTime` retrieved by the load of the wallet has this value set as the confirmation time.
</details>
<details>
<summary>Why to not be STRICT with each statement?</summary>
It is a constraint only applicable to tables on creation.
</details>
<details>
<summary>Why not creating a whole new table without anchor column and with the confirmation_time column, copy the content from one to the other and drop the former table?</summary>
Computation cost. I didn't benchmark it, and I don't know how efficient is SQLite engine under the hood, but at first sight it seems copying a single column is better than copying four.
</details>
<!-- In this section you can include notes directed to the reviewers, like explaining why some parts
of the PR were done in a specific way -->
### Changelog notice
- Reduce `rusqlite` implementation only to `ChangeSet<ConfirmationBlockTime>`.
- Replace `anchor` column in sqlite by `confirmation_time`.
- Modify `migrate_schema` `versioned_script` parameter's type to `&[&str]`.
- Transformed existing schemas in methods to allow their individual application.
### 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
#### New Features:
* [x] I've added tests for the new feature
* [ ] I've added docs for the new feature
#### Bugfixes:
* [x] This pull request breaks the existing API
* [ ] I've added tests to reproduce the issue which are now passing
* [x] I'm linking the issue being fixed by this PR
`fetch_prev_txout` should not try to query the prevouts of coinbase transactions, as it may result in the Electrum server returning an error which would cause the overall `sync`/`full_scan` to fail. Without this critical bug fix, `bdk_electrum` will crash when someone tracks an address being mined to.
### Notes to the reviewers
### Changelog notice
* `fetch_prev_txout` no longer queries coinbase transactions.
### 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:
* [ ] This pull request breaks the existing API
* [x] I've added tests to reproduce the issue which are now passing
* [x] I'm linking the issue being fixed by this PR
Wei Chen [Wed, 6 Nov 2024 16:33:51 +0000 (00:33 +0800)]
fix(electrum): prevent `fetch_prev_txout` from querying coinbase transactions
\`fetch_prev_txout\` should not try to query the prevouts of coinbase
transactions, as it may result in the Electrum server returning an error
which would cause the overall `sync` to fail.
Update `bdk_esplora` to depend on esplora-client 0.11.0
### Notes to the reviewers
bitcoindevkit/rust-esplora-client#103 added a generic type parameter to `AsyncClient` representing a user-defined `Sleeper` and that change is reflected here in order to call the underlying API methods. We also add a new build feature "tokio" that enables the corresponding feature in rust-esplora-client.
closes #1742
### Changelog notice
`bdk_esplora`: Bump `esplora-client` to 0.11.0
### 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
* [x] This pull request breaks the existing API
* [x] I'm linking the issue being fixed by this PR
projects / bdk / log