2c02a44586c67d1ec9720f17a3748f28c1d18643 Test: No address reuse for single descriptor (志宇)
Pull request description:
### Description
Just a simple new test.
This test is to ensure there are no regressions when we later change
internal logic of `Wallet`. A single descriptor wallet should always get
a new address with `AddressIndex::New` even if we alternate grabbing
internal/external keychains.
I thought of adding this during work on #647
### Checklists
#### All Submissions:
* [x] I've signed all my commits
* [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md)
* [x] I ran `cargo fmt` and `cargo clippy` before committing
ACKs for top commit:
danielabrozzoni:
tACK 2c02a44586c67d1ec9720f17a3748f28c1d18643
rajarshimaitra:
tACK 2c02a44586c67d1ec9720f17a3748f28c1d18643
Tree-SHA512: d065ae0979dc3ef7c26d6dfc19c88498e4bf17cc908e4f5677dcbf62ee59162e666cb00eb87b96d4c2557310960e3677eec7b6d907a5a4860cb7d2d74dba07b0
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.
e3a17f67d90f11d1d1a27a98ec97674c8cd3d2f7 add try_finalize to SignOptions (KaFai Choi)
c2e4ba8cbd77f1a41d269b621b0001d042c1ea57 add remove_partial_sigs to SignOptions (KaFai Choi)
Pull request description:
<!-- 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
ACKs for top commit:
notmandatory:
ReACK e3a17f67d90f11d1d1a27a98ec97674c8cd3d2f7
Tree-SHA512: 781b31d3ecf0bcd605206c0481fd5de3125f1c8ff18a463dbf4c821e5557847f7d70a3fe8618e100fb89f4f6899655ac0efa3593f77f915ad5bcb7e558bb2a7a
5d00f8238886a993ef21056e5b3e216a4aae6951 test that BDK won't add unconf inputs when fee bumping (Daniela Brozzoni)
98748906f6799041341227de33bec20e8c6ef4b0 test: fix populate_test_db conf calculation (Daniela Brozzoni)
1d9fdd01faf3a0f17c57381a7c54d136e9d69ffe Remove wrong TODO comment in build_fee_bump (Daniela Brozzoni)
Pull request description:
### Description
Closes#144
### Notes to reviewers
#144 is describing a bug that doesn't seem to happen in BDK master anymore (BDK not respecting BIP125 rule 2). This PR just adds a test to check that the bug is 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
#### Bugfixes:
* [ ] This pull request breaks the existing API
* [x] I've added tests to reproduce the issue which are now passing
* [x] I'm linking the issue being fixed by this PR
ACKs for top commit:
afilini:
ACK 5d00f8238886a993ef21056e5b3e216a4aae6951
Tree-SHA512: 95833f3566f9716762884d65f3f656346482e45525a3e92efa86710b9f574fdd9af7d235f1f425e4298d6ff380db9af60d1d2008ccde2588d971757db2d136b8
populate_test_db would previously give back a transaction with N + 1
confirmations when you asked for N.
This commit also fixes test_spend_coinbase, which would improperly
ask for a transaction with 0 confirmations instead of 1.
The proposed solution is bad for privacy as well.
Let's call the initial change output, which is normally shrink when you
fee bump, change#1, and the extra output aforementioned change#2 (as,
in this case, it's going to be a change output as well). If you add change#2
you might not revel change#1, but you're still revealing change#2.
You're not improving your privacy, and you're wasting money in fees.
e85aa247cb85601e96f4d82b7a996f709559223f Avoid using immature coinbase inputs (Daniela Brozzoni)
0e0d5a0e957fbf602023011d9114d8bcb8effb67 populate_test_db accepts a `coinbase` param (Daniela Brozzoni)
Pull request description:
### Description
With this PR we start considering how many confirmations a coinbase has. If it's not mature yet, we don't use it for building transactions.
Fixes#413
### Notes to the reviewers
This PR is based on #611, review that one before reviewing this 😄
007c5a78335a3e9f6c9c28a077793c2ba34bbb4e adds a coinbase parameter to `populate_test_db`, to specify if you want the db to be populated with immature coins. This is useful for `test_spend_coinbase`, but that's probably going to be the only use case.
I don't think it's a big deal to have a test function take an almost_always_useless parameter - it's not an exposed API, anyways. But, if you can come up with a different way of implementing `test_spend_coinbase` that doesn't require 007c5a78335a3e9f6c9c28a077793c2ba34bbb4e, even better! I looked for it for a while, but other than duplicating the whole `populate_test_db` code, which made the test way harder to comprehend, I didn't find any other 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:
* [ ] This pull request breaks the existing API
* [x] I've added tests to reproduce the issue which are now passing
* [x] I'm linking the issue being fixed by this PR
ACKs for top commit:
afilini:
ACK e85aa24
Tree-SHA512: 30f470c33f9ffe928500a58f821f8ce445c653766459465eb005031ac523c6f143856fc9ca68a8e1f23a485c6543a9565bd889f9557c92bf5322e81291212a5f
6a150368674046f796f5c37755896f16d8345fbc Restrict `drain_to` usage (Daniela Brozzoni)
Pull request description:
### Description
Before this commit, you could create a transaction with `drain_to` set
without specifying recipients, nor `drain_wallet`, nor `utxos`. What would
happen is that BDK would pick one input from the wallet and send
that one to `drain_to`, which is quite weird.
This PR restricts the usage of `drain_to`: if you want to use it as a
change output, you need to set recipients as well. If you want to send
a specific utxo to the `drain_to` address, you specify it through
`add_utxos`. If you want to drain the whole wallet, you set
`drain_wallet`. In any other case, if `drain_to` is set, we return a
`NoRecipients` error.
Fixes#620
### 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 - kinda?
* [x] I've added tests to reproduce the issue which are now passing
* [x] I'm linking the issue being fixed by this PR
ACKs for top commit:
afilini:
ACK 6a150368674046f796f5c37755896f16d8345fbc
Tree-SHA512: 69076977df37fcaac92dd99d2f2c9c37098971817d5b0629fc7e3069390eb5789331199b3b7c5d0569d70473f4f37e683a5a0b30e2c6b4e2ec22a5ef1d0f2d77
By default bdk sets the transaction's nLockTime to current_height
to discourage fee sniping.
current_height can be provided by the user through TxParams; if the user
didn't provide it, we use the last sync height, or 0 if we never synced.
Fixes#533
Before this commit, you could create a transaction with `drain_to` set
without specifying recipients, nor `drain_wallet`, nor `utxos`. What would
happen is that BDK would pick one input from the wallet and send
that one to `drain_to`, which is quite weird.
This PR restricts the usage of `drain_to`: if you want to use it as a
change output, you need to set recipients as well. If you want to send
a specific utxo to the `drain_to` address, you specify it through
`add_utxos`. If you want to drain the whole wallet, you set
`drain_wallet`. In any other case, if `drain_to` is set, we return a
`NoRecipients` error.
Fixes#620
This allows the signer to know the signing context precisely without
relying on heuristics on the psbt fields.
Due to the context being static, we still have to look at the PSBT when
producing taproot signatures to determine the set of leaf hashes that
the key can sign for.
Add two new traits:
- `StatelessBlockchain` is used to tag `Blockchain`s that don't have any
wallet-specic state, i.e. they can be used as-is to sync multiple wallets.
- `BlockchainFactory` is a trait for objects that can build multiple
blockchains for different descriptors. It's implemented automatically
for every `Arc<T>` where `T` is a `StatelessBlockchain`. This allows a
piece of code that deals with multiple sub-wallets to just get a
`&B: BlockchainFactory` to sync all of them.
These new traits have been implemented for Electrum, Esplora and RPC
(the first two being stateless and the latter having a dedicated
`RpcBlockchainFactory` struct). It hasn't been implemented on the CBF
blockchain, because I don't think it would work in its current form
(it throws away old block filters, so it's hard to go back and rescan).
This is the first step for #549, as BIP47 needs to sync many different
descriptors internally.
It's also very useful for #486.
A `is_spent` field is added to LocalUtxo; when a txo is spent we set
this field to true instead of deleting the entire utxo from the
database.
This allows us to create txs double-spending txs already in blockchain.
Listunspent won't return spent utxos, effectively excluding them from the
coin selection and balance calculation
Although somewhat convenient to have, coupling the Wallet with
the blockchain trait causes development friction and complexity.
What if sometimes the wallet is "offline" (no access to the blockchain)
but sometimes its online?
The only thing the Wallet needs the blockchain for is to sync.
But not all applications will even use the sync method and the sync
method doesn't require the full blockchain functionality.
So we instead pass the blockchain in when we want to sync.
- To further reduce the coupling with blockchain I removed the get_height call from `new` and just use the height of the
last sync in the database.
- I split up the blockchain trait a bit into subtraits.
There are good reasons for applications to need to get internal
addresses too. For example creating a transactions that splits an output
into several smaller ones.
The default sync verification is removed from wallet module.
By default sync time verification only makes sense for `electrum` and
`esplora` backend as they are usually untrusted 3rd party services.
script verification for transaction is costly, so removing default
script verification optimizes performance.
