Steve Myers [Sun, 21 Aug 2022 01:54:46 +0000 (20:54 -0500)]
Add fee_amount() and fee_rate() functions to PsbtUtils trait
PsbtUtils.fee_amount(), calculates the PSBT total transaction fee amount in Sats.
PsbtUtils.fee_rate(), calculates the PSBT FeeRate, the value is only accurate AFTER the PSBT is finalized.
* [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 also changes the code coverage front end to coveralls instead of codecov, which had some issues with other changes in the PR. This will provide better and more accurate code coverage reports.
### Notes to the reviewers
The tests run before generating the report are not exhaustive (not exhaustive earlier too, but I added as many as I could), and hence the report won't be 100% accurate.
### 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
#### New Features:
* [ ] I've added tests for the new feature
* [ ] I've added docs for the new feature
* [x] I've updated `CHANGELOG.md`
* [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
* [ ] I've updated `CHANGELOG.md`
Update DEVELOPMENT_CYCLE and release instructions to make [overriding dependencies] possible for downstream projects with unreleased `bdk` versions for development and testing. Also simplifies the release process by capturing changelog information in the `pull_request_template` and recording release changelog information in the release tag message instead of in a `CHANGELOG.md` file which causes too many merge conflicts and complicates the release process.
Fixes #536
Fixes #496
### Notes to the reviewers
The primary changes to our current release process are:
1. Don't add `-dev` or `-rc.x` to unreleased `bdk` cargo versions because those extensions do not work with [overriding dependencies].
2. Increment the `master` branch version as soon as a `release/MAJOR.MINOR` branch is created, the next release `release/MAJOR.MINOR` branch version with be **MAJOR.MINOR.PATCH**, and the `master` branch development version will be **MAJOR.MINOR+1.0**; either version can be used with [overriding dependencies].
4. Remove the `bdk` version from the `src/lib.rs` file so that it doesn't need to be changed on every release, because it isn't needed in the rust docs for most developers and removing it will help simplify the release process.
5. The new release process is now documented as a checklist in a new `release.md` github issue template.
6. Putting changelog information in the release tag message is how the tokio project does it. ~~After this PR is merged I will replace old tags with new ones containing changelog information and then do a new PR to remove the CHANGELOG.md file.~~ After this PR is merged I don't think we need to update old tags, only rename the CHANGELOG.md file to CHANGELOG-OLD.md with a note to check tags for future change log info.
* [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 needed this for testing our wallet with dust, does this look like a reasonable feature? If so, I'll go ahead and add a test, update the changelog, etc.
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`
<!-- 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
Lightning denotes transaction fee rate sats / 1000 weight units and sats / 1000 vbytes.
Here we add support for creating BDK FeeRate from lightning fee rate. We also move all FeeRate tests to
types.rs and rename as_sat_vb to as_sat_per_vb.
### Notes to the reviewers
Matt was concerned that we might round down value in fee calculation in such a way that a transaction may not be relayed because it is below Bitcoin Core's min relay fee (1 sat/vbyte). I don't think we need to worry about that because we [round up(ceil)](https://github.com/bitcoindevkit/bdk/blob/master/src/types.rs#L91) during fee calculation, we don't round down. I will love to hear what you think. Is there something I'm missing? @johncantrell97, I will appreciate your review on this one.
### 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`
#### 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
Vladimir Fomene [Thu, 21 Jul 2022 11:58:30 +0000 (12:58 +0100)]
Implement conversion for Lightning fee rate
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.
rajarshimaitra [Thu, 25 Aug 2022 12:20:27 +0000 (17:50 +0530)]
Set the db sync height
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.
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.
* [ ] 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
There is currently no way to access the client from the EsploraBlockchain. This makes it difficult for users to extend it's functionality. This PR exposes both the reqwest and ureq clients. This PR is related to PR #705.
### 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
As pointed out in https://github.com/bitcoindevkit/rust-electrum-client/pull/58#issuecomment-1207890096 there was no way to keep using the client once it was given to BDK.
### 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
For the same reason as #705 and #722 ..
### 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
Scott Robinson [Mon, 15 Aug 2022 05:07:14 +0000 (15:07 +1000)]
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.
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.
### Description
### Notes to the reviewers
### 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
* [ ] I've updated `CHANGELOG.md`
#### 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
Daniela Brozzoni [Tue, 16 Aug 2022 16:37:03 +0000 (17:37 +0100)]
Fix P2WPKH_SATISFACTION_SIZE in CS tests
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.
* [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
* [ ] I've updated `CHANGELOG.md`
#### 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
Vladimir Fomene [Mon, 15 Aug 2022 16:31:35 +0000 (19:31 +0300)]
Implement Deref<Target=UrlClient> for EsploraBlockchain
There is currently no way to access the client
from the EsploraBlockchain. This makes it difficult
for users to extend it's functionality. This PR exposes
both the reqwest and ureq clients. This PR is related to
PR #705.
### Description
This PR fixes:
1. The use of "i.e." in docs, sometimes spelled as "ie."
2. A small typo in the sentence "Note that this methods only operate on the internal database..."
3. A small typo in the sentence "Finish the building the transaction"
I came across these while building docs for bdk-kotlin.
### Notes to the reviewers
### Checklists
#### All Submissions:
* [x] I've signed all my commits
* [x] I ran `cargo fmt` and `cargo clippy` before committing
This fixes a bug where `CoreTxIter` attempts to call `listtransactions` immediately after a tx result is filtered (instead of being returned), when in fact, the correct logic will be to pop another tx result.
The new logic also ensures that tx results are returned in chonological order. The test `test_list_transactions` verifies this. We also now ensure that `page_size` is between the range `[0 to 1000]` otherwise an error is returned.
Some needless cloning is removed from `from_config` as well as logging improvements.
### Notes to the reviewers
This is an oversight by me (sorry) for PR #683
### 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
Replace `rpc::CoreTxIter` with `list_transactions` fn.
This fixes a bug where `CoreTxIter` attempts to call `listtransactions`
immediately after a tx result is filtered (instead of being returned),
when in fact, the correct logic will be to pop another tx result.
The new logic also ensures that tx results are returned in chonological
order. The test `test_list_transactions` verifies this. We also now
ensure that `page_size` is between the range `[0 to 1000]` otherwise an
error is returned.
Some needless cloning is removed from `from_config` as well as logging
improvements.
Alekos Filini [Mon, 8 Aug 2022 10:03:35 +0000 (12:03 +0200)]
Implement `Deref<Target=Client>` for `ElectrumBlockchain`
As pointed out in https://github.com/bitcoindevkit/rust-electrum-client/pull/58#issuecomment-1207890096
there was no way to keep using the client once it was given to BDK.
### Description
This changes `get_balance()` function so that it returns balance separated in 4 categories:
- available
- trusted-pending
- untrusted-pending
- immature
Fixes #238
### Notes to the reviewers
Based on #614
### 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 updated tests for the new feature
* [x] I've added docs for the new feature
* [x] I've updated `CHANGELOG.md`
Unfortunately to fix all the problems, I had to do a complete re-implementation of `RpcBlockchain`.
**The new implementation fixes the following:**
* We can track more than 100 scriptPubKeys
* We can obtain more than 1000 transactions per sync
* Transaction "metadata" for already-syned transactions are updated when we introduce new scriptPubKeys
**`RpcConfig` changes:**
* Introduce `RpcSyncParams`.
* Remove `RpcConfig::skip_blocks` (this is replaced by `RpcSyncParams::start_time`).
### Notes to the reviewers
* The `RpcConfig` structure is changed. It will be good to confirm whether this is an okay 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
#### New Features:
~* [ ] I've added tests for the new feature~
* [x] I've added docs for the new feature
* [x] I've updated `CHANGELOG.md`
#### 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
Before this commit, the rpc backend would not notice immature utxos
(`listunspent` does not return them), making the rpc balance different
to other blockchain implementations.
New `RpcBlockchain` implementation with various fixes
The new implementation fixes the following:
* We can track more than 100 scriptPubKeys
* We can obtain more than 1000 transactions per sync
* `TransactionDetails` for already-synced transactions are updated when
new scriptPubKeys are introduced (fixing the missing balance/coins
issue of supposedly tracked scriptPubKeys)
`RpcConfig` changes:
* Introduce `RpcSyncParams`.
* Remove `RpcConfig::skip_blocks` (this is replaced by
`RpcSyncParams::start_time`).
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
Fix the early InsufficientFunds error in the branch and bound
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.
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.
Resolves: #641
### Notes to the reviewers
I just updated old tests and didn't create new ones because almost all changes
are renames and "logic changes" (like the addition of the selection fee) are
tested in the modified tests.
### 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
* [x] I've updated `CHANGELOG.md`
#### Bugfixes:
* [x] This pull request breaks the existing API
* [x] I'm linking the issue being fixed by this PR
ACKs for top commit:
danielabrozzoni:
re-ACK e8df3d2d91927edb9a339c664f0603c47622e4b0 - I tested with the fuzzer, run it for 13,000,000 iterations, couldn't find any crash :)
```
$ cargo run --features="keys-bip39 key-value-db rpc" --example rpcwallet
Compiling bitcoin_hashes v0.9.7
Compiling bip39 v1.0.1
Compiling bdk v0.20.1-dev (/home/node01/Dev/wbdk)
Finished dev [unoptimized + debuginfo] target(s) in 19.64s
Running `target/debug/examples/rpcwallet`
>> Setting up bitcoind
thread 'main' panicked at 'We should always have downloaded path: Called a method requiring a feature to be set, but it's not', examples/rpcwallet.rs:56:51
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
```
This PR adds `electrsd/bitcoind_22_0` to `required-features`, making clear that this lib is needed to run this example..
```
$ cargo run --features="keys-bip39 key-value-db rpc electrsd/bitcoind_22_0" --example rpcwallet
Blocking waiting for file lock on package cache
Compiling electrsd v0.19.1
Compiling bdk v0.20.1-dev (/home/node01/Dev/wbdk)
Finished dev [unoptimized + debuginfo] target(s) in 10.27s
Running `target/debug/examples/rpcwallet`
>> Setting up bitcoind
>> bitcoind setup complete
Available coins in Core wallet : 50.00000000 BTC
>> Setting up BDK wallet
>> BDK wallet setup complete.
Available initial coins in BDK wallet : 0 sats
>> Sending coins: Core --> BDK, 10 BTC
>> Received coins in BDK wallet
Available balance in BDK wallet: 1000000000 sats
>> Sending coins: BDK --> Core, 5 BTC
>> Coins sent to Core wallet
Remaining BDK wallet balance: 499999859 sats
Congrats!! you made your first test transaction with bdk and bitcoin core.
```
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.
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
* [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`
Daniela Brozzoni [Wed, 22 Jun 2022 16:08:23 +0000 (18:08 +0200)]
Better customize signing in taproot transactions
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.
Take into account the segwit tx header when...
...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.
Disallow negative, NaN, infinite or subnormal fee rate values.
### Notes to the reviewers
This commit is technically an API break because it makes the `FeeRate::from_sat_per_vb` function non-const. I think it's worth it compared to the risk of having completely nonsensical fee rates (that can break the coin selection in interesting ways).
EDIT: it's also a breaking change because our code can now panic in scenarios where it didn't before. Again, I think it's worth it.
### 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
* [x] I've added tests to reproduce the issue which are now passing
* [ ] I'm linking the issue being fixed by this PR
Currently SqliteDatabase::new takes a String as path,
with this change, it now accepts any type that implements
AsRef<Path>.
### Notes to the reviewers
### 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
* [x] I've updated `CHANGELOG.md`
#### 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
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 is not 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.
Fixes #147.
<!-- Describe the purpose of this PR, what's being adding and/or fixed -->
<!-- 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
#### New Features:
* [ ] I've added tests for the new feature
* [x] I've added docs for the new feature
* [x] I've updated `CHANGELOG.md`
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.
^ However, only for electrum-based `Blockchain` implementations. For RPC and Compact Block Filters, syncing works differently, and so are the bugs - I have created a separate ticket for this (#677).
### Description
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, caching in-between subsequent calls (if needed).
#### Notes to reviewers
1. The caveat to this solution is that it is not the most efficient. Every call to `WalletSync::wallet_setup` starts polling the Electrum-based server for `scriptPubKey`s starting from index 0.
However, I feel like this solution is the least "destructive" to the API of `Blockchain`. Also, once the `bdk_core` sync logic is integration, we can select specific ranges of `scriptPubKey`s to sync.
2. Also note that this PR only fixes the issue for electrum-based `Blockchain` implementations (currently `blockchain::electrum` and `blockchain::esplora` only).
3. Another thing to note is that, although `Database` assumes 1-2 keychains, the current `WalletSync` "feedback" only returns one number (which is interpreted as the larger "missing count" of the two keychains). This is done for simplicity, and because we are planning to only have one keychain per database in the future.
4. Please have a read of https://github.com/bitcoindevkit/bdk/pull/672#issuecomment-1186929465 for additional context.
### 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
* [x] I've added tests to reproduce the issue which are now passing
* [x] I'm linking the issue being fixed by this PR
Fix wallet sync not finding coins of addresses which are not cached
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).
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
### Description
Seems like `doc(include = "../README.md")` doesn't include the readme file as doc for the dummy struct. This might be due to a difference in Rust edition used back then or something.
Fixes #637
### 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
`get_checksum_bytes()` returns a descriptor checksum as `[u8; 8]` instead of `String`, potentially improving performance and memory usage.
In addition to this, since descriptors only use characters that fit within a UTF-8 8-bit code unit ([US-ASCII](https://www.charset.org/charsets/us-ascii)), there is no need to use the `char` type (which is 4 bytes). This can also potentially bring in some performance and memory-usage benefits.
### Notes to the reviewers
This is useful because we will be using descriptor checksums for indexing operations in the near future (multi-descriptor wallets #486 ).
Refer to comments by @afilini :
* https://github.com/bitcoindevkit/bdk/pull/647#discussion_r921184366
* https://github.com/bitcoindevkit/bdk/pull/647#discussion_r921914696
* https://github.com/bitcoindevkit/bdk/pull/654#discussion_r921980876
### 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`
Introduce `get_checksum_bytes` method and improvements
`get_checksum_bytes` returns a descriptor checksum as `[u8; 8]` instead
of `String`, potentially improving performance and memory usage.
In addition to this, since descriptors only use charaters that fit
within a UTF-8 8-bit code unit, there is no need to use the `char` type
(which is 4 bytes). This can also potentially bring in some performance
and memory-usage benefits.
Seems like `doc(include = "../README.md")` doesn't include the readme file as docs for the dummy struct. This might be due to a difference in Rust edition used back then or something
📢 Release 0.20.0 is out! Highlights include bug fixes for the ElectrumBlockchain and descriptor templates, discourage fee sniping in tx building, and new tx signing options. A big thanks to our past and latest new contributors. For all changes see: https://github.com/bitcoindevkit/bdk/releases/tag/v0.20.0
Usually we don't have any prefix except for methods that can *add* to a list or replace the list entirely (e.g. `add_recipients` vs `set_recipients`)
I missed this during review of #611
### 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
It is critical to ensure `Wallet::get_address` with `AddressIndex::new` always returns a new and unused address.
This bug seems to be Electrum-specific. The fix is to check address index updates to ensure that newly suggested indexes are not smaller than indexes already in database.
### Notes to the reviewers
I have written new tests in `/testutils/blockchain_tests.rs` that tests all `Blockchain` implementations.
### 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 bug seems to be Electrum-specific. The fix is to check the
proposed changes against the current state of the database. Ensure
newly suggested indexes are not smaller than indexes already in
database.
Changes:
* Check index updates before they are applied to database during
Electrum Blockchain sync (Thank you @rajarshimaitra for providing
an elegant solution).
> address validators are supposed to be used for a slightly different thing, which is when you ask the hardware wallet to independently generate the address for a derivation index and then you compare what you see on your computer/phone with what the hardware wallet is displaying
> in the case of change addresses i agree that it's not as important (because as you said the device can just refuse to sign) but for consistency we implemented it for both external and internal addresses
> more broadly, they can be thought of as a way to get a callback every time an address is generated, which may also be useful for other things (for example when i was working on a green-compatible client written in bdk i used that feature to ping the server every time a new address was generated, because that's required in their protocol)
> that said, i think currently pretty much nobody uses them and i am myself moving away from the concept that "everything needs to happen inside bdk": currently my mindset is targeted more towards reducing complexity by breaking down individual parts and wrapping them or making them "extensible" in some way
> that is to say: if you want to verify addresses in your hardware wallet you don't necessarily need bdk to do it for you (actually, you would still have to implement the callback manually), you can just call bdk to get a new addr and then ping the device yourself. and this would allow us to reduce complexity and delete some code
> actually, here's an idea: unless somebody here is opposed to this, i can make a pr to deprecate address validators in the next (0.20) release. if after that again nobody complains we can completely remove them and point users towards different strategies to achieve the same goal
### 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 `CHANGELOG.md`
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.
<!-- You can erase any parts of this template not applicable to your Pull Request. -->
### Description
This PR is to add 2 keys(`try_finalize` and `remove_partial_sigs`) in `SignOptions`. See this issue for detail https://github.com/bitcoindevkit/bdk/issues/612
### Notes to the reviewers
~I found the negative naming of these 2 new keys `do_not_finalize` and `do_not_remove_partial_sigs` are a bit confusing(like most negative named paremeter/variable). Should we actually change it back to positive naming(`do_finalize` and `do_remove_partial_sigs`)?~
### 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`
#### 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
### Description
This PR create a new trait `blockchain::GetBlockHash` with a `get_block_hash` method which returns a block hash given the block height. This has been implemented for all blockchain backends.
Fixes #603
### Notes to the reviewers
I haven't updated the `CHANGELOG.md` and docs. Am I suppose to update it for this 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
#### New Features:
* [x] I've added tests for the new feature
* [ ] I've added docs for the new feature
* [ ] I've updated `CHANGELOG.md`
projects / bdk / log