We would previously insert just one coinbase transaction in the database
if we caught multiple in the same sync.
When we sync with electrum, before committing to the database, we remove
from the update conflicting transactions, using the
`make_txs_consistent` function. This function considers two txs to be
conflicting if they spend from the same outpoint - but every coinbase
transaction spends from the same outpoint!
Here we make sure to avoid filtering out coinbase transactions, by
adding a check on the txid just before we do the filtering.
Fixes#1051
Before this commit, the rpc backend would not notice immature utxos
(`listunspent` does not return them), making the rpc balance different
to other blockchain implementations.
Co-authored-by: Daniela Brozzoni <danielabrozzoni@protonmail.com>
The new implementation fixes the following:
* We can track more than 100 scriptPubKeys
* We can obtain more than 1000 transactions per sync
* `TransactionDetails` for already-synced transactions are updated when
new scriptPubKeys are introduced (fixing the missing balance/coins
issue of supposedly tracked scriptPubKeys)
`RpcConfig` changes:
* Introduce `RpcSyncParams`.
* Remove `RpcConfig::skip_blocks` (this is replaced by
`RpcSyncParams::start_time`).
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).
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.
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.
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`
1803f5ea8a7a2f8f9ebdfa520baad91f84bd6ba0 Update `electrsd` to v0.15 (Alekos Filini)
Pull request description:
<!-- You can erase any parts of this template not applicable to your Pull Request. -->
### Description
PR RCasatta/electrsd#33 has been merged and the new changes released as part of `v0.15.0`: the PR allows setting an env variable (`ELECTRSD_EXE`) to use a custom electrsd binary, which can be useful when the one that is downloaded automatically doesn't work for whatever reason.
### 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:
LLFourn:
ACK 1803f5e
notmandatory:
ACK 1803f5ea8a7a2f8f9ebdfa520baad91f84bd6ba0
Tree-SHA512: 01d84763a5acf3aa1686b91ba7cb0c7ec3a0ca6c7b9bac29aa74a72f54ed31c476b7a8b0f8c6c8a8ccf530ad8a19e90e144ac4cacbe436699ef9e319b5732f1c
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
For some reason while doing the "remove blockchain from wallet PR" I
changed the tests around in what I thought was a benign way. But it
meant (apparently) that both parties "test_sync_double_receive" were
using the same "blockchain". This meant that when the blockchain was RPC
they both imported their addresses to it and got each other's results
when syncing. This bugged out the sync and this commit fixes that.
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.
We used to consider a tx input as ours if we had the
tx that creates it in the database.
This commit actually checks if an input is ours before adding
its value to the `sent` field.
Now works with latest released versions of rust-bitcoincore-rpc and
bitcoind. Once these crates are updated to support creating descriptor
wallets and add importdescriptors and bech32m support this test will
need to be updated.
