Previously, we would update the `TxGraph` and `KeychainTxOutIndex`
first, then create a second update for `LocalChain`. This required
locking the receiving structures 3 times (instead of twice, which
is optimal).
This PR eliminates this requirement by making use of the new `query`
method of `CheckPoint`.
Examples are also updated to use the new API.
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.
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
- changes the code implementation to "the maximum number of consecutive unused addresses"
in esplora async and blocking extjensions.
- treat `stop_gap = 0` as `stop_gap = 1` for all purposes.
- renames `past_gap_limit` to `gap_limit_reached` to indicate we want to break once the gap
limit is reached and not go further in `full_scan`, as suggested in
https://github.com/bitcoindevkit/bdk/issues/1227#issuecomment-1859040463
- change the tests according to the new implementation.
- add notes on what `stop_gap` means and links to convergent definition in other
Bitcoin-related software.
Closes#1227
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.
552f11cb5f 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
ACKs for top commit:
evanlinjin:
re-ACK 552f11cb5f
danielabrozzoni:
ACK 552f11cb5f
Tree-SHA512: 752a24ebd0b9ad7952c1b093ecb251473e346c77b860c1a80c73418130189227405a0f0d7652967cf8c7b89994e8c37df96cd52b52b6daff9cc8c88b5194069a
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
c5afbaa95d ci: update zstd-sys version to work with MSRV 1.63 (Steve Myers)
929b5ddb0c refactor(esplora): better variable naming and docs (志宇)
216648bcfd chore(esplora): Clarify consistency guarantees (LLFourn)
63fa710319 fix(esplora): reuse returned height instead of zipping (志宇)
6f824cf325 test(esplora): introduce test cases for `update_local_chain` (志宇)
f05e8502e6 feat(esplora): greatly simplify `update_local_chain` (志宇)
Pull request description:
Fixes#1199
### Description
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`.
### Changelog notice
Fixed
- Simplified `EsploraExt::update_local_chain` logic.
### 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~ (there are now lots of tests though)
* [x] I'm linking the issue being fixed by this PR
ACKs for top commit:
evanlinjin:
ACK c5afbaa95d
notmandatory:
utACK c5afbaa95d
Tree-SHA512: f588493a4643f0f68d3181f27adf91793d4e336be6e853a26289e0916ed83169e1067260b75e627f190842f691ec095e668c0b799ca80e7da2849dd28de32754
b6a58d4f9b doc(chain,esplora): minor documentation improvements (vmammal)
Pull request description:
* 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
ACKs for top commit:
danielabrozzoni:
ACK b6a58d4f9b
Tree-SHA512: 42f12f218a9849b6fc1c8da28c4d41ca86fcc9ea52311d28f67fadbec030b6cb6986774d89a30e7a2b5ee569b08c7cfa2af71bbb9b06efacfa685dbe95820f73
7c0f4653b2 doc(esplora): fix broken link in README (vmammal)
Pull request description:
### 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:
danielabrozzoni:
ACK 7c0f4653b2
Tree-SHA512: 6e09a0b74c1a64bc9d0d0a067daf7766c4a46511c22b676251f61608d770d3ea2c1df81d4e9bbbee69720cad5cd2deb9b146cb7751277871f0cbf1be96efcc9f
These crates need to have their versions bumped because the version of bdk_chain was bumped to 0.7.0 with the 1.0.0-alpha.3 release.
bdk_esplora to 0.5.0
bdk_file_store to 0.3.0
6817ca9bcb ci: pin hyper-rustls version to 0.24.0 for 1.57 MSRV (thunderbiscuit)
4ee41dbc40 build(esplora): Add async-https-rustls flag to esplora client (thunderbiscuit)
Pull request description:
### Description
The bdk_esplora crate currently doesn't expose the [`async-https-rustls` flag offered by the rust-esplora-client](ef1925e1ee/Cargo.toml (L44)) crate and instead requires users to build using the `default-tls` flag on reqwest, which uses the platform-specific openssl library when compiling. This creates complications for cross-compilation, notably for our Android builds that currently support 3 architectures (`arm64-v8a`, `armeabi-v7a`, and `x86_64`). In order to solve this we can either compile the openssl libraries for each of the platforms we want to support, or use the rustls-tls version of reqwest. The second options is much easier and requires less fiddling with the internals of the Android native development kit and cross-compilation rabbit holes.
Before we merge this I want to make sure I understand the tradeoffs between the `native-tls` and the `rustls-tls` and confirm that there are not potential issues there, but from what I understand they should provide the same functionality/security, and because these are already available/exposed in reqwest and rust-esplora-client, I think this should be a fairly straightforward additional feature we offer.
### Changelog notice
```txt
Added:
- New async-https-rustls feature flag for the bdk_esplora crate, allowing to compile rust-esplora-client using rustls-tls instead of the default native-tls.
```
### 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:
notmandatory:
ACK 6817ca9bcb
realeinherjar:
ACK 6817ca9bcb
danielabrozzoni:
ACK 6817ca9bcb
Tree-SHA512: 1d417da7cf85e157d71f56442a06e817e8741822d7bff9089f7fbb70ff8b4854f1f52befbc348b849e9c98cae848b792d426cd5bf551e7a9089b15467d28efdd
The gap limit was checked such as if the last_index was not None but the
last_active_index was, we'd consider having reached it. But the
last_index is never None for this check. This effectively made it so the
gap limit was always 1: if the first address isn't used
last_active_index will be None and we'd return immediately.
Fix this by avoiding error-prone Option comparisons and correctly
checking we've reached the gap limit before breaking out of the loop.
Thank you @vladimirfomene for these suggestions.
* Rename `LocalUpdate::keychain` to `LocalUpdate::last_active_indices`.
* Change docs for `CheckPoint` to be more descriptive.
* Fix incorrect logic in `update_local_chain` for `EsploraExt` and
`EsploraAsyncExt`.
