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.
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
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.
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
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
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.
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
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
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.
97bc9dc7170c336e97cd756b1f07ac3c23a39626 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 97bc9dc7170c336e97cd756b1f07ac3c23a39626
Tree-SHA512: e92d1ae907687d9fee44d120d790f1ebdf14b698194979e1be8433310fd5636afa63808effed12fce6091f968ec6b76b727cfee6fed54068af0a7450239fdd26
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
a85ef62698eae748242f31db80fa3aabd3b2b64e fix typo (Buck Perley)
Pull request description:
<!-- You can erase any parts of this template not applicable to your Pull Request. -->
### Description
<!-- Describe the purpose of this PR, what's being adding and/or fixed -->
just a small typo fix
### Notes to the reviewers
<!-- 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
ACKs for top commit:
rajarshimaitra:
ACK a85ef62698eae748242f31db80fa3aabd3b2b64e
afilini:
ACK a85ef62698eae748242f31db80fa3aabd3b2b64e
Tree-SHA512: 089de23adae62492a0b39a27c9cb8cb8afc99e5634194118681b8a9a46ff0b073558f9cd515cd4db4c9c6e6f9c813bfa4b193d4e3f9558b34ad29cbd46cf028c
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`
d9b9b3dc464d6a9cd4157cc4135a2e7b88ce4ab3 Fix InvalidColumnIndex error (Philipp Hoenisch)
Pull request description:
This query returns 7 rows, so last row is index 6
### 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] I've added tests to reproduce the issue which are now passing
ACKs for top commit:
danielabrozzoni:
tACK d9b9b3dc464d6a9cd4157cc4135a2e7b88ce4ab3
rajarshimaitra:
tACK d9b9b3dc464d6a9cd4157cc4135a2e7b88ce4ab3
Tree-SHA512: 8a3d8a291daa4af86a2a2eacc31f002972dd9cdb9bf300a4b09e2e015c4a967dc4fa7e925afbcce8b104a01e1d7f7c8cb0badda8e1ac5ade511681f490c719d5
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.
35feb107ed5969720ce54a6aa76b7b2176f6c7c1 [CI] Fix cont_integration test-blockchains to run all tests (Steve Myers)
24719081511969299459d01566b0d669a7b51f7b Update CHANGELOG with warning about sqlite-db deleted wallet data (Steve Myers)
0b1a399f4e556a981bb992cc9b1d34318e260a7c Update sqlite schema with unique index for utxos, change insert_utxo to upsert (Steve Myers)
cea79872d717b395560a344cc4dd0e022c3bd9a9 Update database tests to verify set_utxo upserts (Steve Myers)
Pull request description:
### Description
This PR fixes#591 by:
1. Add sqlite `MIGRATIONS` statements to remove duplicate utxos and add unique utxos index on txid and vout.
2. Do an upsert (if insert fails update) instead of an insert in `set_utxo()`.
3. Update database::test::test_utxo to also verify `set_utxo()` doesn't insert duplicate utxos.
### Notes to the reviewers
I verified the updated `test_utxo` fails as expected before my fix and passes after the fix. I tested the new migrations using the below `bdk-cli` command and a manually updated sqlite db with duplicate utxos.
```shell
cargo run --no-default-features --features cli,sqlite-db,esplora-ureq -- wallet -w test1 --descriptor "wpkh(tpubEBr4i6yk5nf5DAaJpsi9N2pPYBeJ7fZ5Z9rmN4977iYLCGco1VyjB9tvvuvYtfZzjD5A8igzgw3HeWeeKFmanHYqksqZXYXGsw5zjnj7KM9/*)" sync
```
### 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
* [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:
utACK 35feb107ed5969720ce54a6aa76b7b2176f6c7c1 - Code looks good, but I didn't do any local test to see if the db gets wiped
Tree-SHA512: 753c7a0cfd0e803b5e12f39181d9a718791c4ce229d5072e6498db75a7008e94d447b3d0b4b0c205e7a8f127f60102e12bac2d271b8bad3a3038856bfd54e99c
