descriptors: Try pubkeys of both parities when retrieving the private keys for an xonly pubkey in a descriptor

achow101 commented at 5:04 am on January 2, 2025: member

When a ConstPubkeyProvider is xonly, the stored pubkey does not necessarily have the correct parity bit. ToPrivateString() is correctly handling this by looking up the keys for both parity bits, but GetPrivKey does not. This results in not finding the private key when it is actually available if its pubkey has the other parity bit value.

To fix this, this key finding is refactored into GetPrivKey() so that its behavior is corrected, and ToPrivateString() is changed to use GetPrivKey() as well.

Additionally, the descriptor test checks are updated to include a check for ExpandPrivate() to verify that both the parsed public and private descriptors produce SigningProviders with the same contents.

Fixes #31589

DrahtBot commented at 5:04 am on January 2, 2025: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31590.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	Pttn, furszy, davidgumberg, theStack, kevkevinpal
Concept ACK	jonatack, rkrux

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:

#31244 (descriptors: MuSig2 by achow101)
#31243 (descriptor: Move filling of keys from DescriptorImpl::MakeScripts to PubkeyProvider::GetPubKey by achow101)
#30866 (descriptor: Add proper Clone function to miniscript::Node by achow101)
#29675 (wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys 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.

DrahtBot added the label Descriptors on Jan 2, 2025

jonatack commented at 5:14 am on January 2, 2025: member

Concept ACK

in src/script/descriptor.cpp:337 in 739af13894 outdated

333@@ -321,18 +334,8 @@ class ConstPubkeyProvider final : public PubkeyProvider
334             arg.GetKey(m_pubkey.GetID(), key);
335         }
336         if (!key.IsValid()) return false;
337-        ret = EncodeSecret(key);
338         return true;

furszy commented at 11:03 pm on January 2, 2025:

nit:

0return key.IsValid();

achow101 commented at 7:59 pm on January 6, 2025:

Done

in src/script/descriptor.cpp:319 in 739af13894 outdated

311@@ -312,6 +312,19 @@ class ConstPubkeyProvider final : public PubkeyProvider
312     bool ToPrivateString(const SigningProvider& arg, std::string& ret) const override
313     {
314         CKey key;
315+        if (GetPrivKey(/*pos=*/0, arg, key)) {
316+            ret = EncodeSecret(key);
317+            return true;
318+        }
319+        return false;

furszy commented at 11:05 pm on January 2, 2025:

nano nit:

0CKey key;
1if (!GetPrivKey(/*pos=*/0, arg, key)) return false;
2ret = EncodeSecret(key);
3return true;

achow101 commented at 7:59 pm on January 6, 2025:

Done

furszy commented at 11:09 pm on January 2, 2025: member

looking good, only left few nits. No need to tackle them. Will test it properly in the coming days.

theStack approved

theStack commented at 8:16 am on January 3, 2025: contributor

Concept and code-review ACK 116ed1d543641006f74bf089c23714259b6d4904

commit message / PR description micro-nit: haven’t seen the term “evenness” before in this context, I think we usually call it “parity bit” (as in BIP-341)

DrahtBot requested review from jonatack on Jan 3, 2025

in src/script/descriptor.cpp:325 in 739af13894 outdated

322+    {
323+        ret = ToString(StringType::PUBLIC);
324+        return true;
325+    }
326+    bool GetPrivKey(int pos, const SigningProvider& arg, CKey& key) const override
327+    {

furszy commented at 8:52 pm on January 3, 2025:

fun fact: this function could be reduced to a single line:

0bool GetPrivKey(int pos, const SigningProvider& arg, CKey& key) const override
1{
2    return m_xonly ? arg.GetKeyByXOnly(XOnlyPubKey(m_pubkey), key) : 
3                     arg.GetKey(m_pubkey.GetID(), key);
4}

achow101 commented at 7:59 pm on January 6, 2025:

Done

furszy commented at 9:02 pm on January 3, 2025: member

q: why not store the id belonging to the pubkey with the correct parity bit inside the SigningProvider arg at the point where such a structure is loaded? (which most likely happens inside ParsePubkeyInner()).

achow101 commented at 3:03 am on January 4, 2025: member

q: why not store the id belonging to the pubkey with the correct parity bit inside the SigningProvider arg at the point where such a structure is loaded? (which most likely happens inside ParsePubkeyInner()).

The pubkey in the descriptor has no parity bit information at all, and parsing is context independent.

descriptor: Try the other parity in ConstPubkeyProvider::GetPrivKey()

GetPrivKey() needs the same handling of all keyids for xonly keys that
ToPrivateString() does. Refactor that into GetPrivKey() and reuse it in
ToPrivateString() to resolve this.

092569e858

tests: Check ExpandPrivate matches for both parsed descriptors 4c50c21f6b

achow101 force-pushed on Jan 6, 2025

achow101 commented at 7:59 pm on January 6, 2025: member

commit message / PR description micro-nit: haven’t seen the term “evenness” before in this context, I think we usually call it “parity bit” (as in BIP-341)

Fixed those.

DrahtBot added the label CI failed on Jan 6, 2025

descriptor: Use InferXOnlyPubkey for miniscript XOnly pubkey from script b4ac48090f

Add test for multipath miniscript expression c0045e6cee

achow101 commented at 0:12 am on January 7, 2025: member

@davidgumberg Wrote a test for a different PR that is failing for a similar issue, but in miniscript inferring. I’ve pushed that test and a commit to fix it as it’s also a parity bit issue.

DrahtBot removed the label CI failed on Jan 7, 2025

Pttn commented at 3:02 pm on January 9, 2025: contributor

ACK c0045e6cee06bc0029fb79b5a531aa1f2b817424 Solves the issue, looks good to me, thank you for working on this!

in src/test/descriptor_tests.cpp:72 in 4c50c21f6b outdated

67+    return a.scripts == b.scripts
68+        && a.pubkeys == b.pubkeys
69+        && a.origins == b.origins
70+        && a.keys == b.keys
71+        && a.tr_trees == b.tr_trees;
72+}

davidgumberg commented at 3:20 am on January 10, 2025:

~~nit:~~ The equality operator could be explicitly defaulted for SigningProvider:

0diff --git a/src/script/signingprovider.h b/src/script/signingprovider.h
1index 5b1da681f8..8262db0b52 100644
2--- a/src/script/signingprovider.h
3+++ b/src/script/signingprovider.h
4@@ -153,6 +153,7 @@ class SigningProvider
5 {
6 public:
7     virtual ~SigningProvider() = default;
8+    virtual bool operator==(const SigningProvider&) const = default;

Edit: Actually, I’m not sure if I can defend why that would be better aside from being shorter and maybe one less place that has to be touched if the members of FlatSigningProvider ever change, not that important.

DrahtBot requested review from theStack on Jan 10, 2025

furszy commented at 8:54 pm on January 13, 2025: member

ACK c0045e6cee06bc0029fb79b5a531aa1f2b817424

davidgumberg commented at 9:29 pm on January 13, 2025: contributor

utACK https://github.com/bitcoin/bitcoin/pull/31590/commits/c0045e6cee06bc0029fb79b5a531aa1f2b817424

I think this fixes #31589, and fixes a bug in miniscript parsing of xonly pubkeys. ¹

I think there might still be an issue with ConstPubkeyProvider::GetSize(), I’ll try to look into this and open a follow-up if necessary. ↩︎

theStack approved

theStack commented at 11:10 am on January 14, 2025: contributor

re-ACK c0045e6cee06bc0029fb79b5a531aa1f2b817424

in src/script/descriptor.cpp:324 in 092569e858 outdated

322@@ -331,7 +323,8 @@ class ConstPubkeyProvider final : public PubkeyProvider
323     }
324     bool GetPrivKey(int pos, const SigningProvider& arg, CKey& key) const override

rkrux commented at 11:27 am on January 15, 2025:

Why does GetPrivKey of ConstPubkeyProvider not end up using the pos argument? Is it because it’s supposed to be a “constant” provider?

achow101 commented at 10:14 pm on January 20, 2025:

pos is really only used by BIP32PubkeyProvider to indicate child derivation index. Since no other PubkeyProvider can do derivation, it is ignored for all of them.

rkrux commented at 11:42 am on January 15, 2025: none

Concept ACK c0045e6cee06bc0029fb79b5a531aa1f2b817424

build and unit tests successful.

nit in pr title:

0- "descriptors: Try pubkeys of both evenness when retrieving the private keys for an xonly pubkey in a descriptor"
1+ "descriptors: Try pubkeys of both parities when retrieving the private keys for an xonly pubkey in a descriptor"

kevkevinpal commented at 2:37 pm on January 20, 2025: contributor

Concept and Code review ACK c0045e6

fixes issue and looks good to me!

DrahtBot requested review from rkrux on Jan 20, 2025

achow101 renamed this:
~~descriptors: Try pubkeys of both evenness when retrieving the private keys for an xonly pubkey in a descriptor~~
descriptors: Try pubkeys of both parities when retrieving the private keys for an xonly pubkey in a descriptor
on Jan 20, 2025

descriptors: Try pubkeys of both parities when retrieving the private keys for an xonly pubkey in a descriptor #31590

Code Coverage & Benchmarks

Reviews

Conflicts