e9bbb8724f92ada33228bbfc22acdaefbd79c44e Fix wallet export rescan height (LLFourn)
Pull request description:
It would return the latest transaction height rather than the earliest as the height to rescan from.
Found by @evanlinjin and I while implementing `bdk_core` stuff into bdk's wallet.
### Changelog notice
- Fix wallet export transaction height
#### All Submissions:
* [x] I've signed all my commits
* [x] This pull request breaks the existing API
* [x] I've added tests to reproduce the issue which are now passing
ACKs for top commit:
rajarshimaitra:
tACK e9bbb8724f92ada33228bbfc22acdaefbd79c44e
notmandatory:
ACK e9bbb8724f92ada33228bbfc22acdaefbd79c44e
Tree-SHA512: 9b29ef0df39d26806f48b38fa5c3643bad32f58b993ffdcfc7811aca64a025bd8f163967321f874aa2ef3d29c3e7bc6e2f44d348306a37111f4def036d4c095e
1437e1ecfe663b819156d98c5e1975fb357a763f Add the hardware_signer example (Daniela Brozzoni)
1a71eb1f4736651ad82e0abd64792b6cc7b16c20 Update the hardwaresigner module documentation (Daniela Brozzoni)
0695e9fb3e41727e5732561a993411147487afd3 Bump HWI to 0.2.3 (Daniela Brozzoni)
a4a43ea86060fa0a62b47dedc7de820459b3a472 Re-export HWI if the hardware-signer feature is set (Daniela Brozzoni)
Pull request description:
### Description
### Notes to the reviewers
### Changelog notice
- bdk re-exports the `hwi` create when the feature `hardware-signer` is on
- Add `examples/hardware_signer.rs`
### 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:
afilini:
ACK 1437e1ecfe663b819156d98c5e1975fb357a763f
Tree-SHA512: 181f4d14dce11e19497fbf30e0af8de21c2c210d37129d7d879ed5670ed09a25be1c8d371389c431e18df9e76870cf5e4afe7b29a6c05fe59b3e1816bc8cf673
This PR adds a new field called `allow_grinding`
in the Signer's `SignOptions` struct that is used
to determine whether or not to grind an ECDSA signature
during the signing process.
d7bfe68e2df270ab799d36ebf3563e178ae50c6e Fix broken nightly docs (Alekos Filini)
b11c86d074a8f56f99bd5f3af77d3b056af71de4 Rename internal esplora modules, fix docs (Alekos Filini)
b5b92248c76aeb42ac747931efd45bc2e7af5ebd Rename esplora features to -async and -blocking (Alekos Filini)
cf2bc388f22b069fc25fba482e59da6305207864 Re-export `esplora_client` (Elias Rohrer)
5baf46f84d41fb714f200a7b26170c9b77823dc1 Use the external esplora client library (Alekos Filini)
Pull request description:
### Description
Use the external esplora client crate now that it's published
### Changelog notice
- Start using the external esplora client crate
- Deprecate the `use-esplora-reqwest` and `use-esplora-ureq` features in favor of `use-esplora-async` and `use-esplora-blocking`
### 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
ACKs for top commit:
notmandatory:
ACK d7bfe68e2df270ab799d36ebf3563e178ae50c6e
Tree-SHA512: 23bd47536fe6f723602cbcc51d909eb9aed28376430f4453eea832e30a587be3d312cdca993d114391132bfb39c48637030f974ab1a742f7defe44f40a82ef8b
`Wallet` stores the descriptors' checksum in the database for safety.
Previously, the checksum used was a checksum of a descriptor that
already had a checksum.
This PR allows for backward-compatibility of databases created with this
bug.
If `exclude_hash` is set, we split the input data, and if a checksum
already existed within the original data, we check the calculated
checksum against the original checksum.
Additionally, the implementation of `IntoWalletDescriptor` for `&str`
has been refactored for clarity.
Some wallets may only specify the `non_witness_utxo` for a PSBT input.
If that's the case, BDK should still be able to sign.
This was pointed out in the discussion of #734
138acc3b7d137788d0518182e2167504e58ebc48 Change `populate_test_db` to not return empty input (wszdexdrf)
d6e1dd104063075f49b617786d82d29c1f9c6a0a Change CI to add test using ledger emulator (wszdexdrf)
76034772cba4d3d6fa1bdcb08977c2b9d7a157c2 Add a custom signer for hardware wallets (wszdexdrf)
Pull request description:
Also adds a new test in CI for building and testing on a virtual
hardware wallet.
### Description
This PR would enable BDK users to sign transactions using a hardware wallet. It is just the beginning hence there are no complex features, but I hope not for long.
I have added a test in CI for building a ledger emulator and running the new test on it. The test is similar to the one on bitcoindevkit/rust-hwi.
### Notes to the reviewers
The PR is incomplete (and wouldn't work, as the rust-hwi in `cargo.toml` is pointing to a local crate, temporarily) as a small change is required in rust-hwi (https://github.com/bitcoindevkit/rust-hwi/pull/42).
### 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
* [x] I've updated `CHANGELOG.md`
ACKs for top commit:
afilini:
ACK 138acc3b7d137788d0518182e2167504e58ebc48
Tree-SHA512: 54337f06247829242b4dc60f733346173d957de8e9f8b80beb91304d679cfb4e0e4db722c967469265a5b6ede2bd641ba5c089760391c671995dc30de37897de
08668ac46247d527cc53af5b6f359b1fa4e3b6aa Set the db sync height (rajarshimaitra)
Pull request description:
<!-- You can erase any parts of this template not applicable to your Pull Request. -->
### Description
Fixes#719
Previously we weren't setting the db sync height in populate_test_db
macro even when current height is provided.. This creates a bug that
get_funded_wallet will return 0 balance.
This PR fixes the populate_test_db macro and updates tests which were
previously dependent on the unsynced wallet behavior.
### 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 -->
### Checklists
#### All Submissions:
* [x] I've signed all my commits
* [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md)
* [x] I ran `cargo fmt` and `cargo clippy` before committing
#### Bugfixes:
* [x] I've added tests to reproduce the issue which are now passing
* [x] I'm linking the issue being fixed by this PR
ACKs for top commit:
afilini:
ACK 08668ac46247d527cc53af5b6f359b1fa4e3b6aa
Tree-SHA512: 1dcc968e4b3551e916b450c5ff2fab6636083f104cc982eb3f7602c624382434e0170d9f0c0a356e6c9c5f834eebe5cb1365b37ef73d7b4ef15d652a364dc2ab
Lightning denotes transaction fee rate
sats / 1000 weight units and sats / 1000 vbytes.
Here we add support for creating BDK fee rate from
lightning fee rate. We also move all FeeRate test to
types.rs and rename as_sat_vb to as_sat_per_vb.
Previously we weren't setting the db sync height in populate_test_db
macro even when current height is provided.. This creates a bug that
get_funded_wallet will return 0 balance.
This PR fixes the populate_test_db macro and updates tests which were
previously dependent on the unsynced wallet behavior.
7b1ad1b62914a26d6f445364ace4e784bb2901c2 Verify signatures after signing (Scott Robinson)
Pull request description:
### Description
Verify signatures after signing
As per [BIP-340, footnote 14][fn]:
> Verifying the signature before leaving the signer prevents random or
> attacker provoked computation errors. This prevents publishing invalid
> signatures which may leak information about the secret key. It is
> recommended, but can be omitted if the computation cost is prohibitive.
[fn]: https://github.com/bitcoin/bips/blob/master/bip-0340.mediawiki#cite_note-14
### Notes to the reviewers
How do we test this?
### Checklists
#### All Submissions:
* [ ] 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:
afilini:
re-ACK 7b1ad1b62914a26d6f445364ace4e784bb2901c2
Tree-SHA512: 7319db1f8cec2fcfe4ac443ab5728893f9fb6133b33331b35ec6910662c45de8a7cdcf80ac1f3bb435815e914ccf639682a5c07ff0baef42605bf044a34a8232
As per [BIP-340, footnote 14][fn]:
> Verifying the signature before leaving the signer prevents random or
> attacker provoked computation errors. This prevents publishing invalid
> signatures which may leak information about the secret key. It is
> recommended, but can be omitted if the computation cost is prohibitive.
[fn]: https://github.com/bitcoin/bips/blob/master/bip-0340.mediawiki#cite_note-14
Our costant for the P2WPKH satisfaction size was wrong: in
7ac87b8f99fc0897753ce57d48ea24adf495a633 we added 1 WU for the script
sig len - but actually, that's 4WU! This resulted in
P2WPKH_SATISFACTION_SIZE being equal to 109 instead of 112.
This also adds a comment for better readability.
9d85c9667f7d12902afef3ba08ea7231f6868a78 Fix the early InsufficientFunds error in the branch and bound (Alekos Filini)
Pull request description:
### Description
We were wrongly considering the sum of "effective value" (i.e. value -
fee cost) when reporting an early "insufficient funds" error in the
branch and bound coin selection.
This commit fixes essentially two issues:
- Very high fee rates could cause a panic during the i64 -> u64
conversion because we assumed the sum of effective values would never
be negative
- Since we were comparing the sum of effective values of *all* the UTXOs
(even the optional UTXOs with negative effective value) with the target
we'd like to reach, we could in some cases error and tell the user we
don't have enough funds, while in fact we do! Since we are not required
to spend any of the optional UTXOs, so we could just ignore the ones
that *cost us* money to spend and excluding them could potentially
allow us to reach the target.
There's a third issue that was present before and remains even with this
fix: when we report the "available" funds in the error, we are ignoring
UTXOs with negative effective value, so it may look like there are less
funds in the wallet than there actually are.
I don't know how to convey the right message the user: if we actually
consider them we just make the "needed" value larger and larger (which
may be confusing, because if the user asks BDK to send 10k satoshis, why
do we say that we actually need 100k?), while if we don't we could report
an incorrect "available" value.
### Notes to the reviewers
I'm opening this as a draft before adding tests because I want to gather some feedback on the available vs needed error reporting. I personally think reporting a reasonable "needed" value is more important than the "available", because in a wallet app I would expect this is the value that would be shown to the user.
### 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
* [ ] I'm linking the issue being fixed by this PR
ACKs for top commit:
danielabrozzoni:
utACK 9d85c9667f7d12902afef3ba08ea7231f6868a78
Tree-SHA512: 9a06758cba61ade73198f35b08070987d5eb065e01750ce62409f86b37cd0b0894640e9f75c8b2c26543c0da04e3f77bd397fab540e789f221661aae828db224
We were wrongly considering the sum of "effective value" (i.e. value -
fee cost) when reporting an early "insufficient funds" error in the
branch and bound coin selection.
This commit fixes essentially two issues:
- Very high fee rates could cause a panic during the i64 -> u64
conversion because we assumed the sum of effective values would never
be negative
- Since we were comparing the sum of effective values of *all* the UTXOs
(even the optional UTXOs with negative effective value) with the target
we'd like to reach, we could in some cases error and tell the user we
don't have enough funds, while in fact we do! Since we are not required
to spend any of the optional UTXOs, so we could just ignore the ones
that *cost us* money to spend and excluding them could potentially
allow us to reach the target.
There's a third issue that was present before and remains even with this
fix: when we report the "available" funds in the error, we are ignoring
UTXOs with negative effective value, so it may look like there are less
funds in the wallet than there actually are.
I don't know how to convey the right message the user: if we actually
consider them we just make the "needed" value larger and larger (which
may be confusing, because if the user asks BDK to send 10k satoshis, why
do we say that we actually need 100k?), while if we don't we could report
an incorrect "available" value.
7fdacdbad40f4e9f6726b064d8eb4d93789e9990 doc: Document that list_transactions() might return unsorted txs, show how to sort them if needed (w0xlt)
Pull request description:
This PR documents that `list_transactions()` might return unsorted transaction and shows how to sort them if needed.
Closes#518.
ACKs for top commit:
danielabrozzoni:
re-ACK 7fdacdbad40f4e9f6726b064d8eb4d93789e9990
Tree-SHA512: 83bec98e1903d6dc6b8933e8994cb9d04aad059cee8a7b8e1e3a322cf52511364b36d0cd6be1c8cb1fd82c67f8be5a262bbd2c76e30b24eb4097c30f38aa8b10
Before this commit `fee_amount` and `amount_needed` were passed as independent
parameters. From the perspective of coin selection algorithms, they are always
used jointly for the same purpose, to create a coin selection with a total
effective value greater than it's summed values.
This commit removes the abstraction that the use of the two parameter
introduced by consolidating both into a single parameter, `target_amount`, who
carries their values added up.
419dc248b667db05295cd4c68347c4ef51f51023 test: Document `test_bump_fee_add_input_change_dust` (Daniela Brozzoni)
632dabaa07ef9c58926facf0af5190f62bb65d12 test: Check tx feerate with longer signatures (Daniela Brozzoni)
2756411ef7cf0415baf2f2401e2d5a78481d0aa1 test: Reproduce #660 conditions (Daniela Brozzoni)
50af51da5a5c906d8bf660d35a4f922ceb996068 test: Fix P2WPKH_FAKE_WITNESS_SIZE (Daniela Brozzoni)
ae919061e2b341ae74c90f0133ba392e835cb4e1 Take into account the segwit tx header when... ...selecting coins (Daniela Brozzoni)
7ac87b8f99fc0897753ce57d48ea24adf495a633 TXIN_BASE_WEIGHT shouldn't include the script len (Daniela Brozzoni)
ac051d7ae9512883e11a89ab002ad2d2c3c55c19 Calculate fee amount after output addition (Daniela Brozzoni)
00d426b88546a346820c102386cd1bfff82ca8f6 test: Check that the feerate is never below... ...the requested one in assert_fee_rate (Daniela Brozzoni)
42fde6d4575b4aea121286f884f712b1c1cf64be test: Check fee_amount in assert_fee_rate (Daniela Brozzoni)
Pull request description:
### Description
This PR mainly fixes two bugs:
1. TXIN_BASE_WEIGHT wrongly included the `script_len` (Fixes#160)
2. We wouldn't take into account the segwit header in the fee calculation, which could have resulted in a transaction with a lower feerate than the requested one
3. In tests we used to push 108 bytes on the witness as a fake signature, but we should have pushed 106 instead
I also add a test to reproduce the conditions of #660, to check if it's solved. Turns out it's been solved already in #630, but if you're curious about what the bug was, here it is: https://github.com/bitcoindevkit/bdk/issues/660#issuecomment-1196436776
### 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:
afilini:
ACK 419dc248b667db05295cd4c68347c4ef51f51023
Tree-SHA512: c7b55342eac440a3607a16b94560cb9c08c4805c853432adfda8e21c5177f85d5a8afe0e7e61140e92c8f10934332459c6234fc5f1509ea699d97b1d04f030c6
We would previously always try to sign with the taproot internal
key, and try to sign all the script leaves hashes.
Instead, add the `sign_with_tap_internal_key` and `TapLeaveOptions`
parameters, to be able to specify if we should sign with the internal
key, and exactly which leaves we should sign.
Fixes#616
Issue #660 has been fixed by 32ae95f463f62c42c6d6aec62c1832a30298fce4,
when we moved the change calculation inside the coin selection.
This commit just adds a test to make sure that the problem is fixed.
We would previously push 108 bytes on a P2WPKH witness
to simulate signature + pubkey. This was wrong: we should push
106 bytes instead.
The max satisfaction size for a P2WPKH is 112 WU:
elements in witness (1 byte, 1WU) + OP_PUSH (1 byte, 1WU) +
pk (33 bytes, 33 WU) + OP_PUSH (1 byte, 1WU) + signature and sighash
(72 bytes, 72 WU) + scriptsig len (1 byte, 4WU)
We should push on the witness pk + signature and sighash. This is 105
WU. Since we push just once instead of twice, we add 1WU for the OP_PUSH
we are omitting.
...selecting coins
We take into account the larger segwit tx header for every
transaction, not just the segwit ones. The reason for this is that
we prefer to overestimate the fees for the transaction than
underestimating them - the former might create txs with a slightly
higher feerate than the requested one, while the latter might
create txs with a slightly lower one - or worse, invalid (<1 sat/vbyte)!
We would before calculate the TXIN_BASE_WEIGHT as prev_txid (32 bytes) +
prev_vout (4 bytes) + sequence (4 bytes) + script_sig_len (1 bytes), but
that's wrong: the script_sig_len shouldn't be included, as miniscript
already includes it in the `max_satisfaction_size` calculation.
Fixes#160
We would previously calculate the fee amount in two steps:
1. Add the weight of the empty transaction
2. Add the weight of each output
That's unnecessary: you can just use the weight of the transaction
*after* the output addition. This is clearer, but also avoids a
rare bug: if there are many outputs, adding them would cause the
"number of outputs" transaction parameter lenght to increase, and we
wouldn't notice it.
This might still happen when adding the drain output - this
commit also adds a comment as a reminder.
The former way to compute and create change was inside `create_tx`, just after
performing coin selection.
It blocked the opportunity to have an "ensemble" algorithm to decide between
multiple coin selection algorithms based on a metric, like Waste.
Now, change isn't created inside `coin_select` but the change amount and the
possibility to create change is decided inside the `coin_select` method. In
this way, change is associated with the coin selection algorithm that generated
it, and a method to decide between them can be implemented.
Previously, electrum-based blockchain implementations only synced for
`scriptPubKey`s that are already cached in `Database`.
This PR introduces a feedback mechanism, that uses `stop_gap` and the
difference between "current index" and "last active index" to determine
whether we need to cache more `scriptPubKeys`.
The `WalletSync::wallet_setup` trait now may return an
`Error::MissingCachedScripts` error which contains the number of extra
`scriptPubKey`s to cache, in order to satisfy `stop_gap` for the next call.
`Wallet::sync` now calls `WalletSync` in a loop, cacheing inbetween
subsequent calls (if needed).
2c02a44586c67d1ec9720f17a3748f28c1d18643 Test: No address reuse for single descriptor (志宇)
Pull request description:
### Description
Just a simple new test.
This test is to ensure there are no regressions when we later change
internal logic of `Wallet`. A single descriptor wallet should always get
a new address with `AddressIndex::New` even if we alternate grabbing
internal/external keychains.
I thought of adding this during work on #647
### 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:
tACK 2c02a44586c67d1ec9720f17a3748f28c1d18643
rajarshimaitra:
tACK 2c02a44586c67d1ec9720f17a3748f28c1d18643
Tree-SHA512: d065ae0979dc3ef7c26d6dfc19c88498e4bf17cc908e4f5677dcbf62ee59162e666cb00eb87b96d4c2557310960e3677eec7b6d907a5a4860cb7d2d74dba07b0
