This reverts commit 71056982636b408485ab24dab6628a555a6e7924, reversing
changes made to 03c37bd378f4f6bf11d90b224ed1db74b3596eaf.
This reverts PR #722. It turns out the Ledger Bitcoin app needs the
BIP32 derivation for all the keys in the Script, not only for the
spending path used. Therefore always create PSBT with all the BIP32
derivations. We'll add a way to prune them for talking to the Bitbox in
a future commit.
This reverts commit ec0c2426aa5fa6cee2efabd3ee6f175b41c35f64, reversing
changes made to 26d750d09c84734f56c2dc18cb332a232e24fb6d.
This reverts the fixes to the pruning of BIP32 derivation paths when
creating a PSBT, in preparation of reverting the merge of this feature
altogether. This is because always creating PSBT with only the BIP32
derivations for a single path broke the Ledger support.
When creating a PSBT, we were checking whether the key was for this path
by checking its origin. This check would return false positive for keys
from other paths but same signer (which shares the same fingerprint).
Instead, check the entire origin for each key to make sure it's actually
the one used in the path we are interested about.
Thanks to Edouard Paris for finding this bug.
7f3b0b021858cfb2fe914f3ba6b30a39e3ae05ff qa: test a PSBT has only the BIP32 derivations for a single spending path (Antoine Poinsot)
b71bd693d6ea4c1c3567194fc82be43fb70c05bb qa: don't use a static dummy origin for descriptor xpubs (Antoine Poinsot)
a81d39c81a89cdf5e70b9888cbeb3abaf290a365 commands: do not include BIP32 derivations for other spending paths (Antoine Poinsot)
Pull request description:
ACKs for top commit:
edouardparis:
ACK 7f3b0b021858cfb2fe914f3ba6b30a39e3ae05ff
Tree-SHA512: f0e132edf8d653c5575f843b1e85d995f155a2435a6e6257564dc945562df70ec2049c29d77f7580858d9e4a58290b0798f298f77c18255415c4cf26ccc07f33
730409eb5299724cdbe1a4da06d272098a791621 descriptors: prevent using a signer more than once in a single path (Antoine Poinsot)
0a95266cce618f83782d3d30362cb2d383b659bf qa: don't use a static dummy origin for descriptor xpubs (Antoine Poinsot)
Pull request description:
This is necessary to support signers which sign for a single key at once. It also doesn't make any sense to reuse a signer within the same spending path, so rule it out before it creates any new edge cases.
For more about the Bitbox signer support, which motivated this change, see
https://github.com/wizardsardine/liana/pull/706#issuecomment-1744705808.
ACKs for top commit:
edouardparis:
ACK 730409eb5299724cdbe1a4da06d272098a791621
Tree-SHA512: 3fb34f2a85e103ca5f8eeec901c283c649636971f7730743af24b270626a7fdf11f207a8f3535c9ff69ce475f34c69472595219ac97b8cfd5749379c4213a6e5
When not in IBD but catching up to the latest tip, our rounding up of
verificationprogress makes us start while bitcoind is still kind of far
from being caught up. Make sure it doesn't happen by not returning until
bitcoind validated all the blocks we've fetched the headers for.
This was necessary to transition wallets that were created without this
flag, but since v2 they are always created with it so this is redundant:
it'd unnecessarily try to update the flag over and over again on
bitcoind's side, always with the same value.
This is necessary to support signers which sign for a single key at
once. It also doesn't make any sense to reuse a signer within the same
spending path, so rule it out before it creates any new edge cases.
For more about the Bitbox signer support, which motivated this change,
see
https://github.com/wizardsardine/liana/pull/706#issuecomment-1744705808.
When creating a PSBT, only include the BIP32 derivations in each input
for the spending path this PSBT was created for.
This is to workaround Bitbox only providing a single signature per
input. Most likely other signing devices will have this behaviour too in
the future. See
https://github.com/wizardsardine/liana/pull/706#issuecomment-1744705808.
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
