22aa534d76 feat: use `Amount` on `TxBuilder::add_recipient` (Leonardo Lima)
d5c0e7200c feat: use `Amount` on `spk_txout_index` and related (Leonardo Lima)
8a33d98db9 feat: update `wallet::Balance` to use `bitcoin::Amount` (Leonardo Lima)
Pull request description:
fixes#823
<!-- You can erase any parts of this template not applicable to your Pull Request. -->
### Description
It's being used on `Balance`, and throughout the code, an `u64` represents the amount, which relies on the user to infer its sats, not millisats, or any other representation.
It updates the usage of `u64` on `Balance`, and other APIs:
- `TxParams::add_recipient`
- `KeyChainTxOutIndex::sent_and_received`, `KeyChainTxOutIndex::net_value`
- `SpkTxOutIndex::sent_and_received`, `SpkTxOutIndex::net_value`
<!-- 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 -->
It updates some of the APIs to expect the `bitcoin::Amount`, but it does not update internal usage of u64, such as `TxParams` still expects and uses `u64`, please see the PR comments for related discussion.
### Changelog notice
<!-- Notice the release manager should include in the release tag message changelog -->
<!-- See https://keepachangelog.com/en/1.0.0/ for examples -->
- Changed the `keychain::Balance` struct fields to use `Amount` instead of `u64`.
- Changed the `add_recipient` method on `TxBuilder` implementation to expect `bitcoin::Amount`.
- Changed the `sent_and_received`, and `net_value` methods on `KeyChainTxOutIndex` to expect `bitcoin::Amount`.
- Changed the `sent_and_received`, and `net_value` methods on `SpkTxOutIndex` to expect `bitcoin::Amount`.
### 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
* [ ] 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:
evanlinjin:
ACK 22aa534d76
Tree-SHA512: c4e8198d96c0d66cc3d2e4149e8a56bb7565b9cd49ff42113eaebd24b1d7bfeecd7124db0b06524b78b8891ee1bde1546705b80afad408f48495cf3c02446d02
- update all fields `immature`, ` trusted_pending`, `unstrusted_pending`
and `confirmed` to use the `bitcoin::Amount` instead of `u64`
- update all `impl Balance` methods to use `bitcoin::Amount`
- update all tests that relies on `keychain::Balance`
db47347472 test(wallet): add thread safety test (Rob N)
Pull request description:
### Description
`Wallet` auto-implements `Send` and `Sync` after removing the generic. This test is a compile time error if there are changes to `Wallet` in the future that make it unsafe to send between threads. See #1387 for discussion.
### 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:
* [ ] 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
ACKs for top commit:
evanlinjin:
ACK db47347472
Tree-SHA512: 490e666bc503f15286268db7e5e2f75ee44ad2f80251d6f7a01af2a435023b87607eee33623712433ea8d27511be63c6c1e9cad4159b3fe66a4644cfa9e344fb
bdk_chain to 0.13.0
bdk_bitcoind_rpc to 0.9.0
bdk_electrum to 0.12.0
bdk_esplora to 0.12.0
bdk_file_store to 0.10.0
bdk_testenv to 0.3.0
bdk_persist to 0.2.0
c0374a0eeb feat(chain): `SyncRequest` now uses `ExactSizeIterator`s (志宇)
0f94f24aaf feat(esplora)!: update to use new sync/full-scan structures (志宇)
4c52f3e08e feat(wallet): make wallet compatible with sync/full-scan structures (志宇)
cdfec5f907 feat(chain): add sync/full-scan structures for spk-based syncing (志宇)
Pull request description:
Fixes#1153
Replaces #1194
### Description
Introduce universal structures that represent sync/full-scan requests/results.
### Notes to the reviewers
This is based on #1194 but is different in the following ways:
* The functionality to print scan/sync progress is not reduced.
* `SyncRequest` and `FullScanRequest` is simplified and fields are exposed for more flexibility.
### Changelog notice
* Add universal structures for initiating/receiving sync/full-scan requests/results for spk-based syncing.
* Updated `bdk_esplora` chain-source to make use of new universal sync/full-scan 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
ACKs for top commit:
notmandatory:
tACK c0374a0eeb
Tree-SHA512: c2ad66d972a6785079bca615dfd128edcedf6b7a02670651a0ab1ce5b5174dd96f54644680eedbf55e3f1955fe5c34f632eadbd3f71d7ffde658753c6c6d42be
* Changed `Wallet::apply_update` to also take in anything that
implements `Into<Update>`. This allows us to directly apply a
`FullScanResult` or `SyncResult`.
* Added `start_full_scan` and `start_sync_with_revealed_spks` methods to
`Wallet`.
Co-authored-by: Steve Myers <steve@notmandatory.org>
96a9aa6e63 feat(chain): refactor `merge_chains` (志宇)
2f22987c9e chore(chain): fix comment (志宇)
daf588f016 feat(chain): optimize `merge_chains` (志宇)
77d35954c1 feat(chain)!: rm `local_chain::Update` (志宇)
1269b0610e test(chain): fix incorrect test case (志宇)
72fe65b65f feat(esplora)!: simplify chain update logic (志宇)
eded1a7ea0 feat(chain): introduce `CheckPoint::insert` (志宇)
519cd75d23 test(esplora): move esplora tests into src files (志宇)
a6e613e6b9 test(esplora): add `test_finalize_chain_update` (志宇)
494d253493 feat(testenv): add `genesis_hash` method (志宇)
886d72e3d5 chore(chain)!: rm `missing_heights` and `missing_heights_from` methods (志宇)
bd62aa0fe1 feat(esplora)!: remove `EsploraExt::update_local_chain` (志宇)
1e99793983 feat(testenv): add `make_checkpoint_tip` (志宇)
Pull request description:
Fixes#1354
### Description
Built on top of both #1369 and #1373, we simplify the `EsploraExt` API by removing the `update_local_chain` method and having `full_scan` and `sync` update the local chain in the same call. The `full_scan` and `sync` methods now takes in an additional input (`local_tip`) which provides us with the view of the `LocalChain` before the update. These methods now return structs `FullScanUpdate` and `SyncUpdate`.
The examples are updated to use this new API. `TxGraph::missing_heights` and `tx_graph::ChangeSet::missing_heights_from` are no longer needed, therefore they are removed.
Additionally, we used this opportunity to simplify the logic which updates `LocalChain`. We got rid of the `local_chain::Update` struct (which contained the update `CheckPoint` tip and a `bool` which signaled whether we want to introduce blocks below point of agreement). It turns out we can use something like `CheckPoint::insert` so the chain source can craft an update based on the old tip. This way, we can make better use of `merge_chains`' optimization that compares the `Arc` pointers of the local and update chain (before we were crafting the update chain NOT based on top of the previous local chain). With this, we no longer need the `Update::introduce_older_block` field since the logic will naturally break when we reach a matching `Arc` pointer.
### Notes to the reviewers
* Obtaining the `LocalChain`'s update now happens within `EsploraExt::full_scan` and `EsploraExt::sync`. Creating the `LocalChain` update is now split into two methods (`fetch_latest_blocks` and `chain_update`) that are called before and after fetching transactions and anchors.
* We need to duplicate code for `bdk_esplora`. One for blocking and one for async.
### Changelog notice
* Changed `EsploraExt` API so that sync only requires one round of fetching data. The `local_chain_update` method is removed and the `local_tip` parameter is added to the `full_scan` and `sync` methods.
* Removed `TxGraph::missing_heights` and `tx_graph::ChangeSet::missing_heights_from` methods.
* Introduced `CheckPoint::insert` which allows convenient checkpoint-insertion. This is intended for use by chain-sources when crafting an update.
* Refactored `merge_chains` to also return the resultant `CheckPoint` tip.
* Optimized the update `LocalChain` logic - use the update `CheckPoint` as the new `CheckPoint` tip when possible.
### 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
ACKs for top commit:
LLFourn:
ACK 96a9aa6e63
Tree-SHA512: 3d4f2eab08a1fe94eb578c594126e99679f72e231680b2edd4bfb018ba1d998ca123b07acb2d19c644d5887fc36b8e42badba91cd09853df421ded04de45bf69
Introduce a new API for getting addresses from the Wallet that
reflects a similiar interface as the underlying indexer
`KeychainTxOutIndex` in preparation for removing `AddressIndex` enum.
Before this change, the only way to get an address was via the methods
`try_get{_internal}_address` which required a `&mut` reference to the
wallet, matching on the desired AddressIndex variant. This is too
restrictive since for example peeking or listing unused addresses
shouldn't change the state of the wallet. Hence we provide separate
methods for each use case which makes for a more efficient API.
e51af49ffa fix(wallet): remove generic from wallet (Rob N)
Pull request description:
### Description
The `PersistenceBackend` uses generics to describe errors returned while applying the change set to the persistence layer. This change removes generics wherever possible and introduces a new public error enum. Removing the generics from `PersistenceBackend` errors is the first step towards #1363
*Update*: I proceeded with removing the generics from `Wallet` by introducing a `Box<dyn PersistenceBackend>` .
### Notes to the reviewers
This one sort of blew up in the number of changes due to the use of generics for most of the `Wallet` error variants. The generics were only used for the persistence errors, so I removed the generics from higher level errors whenever possible. The error variants of `PersistenceBackend` may also be more expressive, but I will level that up for discussion and make any changes required.
### Changelog notice
- Changed `PersistenceBackend` errors to depend on the `anyhow` crate.
- Remove the generic `T` from `Wallet`
### 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
ACKs for top commit:
evanlinjin:
ACK e51af49ffa
Tree-SHA512: 8ce4f1c495310e16145555f4a6a29a0f42cf8944eda68004595c3532580767f64f779185022147a00d75001c40d69fdf8f8de2d348eb68484b170d2a181117ff
The intention is to remove the `Update::introduce_older_blocks`
parameter and update the local chain directly with `CheckPoint`.
This simplifies the API and there is a way to do this efficiently.
Upgrade:
- bitcoin to v0.31.0
- miniscript to v11.0.0
Note: The bitcoin upgrade includes improvements to the
`Transaction::weight()` function, it appears those guys did good, we
no longer need to add the 2 additional weight units "just in case".
These methods allow us to query for checkpoints contained within the
linked list by height and height range. This is useful to determine
checkpoints to fetch for chain sources without having to refer back to
the `LocalChain`.
Currently this is not implemented efficiently, but in the future, we
will change `CheckPoint` to use a skip list structure.
798ed8ced2 fix: remove deprecated `max_satisfaction_weight (Jose Storopoli)
Pull request description:
### Description
Continuation of #1115.
Closes#1036.
* Change deprecated `max_satisfaction_weight` to `max_weight_to_satisfy`
* Remove `#[allow(deprecated)]` flags
### Notes to the reviewers
I've changed all `max_satisfaction_weight()` to `max_weight_to_satisfy()` in `Wallet.get_available_utxo()` and `Wallet.build_fee_bump()`. Checking the docs on the `miniscript` crate for `max_weight_to_satisfy` has the following note:
We are testing if the underlying descriptor `is.segwit()` or `.is_taproot`,
then adding 4WU if true or leaving as it is otherwise.
Another thing, we are not testing in BDK tests for legacy (pre-segwit) descriptors.
Should I also add them to this PR?
### Changelog notice
### Fixed
Replace the deprecated `max_satisfaction_weight` from `rust-miniscript` to `max_weight_to_satisfy`.
### 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
* [ ] 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:
evanlinjin:
ACK 798ed8ced2
Tree-SHA512: 60babecee13c24915348ddb64894127a76a59d9421d52ea37acc714913685d57cc2be1904f9d0508078dd1db1f7d7dad83a734af5ee981801ca87de2e9984429
8ab58af093 feat(chain)!: wrap `TxGraph` txs with `Arc` (志宇)
Pull request description:
### Description
This PR makes `TxGraph` store transactions as `Arc<Transaction>`.
`Arc<Transaction>` can be shared between the chain-source and receiving structures. This allows the chain-source to keep the already-fetched transactions (save bandwith) and have a shared pointer to the transaction (save memory).
Our current logic to avoid re-fetching transactions is to refer back to the receiving structures. However, this means an additional round of locking our receiving structures.
### Notes to the reviewers
This will make more sense once I update the esplora/electrum chain sources to make use of both #1369 and this PR. The result would be chain sources which would only require locking the receiving structures twice (once for initiating the update, and once for applying the update).
### Changelog notice
* Changed `TxGraph` to store transactions as `Arc<Transaction>`. This allows chain-sources to cheaply keep a copy of already-fetched 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
#### New Features:
* [ ] I've added tests for the new feature
* [x] I've added docs for the new feature
ACKs for top commit:
LLFourn:
ACK 8ab58af093
notmandatory:
ACK 8ab58af093
Tree-SHA512: 81d7ad35fed7f253a3b902f09a41aaef2877f6201d4f6e78318741bf00e4b898a8000d878ffcbfe75701094ce3e9021bd718b190a655331a9e11f0ad66bdb02f
Wrapping transactions as `Arc<Transaction>` allows us to share
transactions cheaply between the chain-source and receiving structures.
Therefore the chain-source can keep already-fetched transactions (save
bandwidth) and have a shared pointer to the transactions (save memory).
This is better than the current way we do things, which is to refer back
to the receiving structures mid-sync.
Documentation for `TxGraph` is also updated.
06d7dc5c3a doc(bdk): Update bdk README (vmammal)
Pull request description:
fixes#1044
### Notes
The code snippet is a compile fail because variables `txout` and `outpoint` are not initialized. Any other suggestions are welcome.
There is still some old commented code in the file that's probably not needed unless we want to provide another example snippet, say for getting new address info.
* [x] Fix broken links
* [x] Use a new unused tprv
* [ ] Try to make persistence example make sense, or maybe just do away with it
#### 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
ACKs for top commit:
evanlinjin:
ACK 06d7dc5c3a
Tree-SHA512: 09a671bc6bea574d7a4b42b64718380ee71a0c5d36c6b7ca1dc19a2c567de510b27ccc91fe05e7178bf31c562db66bc64f660415de5bb2f32f369b13c44ad3d2
5840ce473e fix(bdk): Remove extra taproot fields when finalizing Psbt (vmammal)
8c78a42163 test(psbt): Fixup test_psbt_multiple_internalkey_signers (vmammal)
Pull request description:
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
ACKs for top commit:
evanlinjin:
ACK 5840ce473e
Tree-SHA512: 69f022c6f736500590e36880dd2c845321464f89af6a2c67987d1c520f70a298328363cade0f55f270c4e7169e740bd4ada752ec2c75afa02b6a5a851f9030c3
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.
9bb39a3a3f Avoid a wildcard match in tx construction (Steven Roose)
9e098a5b6d tx_builder: Support setting explicit nSequence for foreign inputs (Steven Roose)
Pull request description:
Fixes https://github.com/bitcoindevkit/bdk/issues/1315.
ACKs for top commit:
evanlinjin:
ACK 9bb39a3a3f
danielabrozzoni:
utACK 9bb39a3a3f
Tree-SHA512: 42c96a58a762fa8737402ebd0132ce20ce0359c996cee9feeecb0b84e6fb73305be1aec9429fb97ba932486f0b35ac5caed7b43656456c5fb053e55330a12d47
1c15cb2f91 ref(example_cli): Add new struct Init (vmammal)
89a7ddca7f ref(esplora): `Box` a large `esplora_client::Error` (vmammal)
097d818d4c ref(wallet): `Wallet::preselect_utxos` now accepts a `&TxParams` (vmammal)
f11d663b7e ref(psbt): refactor body of `get_utxo_for` to address `clippy::manual_map` (vmammal)
4679ca1df7 ref(example_cli): add typedefs to reduce type complexity (vmammal)
64a90192d9 refactor: remove old clippy allow attributes (vmammal)
Pull request description:
closes#1127
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
ACKs for top commit:
evanlinjin:
ACK 1c15cb2f91
Tree-SHA512: 5fa3796a33678651414e7aad7ef8309b4cbe2a9ab00dce094964b40784edb2f46a44067785d95ea26f4cd88d593420485be94c9b09ac589f632453fbd8c94d85
These lints either resolved themselves, or the code has changed such that
they no longer apply, hence they can be removed with no further changes.
`clippy::derivable_impls`
`clippy::needless_collect`
`clippy::almost_swapped`
