We used to store it there, if it's not within our own datadir copy it
from where it would have been stored by Liana v1.
Note we don't conditionally compile this on Windows so the codepath can
be tested with a functional test.
5c87937d4676354f19357e17b36655eb4853dbf9 Add more bitcoind-related setup logging (Antoine Poinsot)
Pull request description:
At startup it sometimes appear we may be hanging when setting up bitcoind. Add more logging to give more information about what's taking long to setup (for instance, loading the watchonly wallet).
Related to https://github.com/wizardsardine/liana/issues/475.
ACKs for top commit:
darosior:
ACK 5c87937d4676354f19357e17b36655eb4853dbf9
Tree-SHA512: 75c553cabf545d57a5fc2d251e5b4cd880a931a408c6f1b1052067f2ccc8e0728ad779f30de5a88c2566f9e67ac085b713393ee4fa193331e49c3a8e6112ecc8
0d5041ca4a972a946244643db738e6d35b0d114e bitcoin: looper: avoid large sleeps when bitcoind is syncing (Antoine Poinsot)
Pull request description:
Sleeping for 30 whole seconds impedes the shutdown check. Sleep only .5s but still only do poll bitcoind one every 30s when it's syncing.
It was reported to make the GUI hang when closing it while syncing.
ACKs for top commit:
edouardparis:
utACK 0d5041ca4a972a946244643db738e6d35b0d114e
Tree-SHA512: 48c9121a02abaf8311d7b646ea64cbde4e14fda737c6f78521739bc185e6081642d05b3ca8088d3ce6782ffa00d6e5879e6a6e09da7f05c85f8476ac83b6860b
This makes it possible to trigger the shutdown of the daemon through the
DaemonHandle, without having to block while waiting for the poller
thread to join.
Incidently, this allows to avoid having to move `self` which in turns
allows to fix a GUI bug (see
https://github.com/wizardsardine/liana/issues/622).
Only available as an optional feature since `is_finished` needs rustc
1.61.
At startup it sometimes appear we may be hanging when setting up
bitcoind. Add more loading to give more information about what's taking
long to setup (for instance, loading the watchonly wallet).
d4da2bf6b451de0cd44630c077b83426ed581940 daemon bin: try to detect '--help' or '-h' (Antoine Poinsot)
01ce0f179d8ef059f04e56e8f7c125264e9949b1 daemon bin: more helpful error message on config file parsing error (Antoine Poinsot)
222c8bba810cb20eeb60270e89857ab09324884d daemon bin: a more helpful error message on unknown arguments. (Antoine Poinsot)
Pull request description:
Specifically, link to a sample configuration file. How are users supposed to know how to write the config file otherwise?
Fixes#559.
ACKs for top commit:
darosior:
self-ACK d4da2bf6b451de0cd44630c077b83426ed581940 -- trivial.
Tree-SHA512: 7ca05aa0e351c390b67051ad36c2e767b019cf6339d8207e1fdbd1ead47a3d79ab6cd9584c412d98ae876aa7e8ea906885c52261565917a7cd1126798806b5d2
23a63f35e1344a0e6c247e564311cac3e965693c bitcoind: override watchonly wallet as to be loaded on startup (Antoine Poinsot)
Pull request description:
we used to not set 'load_on_startup' when creating a watchonly wallet. This could create some complications when people are using agressive pruning configurations.
Fix this by overwriting this parameter when loading watchonly wallets. We can always revert this in the future if for some reason a user wants to have it set to false and no be overwritten everytime Liana loads it.
Fixes#594.
ACKs for top commit:
darosior:
self-ACK 23a63f35e1344a0e6c247e564311cac3e965693c -- tested locally
Tree-SHA512: 8310918286a8a0102bfc76e9d5b2ba4a8b50cac3e4ce2fd26bdfae7c153eeba3bf516d70ed6ef6256c5f4bcc3955559c011a7035cbc293cc594b829ea9038a5c
we used to not set 'load_on_startup' when creating a watchonly wallet.
This could create some complications when people are using agressive
pruning configurations.
Fix this by overwriting this parameter when loading watchonly wallets.
We can always revert this in the future if for some reason a user wants
to have it set to false and no be overwritten everytime Liana loads it.
Ledger and wallet policies disallow having more than 2 depth after the
placeholder, therefore we can't do `@1/0/<0;1>/*`, `@1/1/<0;1>/*`, ..
Instead we have to do `@1/<0;1>/*`, `@1/<2;3>/*`, ..
Why not? Salvatore also says the cost of deriving another depth is
non-trivial on a signing device.
Don't pick a fight with Salvatore, instead just let the GUI (or whatever
creates the desc) use different multipath steps for keys derived from
the same xpubs.
It's supposed to represent the number of signature per "master" key, i
guess. At the moment it would always be 1 because the origin changes
when we queried more keys from the signing device (because we increased
the account).
This adds an extraneous DEFAULT compared to the schema in freshly
created databases, but anything else (altering the column in an
SQLite-friendly way after setting all NULL values to 0) would be way too
involved.
097d5e71c19c0c5fc53d1e91087d91edfdd218f3 qa: adapt the migration functional test to also support 1.0 (Antoine Poinsot)
289f6581cccefad42afa870b382fc22eb0c5e46c qa: functional test createspend refuses immature outpoints (Antoine Poinsot)
95dd3e52935cf7a2edb1abdee428efe26ccff36c qa: fix and improve the coinbase deposit functional test (Antoine Poinsot)
6b82894614df3351d788451211d864ecff3f3544 bitcoin: track maturity of coinbase deposits (Antoine Poinsot)
6ab6161078af1932dd43bab813963c78887d7672 commands: expose whether a coin is immature in listcoins (Antoine Poinsot)
fd717123be45b73758f1e965659de8f44031fc90 commands: don't create spends with immature coins (Antoine Poinsot)
26add29b197eb6fdf8f5f86ece4c52576700fc6f database: record whether a coin comes from an immature coinbase (Antoine Poinsot)
Pull request description:
#567 uncovered that we were actually not tracking coinbase deposits correctly. In fact we would most likely miss them all in any real situation. This is because we would filter out immature coinbase deposits from the result of `listsinceblock` and not consider them newly received coins. But they would be confirmed, and as the chain moves forward we'd not scan this range anymore even once they've become mature.
This PR fixes it by the simplest possible manner: record immature coinbase deposits as unconfirmed and only mark them as confirmed once they've become mature. This is a bit clumsy, but should be fine for the number of users that would receive payouts from coinbase transactions (ie most likely 0). Also, we don't accurately update coinbase coins on reorg. This is unnecessary as it'd be very unlikely that a mature coinbase would become immature and if there is a 100 blocks reorg that would invalidate it altogether we'd have bigger problems.
Fixes#567.
~~Still as draft as i want to go over this one more time before asking for review, as this if pretty intricate and touches core parts of our codebase.~~
ACKs for top commit:
darosior:
self-ACK 097d5e71c19c0c5fc53d1e91087d91edfdd218f3. Didn't have the chance to re-review it but getting it in is best at this time. Edouard will still have a look post-merge.
Tree-SHA512: 4a5f0fb7561af1d4c51dcba26bc20ef5e7a8b2c730547f762782f75c1f28c26e2a577573aa514db8927c1eeb601685071e9eb85c11537621de58c75824435b05
28049d4f7cd2001207cc2b204c31b33d9640077d descriptor: make it possible to use the same xpub at different paths (Antoine Poinsot)
Pull request description:
This is unnecessary to bump a hardened step to get another xpub for the same participant:
- It's more hurdle when sharing it
- It's more hurdle when verifying it on the device's screen
- It's for the same person within the same script anyways
Fix this by allowing to use the same xpub multiple times in a descriptor as long as the derivation path is different. Then the GUI will be able to use a new unhardened derivation step instead of bumping the BIP48 "accounts". A unit test is introduced that showcases this.
See also https://github.com/wizardsardine/liana/issues/542.
ACKs for top commit:
edouardparis:
ACK 28049d4f7cd2001207cc2b204c31b33d9640077d
Tree-SHA512: e864a8193998667d36b34d96e6e32bfff1c507b3c31faa324b7f264604c41b5d2d4aefeec3b71a1d3edf5b0242966861887baf97d7a410e43e7449f22c3453f4
We detect and store immature deposits (cause otherwise we won't go
through them again with listsinceblock), but mark them as such and
unconfirmed.
We only mark them as confirmed once they've matured. It's a bit clumsy
but it's not as if most of our users had coinbase deposits.
We need to keep track of such coins to:
- Track their maturation
- Avoid using them (for instance in coin selection)
Reorg handling for coinbase deposits that become immature is not
implemented (yet). That's reasonable because:
1. It would be very unlikely that we'd move back, so it's most likely
gonna be mature again immediately.
2. If there's a reorg of more than 100 blocks we've got bigger problems.
It's a much better UX to be able to share and verify one xpub and just
use different unhardened paths for the various public keys used in the
same script that originate from the same master key.
Instead of the hand-written HTTP implementation in the rust-jsonrpc
library, use the minreq crate. It's a small, maintained, low dependency
library actually focused on writing an HTTP client.
This also reworks and better document the request retry logic.
The latest rust-miniscript version deprecated the helper we were using,
in favour of one that gives the maximum size difference of a transaction
input before and after satisfaction. The new helper differs in that it
does not account for the empty ScriptSig byte (which uncovered we were
actually double-counting it), and assumes the non-satisfied transaction
input is already part of a Segwit transaction (which we rectified).
This uncovered a mistake in the computation of the witness script size
in the unit test. We also get rid of the needless wu_to_vb() standalone
function.
Rust-bitcoin, that we use through rust-miniscript, has seen plenty of
breaking changes in the latest version. I've tried to keep the necessary
changes here minimal, still it had to be a single commit to keep it
hygienic. But i'll try to summarize the main things here. Tobin also
wrote a guide about the release at
https://rust-bitcoin.org/blog/release-0.30.0/.
The most verbose change in this commit is probably due to the `Address`
type overhaul. It's overengineered if you ask me but hey here we are. I
tried to keep network validation in commands, and otherwise passing
around unchecked addresses (to avoid having to pass around a global
state between our various components).
Another non-obvious change was changes in hash types upstream and the
removal of `ToHex`, forcing us to get the hex representation of a txid
through its `Display` implementation. It is however displayed backward
in this case ("little-endian" if you will), and we need a regular hex
encoding for some queries to the database. We needed to make sure we
didn't implement any silent bug here.
The rest (Script type changes, PSBT serialization updates, ..) is
probably self-explanatory.
039b82d9b3e11786f09c761a6d0a6d137cdff8ad sqlite: prevent races when updating the next derivation index (Antoine Poinsot)
Pull request description:
You can check the unit test triggering the race fails on master. I'm pretty sure it's the reason for the address generation bug i experienced yesterday when testing.
ACKs for top commit:
edouardparis:
utACK 039b82d9b3e11786f09c761a6d0a6d137cdff8ad
Tree-SHA512: e245d8732346731c5a98223363c9d2e77f3a568e046055007162167f7a7209cc0e9d0f57c7219e469788ff73c570962396c4da0e056c790ffffbd953c75ab3ee
