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

importmulti falsely warns private keys present when importing descriptor #15742

Closed
gwillen opened this issue Apr 4, 2019 · 5 comments

Comments

Projects
None yet
4 participants
@gwillen
Copy link
Contributor

commented Apr 4, 2019

Command:

importmulti '[{"desc": "sh(wsh(multi(2,[aeee1e6a/49h/1h/0h]tpubDC5bVdaveQYxGFztMShFAjU637kKjrs9FD8ne1oHR7z8tLeQJExsaxWRVeSpcCcYMdp5ecf2LQhuVaeYFZ7YEpvWTJr4nDGUJrmuvLuTSsp/1/*,[f04973fd/49h/1h/0h]tpubDDSx3fQ8QyVN21PqRwobfAeDnjesTRMG3U6MPs2aUW5tYg9Arhz4tBRiUWTc9VrJBxRVXJqSrULfGs5HJZTqMJC7PBtDGT7cVtsySf76wmS/1/*,[149b5aca/49h/1h/0h]tpubDCCrKqf5e4eiXxRsma3QkcoKbd6MS8zxK5NTfsvNerTPVZcZqFMbn12D3qtWNLUBdxFkRhw2ZToRCMCNQzxG1T8dCVNMRuLwCmUngyf3CbB/1/*)))", "keypool": true, "timestamp": "now", "watchonly": true, "internal": true, "range": {"end": 100}}]' '{"rescan": false}'

Result:

[
  {
    "success": true,
    "warnings": [
      "All private keys are provided, outputs will be considered spendable. If this is intentional, do not specify the watchonly flag."
    ]
  }
]

And according to getdescriptoinfo:

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

{
  "descriptor": "sh(wsh(multi(2,[aeee1e6a/49'/1'/0']tpubDC5bVdaveQYxGFztMShFAjU637kKjrs9FD8ne1oHR7z8tLeQJExsaxWRVeSpcCcYMdp5ecf2LQhuVaeYFZ7YEpvWTJr4nDGUJrmuvLuTSsp/1/*,[f04973fd/49'/1'/0']tpubDDSx3fQ8QyVN21PqRwobfAeDnjesTRMG3U6MPs2aUW5tYg9Arhz4tBRiUWTc9VrJBxRVXJqSrULfGs5HJZTqMJC7PBtDGT7cVtsySf76wmS/1/*,[149b5aca/49'/1'/0']tpubDCCrKqf5e4eiXxRsma3QkcoKbd6MS8zxK5NTfsvNerTPVZcZqFMbn12D3qtWNLUBdxFkRhw2ZToRCMCNQzxG1T8dCVNMRuLwCmUngyf3CbB/1/*)))#sddrjdjm",
  "isrange": true,
  "issolvable": true,
  "hasprivatekeys": false
}
@gwillen

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2019

It seems that ProcessImportDescriptor finds no pubkeys in this descriptor (pubkey_map is empty), so it concludes that, trivially, all privkeys are provided.

This probably has the same cause as #15743.

@gwillen

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2019

Yeah I'm going to close this as a duplicate of #15743, which is more substantive, because the cause is the same.

@gwillen gwillen closed this Apr 4, 2019

@gwillen gwillen reopened this Apr 6, 2019

@gwillen

This comment has been minimized.

Copy link
Contributor Author

commented Apr 6, 2019

Reopened because the fix for #15743 won't end up fixing this.

THAT fix is going to cause key origins to be successfully record for multisigs in ProcessImportDescriptor, but according to @sipa it is intentional that pubkeys are not imported, and this will not be changed.

Possibly all that needs to be changed is the comment:

    // Check if all the public keys have corresponding private keys in the import for spendability.
    // This does not take into account threshold multisigs which could be spendable without all keys.	    
    // Thus, threshold multisigs without all keys will be considered not spendable here, even if they are,
    // perhaps triggering a false warning message. This is consistent with the current wallet IsMine check.

The comment claims that we will falsely warn that threshold multisigs are not spendable, even when they are, but ACTUALLY we falsely warn that they are spendable, even when they are not.

@sipa

This comment has been minimized.

Copy link
Member

commented Apr 6, 2019

Right, there is an additional issue here; namely that only to-be-imported pubkeys are being analyzed for spendability, rather than all involved keys.

I'll PR a fix soon.

meshcollider added a commit that referenced this issue Apr 9, 2019

Merge #15749: Fix: importmulti only imports origin info for PKH outputs
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
@meshcollider

This comment has been minimized.

Copy link
Member

commented Apr 9, 2019

Fixed by #15749

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.