From 07d3bdf4ebc06825ea24ab6f7c87aef6a22238c6 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Wed, 9 Aug 2023 09:46:25 -0400 Subject: Add PubKeyDestination for P2PK scripts P2PK scripts are not PKHash destinations, they should have their own type. This also results in no longer showing a p2pkh address for p2pk outputs. However for backwards compatibility, ListCoinst will still do this conversion. --- src/addresstype.cpp | 31 +++++++++++++++++++++++++------ src/addresstype.h | 22 ++++++++++++++++++---- src/key_io.cpp | 1 + src/rpc/output_script.cpp | 5 +++++ src/rpc/util.cpp | 5 +++++ src/test/fuzz/script.cpp | 9 ++++++--- src/test/fuzz/util.cpp | 9 +++++++++ src/test/script_standard_tests.cpp | 4 ++-- src/wallet/rpc/addresses.cpp | 1 + src/wallet/spend.cpp | 11 +++++++++-- test/functional/rpc_deriveaddresses.py | 5 ++++- 11 files changed, 85 insertions(+), 18 deletions(-) diff --git a/src/addresstype.cpp b/src/addresstype.cpp index 78df1c13a4..f199d1b479 100644 --- a/src/addresstype.cpp +++ b/src/addresstype.cpp @@ -54,11 +54,12 @@ bool ExtractDestination(const CScript& scriptPubKey, CTxDestination& addressRet) switch (whichType) { case TxoutType::PUBKEY: { CPubKey pubKey(vSolutions[0]); - if (!pubKey.IsValid()) - return false; - - addressRet = PKHash(pubKey); - return true; + if (!pubKey.IsValid()) { + addressRet = CNoDestination(scriptPubKey); + } else { + addressRet = PubKeyDestination(pubKey); + } + return false; } case TxoutType::PUBKEYHASH: { addressRet = PKHash(uint160(vSolutions[0])); @@ -108,6 +109,11 @@ public: return dest.GetScript(); } + CScript operator()(const PubKeyDestination& dest) const + { + return CScript() << ToByteVector(dest.GetPubKey()) << OP_CHECKSIG; + } + CScript operator()(const PKHash& keyID) const { return CScript() << OP_DUP << OP_HASH160 << ToByteVector(keyID) << OP_EQUALVERIFY << OP_CHECKSIG; @@ -138,6 +144,19 @@ public: return CScript() << CScript::EncodeOP_N(id.GetWitnessVersion()) << id.GetWitnessProgram(); } }; + +class ValidDestinationVisitor +{ +public: + bool operator()(const CNoDestination& dest) const { return false; } + bool operator()(const PubKeyDestination& dest) const { return false; } + bool operator()(const PKHash& dest) const { return true; } + bool operator()(const ScriptHash& dest) const { return true; } + bool operator()(const WitnessV0KeyHash& dest) const { return true; } + bool operator()(const WitnessV0ScriptHash& dest) const { return true; } + bool operator()(const WitnessV1Taproot& dest) const { return true; } + bool operator()(const WitnessUnknown& dest) const { return true; } +}; } // namespace CScript GetScriptForDestination(const CTxDestination& dest) @@ -146,5 +165,5 @@ CScript GetScriptForDestination(const CTxDestination& dest) } bool IsValidDestination(const CTxDestination& dest) { - return dest.index() != 0; + return std::visit(ValidDestinationVisitor(), dest); } diff --git a/src/addresstype.h b/src/addresstype.h index a41d3bda9e..d3422c6813 100644 --- a/src/addresstype.h +++ b/src/addresstype.h @@ -27,6 +27,19 @@ public: friend bool operator<(const CNoDestination& a, const CNoDestination& b) { return a.GetScript() < b.GetScript(); } }; +struct PubKeyDestination { +private: + CPubKey m_pubkey; + +public: + PubKeyDestination(const CPubKey& pubkey) : m_pubkey(pubkey) {} + + const CPubKey& GetPubKey() const LIFETIMEBOUND { return m_pubkey; } + + friend bool operator==(const PubKeyDestination& a, const PubKeyDestination& b) { return a.GetPubKey() == b.GetPubKey(); } + friend bool operator<(const PubKeyDestination& a, const PubKeyDestination& b) { return a.GetPubKey() < b.GetPubKey(); } +}; + struct PKHash : public BaseHash { PKHash() : BaseHash() {} @@ -103,6 +116,7 @@ public: /** * A txout script categorized into standard templates. * * CNoDestination: Optionally a script, no corresponding address. + * * PubKeyDestination: TxoutType::PUBKEY (P2PK), no corresponding address * * PKHash: TxoutType::PUBKEYHASH destination (P2PKH address) * * ScriptHash: TxoutType::SCRIPTHASH destination (P2SH address) * * WitnessV0ScriptHash: TxoutType::WITNESS_V0_SCRIPTHASH destination (P2WSH address) @@ -111,9 +125,9 @@ public: * * WitnessUnknown: TxoutType::WITNESS_UNKNOWN destination (P2W??? address) * A CTxDestination is the internal data type encoded in a bitcoin address */ -using CTxDestination = std::variant; +using CTxDestination = std::variant; -/** Check whether a CTxDestination is a CNoDestination. */ +/** Check whether a CTxDestination corresponds to one with an address. */ bool IsValidDestination(const CTxDestination& dest); /** @@ -123,8 +137,8 @@ bool IsValidDestination(const CTxDestination& dest); * is assigned to addressRet. * For all other scripts. addressRet is assigned as a CNoDestination containing the scriptPubKey. * - * Returns true for standard destinations - P2PK, P2PKH, P2SH, P2WPKH, P2WSH, and P2TR scripts. - * Returns false for non-standard destinations - P2PK with invalid pubkeys, P2W???, bare multisig, null data, and nonstandard scripts. + * Returns true for standard destinations with addresses - P2PKH, P2SH, P2WPKH, P2WSH, P2TR and P2W??? scripts. + * Returns false for non-standard destinations and those without addresses - P2PK, bare multisig, null data, and nonstandard scripts. */ bool ExtractDestination(const CScript& scriptPubKey, CTxDestination& addressRet); diff --git a/src/key_io.cpp b/src/key_io.cpp index 96dc01550c..1a43b5846f 100644 --- a/src/key_io.cpp +++ b/src/key_io.cpp @@ -77,6 +77,7 @@ public: } std::string operator()(const CNoDestination& no) const { return {}; } + std::string operator()(const PubKeyDestination& pk) const { return {}; } }; CTxDestination DecodeDestination(const std::string& str, const CChainParams& params, std::string& error_str, std::vector* error_locations) diff --git a/src/rpc/output_script.cpp b/src/rpc/output_script.cpp index 4dd424fa14..f9343f48a8 100644 --- a/src/rpc/output_script.cpp +++ b/src/rpc/output_script.cpp @@ -280,6 +280,11 @@ static RPCHelpMan deriveaddresses() for (const CScript& script : scripts) { CTxDestination dest; if (!ExtractDestination(script, dest)) { + // ExtractDestination no longer returns true for P2PK since it doesn't have a corresponding address + // However combo will output P2PK and should just ignore that script + if (scripts.size() > 1 && std::get_if(&dest)) { + continue; + } throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Descriptor does not have a corresponding address"); } diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp index e5ee6d7496..9a941be181 100644 --- a/src/rpc/util.cpp +++ b/src/rpc/util.cpp @@ -253,6 +253,11 @@ public: return UniValue(UniValue::VOBJ); } + UniValue operator()(const PubKeyDestination& dest) const + { + return UniValue(UniValue::VOBJ); + } + UniValue operator()(const PKHash& keyID) const { UniValue obj(UniValue::VOBJ); diff --git a/src/test/fuzz/script.cpp b/src/test/fuzz/script.cpp index acc82f55f6..fe41a8c6ae 100644 --- a/src/test/fuzz/script.cpp +++ b/src/test/fuzz/script.cpp @@ -149,13 +149,16 @@ FUZZ_TARGET(script, .init = initialize_script) const CTxDestination tx_destination_2{ConsumeTxDestination(fuzzed_data_provider)}; const std::string encoded_dest{EncodeDestination(tx_destination_1)}; const UniValue json_dest{DescribeAddress(tx_destination_1)}; - Assert(tx_destination_1 == DecodeDestination(encoded_dest)); (void)GetKeyForDestination(/*store=*/{}, tx_destination_1); const CScript dest{GetScriptForDestination(tx_destination_1)}; const bool valid{IsValidDestination(tx_destination_1)}; - Assert(dest.empty() != valid); - Assert(valid == IsValidDestinationString(encoded_dest)); + if (!std::get_if(&tx_destination_1)) { + // Only try to round trip non-pubkey destinations since PubKeyDestination has no encoding + Assert(dest.empty() != valid); + Assert(tx_destination_1 == DecodeDestination(encoded_dest)); + Assert(valid == IsValidDestinationString(encoded_dest)); + } (void)(tx_destination_1 < tx_destination_2); if (tx_destination_1 == tx_destination_2) { diff --git a/src/test/fuzz/util.cpp b/src/test/fuzz/util.cpp index 201495a47e..87ca2f6aed 100644 --- a/src/test/fuzz/util.cpp +++ b/src/test/fuzz/util.cpp @@ -172,6 +172,15 @@ CTxDestination ConsumeTxDestination(FuzzedDataProvider& fuzzed_data_provider) no [&] { tx_destination = CNoDestination{}; }, + [&] { + bool compressed = fuzzed_data_provider.ConsumeBool(); + CPubKey pk{ConstructPubKeyBytes( + fuzzed_data_provider, + ConsumeFixedLengthByteVector(fuzzed_data_provider, (compressed ? CPubKey::COMPRESSED_SIZE : CPubKey::SIZE)), + compressed + )}; + tx_destination = PubKeyDestination{pk}; + }, [&] { tx_destination = PKHash{ConsumeUInt160(fuzzed_data_provider)}; }, diff --git a/src/test/script_standard_tests.cpp b/src/test/script_standard_tests.cpp index c3d5990e01..58bdb37b7c 100644 --- a/src/test/script_standard_tests.cpp +++ b/src/test/script_standard_tests.cpp @@ -203,8 +203,8 @@ BOOST_AUTO_TEST_CASE(script_standard_ExtractDestination) // TxoutType::PUBKEY s.clear(); s << ToByteVector(pubkey) << OP_CHECKSIG; - BOOST_CHECK(ExtractDestination(s, address)); - BOOST_CHECK(std::get(address) == PKHash(pubkey)); + BOOST_CHECK(!ExtractDestination(s, address)); + BOOST_CHECK(std::get(address) == PubKeyDestination(pubkey)); // TxoutType::PUBKEYHASH s.clear(); diff --git a/src/wallet/rpc/addresses.cpp b/src/wallet/rpc/addresses.cpp index c1b99a4f97..e9b93afc30 100644 --- a/src/wallet/rpc/addresses.cpp +++ b/src/wallet/rpc/addresses.cpp @@ -427,6 +427,7 @@ public: explicit DescribeWalletAddressVisitor(const SigningProvider* _provider) : provider(_provider) {} UniValue operator()(const CNoDestination& dest) const { return UniValue(UniValue::VOBJ); } + UniValue operator()(const PubKeyDestination& dest) const { return UniValue(UniValue::VOBJ); } UniValue operator()(const PKHash& pkhash) const { diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 750b6c100b..5d2c299a69 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -490,8 +490,15 @@ std::map> ListCoins(const CWallet& wallet) coins_params.skip_locked = false; for (const COutput& coin : AvailableCoins(wallet, &coin_control, /*feerate=*/std::nullopt, coins_params).All()) { CTxDestination address; - if ((coin.spendable || (wallet.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) && coin.solvable)) && - ExtractDestination(FindNonChangeParentOutput(wallet, coin.outpoint).scriptPubKey, address)) { + if ((coin.spendable || (wallet.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) && coin.solvable))) { + if (!ExtractDestination(FindNonChangeParentOutput(wallet, coin.outpoint).scriptPubKey, address)) { + // For backwards compatibility, we convert P2PK output scripts into PKHash destinations + if (auto pk_dest = std::get_if(&address)) { + address = PKHash(pk_dest->GetPubKey()); + } else { + continue; + } + } result[address].emplace_back(coin); } } diff --git a/test/functional/rpc_deriveaddresses.py b/test/functional/rpc_deriveaddresses.py index e96b6bda90..64994d6bb3 100755 --- a/test/functional/rpc_deriveaddresses.py +++ b/test/functional/rpc_deriveaddresses.py @@ -42,7 +42,10 @@ class DeriveaddressesTest(BitcoinTestFramework): assert_raises_rpc_error(-8, "Range should be greater or equal than 0", self.nodes[0].deriveaddresses, descsum_create("wpkh(tprv8ZgxMBicQKsPd7Uf69XL1XwhmjHopUGep8GuEiJDZmbQz6o58LninorQAfcKZWARbtRtfnLcJ5MQ2AtHcQJCCRUcMRvmDUjyEmNUWwx8UbK/1/1/*)"), [-1, 0]) combo_descriptor = descsum_create("combo(tprv8ZgxMBicQKsPd7Uf69XL1XwhmjHopUGep8GuEiJDZmbQz6o58LninorQAfcKZWARbtRtfnLcJ5MQ2AtHcQJCCRUcMRvmDUjyEmNUWwx8UbK/1/1/0)") - assert_equal(self.nodes[0].deriveaddresses(combo_descriptor), ["mtfUoUax9L4tzXARpw1oTGxWyoogp52KhJ", "mtfUoUax9L4tzXARpw1oTGxWyoogp52KhJ", address, "2NDvEwGfpEqJWfybzpKPHF2XH3jwoQV3D7x"]) + assert_equal(self.nodes[0].deriveaddresses(combo_descriptor), ["mtfUoUax9L4tzXARpw1oTGxWyoogp52KhJ", address, "2NDvEwGfpEqJWfybzpKPHF2XH3jwoQV3D7x"]) + + # P2PK does not have a valid address + assert_raises_rpc_error(-5, "Descriptor does not have a corresponding address", self.nodes[0].deriveaddresses, descsum_create("pk(tprv8ZgxMBicQKsPd7Uf69XL1XwhmjHopUGep8GuEiJDZmbQz6o58LninorQAfcKZWARbtRtfnLcJ5MQ2AtHcQJCCRUcMRvmDUjyEmNUWwx8UbK)")) # Before #26275, bitcoind would crash when deriveaddresses was # called with derivation index 2147483647, which is the maximum -- cgit v1.2.3