wallet, desc spkm: Return SigningProvider only if we have the privkey

achow101 commented at 4:52 pm on November 7, 2024: member

If we know about a pubkey that’s in our descriptor, but we don’t have the private key, don’t return a SigningProvider for that pubkey.

This is specifically an issue for Taproot outputs that use the H point as the resulting PSBTs may end up containing irrelevant information because the H point was detected as a pubkey each unrelated descriptor knew about.

Split from #29675

desc spkm: Return SigningProvider only if we have the privkey

If we know about a pubkey that's in our descriptor, but we don't have
the private key, don't return a SigningProvider for that pubkey.

This is specifically an issue for Taproot outputs that use the H point
as the resulting PSBTs may end up containing irrelevant information
because the H point was detected as a pubkey each unrelated descriptor
knew about.

493656763f

DrahtBot commented at 4:52 pm on November 7, 2024: 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/31242.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	theStack, fjahr, furszy
Concept ACK	josibake, jonatack

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

Conflicts

No conflicts as of last run.

josibake commented at 1:49 pm on November 14, 2024: member

Concept ACK

theStack approved

theStack commented at 1:55 pm on November 14, 2024: contributor

Concept and code-review ACK 493656763f73e5ef1cfb979a513c12983dca99dd

Convinced myself that this works as intended by writing a simple unit test: https://github.com/theStack/bitcoin/tree/pr31242_add_test (could be added here or in a follow-up, though having to change the visibility of GetSigningProvider from private to public only for the sake of testing is a bit ugly…)

DrahtBot requested review from josibake on Nov 14, 2024

in src/script/signingprovider.cpp:65 in 493656763f outdated

61@@ -62,6 +62,11 @@ bool FlatSigningProvider::GetKeyOrigin(const CKeyID& keyid, KeyOriginInfo& info)
62     if (ret) info = std::move(out.second);
63     return ret;
64 }
65+bool FlatSigningProvider::HaveKey(const CKeyID &keyid) const

jonatack commented at 1:13 pm on November 26, 2024:

0bool FlatSigningProvider::HaveKey(const CKeyID& keyid) const

in src/script/signingprovider.h:218 in 493656763f outdated

214@@ -215,6 +215,7 @@ struct FlatSigningProvider final : public SigningProvider
215     bool GetCScript(const CScriptID& scriptid, CScript& script) const override;
216     bool GetPubKey(const CKeyID& keyid, CPubKey& pubkey) const override;
217     bool GetKeyOrigin(const CKeyID& keyid, KeyOriginInfo& info) const override;
218+    bool HaveKey(const CKeyID &keyid) const override;

jonatack commented at 1:14 pm on November 26, 2024:

0    bool HaveKey(const CKeyID& keyid) const override;

in src/wallet/scriptpubkeyman.cpp:2459 in 493656763f outdated

2455@@ -2456,7 +2456,11 @@ std::unique_ptr<FlatSigningProvider> DescriptorScriptPubKeyMan::GetSigningProvid
2456     int32_t index = it->second;
2457 
2458     // Always try to get the signing provider with private keys. This function should only be called during signing anyways
2459-    return GetSigningProvider(index, true);
2460+    std::unique_ptr<FlatSigningProvider> out = GetSigningProvider(index, true);

jonatack commented at 1:14 pm on November 26, 2024:

0    std::unique_ptr<FlatSigningProvider> out = GetSigningProvider(index, /*include_private=*/true);

jonatack commented at 1:18 pm on November 26, 2024: member

ACK modulo test coverage, a few suggestions if you repush.

fjahr commented at 12:59 pm on December 30, 2024: contributor

utACK 493656763f73e5ef1cfb979a513c12983dca99dd

Test coverage would be nice but I suppose it might be skipped if this get coverage as part of wider coverage of the musig2 code paths.

DrahtBot requested review from jonatack on Dec 30, 2024

test: refactor: move `CreateDescriptor` helper to wallet test util module

Can be reviewed via `--color-moved=dimmed-zebra`.

62a95f5af9

test: add check for getting SigningProvider for a CPubKey

Verify that the DescriptorSPKM method `GetSigningProvider` should
only return a signing provider for the passed public key if its
corresponding private key of the passed public key is available.

f6a6d91205

achow101 commented at 6:13 pm on January 3, 2025: member

Pulled in @theStack’s test

theStack approved

theStack commented at 12:07 pm on January 4, 2025: contributor

re-ACK f6a6d912059c66792f48689632d2a7f14f8bdad9

DrahtBot requested review from fjahr on Jan 4, 2025

fjahr commented at 3:56 pm on January 4, 2025: contributor

ACK f6a6d912059c66792f48689632d2a7f14f8bdad9

Reviewed and verified that the test works.

in src/script/signingprovider.cpp:68 in 493656763f outdated

61@@ -62,6 +62,11 @@ bool FlatSigningProvider::GetKeyOrigin(const CKeyID& keyid, KeyOriginInfo& info)
62     if (ret) info = std::move(out.second);
63     return ret;
64 }
65+bool FlatSigningProvider::HaveKey(const CKeyID &keyid) const
66+{
67+    CKey key;
68+    return LookupHelper(keys, keyid, key);

furszy commented at 6:37 pm on January 8, 2025:

nit: return keys.contains(keyid);

in src/wallet/scriptpubkeyman.cpp:2461 in 493656763f outdated

2455@@ -2456,7 +2456,11 @@ std::unique_ptr<FlatSigningProvider> DescriptorScriptPubKeyMan::GetSigningProvid
2456     int32_t index = it->second;
2457 
2458     // Always try to get the signing provider with private keys. This function should only be called during signing anyways
2459-    return GetSigningProvider(index, true);
2460+    std::unique_ptr<FlatSigningProvider> out = GetSigningProvider(index, true);
2461+    if (!out->HaveKey(pubkey.GetID())) {
2462+        return nullptr;

furszy commented at 6:51 pm on January 8, 2025:

for the sake of consistency across overloaded methods, GetSigningProvider(script, include_private) should behave in the same manner (the function that is just above this one).

furszy commented at 6:52 pm on January 8, 2025: member

utACK f6a6d912059. Only reviewed the actual change in detail, not the test commit.

fanquake merged this on Jan 16, 2025

fanquake closed this on Jan 16, 2025

wallet, desc spkm: Return SigningProvider only if we have the privkey #31242

Code Coverage & Benchmarks

Reviews

Conflicts