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

getaddressinfo missing key origin information for multisig #15743

Closed
gwillen opened this issue Apr 4, 2019 · 6 comments · Fixed by #15749
Closed

getaddressinfo missing key origin information for multisig #15743

gwillen opened this issue Apr 4, 2019 · 6 comments · Fixed by #15749

Comments

@gwillen
Copy link
Contributor

gwillen commented Apr 4, 2019

When I call

deriveaddresses "sh(wsh(multi(2,[aeee1e6a/49h/1h/0h]tpubDC5bVdaveQYxGFztMShFAjU637kKjrs9FD8ne1oHR7z8tLeQJExsaxWRVeSpcCcYMdp5ecf2LQhuVaeYFZ7YEpvWTJr4nDGUJrmuvLuTSsp/1/*,[f04973fd/49h/1h/0h]tpubDDSx3fQ8QyVN21PqRwobfAeDnjesTRMG3U6MPs2aUW5tYg9Arhz4tBRiUWTc9VrJBxRVXJqSrULfGs5HJZTqMJC7PBtDGT7cVtsySf76wmS/1/*,[149b5aca/49h/1h/0h]tpubDCCrKqf5e4eiXxRsma3QkcoKbd6MS8zxK5NTfsvNerTPVZcZqFMbn12D3qtWNLUBdxFkRhw2ZToRCMCNQzxG1T8dCVNMRuLwCmUngyf3CbB/1/*)))" 0 1

I get back the following descriptors from getaddressinfo for the two addresses generated:

sh(wsh(multi(2,[aeee1e6a/49'/1'/0'/1/0]021387edfa7024c6bced022d8ab95733e38f8fbe4151e28532feb29f8906a4fb5e,[80a9880b]039116115d10440ec1ba2f634e9b7f3b2a3f54dee5b9699501fc58394ddebd62c6,[4f4c39da]03539160098ca953042302cb577ae6103f44f58bd2d093819d3f3c8e267329f99c)))

sh(wsh(multi(2,[aeee1e6a/49'/1'/0'/1/1]03789212b1eeae74ea7d2729bedbe72f41e295543c0b8641ed8b883556dddef55f,[6e6574dc]028b4ec07462a0dd937b8d608b5951e8d2f7494145180e0265b17cf02b280d2629,[80b02878]0212a0f24ccc397b5bef557a3270b6c2cfb148fd726e4cdb417ef5c7655dd31838)))

It looks like all three "*" path elements are incrementing together, correctly, but the key origin information is lost for the second and third ones.

@gwillen
Copy link
Contributor Author

gwillen commented Apr 4, 2019

It seems that ProcessImportDescriptor doesn't find any pubkeys in this descriptor (map_pubkeys is empty.) I suspect the first one appearing to be have origin info is a red herring -- I think I independently imported it by itself earlier, when testing other stuff.

@gwillen gwillen changed the title getaddressinfo missing key origin information for pubkeys in multisig after the first getaddressinfo missing key origin information for multisig Apr 4, 2019
@gwillen
Copy link
Contributor Author

gwillen commented Apr 4, 2019

ok, the problem is indeed that ExpandHelper does not extract pubkeys from multisig descriptors. It does from single-key descriptors, but seemingly sort of by accident. MakeScripts on single-key descriptors fills in the pubkey into the FlatSigningProvider, as well as making the scripts. MakeScripts on a multisig descriptor does not. So even though ExpandHelper does not add pubkeys to the FlatSigningProvider, MakeScripts does (usually), so they make it in there anyway.

This logic seems kind of tangled, and it's not clear to me exactly what the right thing is to do here.

@gwillen
Copy link
Contributor Author

gwillen commented Apr 4, 2019

@sipa, it looks like you wrote this -- want to shed light?

See e.g. 11e0fd8 -- you note that this is only required for public key descriptor types, but I think you missed that multisig is one such. You also edited the comment for MakeScripts to mention origin info, but this confuses me because MakeScripts doesn't seem to handle that, that gets handled in ExpandHelper.

@sipa
Copy link
Member

sipa commented Apr 4, 2019

@gwillen No, for multisig we intentionally do not import the public keys:

  • It's unnecessary; we only need pubkeys imported if they'd be encountered in hashed form. For multisig scripts, they appear in full as public keys in the (known) script.
  • It's damaging; importing the public keys would imply making payments to those keys themselves (outside of a multisig context) treated as watched, setting the user up to be scammed.

I'm going to investigate why the origin info disappears and why importmultisig gives a weird warning; perhaps those are indirect side effects of not importing the pubkey.

@gwillen
Copy link
Contributor Author

gwillen commented Apr 4, 2019

@sipa Importmultisig gives a weird warning because it warns if (1) you import watchonly and (2) you have private keys for all the pubkeys you import.

Since for multisig we import zero pubkeys, we always (trivially) have the private keys for all of them, so it warns that we are importing something watchonly even though it is "spendable" since we have "all" the private keys.

@gwillen
Copy link
Contributor Author

gwillen commented Apr 4, 2019

My assumption was that key origin info has to be attached to a key, so that not importing a key will prevent the origin info from being used. That's why I was focused on the keys not being imported.

meshcollider added a commit that referenced this issue Apr 9, 2019
b5d3987 Take non-importing keys into account for spendability warning in descriptor import (Pieter Wuille)
6e59700 Import all origin info in importmulti; even for non-importing pubkeys (Pieter Wuille)
9a93c91 Keep full pubkeys in FlatSigningProvider::origins (Pieter Wuille)

Pull request description:

  This fixes #15743 and #15742.

  Since #15263, pubkeys are no longer imported for non-PKH (or WPKH, or any wrapped form of those) outputs, as that would incorrectly mark outputs to single-key versions of multisig policies as watched.

  As a side effect, this change also caused origin info not to be imported anymore for multisig policies.

  Fix this by plumbing through the full pubkey information for origins in FlatSigningProvider, and then importing all origin info we have in `importmulti` (knowing more never hurts, and additional origin information has no negative consequences like importing the pubkeys themselves).

ACKs for commit b5d398:
  MeshCollider:
    utACK b5d3987

Tree-SHA512: 37caa2be8d01b8baa12f70a58eaa7c583f5f0afbe012e02936dd8790dc5dc852f880b77258b34ddb68cae30c029585f2d1c4f5d00015380557a1e8b471e500f3
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants
@gwillen @sipa @fanquake and others