Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hsmtool: enable dumping output descriptors of onchain wallet #4171

Merged
merged 8 commits into from
Nov 10, 2020

Conversation

darosior
Copy link
Collaborator

@darosior darosior commented Oct 30, 2020

This adds a new dumponchaindescriptors command to hsmtool which can:

TODO:

  • A functional test restoring a descriptor on bitcoind asserting we retrieve all the funds
  • Manpage and an extended doc on how to restore the onchain wallet using bitcoind
  • Maybe add a network parameter finally ?.. EDIT: maybe output both main and test extended keys ?.. EDIT 2: added a testnet parameter

Fixes #4110
Ping @jb55 : finally got to jb55/clightning-dumpkeys#2 :-)

@darosior darosior requested a review from cdecker as a code owner October 30, 2020 15:13
@darosior darosior force-pushed the hsmtool_descriptors branch 5 times, most recently from fd74279 to fc008f4 Compare November 1, 2020 23:17
@darosior
Copy link
Collaborator Author

darosior commented Nov 1, 2020

Ok, added a test, documented (and rebased on #4146 so that i can build locally)

@darosior
Copy link
Collaborator Author

darosior commented Nov 2, 2020

Travis failure is an unrelated timeout

@niftynei niftynei added this to the v0.9.2 milestone Nov 2, 2020
@darosior darosior force-pushed the hsmtool_descriptors branch from fc008f4 to f9c2d4f Compare November 4, 2020 20:48
@darosior
Copy link
Collaborator Author

darosior commented Nov 4, 2020

Rebased on master after #4146 merge, added a missing changelog.

@niftynei niftynei added the wallet label Nov 5, 2020
Copy link
Collaborator

@niftynei niftynei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool addition. I'm not sure how anchor outputs affects the assumption that all outputs are some variant of p2wpkh, may not be relevant for this patch.

common/descriptor_checksum.c Outdated Show resolved Hide resolved
common/descriptor_checksum.c Outdated Show resolved Hide resolved
common/descriptor_checksum.c Outdated Show resolved Hide resolved
common/descriptor_checksum.c Outdated Show resolved Hide resolved
common/descriptor_checksum.c Outdated Show resolved Hide resolved

/* The root seed is derived from hsm_secret using hkdf.. */
do {
hkdf_sha256(bip32_seed, sizeof(bip32_seed),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicating this code from hsmd/hsmd.c#populate_secretstuff is a bad idea. Needs to be a shared routine that both hsmd and here use.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's how we've created tool since now ?..

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't follow. We really want this code to be shared so that if one changes, the other also gets updated.

tools/hsmtool.c Show resolved Hide resolved
tools/hsmtool.c Outdated Show resolved Hide resolved
tools/hsmtool.c Outdated Show resolved Hide resolved
tools/hsmtool.c Outdated
{
struct secret hsm_secret;
u8 bip32_seed[BIP32_ENTROPY_LEN_256];
u32 salt = 0;
u32 version = testnet ? BIP32_VER_TEST_PRIVATE : BIP32_VER_MAIN_PRIVATE;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we infer this from the directory structure or some other info source?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, as we can use all commands of the hsmtool on a hsm_secret without depending on its environment at all
and this would be quite convoluted too..

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have all of these parameters in chainparams.c, so why not use them? We can even steal the --network option from lightningd itself :-)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This'd mean bringing the opt() system here: that's a much bigger change :/

@darosior
Copy link
Collaborator Author

darosior commented Nov 5, 2020

I'm not sure how anchor outputs affects the assumption that all outputs are some variant of p2wpkh, may not be relevant for this patch.

You are right that we actually don't sweep the 1 CSV-obstructed to_remotes in onchaind and just adverise them to the wallet, so i don't think i should actually sell this command as a "backup-and-recovery mechanism" in the FAQ..
I was also in doubt wrt dumping xprivs at all then, but it might turns to be useful (if i remove all mentions that it is exhaustive from the doc).

Edit: ah also, i could make it a miniscript descriptor then but i think it's a bad idea because there is virtually no support for them (including bitcoind) currently and this might confuse users. Also this makes me think that it's not wise to advise it as "backup all onchain funds for sure" since it might not be true in a so distant future ?

@niftynei
Copy link
Collaborator

niftynei commented Nov 6, 2020

This functionality is Cool and Interesting, but until miniscript is widely adopted I think it's best to advertise this as a subset of your lightning funds tracking, at best. There's definitely "outputs we own, eventually" that haven't been swept to a 'recognizable wallet output' that will be missed by this -- if for some reason you take your lightning node down before these are swept there's a possibility that they'll be lost.

With these considerations in mind, I'd also be tempted to remove the xpriv. Rationale: it gives the false impression of 'total wallet funds control' when that's really not the case.

i could make it a miniscript descriptor then but i think it's a bad idea because there is virtually no support for them

I agree, this is a good intuition. I suspect that when miniscript descriptors are more formally nailed down the lightning spec itself will also move that direction, which will then make this export functionality extremely powerful.

Copy link
Collaborator

@niftynei niftynei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

few more things I noticed while testing.

tools/hsmtool.c Outdated
return dumponchaindescriptors(argv[2], argc > 3 ? argv[3] : NULL);
return dumponchaindescriptors(argv[2],
argc > 3 && !streq(argv[3], "") ? argv[3] : NULL,
argc > 4 && streq(argv[4], "private"));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's weird to me that you have to physically type in the phrase "private" and not look for a true/false here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find it better: at usage typing testnet is way more meaningful than true

Copy link
Collaborator

@niftynei niftynei Nov 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update the parameter name then. As written, the parameter testnet suggests to say 'flag true if testnet'.

tools/hsmtool.c Outdated Show resolved Hide resolved
@darosior darosior force-pushed the hsmtool_descriptors branch from f9c2d4f to 11c1a11 Compare November 6, 2020 11:29
@darosior
Copy link
Collaborator Author

darosior commented Nov 6, 2020

  • Used a proper API for the imported descriptor checksum file (use a struct instead and return a bool) as requested
  • Addressed misc nits
  • Removed the "output xprivs" functionality altogether, as @niftynei agrees it's a footgun. (We now use a watchonly wallet in the functional test).
  • Rebased on master

tools/hsmtool.c Outdated
{
struct secret hsm_secret;
u8 bip32_seed[BIP32_ENTROPY_LEN_256];
u32 salt = 0;
u32 version = testnet ? BIP32_VER_TEST_PRIVATE : BIP32_VER_MAIN_PRIVATE;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have all of these parameters in chainparams.c, so why not use them? We can even steal the --network option from lightningd itself :-)

Comment on lines +140 to +141
if not bitcoind.rpc.listwallets():
bitcoind.rpc.createwallet("lightningd-tests")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is slightly nicer than 90f4ea6

@@ -1045,6 +1045,36 @@ def test_hsmtool_secret_decryption(node_factory):
assert node_id == l1.rpc.getinfo()["id"]


@unittest.skipIf(TEST_NETWORK == 'liquid-regtest', '')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a comment saying that this is only supported for mainnet and testnet (technically we'd require it to be regtest but that appears to match testnet).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah descriptors for regtest are the same than for testnet (but addresses the same as mainnet!! 😭 )

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hurray for consistency 🤦

@niftynei
Copy link
Collaborator

niftynei commented Nov 6, 2020

I opened an issue for the password inputs for all of the hsmtool commands, #4186. I think it's ok to merge this as is. We should address this issue though, as it's a potential security hole.

@rustyrussell
Copy link
Contributor

You should use ccan/opt, now that cmdline parsing is non-trivial. But can be added after....

@niftynei niftynei force-pushed the hsmtool_descriptors branch from 11c1a11 to f49d84a Compare November 9, 2020 23:33
darosior and others added 8 commits November 9, 2020 17:51
We want to use it outside of fuzz-amount

Signed-off-by: Antoine Poinsot <[email protected]>
A small one just to check that we don't crash nor go out of bounds!

Signed-off-by: Antoine Poinsot <[email protected]>
This adds a command which outputs the two output descriptors
corresponding to our onchain wallet.

This can be useful for an external service to monitor / send fund to our
wallet.

Further, an "xpriv" version of such descriptors could be used to import
onchain funds on a new wallet.

Changelog-Added: lightning-hsmtool: a new command was added to hsmtool for dumping descriptors of the onchain wallet
Signed-off-by: Antoine Poinsot <[email protected]>
Actually, it's more complex to translate the xpub descriptor to
testnet because of the descriptor checksum.

Signed-off-by: Antoine Poinsot <[email protected]>
Touch a bit about it as a backup/recovery mechanism in the FAQ

Signed-off-by: Antoine Poinsot <[email protected]>
@niftynei niftynei force-pushed the hsmtool_descriptors branch from f49d84a to 29e5764 Compare November 9, 2020 23:56
@niftynei
Copy link
Collaborator

niftynei commented Nov 9, 2020

Updated the interface to use "network" instead of 'testnet', rebased on master now that #4065 is merged.

@darosior
Copy link
Collaborator Author

ACK 29e5764

Thanks..

@niftynei niftynei merged commit 6b347e1 into ElementsProject:master Nov 10, 2020
@darosior darosior deleted the hsmtool_descriptors branch December 17, 2020 12:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: Is it possible to retrieve the xPub of the on-chain wallet?
4 participants