`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.
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.
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
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.
612da165f8cfbc2b0aee0a37b2cdc44d6da017c8 `Blockchain` stop_gap testing improvements (志宇)
8a5f89e129d421a41af02ea85383d5b82f5ff665 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 612da165f8cfbc2b0aee0a37b2cdc44d6da017c8
Tree-SHA512: 56f1bff788855facc21856209922594cff9f639c5c58ecd180a0493322a75a564b72ded330ab0b6d6c90007ce859d2b8a5d2870d619bae5ddf9a3d64837f3753
5ff8320e3b1cb4e3971549a3e3c168f20762d04b add private function ivcec_to_u32 in keyvalue (KaFai Choi)
e68d3b9e63914c1008e1a4bb20a847428659d52e 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 5ff8320e3b1cb4e3971549a3e3c168f20762d04b
Tree-SHA512: eb37de8217efeb89d3ae346da36d0fb55aa67554d591b4759500f793bcf6aa7601c3d717fd473136c88e76aa72dbb6008ecf62b1d4ccf5ba3cbd1598f758522a
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
In this way we can continue using reqwest v0.11, whose MSRV is now
1.56.1
The only difference between v1.56.0 and v1.56.1 is a bug fix for
CVE-2021-42574.
Only use the old `importmulti` with Core versions that don't support
descriptor-based (sqlite) wallets.
Add an extra feature to test against Core 0.20 called `test-rpc-legacy`
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.
6931d0bd1f044bf73c0cb760c0eb0be19aea8de1 add private function select_sorted_utxso to be resued by multiple CoinSelection impl (KaFai Choi)
545beec743412717895d0ba5d32ccd9aac68f4a6 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 6931d0bd1f044bf73c0cb760c0eb0be19aea8de1
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
022256c91ac7a4d9324a1a7925394a948feec808 Fix comment on peek_address (Lloyd Fournier)
00f0901bac805360695d0c492df8fa809918863e Add API for internal addresses (LLFourn)
Pull request description:
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.
### 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:
notmandatory:
ACK 022256c91ac7a4d9324a1a7925394a948feec808
Tree-SHA512: 0ead6669c9974332708ae6cba1a5be69cd4b3b6c7a21896efdce178d4406265ba072e1cf02d1913178d0a551f3b276d99b75676c632833964f87484e262a61b1
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.
