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

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

2. 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
3. 
   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
   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.

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

   Concept ACK
5. theStack approved
6. 
   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…)

7. DrahtBot requested review from josibake on Nov 14, 2024
8. in src/script/signingprovider.cpp:65 in 493656763f

   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
   

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

   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;
   

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

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

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

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

Contributors
achow101 DrahtBot josibake theStack jonatack

Review Requested
josibake