We currently allow removing `partial_sigs` from a finalized PSBT, which is relevant to non-taproot inputs, however taproot related PSBT fields were left in place despite the recommendation of BIP371 to remove them once the `final_script_witness` is constructed. This can cause confusion for parsers that encounter extra taproot metadata in an already satisfied input.
Fix this by introducing a new member to SignOptions `remove_taproot_extras`, which when true will remove extra taproot related data from a PSBT upon successful finalization. This change makes removal of all taproot extras the default but configurable.
fixes #1243
### Notes to the reviewers
If there's a better or more descriptive name for `remove_taproot_extras`, I'm open to changing it.
### Changelog notice
Fixed an [issue](https://github.com/bitcoindevkit/bdk/issues/1243) finalizing taproot inputs following BIP371
### 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
This follows a similar approach to #1141 namely to remove `FeeRate` from `bdk::types` and instead defer to the upstream implementation for all fee rates. The idea is that making the switch allows BDK to benefit from a higher level abstraction, leaving the implementation details largely hidden.
As noted in #774, we should avoid extraneous conversions that can result in deviations in estimated transaction size and calculated fee amounts, etc. This would happen for example whenever calling a method like `FeeRate::to_sat_per_vb_ceil`. The only exception I would make is if we must return a fee rate error to the user, we might prefer to display it in the more familiar sats/vb, but this would only be useful if the rate can be expressed as a float.
### Notes to the reviewers
`bitcoin::FeeRate` is an integer whose native unit is sats per kilo-weight unit. In order to facilitate the change, a helper method `feerate_unchecked` is added and used only in wallet tests and psbt tests as necessary to convert existing fee rates to the new type. It's "unchecked" in the sense that we're not checking for integer overflow, because it's assumed we're passing a valid fee rate in a unit test.
Potential follow-ups can include:
- [x] Constructing a proper `FeeRate` from a `u64` in all unit tests, and thus obviating the need for the helper `feerate_unchecked` going forward.
- [x] Remove trait `Vbytes`.
- Consider adding an extra check that the argument to `TxBuilder::drain_to` is within "standard" size limits.
- Consider refactoring `coin_selection::select_sorted_utxos` to be efficient and readable.
closes #1136
### Changelog notice
- Removed `FeeRate` type. All fee rates are now rust-bitcoin [`FeeRate`](https://docs.rs/bitcoin/latest/bitcoin/blockdata/fee_rate/struct.FeeRate.html).
- Removed trait `Vbytes`.
### 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 method was not used (so it was untested) and it was not working. This fixes it.
The old implementation used `.next_store_index` which returned the keychain's last index stored in `.inner` (which include lookahead spks). This is WRONG because `.replenish_lookahead` needs the difference from last revealed.
### Changelog notice
Fix `KeychainTxOutIndex::lookahead_to_target`
### 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
vmammal [Wed, 31 Jan 2024 21:18:09 +0000 (16:18 -0500)]
fix(bdk): Remove extra taproot fields when finalizing Psbt
We currently allow removing `partial_sigs` from a finalized Psbt,
which is relevant to non-taproot inputs, however taproot related Psbt
fields were left in place despite the recommendation of BIP371 to remove
them once the `final_script_witness` is constructed. This can cause
confusion for parsers that encounter extra taproot metadata in an
already satisfied input.
Fix this by introducing a new member to SignOptions
`remove_taproot_extras`, which when true will remove extra taproot
related data from a Psbt upon successful finalization. This change
makes removal of all taproot extras the default but configurable.
test(wallet): Add test
`test_taproot_remove_tapfields_after_finalize_sign_option`
that checks various fields have been cleared for taproot
Psbt `Input`s and `Output`s according to BIP371.
to verify the signature of the input and ensure the right internal
key is used to sign. This fixes a shortcoming of a previous
version of the test that relied on the result of `finalize_psbt`
but would have erroneously allowed signing with the wrong key.
### 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
bdk_chain to 0.10.0
bdk_bitcoind_rpc to 0.5.0
bdk_electrum to 0.8.0
bdk_esplora to 0.8.0
bdk_file_store to 0.6.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
- Fix bug in `tx_graph::ChangeSet::is_empty` where is returns true even when it wasn't empty
### 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
Merge bitcoindevkit/bdk#1308: feat(esplora): include previous `TxOut`s for fee calculation
552f11cb5f66a9366dbc05c801fcae043ebe5b82 feat(esplora): include previous `TxOut`s for fee calculation The previous `TxOut` for transactions received from an external wallet are added as floating `TxOut`s to `TxGraph` to allow for fee calculation. (Wei Chen)
Pull request description:
### Description
Partially implements #1265.
The previous `TxOut` for transactions received from an external wallet are added as floating `TxOut`s to `TxGraph` to allow for fee calculation.
### Notes to the reviewers
Currently only the `esplora` portion of #1265 has been implemented.
The `electrum` portion will potentially be done in a new PR, as discussed on the 1/30/24 Lib call.
### Checklists
#### To Do:
* [ ] Implement `electrum` portion of #1265.
#### 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
Wei Chen [Wed, 31 Jan 2024 15:41:42 +0000 (23:41 +0800)]
feat(esplora): include previous `TxOut`s for fee calculation
The previous `TxOut` for transactions received from an external
wallet are added as floating `TxOut`s to `TxGraph` to allow for
fee calculation.
* [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
There are several instances in the code where we allow clippy lints that would otherwise be flagged during regular checks. It would be preferable to minimize the number of "clippy allow" attributes either by fixing the affected areas or setting a specific configuration in `clippy.toml`. In cases where we have to allow a particular lint, it should be documented why the lint doesn't apply.
For context see https://github.com/bitcoindevkit/bdk/issues/1127#issuecomment-1784256647 as well as the commit message details.
One area I'm unsure of is whether `Box`ing a large error in 4fc2216 is the right approach. Logically it makes sense to avoid allocating a needlessly heavy `Result`, but I haven't studied the implications or tradeoffs of such a change.
### 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
bdk_chain to 0.9.0
bdk_bitcoind_rpc to 0.4.0
bdk_electrum to 0.7.0
bdk_esplora to 0.7.0
bdk_file_store to 0.5.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
* Changed to implement `ElectrumExt` for all that implements `ElectrumApi`.
### 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
After a second look at the `update_local_chain` implementations, it was clear that they were over complicated. This PR simplifies the `EsploraExt::update_local_chain` method(s) of the `bdk_esplora` crate and adds a whole bunch of tests.
### Notes to the reviewers
The description of #1199 is very brief, however @danielabrozzoni mentioned about potentially-problematic logic with `ASSUME_FINAL_DEPTH`. The logic was indeed convoluted and it did not need to be that way. This PR removes the need for `ASSUME_FINAL_DEPTH`.
* [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~
~* [ ] I've added tests to reproduce the issue which are now passing~ (there are now lots of tests though)
* [x] I'm linking the issue being fixed by this PR
We now filter out duplicate coins before calling `CoinSelectionAlgorithm::coin_select`. If a UTXO exists in both `required_utxos` and `optional_utxos`, only the copy in `required_utxos` will be kept.
Test helper methods are also updated to not create duplicate UTXOs.
### Changelog notice
Fixed
* Filter out duplicate UTXOs before calling `CoinSelectionAlgorithm::coin_select`. If a UTXO exists in both `required_utxos` and `optional_utxos`, only the copy in `required_utxos` will be kept.
### 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
* [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 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 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
We forgot to include the `last_seen` when converting from `ChainPosition` to `ConfirmationTime`.
### Notes to the reviewers
No brainer.
### Changelog notice
* Fix `ConfirmationTime` conversion from `ChainPosition` to include the `last_seen`.
### 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
`EntryIter` performance is improved by reducing syscalls. The underlying file reader is wrapped with `BufReader` (to reduce calls to `read` and `seek`).
Two new tests are introduced. One ensures correct behavior when the last changeset write is too short. The other ensures the next write position is correct after a short read.
### Notes to the reviewers
This is extracted from #1172 as suggested by https://github.com/bitcoindevkit/bdk/pull/1172#pullrequestreview-1817465627.
### Changelog notice
Changed
* `EntryIter` performance is improved by reducing syscalls.
### 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
This method is useful, I'm not sure why it is not public.
I've also updated the docs and tests.
### Changelog notice
Added
* Expose `SpkIterator::new_with_range`
### 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
志宇 [Tue, 23 Jan 2024 04:32:13 +0000 (12:32 +0800)]
fix(file_store): recover file offset after read
Because we use wrap the file with `BufReader` with the `EntryIter`, we
need to sync the `BufReader`'s position with the file's offset when we
drop the `EntryIter`. Therefore we have a custom drop impl for
`EntryIter`.
This PR removes the routines `get_checksum` and `get_checksum_bytes` (in the bdk crate, descriptor/checksum.rs), which have been deprecated in 0.24.0. Consequently, the routine `calc_checksum_bytes_internal` is consolidated into `calc_checksum_bytes`, as the boolean parameter `exclude_hash` is not needed anymore. See also the two TODOs in the touched file.
### Notes to the reviewers
In the second commit, the signature of the function `calc_checksum_bytes` slightly changes, as the `desc` parameter now is declared as `mut`, in order to change the local variable within the function. My rust experience is rather limited, so I'm not sure if this is a problem for users. IIUC, this is comparable to changing a pass-by-value parameter in C++ from `const std::string desc` to `std::string desc`, which is relevant only for the function implementation, but doesn't change the interface.
### Changelog notice
- Remove deprecated `get_checksum` and `get_checksum_bytes` routines
### 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
Some janitor cleaning in the README to update the `example-crates/`
### Notes to the reviewers
None.
### Changelog notice
- fix(readme): update example crates
### 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
Note also, the bit referring to Electrum's API sounds more like a developer note, so I made it a regular code comment.
### 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
* Corrects an awkward use of the word 'an'
* Adds missing backticks, brackets in a few places
### 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] 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
Thank you @thunderbiscuit for raising this issue. I noticed this method borrows the language from `SpkTxOutIndex::sent_and_received`, which this PR doesn't touch. I think in general it's desirable that methods in `bdk_chain` crate would refer to some of the core structures directly.
### 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
Remove gratuitous use of lifetimes in the main persistence struct `Store`. Having lifetimes on this means that you have to keep the magic bytes alive longer than the database which is particularly offensive if you have to send the database to another thread. On top of that the bytes aren't even read.
Introduce block-by-block API for `bdk::Wallet`. A `wallet_rpc` example is added to demonstrate syncing `bdk::Wallet` with the `bdk_bitcoind_rpc` chain-source crate.
The API of `bdk_bitcoind_rpc::Emitter` is changed so the receiver knows how to connect to the block emitted.
### Notes to the reviewers
### Changelog notice
Added
* `Wallet` methods to apply full blocks (`apply_block` and `apply_block_connected_to`) and a method to apply a batch of unconfirmed transactions (`apply_unconfirmed_txs`).
* `CheckPoint::from_block_ids` convenience method.
* `LocalChain` methods to apply a block header (`apply_header` and `apply_header_connected_to`).
* Test to show that `LocalChain` can apply updates that are shorter than original. This will happen during reorgs if we sync wallet with `bdk_bitcoind_rpc::Emitter`.
Fixed
* `InsertTxError` now implements `std::error::Error`.
#### 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
LLFourn [Fri, 19 Jan 2024 00:23:46 +0000 (11:23 +1100)]
feat(persist): Add stage_and_commit to Persist
In the example_cli we were not always committing (seemingly by mistake).
This then caused all the examples to have to compensate by manually
committing.
bdk_chain to 0.8.0
bdk_bitcoind_rpc to 0.3.0
bdk_electrum to 0.6.0
bdk_esplora to 0.6.0
bdk_file_store to 0.4.0
Fixes #1281.
### Notes to the reviewers
Since the version of bdk_chain changed all crates that depend on it also need to have their versions bumped.
### 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 will help avoid confusion with mainnet descriptors.
### 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
Previously `SpkTxOutIndex` methods can be called from `KeychainTxOutIndex` due to the `DeRef` implementation. However, the internal `SpkTxOut` will also contain lookahead spks resulting in an error-prone API.
`SpkTxOutIndex` methods are now not directly callable from `KeychainTxOutIndex`. Methods of `KeychainTxOutIndex` are renamed for clarity. I.e. methods that return an unbounded spk iter are prefixed with `unbounded`.
In addition to this, I also optimized the peek-address logic of `bdk::Wallet` using the optimized `<SpkIterator as Iterator>::nth` implementation.
### Notes to the reviewers
This is mostly refactoring, but can also be considered a bug-fix (as the API before was very problematic).
### Changelog notice
Changed
* Wallet's peek-address logic is optimized by making use of `<SpkIterator as Iterator>::nth`.
* `KeychainTxOutIndex` API is refactored to better differentiate between methods that return unbounded vs stored spks.
* `KeychainTxOutIndex` no longer directly exposes `SpkTxOutIndex` methods via `DeRef`. This was problematic because `SpkTxOutIndex` also contains lookahead spks which we want to hide.
Added
* `SpkIterator::descriptor` method which gets a reference to the internal descriptor.
### 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~
* [x] 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
志宇 [Sat, 13 Jan 2024 12:04:49 +0000 (20:04 +0800)]
refactor(chain)!: revamp `KeychainTxOutIndex` API
Previously `SpkTxOutIndex` methods can be called from
`KeychainTxOutIndex` due to the `DeRef` implementation. However, the
internal `SpkTxOut` will also contain lookahead spks resulting in an
error-prone API.
`SpkTxOutIndex` methods are now not directly callable from
`KeychainTxOutIndex`. Methods of `KeychainTxOutIndex` are renamed for
clarity. I.e. methods that return an unbounded spk iter are prefixed
with `unbounded`.
projects / bdk / log