From 493656763f73e5ef1cfb979a513c12983dca99dd Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Mon, 18 Mar 2024 16:16:48 -0400 Subject: [PATCH 1/3] 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. --- src/script/signingprovider.cpp | 5 +++++ src/script/signingprovider.h | 1 + src/wallet/scriptpubkeyman.cpp | 6 +++++- 3 files changed, 11 insertions(+), 1 deletion(-) diff --git a/src/script/signingprovider.cpp b/src/script/signingprovider.cpp index baabd4d5b5c..597b1a1544b 100644 --- a/src/script/signingprovider.cpp +++ b/src/script/signingprovider.cpp @@ -62,6 +62,11 @@ bool FlatSigningProvider::GetKeyOrigin(const CKeyID& keyid, KeyOriginInfo& info) if (ret) info = std::move(out.second); return ret; } +bool FlatSigningProvider::HaveKey(const CKeyID &keyid) const +{ + CKey key; + return LookupHelper(keys, keyid, key); +} bool FlatSigningProvider::GetKey(const CKeyID& keyid, CKey& key) const { return LookupHelper(keys, keyid, key); } bool FlatSigningProvider::GetTaprootSpendData(const XOnlyPubKey& output_key, TaprootSpendData& spenddata) const { diff --git a/src/script/signingprovider.h b/src/script/signingprovider.h index efdfd9ee566..f0fb21c8b18 100644 --- a/src/script/signingprovider.h +++ b/src/script/signingprovider.h @@ -215,6 +215,7 @@ struct FlatSigningProvider final : public SigningProvider bool GetCScript(const CScriptID& scriptid, CScript& script) const override; bool GetPubKey(const CKeyID& keyid, CPubKey& pubkey) const override; bool GetKeyOrigin(const CKeyID& keyid, KeyOriginInfo& info) const override; + bool HaveKey(const CKeyID &keyid) const override; bool GetKey(const CKeyID& keyid, CKey& key) const override; bool GetTaprootSpendData(const XOnlyPubKey& output_key, TaprootSpendData& spenddata) const override; bool GetTaprootBuilder(const XOnlyPubKey& output_key, TaprootBuilder& builder) const override; diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 62384056dc6..7787e2ff6bf 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -2456,7 +2456,11 @@ std::unique_ptr DescriptorScriptPubKeyMan::GetSigningProvid int32_t index = it->second; // Always try to get the signing provider with private keys. This function should only be called during signing anyways - return GetSigningProvider(index, true); + std::unique_ptr out = GetSigningProvider(index, true); + if (!out->HaveKey(pubkey.GetID())) { + return nullptr; + } + return out; } std::unique_ptr DescriptorScriptPubKeyMan::GetSigningProvider(int32_t index, bool include_private) const From 62a95f5af9b998e241eb72c16ba581e77c480126 Mon Sep 17 00:00:00 2001 From: Sebastian Falbesoner Date: Thu, 14 Nov 2024 13:15:16 +0100 Subject: [PATCH 2/3] test: refactor: move `CreateDescriptor` helper to wallet test util module Can be reviewed via `--color-moved=dimmed-zebra`. --- src/wallet/test/ismine_tests.cpp | 20 -------------------- src/wallet/test/util.cpp | 20 ++++++++++++++++++++ src/wallet/test/util.h | 4 +++- 3 files changed, 23 insertions(+), 21 deletions(-) diff --git a/src/wallet/test/ismine_tests.cpp b/src/wallet/test/ismine_tests.cpp index f6688ed30a0..3b27c7db9a0 100644 --- a/src/wallet/test/ismine_tests.cpp +++ b/src/wallet/test/ismine_tests.cpp @@ -20,26 +20,6 @@ using namespace util::hex_literals; namespace wallet { BOOST_FIXTURE_TEST_SUITE(ismine_tests, BasicTestingSetup) -wallet::ScriptPubKeyMan* CreateDescriptor(CWallet& keystore, const std::string& desc_str, const bool success) -{ - keystore.SetWalletFlag(WALLET_FLAG_DESCRIPTORS); - - FlatSigningProvider keys; - std::string error; - auto parsed_descs = Parse(desc_str, keys, error, false); - BOOST_CHECK(success == (!parsed_descs.empty())); - if (!success) return nullptr; - auto& desc = parsed_descs.at(0); - - const int64_t range_start = 0, range_end = 1, next_index = 0, timestamp = 1; - - WalletDescriptor w_desc(std::move(desc), timestamp, range_start, range_end, next_index); - - LOCK(keystore.cs_wallet); - - return Assert(keystore.AddWalletDescriptor(w_desc, keys,/*label=*/"", /*internal=*/false)); -}; - BOOST_AUTO_TEST_CASE(ismine_standard) { CKey keys[2]; diff --git a/src/wallet/test/util.cpp b/src/wallet/test/util.cpp index 43fd91fe60d..bc53510fe49 100644 --- a/src/wallet/test/util.cpp +++ b/src/wallet/test/util.cpp @@ -192,4 +192,24 @@ MockableDatabase& GetMockableDatabase(CWallet& wallet) { return dynamic_cast(wallet.GetDatabase()); } + +wallet::ScriptPubKeyMan* CreateDescriptor(CWallet& keystore, const std::string& desc_str, const bool success) +{ + keystore.SetWalletFlag(WALLET_FLAG_DESCRIPTORS); + + FlatSigningProvider keys; + std::string error; + auto parsed_descs = Parse(desc_str, keys, error, false); + Assert(success == (!parsed_descs.empty())); + if (!success) return nullptr; + auto& desc = parsed_descs.at(0); + + const int64_t range_start = 0, range_end = 1, next_index = 0, timestamp = 1; + + WalletDescriptor w_desc(std::move(desc), timestamp, range_start, range_end, next_index); + + LOCK(keystore.cs_wallet); + + return Assert(keystore.AddWalletDescriptor(w_desc, keys,/*label=*/"", /*internal=*/false)); +}; } // namespace wallet diff --git a/src/wallet/test/util.h b/src/wallet/test/util.h index c8a89c0e642..b055c6c6930 100644 --- a/src/wallet/test/util.h +++ b/src/wallet/test/util.h @@ -9,6 +9,7 @@ #include #include +#include #include @@ -127,8 +128,9 @@ public: }; std::unique_ptr CreateMockableWalletDatabase(MockableData records = {}); - MockableDatabase& GetMockableDatabase(CWallet& wallet); + +ScriptPubKeyMan* CreateDescriptor(CWallet& keystore, const std::string& desc_str, const bool success); } // namespace wallet #endif // BITCOIN_WALLET_TEST_UTIL_H From f6a6d912059c66792f48689632d2a7f14f8bdad9 Mon Sep 17 00:00:00 2001 From: Sebastian Falbesoner Date: Thu, 14 Nov 2024 14:45:02 +0100 Subject: [PATCH 3/3] 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. --- src/wallet/scriptpubkeyman.h | 5 +++-- src/wallet/test/scriptpubkeyman_tests.cpp | 23 +++++++++++++++++++++++ 2 files changed, 26 insertions(+), 2 deletions(-) diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index d8b6c90178a..4ff35a478ca 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -611,8 +611,6 @@ private: mutable std::map m_map_signing_providers; // Fetch the SigningProvider for the given script and optionally include private keys std::unique_ptr GetSigningProvider(const CScript& script, bool include_private = false) const; - // Fetch the SigningProvider for the given pubkey and always include private keys. This should only be called by signing code. - std::unique_ptr GetSigningProvider(const CPubKey& pubkey) const; // Fetch the SigningProvider for a given index and optionally include private keys. Called by the above functions. std::unique_ptr GetSigningProvider(int32_t index, bool include_private = false) const EXCLUSIVE_LOCKS_REQUIRED(cs_desc_man); @@ -675,6 +673,9 @@ public: bool CanProvide(const CScript& script, SignatureData& sigdata) override; + // Fetch the SigningProvider for the given pubkey and always include private keys. This should only be called by signing code. + std::unique_ptr GetSigningProvider(const CPubKey& pubkey) const; + bool SignTransaction(CMutableTransaction& tx, const std::map& coins, int sighash, std::map& input_errors) const override; SigningResult SignMessage(const std::string& message, const PKHash& pkhash, std::string& str_sig) const override; std::optional FillPSBT(PartiallySignedTransaction& psbt, const PrecomputedTransactionData& txdata, int sighash_type = SIGHASH_DEFAULT, bool sign = true, bool bip32derivs = false, int* n_signed = nullptr, bool finalize = true) const override; diff --git a/src/wallet/test/scriptpubkeyman_tests.cpp b/src/wallet/test/scriptpubkeyman_tests.cpp index 15bff04221d..f27865865db 100644 --- a/src/wallet/test/scriptpubkeyman_tests.cpp +++ b/src/wallet/test/scriptpubkeyman_tests.cpp @@ -3,6 +3,7 @@ // file COPYING or http://www.opensource.org/licenses/mit-license.php. #include +#include #include #include