LedgerHQ / satstack

Bitcoin full node with Ledger Live
BSD 2-Clause "Simplified" License
157 stars 32 forks source link

Output descriptors to specify custom derivations and change #24

Closed gabridome closed 4 years ago

gabridome commented 4 years ago

this is the actual way to specify the derivation of the key
{ "xpub": "xpub...", "index": 0, "derivationMode": "native_segwit", "birthday": "2017/08/29" } Which is straight, simple and compatible with data a user can get from Ledger Live. To make the application more future proof, It would be convenient to be able to specify somehow the Output Descriptor (https://github.com/bitcoin/bitcoin/blob/master/doc/descriptors.md) of the watch-only wallet we are going to import into bitcoin core. It could cover:

  1. Different derivations than bip44, bip49 or bip84
  2. Internal addresses
  3. In future also multisig configurations

Also importmulti RPC call in bitcoin core supports Output Descriptors and the future version of the wallet will be more and more "descriptor based". I would think something like:

  {
      "descriptor": "wpkh([18734cbe/84'/1'/0']tpubDAZhhJ9SaAKCt2gngU8aA2babxVAM93oBgo2wGknMUDmtLrzvLnynFuAYo4eME3MqWpcTM3DpGL9J45nQw7CA7VroAY8e7v9Zo6r7mddUMS/0/*)#4c563hxu",
      "index": 0,
      "birthday": "2017/08/29"
    }
onyb commented 4 years ago

Thanks for your feedback @gabridome. It's a great suggestion indeed.

We actually already import output descriptors (see this). The descriptors, however, are constructed from the xpub, index, and derivationMode. To be fair, this is really brittle and much more error-prone than what you suggest.

I think specifying output descriptors in the config is not so user friendly, and easy to get wrong if you don't understand the schema. So for me, it needs to be complemented with displaying the receive and change descriptors directly on Ledger Live, in the account details section, so users can just copy-paste it in the config.

gabridome commented 4 years ago

So for me, it needs to be complemented with displaying the receive and change descriptors directly on Ledger Live, in the account details section, so users can just copy-paste it in the config.

I agree and will try to suggest it.

Il giorno lun 15 giu 2020 alle ore 12:02 Anirudha Bose < notifications@github.com> ha scritto:

Thanks for your feedback @gabridome https://github.com/gabridome. It's a great suggestion indeed.

We actually already import output descriptors (see this https://github.com/onyb/ledger-sats-stack/blob/634dc011dda2f7c0bc12cbfe478360315457b12e/bus/wallet.go#L91-L98). The descriptors, however, are constructed from the xpub, index, and derivationMode. To be fair, this is really brittle and much more error-prone than what you suggest.

I think specifying output descriptors in the config is not so user friendly, and easy to get wrong if you don't understand the schema. So for me, it needs to be complemented with displaying the receive and change descriptors directly on Ledger Live, in the account details section, so users can just copy-paste it in the config.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/onyb/ledger-sats-stack/issues/24#issuecomment-644032357, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADQQ3RAUDG4VMQFEYFS6B3RWXWSTANCNFSM4N53Q3UA .

-- Please avoid unnecessary attachments. Write in the body of the message. Thank you.

Gabriele Domenichini pgp fingerprint: DC47 A9B4 CF41 6373 6EBB E14E A48F BF05 1C43 F817

onyb commented 4 years ago

@gabridome In your example config, you used only the external chain, i.e. 0, in the output descriptor.

wpkh([18734cbe/84'/1'/0']tpubDAZhhJ9SaAKCt2gngU8aA2babxVAM93oBgo2wGknMUDmtLrzvLnynFuAYo4eME3MqWpcTM3DpGL9J45nQw7CA7VroAY8e7v9Zo6r7mddUMS/0/*)#4c563hxu

This also means that we'll need to add another output descriptor in the JSON config for the change path, i.e. 1, which seems repetitive to me. Do you think it is a problem to have two config entries per account, or is it better to be explicit even at the cost of some repetition?

onyb commented 4 years ago

This has been fixed in https://github.com/onyb/ledger-sats-stack/commit/f1677b44df05152eaacae099b8c3842ce9aa193f, and available in v0.4.0-alpha. The descriptor will be available on Ledger Live in a future release.

The configuration is now purely based on output descriptors. Here's an example of the new config format:

{
  "accounts": [
    {
      "descriptor": "wpkh([18734cbe/84'/1'/0']tpubDAZhhJ9...7mddUMS)#4c563hxu",
      "birthday": "2020/01/01"
    }
  ],
  "rpcurl": "localhost:8332",
  "rpcuser": "user",
  "rpcpass": "pass",
  "notls": true
}

Notice that the descriptor now describes the account rather than the address path, hence there needs to be only one entry per account, to answer my own question in the previous comment.

gabridome commented 4 years ago

Notice that the descriptor now describes the account rather than the address path, hence there needs to be only one entry per account, to answer my own question in the previous comment.

Yeah. Sorry for the late reply.

I would have been very explicit with something like:

{
  "accounts": [
    {
      "external": "wpkh([18734cbe/84'/1'/0']tpubDAZhhJ9...7mddUMS/0/*)#xxxxxxxx", # obviously the checksum don't match
      "internal": "wpkh([18734cbe/84'/1'/0']tpubDAZhhJ9...7mddUMS/1/*)#xxxxxxxx",
      "birthday": "2020/01/01"
    }
  ],
  "rpcurl": "localhost:8332",
  "rpcuser": "user",
  "rpcpass": "pass",
  "notls": true
}

But I think that even if the solution you choose is not perfect (it doesn't define the whole path) at least is not "wrong".

We can see in the future what decisions will be taken at the "Ledger Live Level" to rethink about the issue.

onyb commented 4 years ago

Reopening this issue, because your suggestion is quite cool! 👍

It's always better to have the exact descriptors that'll be imported in the config file. The account-level descriptor is more convenient (less no. of characters), but is less transparent.

onyb commented 4 years ago

Fixed in v0.5.0-alpha. Thanks for your valuable feedback!