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.
achow101 · Aug 18, 2023 · 5727940 · 5727940
1 parent 4684adc
commit 5727940
Show file tree

Hide file tree

Showing 11 changed files with 55 additions and 14 deletions.
diff --git a/src/addresstype.cpp b/src/addresstype.cpp
@@ -51,11 +51,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]));
@@ -109,6 +110,11 @@ class CScriptVisitor
         return dest.script;
     }
 
+    CScript operator()(const PubKeyDestination& dest) const
+    {
+        return CScript() << ToByteVector(dest.pubkey) << OP_CHECKSIG;
+    }
+
     CScript operator()(const PKHash& keyID) const
     {
         return CScript() << OP_DUP << OP_HASH160 << ToByteVector(keyID) << OP_EQUALVERIFY << OP_CHECKSIG;

diff --git a/src/addresstype.h b/src/addresstype.h
@@ -23,6 +23,15 @@ class CNoDestination {
     friend bool operator<(const CNoDestination &a, const CNoDestination &b) { return a.script < b.script; }
 };
 
+struct PubKeyDestination
+{
+    CPubKey pubkey;
+
+    PubKeyDestination(const CPubKey& pubkey) : pubkey(pubkey) {}
+    friend bool operator==(const PubKeyDestination &a, const PubKeyDestination &b) { return a.pubkey == b.pubkey; }
+    friend bool operator<(const PubKeyDestination &a, const PubKeyDestination &b) { return a.pubkey < b.pubkey; }
+};
+
 struct PKHash : public BaseHash<uint160>
 {
     PKHash() : BaseHash() {}
@@ -103,7 +112,7 @@ struct WitnessUnknown
  *  * WitnessUnknown: TxoutType::WITNESS_UNKNOWN destination (P2W???)
  *  A CTxDestination is the internal data type encoded in a bitcoin address
  */
-using CTxDestination = std::variant<CNoDestination, PKHash, ScriptHash, WitnessV0ScriptHash, WitnessV0KeyHash, WitnessV1Taproot, WitnessUnknown>;
+using CTxDestination = std::variant<CNoDestination, PubKeyDestination, PKHash, ScriptHash, WitnessV0ScriptHash, WitnessV0KeyHash, WitnessV1Taproot, WitnessUnknown>;
 
 /** Check whether a CTxDestination is a CNoDestination. */
 bool IsValidDestination(const CTxDestination& dest);

diff --git a/src/key_io.cpp b/src/key_io.cpp
@@ -76,6 +76,7 @@ class DestinationEncoder
     }
 
     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<int>* error_locations)

diff --git a/src/rpc/output_script.cpp b/src/rpc/output_script.cpp
@@ -280,7 +280,10 @@ static RPCHelpMan deriveaddresses()
                 for (const CScript& script : scripts) {
                     CTxDestination dest;
                     if (!ExtractDestination(script, dest)) {
-                        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Descriptor does not have a corresponding address");
+                        if (!std::get_if<PubKeyDestination>(&dest)) {
+                            throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Descriptor does not have a corresponding address");
+                        }
+                        continue;
                     }
 
                     addresses.push_back(EncodeDestination(dest));

diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp
@@ -253,6 +253,11 @@ class DescribeAddressVisitor
         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
@@ -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<PubKeyDestination>(&tx_destination_1)) {
+            // Only try to round trip non-pubkey destinations since PubKeyDestination has no encoding
+            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
@@ -163,6 +163,13 @@ CTxDestination ConsumeTxDestination(FuzzedDataProvider& fuzzed_data_provider) no
         [&] {
             tx_destination = CNoDestination{};
         },
+        [&] {
+            uint8_t pk_type = fuzzed_data_provider.PickValueInArray({0x02, 0x03, 0x04, 0x06, 0x07});
+            std::vector<uint8_t> pk_data = ConsumeFixedLengthByteVector(fuzzed_data_provider, pk_type >= 0x04 ? 65 : 33);
+            pk_data[0] = pk_type;
+            CPubKey pk{pk_data.begin(), pk_data.end()};
+            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
@@ -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<PKHash>(address) == PKHash(pubkey));
+    BOOST_CHECK(!ExtractDestination(s, address));
+    BOOST_CHECK(std::get<PubKeyDestination>(address) == PubKeyDestination(pubkey));
 
     // TxoutType::PUBKEYHASH
     s.clear();

diff --git a/src/wallet/rpc/addresses.cpp b/src/wallet/rpc/addresses.cpp
@@ -427,6 +427,7 @@ class DescribeWalletAddressVisitor
     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
@@ -391,8 +391,14 @@ std::map<CTxDestination, std::vector<COutput>> 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)) {
+                if (auto pk_dest = std::get_if<PubKeyDestination>(&address)) {
+                    address = PKHash(pk_dest->pubkey);
+                } else {
+                    continue;
+                }
+            }
             result[address].emplace_back(coin);
         }
     }

diff --git a/test/functional/rpc_deriveaddresses.py b/test/functional/rpc_deriveaddresses.py
@@ -42,7 +42,7 @@ def run_test(self):
         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"])
 
         # Before #26275, bitcoind would crash when deriveaddresses was
         # called with derivation index 2147483647, which is the maximum