32ae95f463 Move change calculus to coin_select (Cesar Alvarez Vallero)
Pull request description:
### Description
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`
ACKs for top commit:
afilini:
ACK 32ae95f463
Tree-SHA512: 350adb86538949ff50f41151fc46c8d28d9f5fd659e9869882cc3cb30128d76d4b479512c74c721f8beebfdb5423363ad63368e30556efe65ced2b8c52c34ef6
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.
5c940c33cb Fix wallet sync not finding coins of addresses which are not cached (志宇)
Pull request description:
Fixes#521Fixes#451
^ 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.
f0c876e7bf/src/blockchain/mod.rs (L157-L161)
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
ACKs for top commit:
afilini:
ACK 5c940c33cb
Tree-SHA512: aee917ed4821438fc0675241432a7994603a09a77d5a72e96bad863e7cdd55a9bc6fbd931ce096fef1153905cf1b786e1d8d932dc19032d549480bcda7c75d1b
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
9d2024434e Fix: Run README.md example on the CI (meryacine)
Pull request description:
### 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
ACKs for top commit:
danielabrozzoni:
tACK 9d2024434e
Tree-SHA512: 5842f7cdc34d76045596a248ec80bbcf86591ec9abe32d92af8322672e7a5d08d3b4baf1a000b1556542b449271dc8c438e6269eaf0204bee815c67fcf1218a8
6db5b4a094 Introduce `get_checksum_bytes` method and improvements (志宇)
Pull request description:
### Description
`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`
ACKs for top commit:
afilini:
ACK 6db5b4a094
Tree-SHA512: 1cecc3a1514a3ec3ac0a50775f6b3c4dd9785e3606390ceba57cc6248b8ff19c4023add0643c48dd9d84984341c506c036c4880fca4a4358ce1b54ccb4c56687
`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
46c344feb0 Bump version to 0.20.1-dev (Steve Myers)
78d26f6eb3 Bump version to 0.20.0 (Steve Myers)
92b9597f8b Rename `set_current_height` to `current_height` (Alekos Filini)
b5a120c649 Missing newlines (Alekos Filini)
af6bde3997 Fix: Wallet sync may decrement address index (志宇)
45db468c9b Deprecate `AddressValidator` (志宇)
01141bed5a Update CHANGELOG and lib.rs docs version (Steve Myers)
87e8646743 Bump version to 0.20.0-rc.1 (Steve Myers)
Pull request description:
Proposed tweet:
📢 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
ACKs for top commit:
afilini:
ACK 46c344feb0
Tree-SHA512: 7c36a85611f715d76a37d5a285bc72f1a06297fc06b85cca7e38c3350fcbc0a3e35d38ce617a82d191538776aa49362e523beef70bbe3b93b21d8d28d774b75f
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
af6bde3997 Fix: Wallet sync may decrement address index (志宇)
Pull request description:
### Description
Fixes#649
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
ACKs for top commit:
afilini:
ACK af6bde3997
Tree-SHA512: d714bebcf7c2836f8b98129b39b4939b0e36726acf0208e52d501f433be6cdb12f1abebc28bd7da0be8b780ccce6e1e42c8fdc6633dd486bf329bc6f88e1ce67
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).
Tests added:
* bdk_blockchain_tests!::test_sync_address_index_should_not_decrement
* bdk_blockchain_tests!::test_sync_address_index_should_increment
These tests ensure there will be no unexpected address reuse when
grabbing a new address via `Wallet::get_address` with `AddressIndex::New`.
Other changes:
* Tweak `rpc.rs` so that clippy is happy.
45db468c9b Deprecate `AddressValidator` (志宇)
Pull request description:
### Description
`AddressValidator` should be deprecated as noted by @afilini [on Discord](https://discord.com/channels/753336465005608961/753367451319926827/994899488957272064):
> 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`
ACKs for top commit:
afilini:
ACK 45db468c9b
Tree-SHA512: 71071f4494537ece9153f5308cb4f576189016afa8ac87bc57bfdcda03ee94d5f7a3477d04f6dd37eeeea2fada6aaad42ad29c964df0971beeda7418ada65f6d
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
2af678aa84 Get block hash by its height (Vladimir Fomene)
Pull request description:
### 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`
ACKs for top commit:
notmandatory:
ACK 2af678aa84
Tree-SHA512: 9c084a6665ecbf27ee8170fdb06e0dc8373d6a901ce29e5f5a1bec111d1507cb3bee6b03a653a55fd20e0fabe7a5eada3353e24a1e21f3a11f01bb9881ae99e5
Create blockchain::GetBlockHash trait
with a method to get block hash given
a block height. Then, implement this
trait for all backends (Electrum, RPC
, Esplora, CBF). Referenced in issue 603.
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.
db9d43ed2f use network to set coin type (Esraa Jbara)
Pull request description:
resolves#578
* [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:
danielabrozzoni:
re-ACK db9d43ed2f
afilini:
re-ACK db9d43ed2f
Tree-SHA512: 0310a09ef21c6fc792688a9ccc19221b1cffaeceefd34f4c83f206e965abe963a78f9e4ca53db046b39e7bf1be118a101afe5c08c43f06ecf35ed9536102cd9b
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
612da165f8 `Blockchain` stop_gap testing improvements (志宇)
8a5f89e129 Fix hang when `ElectrumBlockchainConfig::stop_gap == 0` (志宇)
Pull request description:
* Ensure `chunk_size` is > 0 during wallet sync.
* Slight refactoring for better readability.
* Add test: `test_electrum_blockchain_factory_sync_with_stop_gaps`
<!-- You can erase any parts of this template not applicable to your Pull Request. -->
### Description
`Wallet::sync` hangs indefinitely when syncing with Electrum with `stop_gap` set as 0.
The culprit is having `chunk_size` set as `stop_gap`. A zero value results in syncing not being able to progress.
Fixes#651
### 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 612da165f8
Tree-SHA512: 56f1bff788855facc21856209922594cff9f639c5c58ecd180a0493322a75a564b72ded330ab0b6d6c90007ce859d2b8a5d2870d619bae5ddf9a3d64837f3753
This is a continuation of the #651 fix. We should also check whether the
same bug affects esplora as noted by @afilini. To achieve this, I've
introduced a `ConfigurableBlockchainTester` trait that can test multiple
blockchain implementations.
* Introduce `ConfigurableBlockchainTester` trait.
* Use the aforementioned trait to also test esplora.
* Change the electrum test to also use the new trait.
* Fix some complaints by clippy in ureq.rs file (why is CI not seeing
this?).
* Refactor some code.
5ff8320e3b add private function ivcec_to_u32 in keyvalue (KaFai Choi)
e68d3b9e63 remove Database::flush (KaFai Choi)
Pull request description:
<!-- You can erase any parts of this template not applicable to your Pull Request. -->
### Description
This PR is to remove Database::flush. See this issue for detail https://github.com/bitcoindevkit/bdk/issues/567
### Notes to the reviewers
The 2nd commit is a small refactoring of adding a new private ivec_to_u32 to avoid too much code duplication. Please let me know if it's ok to include this in this PR or I should make it into a separate PR
Currently existing test cases are shared across for all Databaes implementation so I am not sure if we should add specific test cases for keyvalue(Tree) for this auto-flush behaviour?(and I feel like it's more a implementation detail). Please let me know how should I proceed for test case in this PR
### 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
* [ ] I'm linking the issue being fixed by this PR
ACKs for top commit:
danielabrozzoni:
re-ACK 5ff8320e3b
Tree-SHA512: eb37de8217efeb89d3ae346da36d0fb55aa67554d591b4759500f793bcf6aa7601c3d717fd473136c88e76aa72dbb6008ecf62b1d4ccf5ba3cbd1598f758522a
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
Allows user to ask for a test db populated with clean coins
from coinbases. This is useful for testing the wallet behaviour
when some inputs are coinbases.
97bc9dc717 Discourage fee sniping with nLockTime (Daniela Brozzoni)
Pull request description:
### Description
By default bdk sets the transaction's nLockTime to current_height
to prevent 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 https://github.com/bitcoindevkit/bdk/issues/533
### Notes to the reviewers:
If you want to know more about fee sniping: https://bitcoinops.org/en/topics/fee-sniping/
### 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`
ACKs for top commit:
afilini:
ACK 97bc9dc717
Tree-SHA512: e92d1ae907687d9fee44d120d790f1ebdf14b698194979e1be8433310fd5636afa63808effed12fce6091f968ec6b76b727cfee6fed54068af0a7450239fdd26
77c7d0aae9 Additional comments for `TransactionDetails`. (志宇)
Pull request description:
### Description
I'm not sure if this is needed or helpful, but this PR adds comments to describe how the `sent` and `received` fields of `TransactionDetails` are calculated.
I wasn't sure how it was done until I looked deeper into the codebase (but maybe I am too much of a beginner and this is common sense for most).
### 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:
re-ACK 77c7d0aae9
Tree-SHA512: 8d29d249a70bc2d0631078b6772c5543bdc61ee43df3810ab666f5e97ca59b0d4cfc8acad14bbaf8674baba319f24fa2781a42740ca42bccd9688831aaedea72
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
d020dede37 Fix README.md link to rust 1.56.1 blog post (Steve Myers)
Pull request description:
### Description
Fix link to rust blog for 1.56.1 as pointed out by ulrichard: https://github.com/weareseba/bdk-reserves/pull/5#issuecomment-1159212838
### Checklists
#### All Submissions:
* [x] I've signed all my commits
Top commit has no ACKs.
Tree-SHA512: a5bd76fca97dd64c12617b43230dbc36b1178e47224ce324b67cd13999e5f92d2a05d6a9e909841e6d5c6904f2fa426b6bee1001e757d53cc91fb4fd3803f56b
