wallet: add seeds argument to importdescriptors

[apoelstra]

This PR introduces the ability to import BIP32 master seeds alongside xpubs. It currently expects seeds to be provided in BIP 93/codex32 either as a single seed or as a list of shares which can be assembled via Shamir Secret Sharing.

It could be generalized to other seed formats, e.g. a hex-encoding or the old sethdseeds WIF format, easily enough by just attempting to parse the input in those formats and retrying until we find one that works. (Though we must be extremely careful that these formats are unambiguous! If the user imports a seed they don't expect, which fails to match the one they've backed up, the result could be coin loss. For formats with no prefix or checksum, e.g. hex-encoded bytes, we may need to require the user provide a prefix or a separate flag.)

The motivation here is that users who are importing their coins from another wallet likely have them in the form of a public descriptor (probably one of the standard pkh(BIP44/48/84/whatever) ones) along with a master seed (probably in some seed word format, but this is easy enough to convert to a straight BIP32 seed with an external tool). The seed, presumably, they want to minimize handling of, while the descriptor they are likely to be copy/pasting from somewhere and generally being less careful with.

To import this into Core, they currently need to construct a version of their descriptor in which xpubs are replaced by xprivs, and import that. This is cumbersome and potentially dangerous. With this extra RPC flag they are able to separately tweak their descriptor however they need, and then adding their seed data, unmodified, at the last minute in one place.

My specific use case is: I have an estate planning document which contains careful instructions for reconstructing seed data from shares which are stored separately and externally to the planning document. Meanwhile, the document itself contains a public descriptor, with checksum already computed, which is stored in several locations accessible by an open-ended set of people. If I were forced to mix the secret data with the public data, it would increase the risk of leaking the secrets as well as the risk of losing the public part.

Edit: Other approaches, that I considered but chose not to take:

• Extend the existing sethdseed RPC. This RPC is legacy-only and not supported by descriptor wallets, because it depends on the legacy "bag of keys" model where you could stick keys into the wallet and it'd "just work", often in surprising ways.
• Add a new descriptor-wallet-based importseed RPC or something. This doesn't work for more-or-less the same reason; the descriptor wallet associates all its keys with ScriptPubKeyManagers, so we would need to add support for "global keys" somehow. This is a lot of work, lots of room for bikeshedding, and I worry that @achow101 will NACK anything that looks too much like a "bag of keys" model. (The situation would be much less bad than the legacy one, where 3rd parties could do stuff like mapping p2wpkh addresses to p2pkh ones and the wallet would just accept it ... but it would still potentially lead to user confusion about what exact conditions the wallet could sign for what descriptors, and it may constrain future wallet changes.)
• Adding a codex32(...)/1/2/3 xpriv format to the descriptor import format. This was suggested to me by @sipa and has the benefit that very closely matches the existing import model.

Personally I don't like the latter

• for the reasons stated in the PR description -- I like my descriptors to be public, and my secret data to be limited to 16 bytes, and I think this is a common mode of operation for people coming from other wallets
• ...and maybe worse, a common model for other wallet authors, who would refuse to nicely interoperate with this);
• because it felt like enough of a change to deserve a change to one of the descriptor BIPs, but those are big enough already and it's hard to get tiny changes to propagate to all implementors of standards. Plus it felt wrong to couple codex32 with descriptors like this.i
• because if you have multiple descirptors that use the same seed, it would force you to copy the seed multiple times, which is dangerous. (In some cases BIP 389 will make this better, but it doesn't cover everything and anyway is not accepted or implemented yet. One thing at a time :))

But what cinched it, and made me switch to the approach in the PR, was that @roconnor-blockstream pointed out to me that the codex32 model means that you can't predict, at writing-down-the-descriptor time, exactly what shares you'll be inputting. And because descriptors have a checksum which covers their entire string encoding, this means that you can't predict ahead-of-timei what will go into the codex32(...) slot and therefore what the total checksum ought to be. And this means that either (a) you lose the descriptor checksum, which is a Bad Idea and also inconvenient for importers who' will have to construct a checksum with getdescriptorinfo to get importdescriptors to accept it, even though said checksum had never been used to protect data in storage; or (b) you modify the descriptor checksum algorithm to be aware of codex(...) and to skip checksumming the data inside of that. This means even more implementation complexity that other wallets are unlikely to bring in, plus the result generalizes less easily to other seed formats (e.g. hex) which don't have checksums of their own.

[DrahtBot]

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	josibake, S3RK

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #29607 (refactor: Reduce memory copying operations in bech32 encoding/decoding by paplorinc)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

[apoelstra]

cc @achow101 @sipa @roconnor-blockstream

[josibake]

Concept ACK on adding BIP93 support to core.

Regarding the approach, my first thought was why not extend sethdseed to support BIP93, but I'm guessing this doesn't work out of the box per your findings in #27337 ?

[apoelstra]

Edit: moved the content of this comment into the PR description (was a description of alternate approaches)

[josibake]

This RPC (sethdseed) is legacy-only and not supported by descriptor wallets

Great point, I had forgotten about this

for the reasons stated in the PR description -- I like my descriptors to be public, and my secret data to be limited to 16 bytes, and I think this is a common mode of operation for people coming from other wallets

This is a very compelling argument, and your approach seems like the path of least resistance towards supporting this use case.

the descriptor wallet associates all its keys with ScriptPubKeyManagers, so we would need to add support for "global keys" somehow. This is a lot of work, lots of room for bikeshedding

Longterm, I think it would be great to look into a more comprehensive solution which addresses the above

EDIT: I'd suggest adding your comment to the description; it really helps in understanding why you're taking this approach

[instagibbs]

I don't think that RPC exists? :)

[apoelstra]

Lol. Updated the doccomment.

[S3RK]

Concept ACK. I think we should definitely support the described use case.

For the approach, I think we already have some implementation ready for the second alternative you mentioned. #26728 adds support for a wallet master key (I think this is what you meant by "global keys"). And #23544 by @Sjors, built on top of it, adds a RPC for descriptor wallets similar to sethdseed.

I like the alternative better because it support more use cases:

1. storing wallet master key allows you to generate new descriptors for the existing master key, e.g. to add tr descriptors to existing wallet
2. better support for multisig between core and a hardware wallet (described in rpc, wallet: addhdseed, infer seed when importing descriptor with xpub  #23544)

[apoelstra]

@S3RK I agree that Sjors' PR does what we want (and more), up to the specific import format, but

• It has not been updated in 18 months and now has many conflicts
• It depends on two other unmerged PRs, one of which is marked draft
• It is itself marked draft
• It is 30 commits long and requires significant reviewer attention

Meanwhile this PR is narrow in scope, doesn't even touch the actual wallet code and therefore won't conflict with ongoing mega-refactors, and is currently up to date.

So I don't think we should put this on hold because it may be made redundant by a future addseeds RPC. We may even want both, since the functionality here is slightly different (rather than importing global seeds, it allows importing a specific set of descriptors which share one seed, without affecting others).

[rot13maxi]

is this copyright correct? doesn't match the header file

[rot13maxi]

Would love to have support for codex32 shares/seeds!

Thanks for the detailed comments in codex32.cpp! Really made it easier to follow

It looks like this presupposes that you already have an xpub to use with importdescriptors and lets you reconstruct the seed with codex32-encoded shares. It seems like you'd also need something like sethdseed to load secret shares into a blank descriptor wallet in the first place? I read and understand your rationale in the description, but it seems like there's a missing step between "generate your seed/shares" and "import them along with the xpub"

[apoelstra]

@rot13maxi right -- though I was imagining that there would (later) be an external tool to compute the master xpub from the seed data, if you didn't already know it.

[achow101]

The approach in this PR does not seem to have support.

[Sjors]

Now that we have a createwalletdescriptor RPC call #29130, this could be expanded to take private key material, as suggested by @ryanofsky: #29130 (comment)

codex32 could then be one of the import formats.

[apoelstra]

Thanks! The next time I am working on codex32 wallet support I will try that approach instead.