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

Fix: importmulti only imports origin info for PKH outputs #15749

Merged
merged 3 commits into from Apr 9, 2019

Conversation

@sipa
Copy link
Member

@sipa sipa commented Apr 4, 2019

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).

@sipa
Copy link
Member Author

@sipa sipa commented Apr 4, 2019

@DrahtBot
Copy link
Contributor

@DrahtBot DrahtBot commented Apr 4, 2019

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #15764 (Native descriptor wallets by achow101)
  • #15741 (Batch write imported stuff in importmulti by achow101)

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.

@gwillen
Copy link
Contributor

@gwillen gwillen commented Apr 4, 2019

utACK, thanks for the quick fix!

@achow101
Copy link
Member

@achow101 achow101 commented Apr 4, 2019

utACK 6e59700

@sipa
Copy link
Member Author

@sipa sipa commented Apr 6, 2019

Added an extra commit to address #15742. Can you test, @gwillen?

@sipa sipa force-pushed the 201904_importallorigins branch from a6a1e3e to b5d3987 Apr 6, 2019
@gwillen
Copy link
Contributor

@gwillen gwillen commented Apr 8, 2019

@sipa tACK! Confirmed this fixes both problems. Thanks again!

@sipa sipa added the Bug label Apr 9, 2019
@sipa
Copy link
Member Author

@sipa sipa commented Apr 9, 2019

I'm marking this as a bug, as the current importmulti output is sometimes just wrong (it may tell you your import is spendable while it isn't). I don't think it's serious enough to warrant an 0.18.0rc4, but if there is one, maybe we want it included.

@meshcollider
Copy link
Member

@meshcollider meshcollider commented Apr 9, 2019

utACK b5d3987

@meshcollider meshcollider merged commit b5d3987 into bitcoin:master Apr 9, 2019
2 checks passed
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
@laanwj laanwj added this to the 0.18.0 milestone Apr 11, 2019
meshcollider added a commit to meshcollider/bitcoin that referenced this issue Apr 16, 2019
meshcollider added a commit to meshcollider/bitcoin that referenced this issue Apr 16, 2019
meshcollider added a commit to meshcollider/bitcoin that referenced this issue Apr 16, 2019
MarcoFalke added a commit that referenced this issue Apr 16, 2019
…info for PKH outputs

235550d Take non-importing keys into account for spendability warning in descriptor import (Pieter Wuille)
802dcd3 Import all origin info in importmulti; even for non-importing pubkeys (Pieter Wuille)
7fcbe7d Keep full pubkeys in FlatSigningProvider::origins (Pieter Wuille)

Pull request description:

  Clean backport of #15749 by sipa to 0.18

ACKs for commit 235550:
  fanquake:
    utACK 235550d
  MarcoFalke:
    ACK 235550d (Checked that they are clean cherry-picks)

Tree-SHA512: 1ccc19f51137ac4ef971c0bcca4c87ab2383610aa51c5d02680c485b9ce6abd698dddd7f4a45946d56b1aa741cc3fd94b4180ff15901824d20eeba89b4f12853
@fanquake
Copy link
Member

@fanquake fanquake commented Apr 16, 2019

Backported in #15803.

HashUnlimited pushed a commit to HashUnlimited/chaincoin that referenced this issue Apr 19, 2019
HashUnlimited pushed a commit to HashUnlimited/chaincoin that referenced this issue Apr 19, 2019
HashUnlimited pushed a commit to HashUnlimited/chaincoin that referenced this issue Apr 19, 2019
deadalnix added a commit to Bitcoin-ABC/bitcoin-abc that referenced this issue Jun 5, 2020
Summary:
 * Keep full pubkeys in FlatSigningProvider::origins

 * Import all origin info in importmulti; even for non-importing pubkeys

 * Take non-importing keys into account for spendability warning in descriptor import

This is a backport of Core [[bitcoin/bitcoin#15749 | PR15749]]

Test Plan:
  ninja all check-all

Reviewers: #bitcoin_abc, majcosta

Reviewed By: #bitcoin_abc, majcosta

Subscribers: majcosta

Differential Revision: https://reviews.bitcoinabc.org/D6385
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

7 participants