e7ec5a873328c9b68836306086bd9b021e9c952a refactor(wallet)!: remove redundant get_descriptor_for_keychain (Giovanni Napoli)
Pull request description:
Fixes#1501
### Description
Simplifies `public_descriptor` function by using `get_descriptor` and removes `get_descriptor_for_keychain`.
### Notes to the reviewers
Tested with `cargo test --all-features`.
### Changelog notice
- Simplifies `public_descriptor` function and removes `get_descriptor_for_keychain`
### Checklists
#### All Submissions:
* [X] I've signed all my commits
* [X] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md)
* [X] I ran `cargo fmt` and `cargo clippy` before committing
ACKs for top commit:
notmandatory:
ACK e7ec5a873328c9b68836306086bd9b021e9c952a
storopoli:
ACK e7ec5a8733
Tree-SHA512: 5981190ac882e08e42b1be53c55afb70c4ba7e7a99a8ae2a4e05f0618d0eb23849ce544024bb406e6a6918d9e9757d9ff6ad5a701cd9814b686e36f1ea16b44a
3bee563c810ace941e0e80ee8e410d843a05f5d8 refactor(wallet)!: remove TxOrdering::Bip69Lexicographic (nymius)
e5cb7b206619f5f3437e5ee95ed3e53885b745e6 feat(wallet): add TxOrdering::Custom (FadedCoder)
Pull request description:
<!-- You can erase any parts of this template not applicable to your Pull Request. -->
### Description
Resolves https://github.com/bitcoindevkit/bdk/issues/534.
Resumes from the work in https://github.com/bitcoindevkit/bdk/pull/556.
Add custom sorting function for inputs and outputs through `TxOrdering::Custom` and deprecates `TxOrdering::Bip69Lexicographic`.
<!-- Describe the purpose of this PR, what's being adding and/or fixed -->
### Notes to the reviewers
I tried consider all discussions in https://github.com/bitcoindevkit/bdk/issues/534 while implementing some changes to the original PR. I created a summary of the considerations I had while implementing this:
##### Why use smart pointers?
The size of enums and structs should be known at compilation time. A struct whose fields implements some kind of trait cannot be specified without using a smart pointer because the size of the implementations of the trait cannot be known beforehand.
##### Why `Arc` or `Rc` instead of `Box`?
The majority of the useful smart pointers that I know (`Arc`, `Box`, `Rc`) for this case implement `Drop` which rules out the implementation of `Copy`, making harder to manipulate a simple enum like `TxOrdering`. `Clone` can be used instead, implemented by `Arc` and `Rc`, but not implemented by `Box`.
##### Why `Arc` instead of `Rc`?
Multi threading I guess.
##### Why using a type alias like `TxVecSort`?
cargo-clippy was accusing a too complex type if using the whole type inlined in the struct inside the enum.
##### Why `Fn` and not `FnMut`?
`FnMut` is not allowed inside `Arc`. I think this is due to the `&mut self` ocupies the first parameter of the `call` method when desugared (https://rustyyato.github.io/rust/syntactic/sugar/2019/01/17/Closures-Magic-Functions.html), which doesn't respects `Arc` limitation of not having mutable references to data stored inside `Arc`:
Quoting the [docs](https://doc.rust-lang.org/std/sync/struct.Arc.html):
> you cannot generally obtain a mutable reference to something inside an `Arc`.
`FnOnce` > `FnMut` > `Fn`, where `>` stands for "is supertrait of", so, `Fn` can be used everywhere `FnMut` is expected.
##### Why not `&'a dyn FnMut`?
It needs to include a lifetime parameter in `TxOrdering`, which will force the addition of a lifetime parameter in `TxParams`, which will require the addition of a lifetime parameter in a lot of places more. **Which one is preferable?**
<!-- 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 -->
### Changelog notice
- Adds new `TxOrdering` variant: `TxOrdering::Custom`. A structure that stores the ordering functions to sort the inputs and outputs of a transaction.
- Deprecates `TxOrdering::Bip69Lexicographic`.
<!-- 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:
* [ ] 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
Top commit has no ACKs.
Tree-SHA512: 0d3e3ea9aee3a6c9e9d5e1ae93215be84bd1bd99907a319976517819aeda768a7166860a48a8d24abb30c516e0129decb6a6aebd8f24783ea2230143e6dcd72a
BIP 69 proposed a deterministic way to sort transaction inputs and
outputs with the idea of enhancing privacy. It was later discovered
there was no such enhancement but rather a decrement in privacy due to
this sorting.
To avoid the promotion of bad practices, the
TxOrdering::Bip69Lexicographic variant which implemented this BIP for
BDK is removed with this change.
Notice that you can still produce a BIP 69 compliant transaction
defining order functions for TxOrdering::Custom.
Signed-off-by: Steve Myers <steve@notmandatory.org>
The deterministic sorting of transaction inputs and outputs proposed in
BIP 69 doesn't improve the privacy of transactions as originally
intended. In the search of not spreading bad practices but still provide
flexibility for possible use cases depending on particular order of the
inputs/outpus of a transaction, a new TxOrdering variant has been added
to allow the implementation of these orders through the definition of
comparison functions.
Signed-off-by: Steve Myers <steve@notmandatory.org>
af75817d4bedbb5e148812d1073fd0729a23358b ref(tx_graph): Change last_seen to `HashMap<Txid, u64>` (valued mammal)
6204d2c766f968af6b63c664dda61fa8fc897e85 feat(tx_graph): Add method `txs_with_no_anchor_or_last_seen` (valued mammal)
496601b8b148afd2199a530d40edeafcb7967d46 test(tx_graph): Add test for `list_canonical_txs` (valued mammal)
c4057297a96ccbd49d984b7139994b801c2120dc wallet: delete method `insert_anchor` (valued mammal)
b34790c6b6d612661a4595bf10910294af862323 ref(tx_graph)!: Rename `list_chain_txs` to `list_canonical_txs` (valued mammal)
2ce4bb4dfc4f7b779a815eca45a3f6cee3d6c4e0 test(indexed_tx_graph): Add test_get_chain_position (valued mammal)
36f58870cb6eb24fe8c50ba4cf3ede910dd11fe8 test(wallet): Add test_insert_tx_balance_and_utxos (valued mammal)
bbc19c3536b25c78be8b5f3fe0cd9810aa679742 fix(tx_graph)!: Change tx_last_seen to `Option<u64>` (valued mammal)
324eeb3eb4e5231c6a81e6d197df788ad08b23a8 fix(wallet)!: Rework `Wallet::insert_tx` to no longer insert anchors (valued mammal)
Pull request description:
The PR changes the type of last_seen to `Option<u64>` for `txs` member of `TxGraph`.
This fixes an issue where unbroadcast and otherwise non-canonical transactions were returned from methods `list_chain_txs` and `Wallet::transactions` because every new tx inserted had a last_seen of 0 making it appear unconfirmed.
fixes#1446fixes#1396
### Notes to the reviewers
### Changelog notice
Changed
- Member `last_seen_unconfirmed` of `TxNode` is changed to `Option<u64>`
- Renamed `TxGraph` method `list_chain_txs` to `list_canonical_txs`
- Changed `Wallet::insert_tx` to take a single `tx: Transaction` as parameter
Added
- Add method `txs_with_no_anchor_or_last_seen` for `TxGraph`
- Add method `unbroadcast_transactions` for `Wallet`
### Checklists
#### All Submissions:
* [x] I've signed all my commits
* [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md)
* [x] I ran `cargo fmt` and `cargo clippy` before committing
#### 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
ACKs for top commit:
notmandatory:
Re ACK af75817d4bedbb5e148812d1073fd0729a23358b
Tree-SHA512: e664b3b49e2f547873923f15dffbbc7fa032b6240e5b856b180e9e26123ca141864d10448912dc4a31bbb200c75bef4251a910a4330dac17ee6841b564612d13
Also fixup `test_list_owned_txouts` to check that the right
outputs, utxos, and balance are returned at different local
chain heights.
This fixes an issue where unbroadcast and otherwise non-canonical
transactions were returned from methods `list_chain_txs` and
`Wallet::transactions` because every tx inserted had a last_seen
of 0 making it appear unconfirmed.
Note this commit changes the way `Balance` is represented due to
new logic in `try_get_chain_position` that no longer considers
txs with non-canonical anchors. Before this change, a tx anchored
to a block that is reorged out had a permanent effect on the
pending balance, and now only txs with a last_seen time or an
anchor confirmed in the best chain will return a `ChainPosition`.
8f5b172e59b4d5e199b98777855b0d9b46742f66 test(wallet): verify wallet panics in dev mode if using keychains with duplicate spks (Steve Myers)
46c6f18cc3ff71efda2b8547aa32854f9b88762e refactor(chain): calculate DescriptorId as sha256 hash of spk at index 0 (Steve Myers)
Pull request description:
### Description
Rename `DescriptorId` to `KeychainId` and `descriptor_id()` to `keychain_id()`.
Calculate keychain ids as the hash of the spk derived from its descriptor as index 0.
Added docs to `Wallet` and `KeychainTxOutIndex::insert_descriptor()` explaining that it's the users responsibility not to use wildcard and non-wildcard descriptors that can derive the same script pubkey. I also recommended for `Wallet` that legacy non-wildcard descriptors be added to a temporary `Wallet` and swept into a modern wildcard descriptor.
### Notes to the reviewers
fixes#1483
### Changelog notice
changed
- Renamed DescriptorId to KeychainId, DescriptorExt::descriptor_id() to keychain_id().
### Checklists
#### All Submissions:
* [x] I've signed all my commits
* [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md)
* [x] I ran `cargo fmt` and `cargo clippy` before committing
#### Bugfixes:
* [ ] This pull request breaks the existing API
* [ ] I've added tests to reproduce the issue which are now passing
* [x] I'm linking the issue being fixed by this PR
ACKs for top commit:
LLFourn:
ACK 8f5b172e59b4d5e199b98777855b0d9b46742f66
oleonardolima:
Concept ACK 8f5b172e59b4d5e199b98777855b0d9b46742f66
Tree-SHA512: 07defa208d9cfcd61bc6e31ded06a287c392c51bcc4949f601ecfac635c3443e7d08c62d92618ed894dc5ef13cdcf784771a6bf8904a5397110bedb1563f52d4
In `rust-bitcoin` the `blockdata` module is a code organisation thing,
it should never have been public. One day those guys would like to
remove it, so as not to be a PITA for `bdk` when they do lets remove all
usage of `blockdata::` now.
Internal change only, no externally visible changes.
- Change param `hash` to `&Message` in `sign_psbt_ecdsa`
- Remove unused methods `compute_legacy_sighash`,
and `compute_segwitv0_sighash`.
- Match on `self.ctx` when signing for `SignerWrapper<PrivateKey>`
since we'd be lacking context that should normally occur during
sync with a chain source. The logic for inserting a graph
anchor from a `ConfirmationTime` is moved to the wallet common
test module in order to simulate receiving new txs and
confirming them.
4bddb0de6262fb4014d51baf8c9453eb45a3d0ef feat(wallet): add back TxBuilder finish() and sort_tx() with thread_rng() (Steve Myers)
45c0cae0a461232bf746375083e2005c5df5f913 fix(bdk): remove rand dependency (rustaceanrob)
Pull request description:
### Description
WIP towards removing `rand` fixes#871
The `rand` dependency was imported explicitly, but `rand` is also implicitly used through the `rand-std` feature flag on `bitcoin`.
### Notes to he reviewers
**Updated:**
`rand` was used primarily in two parts of `bdk`. Particularly in signing and in building a transaction.
Signing:
- Used implicitly in [`sign_schnorr`](https://docs.rs/bitcoin/latest/bitcoin/key/struct.Secp256k1.html#method.sign_schnorr), but nowhere else within `signer`.
Transaction ordering:
- Used to shuffle the inputs and outputs of a transaction, the default
- Used in the single random draw __as a fallback__ to branch and bound during coin selection. Branch and bound is the default coin selection option.
See conversation for proposed solutions.
### Changelog notice
- Remove the `rand` dependency from `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
#### New Features:
* [x] I've added tests for the new feature
* [x] I've added docs for the new feature
#### Bugfixes:
* [x] This pull request breaks the existing API
* [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:
ValuedMammal:
ACK 4bddb0de6262fb4014d51baf8c9453eb45a3d0ef
notmandatory:
ACK 4bddb0de6262fb4014d51baf8c9453eb45a3d0ef
Tree-SHA512: 662d9bcb1e02f8195d73df16789b8c2aba8ccd7b37ba713ebb0bfd19c66163acbcb6f266b64f88347cbb1f96b88c8a150581012cbf818d1dc8b4437b3e53fc62
Rather than comingle various `SignOptions` with the finalization
step, we simply clear all fields when finalizing as per the PSBT
spec in BIPs 174 and 371 which is more in line with user
expectations.
bdk_chain to 0.16.0
bdk_bitcoind_rpc to 0.12.0
bdk_electrum to 0.15.0
bdk_esplora to 0.15.0
bdk_file_store to 0.13.0
bdk_sqlite keep at 0.2.0
bdk_testenv to 0.6.0
bdk_hwi to 0.3.0
a0bf45bef1b53f8aec273f7fe89915f3d41974c0 docs: remove PersistBackend from docs, Cargo.toml and wallet README.md (Steve Myers)
feb27df180938759670afd2c9cd15529a22c4772 feat(chain)!: add `take` convenience method to `Append` trait (志宇)
1eca568be5bb0c234fa1cea2eb7f9424f60974eb feat!: rm `persist` submodule (志宇)
Pull request description:
### Description
@LLFourn suggested these changes which greatly simplifies the amount of code we have to maintain and removes the `async-trait` dependency. We remove `PersistBackend`, `PersistBackendAsync`, `StageExt` and `StageExtAsync` completely. Instead, we introduce `Wallet::take_staged(&mut self) -> Option<ChangeSet>`.
The original intention to keep a staging area (`StageExt`, `StageExtAsync`) is to enforce:
1. The caller will not persist an empty changeset.
2. The caller does not take the staged changeset if the database (`PersistBackend`) fails.
We achieve `1.` by returning `None` if the staged changeset is empty.
`2.` is not too important. The caller can try figure out what to do with the changeset if persisting to db fails.
### Notes to the reviewers
I added a `take` convenience method to the `Append` trait. I thought it would be handy for the caller to do a staging area with this.
### Changelog notice
* Remove `persist` submodule from `bdk_chain`.
* Change `Wallet` to outsource it's persistence logic by introducing `Wallet::take_staged`.
* Add `take` convenience method to `Append` trait.
### Checklists
#### All Submissions:
* [x] I've signed all my commits
* [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md)
* [x] I ran `cargo fmt` and `cargo clippy` before committing
#### New Features:
~* [ ] I've added tests for the new feature~
* [x] I've added docs for the new feature
ACKs for top commit:
notmandatory:
ACK a0bf45bef1b53f8aec273f7fe89915f3d41974c0
evanlinjin:
self-ACK a0bf45bef1b53f8aec273f7fe89915f3d41974c0
Tree-SHA512: 38939ab446c84d9baecd4cd36a7b66add5a121212ad5e06ade04a60f7903133dd9a20219e230ab8a40404c47e07b946ccd43085572d71c3a2a80240a2223a500
Remove `PersistBackend`, `PersistBackendAsync`, `StageExt` and
`StageExtAsync`. Remove `async` feature flag and dependency. Update
examples and wallet.
20341a3ca1ab85ad77e022b5028136a635c3f42c fix: typo on `SignedAmount` instead of `Amount` (Leonardo Lima)
Pull request description:
<!-- You can erase any parts of this template not applicable to your Pull Request. -->
### Description
It fixes the typo on the `expect()` message introduced on #1426, noticed at https://github.com/bitcoindevkit/bdk/pull/1426#discussion_r1626761825
<!-- Describe the purpose of this PR, what's being adding and/or fixed -->
### 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:
storopoli:
ACK 20341a3ca1ab85ad77e022b5028136a635c3f42c
notmandatory:
ACK 20341a3ca1ab85ad77e022b5028136a635c3f42c
Tree-SHA512: 62deb7308b2a6891eeb4598ebdf3c5ee1541fc52fd454bca2816b819bd7f6ab25c18e10c4166c80c4e553bcb3ce2207323376d260484a956837ac857854cbd6b
Still enable the `persist` submodule without `miniscript` feature flag.
Only disable `CombinedChangeSet`.
Also stop `cargo clippy` from complaining about unused imports when
`miniscript` is disabled.
