54d768412a Sqlite migrations should either succeed or fail (Vladimir Fomene)
369e17b801 Add datatype for is_spent sqlite column (Vladimir Fomene)
Pull request description:
### Description
During table creation, Sqlite does not throw an error when a column datatype is not defined. In addition, the datatype provided during table creation does not put a constraint on the type of data that can be put in that column. So you can easily put a string value in an integer column. Despite this, I think it is important for us to add the datatype for clarity.
### Notes to the reviewers
You can read more about how Sqlite dynamic typing [here](https://www.sqlite.org/faq.html). I have amended our `migrate` code with a new commit. The idea is to run migrations in a transaction so that they either succeed or fail. This prevents us from having the database in an inconsistent state at any point in time.
### 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
* [ ] I'm linking the issue being fixed by this PR
ACKs for top commit:
rajarshimaitra:
tACK 54d768412a
afilini:
ACK 54d768412a
Tree-SHA512: bb6c0467f799ca917f8d45c6495b766352b3177fc81952fcdd678208abf092fdeae966686528a5dcb3f342d7171783274df6312a08cbef3580063e059f5f7254
The current implementation of the `migrate` method for
Sqlite database does not rollback changes when there is
an error while running one of the migration scripts. This
can leave the database in an inconsistent state. This
change ensures that migrations either succeed completely
or fail.
`del_tx` pulls the TransactionDetails object using
`select_transaction_details_by_txid` method which gets the transaction
details' data with a non-None transaction field even if the
`include_raw` argument is `false`. So it becomes necessary to Set
the transaction field in transactiondetails to None in `del_tx`, when
we make a call to it with `include_raw=false`.
138acc3b7d Change `populate_test_db` to not return empty input (wszdexdrf)
d6e1dd1040 Change CI to add test using ledger emulator (wszdexdrf)
76034772cb Add a custom signer for hardware wallets (wszdexdrf)
Pull request description:
Also adds a new test in CI for building and testing on a virtual
hardware wallet.
### Description
This PR would enable BDK users to sign transactions using a hardware wallet. It is just the beginning hence there are no complex features, but I hope not for long.
I have added a test in CI for building a ledger emulator and running the new test on it. The test is similar to the one on bitcoindevkit/rust-hwi.
### Notes to the reviewers
The PR is incomplete (and wouldn't work, as the rust-hwi in `cargo.toml` is pointing to a local crate, temporarily) as a small change is required in rust-hwi (https://github.com/bitcoindevkit/rust-hwi/pull/42).
### 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 138acc3b7d
Tree-SHA512: 54337f06247829242b4dc60f733346173d957de8e9f8b80beb91304d679cfb4e0e4db722c967469265a5b6ede2bd641ba5c089760391c671995dc30de37897de
Previously we weren't setting the db sync height in populate_test_db
macro even when current height is provided.. This creates a bug that
get_funded_wallet will return 0 balance.
This PR fixes the populate_test_db macro and updates tests which were
previously dependent on the unsynced wallet behavior.
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.
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
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.
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
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.
Verify the unconfirmed transactions we download against the consensus
rules. This is currently exposed as an extra `verify` feature, since it
depends on a pre-release version of `bitcoinconsensus`.
Closes#352
Daniela Brozzoni
Max Fang
Jeremy Mawson
Steve Myers
Alekos Filini
Vladimir Fomene
wszdexdrf
rajarshimaitra
KaFai Choi
Philipp Hoenisch
LLFourn
thunderbiscuit
Sudarsan Balaji
John Cantrell
Lucas Soriano del Pino
Riccardo Casatta
Tobin Harding