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.
### Changelog notice
Breaking change: the BDK Signer now produces low-R signatures by default, saving one byte. If you want to preserve the original behavior, set `allow_grinding` in the `SignOptions` to `false`.
### Notes to the reviewers
This PR resolves issue #695
#### 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:
* [ ] 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, 6 Oct 2022 10:30:43 +0000 (13:30 +0300)]
Add signature grinding for ECDSA signatures
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.
This is a small fix to the psbt_signer example to also use the `descriptor!` macro.
### Notes to the reviewers
I also added more docs to at the beginning of the example.
### Changelog notice
None
### 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
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
`Wallet::descriptor_checksum` should return the checksum, not the descriptor without the checksum.
### Notes to the reviewers
Please merge.
### Changelog notice
Fix `Wallet::descriptor_checksum` to actually return the descriptor checksum.
### 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
~* [ ] I'm linking the issue being fixed by this PR~
ACKs for top commit:
danielabrozzoni:
ACK af0b3698c691cd08ece9ae201cc9699f0d57acc8 - I run the test you added with the old code, and verified that the bug was there. I then run the test (with a few more dbg!() expressions) and manually verified that the problem is fixed.
notmandatory:
ACK af0b3698c691cd08ece9ae201cc9699f0d57acc8
This commit contains a change to address issue #752
cargo test runs successfully.
### Notes to the reviewers
Hi, newbie here learning Rust and BDK! I've removed the lazy_static block in this commit, and when learning about lazy_static also came across something called [once_cell](https://doc.rust-lang.org/std/cell/struct.OnceCell.html), [soon to be available in stdlib](https://github.com/rust-lang/rust/issues/74465). It's like lazy_static but faster and is not a macro. Shall I keep the lazy_static and create an issue to switch to once_cell in the future, or remove the lazy_static (as I've done) and create an issue anyway?
#### All Submissions:
* [x] I've signed all my commits
* [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md)
* [x] I ran `cargo fmt` and `cargo clippy` before committing
This check is redundant since it's already performed by miniscript (see https://docs.rs/miniscript/7.0.0/miniscript/miniscript/analyzable/enum.AnalysisError.html#variant.RepeatedPubkeys) and it was incorrectly failing on tr descriptors that contain duplicated keys across different taproot leaves
Fixes #760
### Changelog notice
### 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
There is an "unmaintained" warning in a old version of clap, which triggered the issue.
Ideally, we should not have clap in bdk's dependency. It was only used for the `compiler.rs` example, which was a very tiny clap app compiling miniscript policies, and it wasn't really an example for bdk.
This PR rewrites the example as a `policy.rs` which demos the BDK's Policy module and policy structures.
- Use a `wsh(multi(2, Privkey, Pubkey))` descriptor, which has only one part private and other part public.
- use `into_wallet_descriptor()` to turn that into a `Descriptor` and `KeyMap`.
- Use the `KeyMap` to create a custom signer.
- Extract the descriptor `Policy` structure from the given keymap.
I am not very sure on how much this example is helpful. I still find it hard to read the Policy structure visually. But if Policy is something we want the user to know about descriptors and bdk wallets, this shows how to extract it for a simple multisig condition.
Note: There is no use of `bdk::wallet` in the example. BDK uses the Policy extraction internally while transaction creation. But all these are exposed publicly, so can be used independently too.
### Questions:
- Should we still have a `minscript::policy::compile()` example? Which IIUC is very different from `bdk::policy:Policy`. I didn't include it in this PR, because I am not sure if it fits inside bdk example categories.
- Should we expose `extract_policy` as an wallet API? All though its possible to get policy without creating a wallet, why not let the wallet also spit one out for itself, if its useful?
### Checklists
#### All Submissions:
* [x] I've signed all my commits
* [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md)
* [x] I ran `cargo fmt` and `cargo clippy` before committing
This check is redundant since it's already performed by miniscript (see
https://docs.rs/miniscript/7.0.0/miniscript/miniscript/analyzable/enum.AnalysisError.html#variant.RepeatedPubkeys)
and it was incorrectly failing on tr descriptors that contain duplicated
keys across different taproot leaves
During table creation, Sqlite does not throw an error when a column datatype is not defined. In addition, the datatype provided during table creation does not put a constraint on the type of data that can be put in that column. So you can easily put a string value in an integer column. Despite this, I think it is important for us to add the datatype for clarity.
### Notes to the reviewers
You can read more about how Sqlite dynamic typing [here](https://www.sqlite.org/faq.html). I have amended our `migrate` code with a new commit. The idea is to run migrations in a transaction so that they either succeed or fail. This prevents us from having the database in an inconsistent state at any point in time.
### 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
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
### Changelog notice
- Enable signing taproot transactions that only specify the `non_witness_utxo`
### 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:
danielabrozzoni:
tACK 5e9965fca7fb50708af0d293a9f6cd27d0c85fed - the code looks good to me, I played around with the test you provided (inspecting the PSBT, adding/removing the witness and non-witness utxos, etc) and everything works as expected.
This PR add more test to the database module and also fixes certain bugs discovered by the written test. I also amended the name used for the database parameter in the test functions.
### Notes to the reviewers
This contributes to fixing #699
### Changelog notice
<!-- Notice the release manager should include in the release tag message changelog -->
<!-- See https://keepachangelog.com/en/1.0.0/ for examples -->
### 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
Vladimir Fomene [Thu, 1 Sep 2022 10:27:09 +0000 (13:27 +0300)]
Sqlite migrations should either succeed or fail
The current implementation of the `migrate` method for
Sqlite database does not rollback changes when there is
an error while running one of the migration scripts. This
can leave the database in an inconsistent state. This
change ensures that migrations either succeed completely
or fail.
This PR includes just a few tiny changes to the docs trying to make it a bit more clear what PSBT finalization is and what to expect.
### Notes to the reviewers
### Changelog notice
### 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
The purpose of the PR is to provide a more convenient way to calculate the transaction fee amount and fee rate for a PSBT. This PR adds `fee_amount` and `fee_rate` functions to the existing `PsbtUtils` trait and implements them for `PartiallySignedBitcoinTransaction`. The `fee_rate` value is only valid if the PSBT it is called on is fully signed and finalized.
See related discussion: https://github.com/bitcoindevkit/bdk-ffi/issues/179
### Changelog
Added
- 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.
### Notes to the reviewers
Ideally I'd like `fee_rate` to return an `Option` and return `None` if the PSBT isn't finalized. But I'm not quite sure how to determine if a PSBT is finalized without having a `Wallet` and running it through the finalize code first. Or there might be a way to fill in missing signatures with properly sized fake data prior to calculating the fee rate. For now I think it's enough to do this simple approach with usage warning in the rust docs.
### 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
Adding a simple example of how to create a PSBT with a watch only wallet and then sign it with a signing wallet.
### Notes to the reviewers
This example was inspired by a question from a user.
### Changelog notice
none.
### 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
Vladimir Fomene [Tue, 13 Sep 2022 08:08:03 +0000 (11:08 +0300)]
Set tx field to none if `include_raw` is false
`del_tx` pulls the TransactionDetails object using
`select_transaction_details_by_txid` method which gets the transaction
details' data with a non-None transaction field even if the
`include_raw` argument is `false`. So it becomes necessary to Set
the transaction field in transactiondetails to None in `del_tx`, when
we make a call to it with `include_raw=false`.
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
projects / bdk / log