We would previously calculate the fee amount in two steps:
1. Add the weight of the empty transaction
2. Add the weight of each output
That's unnecessary: you can just use the weight of the transaction
*after* the output addition. This is clearer, but also avoids a
rare bug: if there are many outputs, adding them would cause the
"number of outputs" transaction parameter lenght to increase, and we
wouldn't notice it.
This might still happen when adding the drain output - this
commit also adds a comment as a reminder.
The former way to compute and create change was inside `create_tx`, just after
performing coin selection.
It blocked the opportunity to have an "ensemble" algorithm to decide between
multiple coin selection algorithms based on a metric, like Waste.
Now, change isn't created inside `coin_select` but the change amount and the
possibility to create change is decided inside the `coin_select` method. In
this way, change is associated with the coin selection algorithm that generated
it, and a method to decide between them can be implemented.
Previously, electrum-based blockchain implementations only synced for
`scriptPubKey`s that are already cached in `Database`.
This PR introduces a feedback mechanism, that uses `stop_gap` and the
difference between "current index" and "last active index" to determine
whether we need to cache more `scriptPubKeys`.
The `WalletSync::wallet_setup` trait now may return an
`Error::MissingCachedScripts` error which contains the number of extra
`scriptPubKey`s to cache, in order to satisfy `stop_gap` for the next call.
`Wallet::sync` now calls `WalletSync` in a loop, cacheing inbetween
subsequent calls (if needed).
2c02a44586 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 2c02a44586
rajarshimaitra:
tACK 2c02a44586
Tree-SHA512: d065ae0979dc3ef7c26d6dfc19c88498e4bf17cc908e4f5677dcbf62ee59162e666cb00eb87b96d4c2557310960e3677eec7b6d907a5a4860cb7d2d74dba07b0
92b9597f8b Rename `set_current_height` to `current_height` (Alekos Filini)
Pull request description:
### Description
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
ACKs for top commit:
danielabrozzoni:
utACK 92b9597f8b - I'm sorry I didn't notice it!
Tree-SHA512: 3391068b2761bcd04d740ef41f9e772039fca7bc0e0736afcbc582ec74b6c91eb155d9e09dd7a07462eec29e32ac86e41ba339d9a550af3f754164cab6bdbf61
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.
e3a17f67d9 add try_finalize to SignOptions (KaFai Choi)
c2e4ba8cbd 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 e3a17f67d9
Tree-SHA512: 781b31d3ecf0bcd605206c0481fd5de3125f1c8ff18a463dbf4c821e5557847f7d70a3fe8618e100fb89f4f6899655ac0efa3593f77f915ad5bcb7e558bb2a7a
5d00f82388 test that BDK won't add unconf inputs when fee bumping (Daniela Brozzoni)
98748906f6 test: fix populate_test_db conf calculation (Daniela Brozzoni)
1d9fdd01fa 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 5d00f82388
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.
e85aa247cb Avoid using immature coinbase inputs (Daniela Brozzoni)
0e0d5a0e95 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
6a15036867 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 6a15036867
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.
e7a56a9268 Change wallet::get_funded_wallet to return Wallet<AnyDatabase> (Steve Myers)
Pull request description:
### Description
Change testing function `wallet::get_funded_wallet` to return `Wallet<AnyDatabase>` instead of `Wallet<MemoryDatabase>`. This will allow us to use this function for testing `bdk-ffi` which only works with `Wallet<AnyDatabase>`.
### Notes to the reviewers
This is required to complete https://github.com/bitcoindevkit/bdk-ffi/pull/148.
### 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
ACKs for top commit:
afilini:
ACK e7a56a9268
Tree-SHA512: 47b53ab6dcee63fc7b24666d3cf9a0ad832782081dd2fe92961c8c9b4c302df90db96b0b518af71d6cbc85319434971219100f8cedb35ce7212d944db29a4295
6931d0bd1f add private function select_sorted_utxso to be resued by multiple CoinSelection impl (KaFai Choi)
545beec743 add OldestFirstCoinSelection (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 `OldestFirstCoinSelection`. See this issue for detail https://github.com/bitcoindevkit/bdk/issues/120
<!-- Describe the purpose of this PR, what's being adding and/or fixed -->
### Notes to the reviewers
Apologize in advance if the quality of this PR is too low.(I am newbie in both bitcoin wallet and rust).
While this PR seemed very straight-forward to me in the first glance, it's actually a bit more complicated than I thought as it involves calling DB get the blockheight before sorting it.
The current implementation should be pretty naive but I would like to get some opinion to see if I am heading to a right direction first before working on optimizations like
~~1. Avoiding calling DB for optional_utxos if if the amount from required_utxos are already enough.~~ Probably not worth to do such optimization to keep code simpler?
### 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:
afilini:
ACK 6931d0bd1f
Tree-SHA512: d297bbad847d99cfdd8c6b1450c3777c5d55bc51c7934f287975c4d114a21840d428a75a172bfb7eacbac95413535452b644cab971efb8c0b5caf0d06d6d8356
The `WalletExport` type can still be used as an alias to
`FullyNodedExport` but it has been marked as deprecated and will be
removed in the next release.
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
