From 1de8a2372ab39386e689b27d15c4d029be239319 Mon Sep 17 00:00:00 2001
From: furszy <matiasfurszyfer@protonmail.com>
Date: Fri, 21 Jul 2023 21:37:31 -0300
Subject: [PATCH 01/17] wallet: disallow migration of invalid or not-watched
 scripts

The legacy wallet allowed to import any raw script, without checking if
it was valid or not. Appending it to the watch-only set.

This causes a crash in the migration process because we are only
expecting to find valid scripts inside the legacy spkm.

These stored scripts internally map to `ISMINE_NO` (same as if they
weren't stored at all..).

So we need to check for these special case, and take into account that
the legacy spkm could be storing invalid not watched scripts.

Which, in code words, means IsMineInner() returning IsMineResult::INVALID
for them.
---
 src/wallet/scriptpubkeyman.cpp | 17 ++++++++++++++++-
 src/wallet/scriptpubkeyman.h   |  6 ++++++
 src/wallet/wallet.cpp          | 14 ++++++++++++++
 3 files changed, 36 insertions(+), 1 deletion(-)

diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp
index 5b110b4d149cd..c3c027b8aae76 100644
--- a/src/wallet/scriptpubkeyman.cpp
+++ b/src/wallet/scriptpubkeyman.cpp
@@ -1714,8 +1714,23 @@ std::unordered_set<CScript, SaltedSipHasher> LegacyScriptPubKeyMan::GetScriptPub
     }
 
     // All watchonly scripts are raw
-    spks.insert(setWatchOnly.begin(), setWatchOnly.end());
+    for (const CScript& script : setWatchOnly) {
+        // As the legacy wallet allowed to import any script, we need to verify the validity here.
+        // LegacyScriptPubKeyMan::IsMine() return 'ISMINE_NO' for invalid or not watched scripts (IsMineResult::INVALID or IsMineResult::NO).
+        // e.g. a "sh(sh(pkh()))" which legacy wallets allowed to import!.
+        if (IsMine(script) != ISMINE_NO) spks.insert(script);
+    }
+
+    return spks;
+}
 
+std::unordered_set<CScript, SaltedSipHasher> LegacyScriptPubKeyMan::GetNotMineScriptPubKeys() const
+{
+    LOCK(cs_KeyStore);
+    std::unordered_set<CScript, SaltedSipHasher> spks;
+    for (const CScript& script : setWatchOnly) {
+        if (IsMine(script) == ISMINE_NO) spks.insert(script);
+    }
     return spks;
 }
 
diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h
index bf35c776ae183..3c89a850cfb5a 100644
--- a/src/wallet/scriptpubkeyman.h
+++ b/src/wallet/scriptpubkeyman.h
@@ -523,6 +523,12 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv
     std::set<CKeyID> GetKeys() const override;
     std::unordered_set<CScript, SaltedSipHasher> GetScriptPubKeys() const override;
 
+    /**
+     * Retrieves scripts that were imported by bugs into the legacy spkm and are
+     * simply invalid, such as a sh(sh(pkh())) script, or not watched.
+     */
+    std::unordered_set<CScript, SaltedSipHasher> GetNotMineScriptPubKeys() const;
+
     /** Get the DescriptorScriptPubKeyMans (with private keys) that have the same scriptPubKeys as this LegacyScriptPubKeyMan.
      * Does not modify this ScriptPubKeyMan. */
     std::optional<MigrationData> MigrateToDescriptor();
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 8fa93b97d6557..50f54e82fb989 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -3920,6 +3920,13 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error)
         return false;
     }
 
+    // Get all invalid or non-watched scripts that will not be migrated
+    std::set<CTxDestination> not_migrated_dests;
+    for (const auto& script : legacy_spkm->GetNotMineScriptPubKeys()) {
+        CTxDestination dest;
+        if (ExtractDestination(script, dest)) not_migrated_dests.emplace(dest);
+    }
+
     for (auto& desc_spkm : data.desc_spkms) {
         if (m_spk_managers.count(desc_spkm->GetID()) > 0) {
             error = _("Error: Duplicate descriptors created during migration. Your wallet may be corrupted.");
@@ -4026,6 +4033,13 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error)
                         continue;
                     }
                 }
+
+                // Skip invalid/non-watched scripts that will not be migrated
+                if (not_migrated_dests.count(addr_pair.first) > 0) {
+                    dests_to_delete.push_back(addr_pair.first);
+                    continue;
+                }
+
                 // Not ours, not in watchonly wallet, and not in solvable
                 error = _("Error: Address book data in wallet cannot be identified to belong to migrated wallets");
                 return false;

From 8e7e3e614955e60d3bf9e9a481ef8916bf9e22d9 Mon Sep 17 00:00:00 2001
From: furszy <matiasfurszyfer@protonmail.com>
Date: Fri, 21 Jul 2023 21:43:30 -0300
Subject: [PATCH 02/17] test: wallet, verify migration doesn't crash for an
 invalid script

The migration process must skip any invalid script inside the legacy
spkm and all the addressbook records linked to them.

These scripts are not being watched by the current wallet, nor should
be watched by the migrated one.

IsMine() returns ISMINE_NO for them.
---
 test/functional/wallet_migration.py | 30 +++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py
index 925376e8cd649..b4ef34de6ebcf 100755
--- a/test/functional/wallet_migration.py
+++ b/test/functional/wallet_migration.py
@@ -6,6 +6,7 @@
 
 import random
 import shutil
+from test_framework.address import script_to_p2sh
 from test_framework.descriptors import descsum_create
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.messages import COIN, CTransaction, CTxOut
@@ -674,6 +675,21 @@ def send_to_script(script, amount):
         wallet.rpc.importaddress(address=script_wsh_pkh.hex(), label="raw_spk2", rescan=True, p2sh=False)
         assert_equal(wallet.getbalances()['watchonly']['trusted'], 5)
 
+        # Import sh(pkh()) script, by using importaddress(), with the p2sh flag enabled.
+        # This will wrap the script under another sh level, which is invalid!, and store it inside the wallet.
+        # The migration process must skip the invalid scripts and the addressbook records linked to them.
+        # They are not being watched by the current wallet, nor should be watched by the migrated one.
+        label_sh_pkh = "raw_sh_pkh"
+        script_pkh = key_to_p2pkh_script(df_wallet.getaddressinfo(df_wallet.getnewaddress())["pubkey"])
+        script_sh_pkh = script_to_p2sh_script(script_pkh)
+        addy_script_sh_pkh = script_to_p2sh(script_pkh)  # valid script address
+        addy_script_double_sh_pkh = script_to_p2sh(script_sh_pkh)  # invalid script address
+
+        # Note: 'importaddress()' will add two scripts, a valid one sh(pkh()) and an invalid one 'sh(sh(pkh()))'.
+        #       Both of them will be stored with the same addressbook label. And only the latter one should
+        #       be discarded during migration. The first one must be migrated.
+        wallet.rpc.importaddress(address=script_sh_pkh.hex(), label=label_sh_pkh, rescan=False, p2sh=True)
+
         # Migrate wallet and re-check balance
         info_migration = wallet.migratewallet()
         wallet_wo = self.nodes[0].get_wallet_rpc(info_migration["watchonly_name"])
@@ -683,6 +699,20 @@ def send_to_script(script, amount):
         # The watch-only scripts are no longer part of the main wallet
         assert_equal(wallet.getbalances()['mine']['trusted'], 0)
 
+        # The invalid sh(sh(pk())) script label must not be part of the main wallet anymore
+        assert label_sh_pkh not in wallet.listlabels()
+        # But, the standard sh(pkh()) script should be part of the watch-only wallet.
+        addrs_by_label = wallet_wo.getaddressesbylabel(label_sh_pkh)
+        assert addy_script_sh_pkh in addrs_by_label
+        assert addy_script_double_sh_pkh not in addrs_by_label
+
+        # Also, the watch-only wallet should have the descriptor for the standard sh(pkh())
+        desc = descsum_create(f"addr({addy_script_sh_pkh})")
+        assert next(it['desc'] for it in wallet_wo.listdescriptors()['descriptors'] if it['desc'] == desc)
+        # And doesn't have a descriptor for the invalid one
+        desc_invalid = descsum_create(f"addr({addy_script_double_sh_pkh})")
+        assert_equal(next((it['desc'] for it in wallet_wo.listdescriptors()['descriptors'] if it['desc'] == desc_invalid), None), None)
+
         # Just in case, also verify wallet restart
         self.nodes[0].unloadwallet(info_migration["watchonly_name"])
         self.nodes[0].loadwallet(info_migration["watchonly_name"])

From 8dd067088d41f021b357d7db5fa5f0a9f61edddc Mon Sep 17 00:00:00 2001
From: Andrew Chow <github@achow101.com>
Date: Tue, 5 Sep 2023 12:03:46 -0400
Subject: [PATCH 03/17] Make WitnessUnknown members private

Make sure that nothing else can change WitnessUnknown's data members by
making them private. Also change the program to use a vector rather than
C-style array.
---
 src/addresstype.cpp                |  8 ++------
 src/addresstype.h                  | 26 +++++++++++++++-----------
 src/key_io.cpp                     | 15 ++++++---------
 src/rpc/util.cpp                   |  4 ++--
 src/test/fuzz/key.cpp              |  2 +-
 src/test/fuzz/util.cpp             | 12 ++++--------
 src/test/script_standard_tests.cpp |  5 +----
 src/util/message.cpp               |  2 +-
 8 files changed, 32 insertions(+), 42 deletions(-)

diff --git a/src/addresstype.cpp b/src/addresstype.cpp
index 2454cfb5d9519..349b50f0c934d 100644
--- a/src/addresstype.cpp
+++ b/src/addresstype.cpp
@@ -87,11 +87,7 @@ bool ExtractDestination(const CScript& scriptPubKey, CTxDestination& addressRet)
         return true;
     }
     case TxoutType::WITNESS_UNKNOWN: {
-        WitnessUnknown unk;
-        unk.version = vSolutions[0][0];
-        std::copy(vSolutions[1].begin(), vSolutions[1].end(), unk.program);
-        unk.length = vSolutions[1].size();
-        addressRet = unk;
+        addressRet = WitnessUnknown{vSolutions[0][0], vSolutions[1]};
         return true;
     }
     case TxoutType::MULTISIG:
@@ -138,7 +134,7 @@ class CScriptVisitor
 
     CScript operator()(const WitnessUnknown& id) const
     {
-        return CScript() << CScript::EncodeOP_N(id.version) << std::vector<unsigned char>(id.program, id.program + id.length);
+        return CScript() << CScript::EncodeOP_N(id.GetWitnessVersion()) << id.GetWitnessProgram();
     }
 };
 } // namespace
diff --git a/src/addresstype.h b/src/addresstype.h
index 6b651e9014019..308840e6aea7f 100644
--- a/src/addresstype.h
+++ b/src/addresstype.h
@@ -69,22 +69,26 @@ struct WitnessV1Taproot : public XOnlyPubKey
 //! CTxDestination subtype to encode any future Witness version
 struct WitnessUnknown
 {
-    unsigned int version;
-    unsigned int length;
-    unsigned char program[40];
+private:
+    unsigned int m_version;
+    std::vector<unsigned char> m_program;
+
+public:
+    WitnessUnknown(unsigned int version, const std::vector<unsigned char>& program) : m_version(version), m_program(program) {}
+    WitnessUnknown(int version, const std::vector<unsigned char>& program) : m_version(static_cast<unsigned int>(version)), m_program(program) {}
+
+    unsigned int GetWitnessVersion() const { return m_version; }
+    const std::vector<unsigned char>& GetWitnessProgram() const LIFETIMEBOUND { return m_program; }
 
     friend bool operator==(const WitnessUnknown& w1, const WitnessUnknown& w2) {
-        if (w1.version != w2.version) return false;
-        if (w1.length != w2.length) return false;
-        return std::equal(w1.program, w1.program + w1.length, w2.program);
+        if (w1.GetWitnessVersion() != w2.GetWitnessVersion()) return false;
+        return w1.GetWitnessProgram() == w2.GetWitnessProgram();
     }
 
     friend bool operator<(const WitnessUnknown& w1, const WitnessUnknown& w2) {
-        if (w1.version < w2.version) return true;
-        if (w1.version > w2.version) return false;
-        if (w1.length < w2.length) return true;
-        if (w1.length > w2.length) return false;
-        return std::lexicographical_compare(w1.program, w1.program + w1.length, w2.program, w2.program + w2.length);
+        if (w1.GetWitnessVersion() < w2.GetWitnessVersion()) return true;
+        if (w1.GetWitnessVersion() > w2.GetWitnessVersion()) return false;
+        return w1.GetWitnessProgram() < w2.GetWitnessProgram();
     }
 };
 
diff --git a/src/key_io.cpp b/src/key_io.cpp
index a06116561396a..96dc01550c0ba 100644
--- a/src/key_io.cpp
+++ b/src/key_io.cpp
@@ -66,12 +66,13 @@ class DestinationEncoder
 
     std::string operator()(const WitnessUnknown& id) const
     {
-        if (id.version < 1 || id.version > 16 || id.length < 2 || id.length > 40) {
+        const std::vector<unsigned char>& program = id.GetWitnessProgram();
+        if (id.GetWitnessVersion() < 1 || id.GetWitnessVersion() > 16 || program.size() < 2 || program.size() > 40) {
             return {};
         }
-        std::vector<unsigned char> data = {(unsigned char)id.version};
-        data.reserve(1 + (id.length * 8 + 4) / 5);
-        ConvertBits<8, 5, true>([&](unsigned char c) { data.push_back(c); }, id.program, id.program + id.length);
+        std::vector<unsigned char> data = {(unsigned char)id.GetWitnessVersion()};
+        data.reserve(1 + (program.size() * 8 + 4) / 5);
+        ConvertBits<8, 5, true>([&](unsigned char c) { data.push_back(c); }, program.begin(), program.end());
         return bech32::Encode(bech32::Encoding::BECH32M, m_params.Bech32HRP(), data);
     }
 
@@ -188,11 +189,7 @@ CTxDestination DecodeDestination(const std::string& str, const CChainParams& par
                 return CNoDestination();
             }
 
-            WitnessUnknown unk;
-            unk.version = version;
-            std::copy(data.begin(), data.end(), unk.program);
-            unk.length = data.size();
-            return unk;
+            return WitnessUnknown{version, data};
         } else {
             error_str = strprintf("Invalid padding in Bech32 data section");
             return CNoDestination();
diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp
index 74ef04033e3be..e5ee6d7496272 100644
--- a/src/rpc/util.cpp
+++ b/src/rpc/util.cpp
@@ -303,8 +303,8 @@ class DescribeAddressVisitor
     {
         UniValue obj(UniValue::VOBJ);
         obj.pushKV("iswitness", true);
-        obj.pushKV("witness_version", (int)id.version);
-        obj.pushKV("witness_program", HexStr({id.program, id.length}));
+        obj.pushKV("witness_version", id.GetWitnessVersion());
+        obj.pushKV("witness_program", HexStr(id.GetWitnessProgram()));
         return obj;
     }
 };
diff --git a/src/test/fuzz/key.cpp b/src/test/fuzz/key.cpp
index 60f4081432a90..be45443172f46 100644
--- a/src/test/fuzz/key.cpp
+++ b/src/test/fuzz/key.cpp
@@ -186,7 +186,7 @@ FUZZ_TARGET(key, .init = initialize_key)
         const CTxDestination tx_destination = GetDestinationForKey(pubkey, output_type);
         assert(output_type == OutputType::LEGACY);
         assert(IsValidDestination(tx_destination));
-        assert(CTxDestination{PKHash{pubkey}} == tx_destination);
+        assert(PKHash{pubkey} == *std::get_if<PKHash>(&tx_destination));
 
         const CScript script_for_destination = GetScriptForDestination(tx_destination);
         assert(script_for_destination.size() == 25);
diff --git a/src/test/fuzz/util.cpp b/src/test/fuzz/util.cpp
index ca2218e94cd4d..201495a47e4dc 100644
--- a/src/test/fuzz/util.cpp
+++ b/src/test/fuzz/util.cpp
@@ -188,15 +188,11 @@ CTxDestination ConsumeTxDestination(FuzzedDataProvider& fuzzed_data_provider) no
             tx_destination = WitnessV1Taproot{XOnlyPubKey{ConsumeUInt256(fuzzed_data_provider)}};
         },
         [&] {
-            WitnessUnknown witness_unknown{};
-            witness_unknown.version = fuzzed_data_provider.ConsumeIntegralInRange(2, 16);
-            std::vector<uint8_t> witness_unknown_program_1{fuzzed_data_provider.ConsumeBytes<uint8_t>(40)};
-            if (witness_unknown_program_1.size() < 2) {
-                witness_unknown_program_1 = {0, 0};
+            std::vector<unsigned char> program{ConsumeRandomLengthByteVector(fuzzed_data_provider, /*max_length=*/40)};
+            if (program.size() < 2) {
+                program = {0, 0};
             }
-            witness_unknown.length = witness_unknown_program_1.size();
-            std::copy(witness_unknown_program_1.begin(), witness_unknown_program_1.end(), witness_unknown.program);
-            tx_destination = witness_unknown;
+            tx_destination = WitnessUnknown{fuzzed_data_provider.ConsumeIntegralInRange<unsigned int>(2, 16), program};
         })};
     Assert(call_size == std::variant_size_v<CTxDestination>);
     return tx_destination;
diff --git a/src/test/script_standard_tests.cpp b/src/test/script_standard_tests.cpp
index 1a205728d6e08..c3d5990e01b4c 100644
--- a/src/test/script_standard_tests.cpp
+++ b/src/test/script_standard_tests.cpp
@@ -249,10 +249,7 @@ BOOST_AUTO_TEST_CASE(script_standard_ExtractDestination)
     s.clear();
     s << OP_1 << ToByteVector(pubkey);
     BOOST_CHECK(ExtractDestination(s, address));
-    WitnessUnknown unk;
-    unk.length = 33;
-    unk.version = 1;
-    std::copy(pubkey.begin(), pubkey.end(), unk.program);
+    WitnessUnknown unk{1, ToByteVector(pubkey)};
     BOOST_CHECK(std::get<WitnessUnknown>(address) == unk);
 }
 
diff --git a/src/util/message.cpp b/src/util/message.cpp
index ec845aeffbdd7..1afb28cc10dc5 100644
--- a/src/util/message.cpp
+++ b/src/util/message.cpp
@@ -47,7 +47,7 @@ MessageVerificationResult MessageVerify(
         return MessageVerificationResult::ERR_PUBKEY_NOT_RECOVERED;
     }
 
-    if (!(CTxDestination(PKHash(pubkey)) == destination)) {
+    if (!(PKHash(pubkey) == *std::get_if<PKHash>(&destination))) {
         return MessageVerificationResult::ERR_NOT_SIGNED;
     }
 

From 1a98a51c666e9ae77364115775ec2e0ba984e8e0 Mon Sep 17 00:00:00 2001
From: Andrew Chow <github@achow101.com>
Date: Wed, 9 Aug 2023 09:31:50 -0400
Subject: [PATCH 04/17] Allow CNoDestination to represent a raw script

Even if a script is not a standard destination type, it can still be
useful to have a CTxDestination that stores the script.
---
 src/addresstype.cpp |  3 ++-
 src/addresstype.h   | 39 ++++++++++++++++++++++++++-------------
 2 files changed, 28 insertions(+), 14 deletions(-)

diff --git a/src/addresstype.cpp b/src/addresstype.cpp
index 349b50f0c934d..78df1c13a4ebd 100644
--- a/src/addresstype.cpp
+++ b/src/addresstype.cpp
@@ -93,6 +93,7 @@ bool ExtractDestination(const CScript& scriptPubKey, CTxDestination& addressRet)
     case TxoutType::MULTISIG:
     case TxoutType::NULL_DATA:
     case TxoutType::NONSTANDARD:
+        addressRet = CNoDestination(scriptPubKey);
         return false;
     } // no default case, so the compiler can warn about missing cases
     assert(false);
@@ -104,7 +105,7 @@ class CScriptVisitor
 public:
     CScript operator()(const CNoDestination& dest) const
     {
-        return CScript();
+        return dest.GetScript();
     }
 
     CScript operator()(const PKHash& keyID) const
diff --git a/src/addresstype.h b/src/addresstype.h
index 308840e6aea7f..a41d3bda9ef9f 100644
--- a/src/addresstype.h
+++ b/src/addresstype.h
@@ -14,9 +14,17 @@
 #include <algorithm>
 
 class CNoDestination {
+private:
+    CScript m_script;
+
 public:
-    friend bool operator==(const CNoDestination &a, const CNoDestination &b) { return true; }
-    friend bool operator<(const CNoDestination &a, const CNoDestination &b) { return true; }
+    CNoDestination() = default;
+    CNoDestination(const CScript& script) : m_script(script) {}
+
+    const CScript& GetScript() const { return m_script; }
+
+    friend bool operator==(const CNoDestination& a, const CNoDestination& b) { return a.GetScript() == b.GetScript(); }
+    friend bool operator<(const CNoDestination& a, const CNoDestination& b) { return a.GetScript() < b.GetScript(); }
 };
 
 struct PKHash : public BaseHash<uint160>
@@ -93,14 +101,14 @@ struct WitnessUnknown
 };
 
 /**
- * A txout script template with a specific destination. It is either:
- *  * CNoDestination: no destination set
- *  * PKHash: TxoutType::PUBKEYHASH destination (P2PKH)
- *  * ScriptHash: TxoutType::SCRIPTHASH destination (P2SH)
- *  * WitnessV0ScriptHash: TxoutType::WITNESS_V0_SCRIPTHASH destination (P2WSH)
- *  * WitnessV0KeyHash: TxoutType::WITNESS_V0_KEYHASH destination (P2WPKH)
- *  * WitnessV1Taproot: TxoutType::WITNESS_V1_TAPROOT destination (P2TR)
- *  * WitnessUnknown: TxoutType::WITNESS_UNKNOWN destination (P2W???)
+ * A txout script categorized into standard templates.
+ *  * CNoDestination: Optionally a script, no corresponding address.
+ *  * PKHash: TxoutType::PUBKEYHASH destination (P2PKH address)
+ *  * ScriptHash: TxoutType::SCRIPTHASH destination (P2SH address)
+ *  * WitnessV0ScriptHash: TxoutType::WITNESS_V0_SCRIPTHASH destination (P2WSH address)
+ *  * WitnessV0KeyHash: TxoutType::WITNESS_V0_KEYHASH destination (P2WPKH address)
+ *  * WitnessV1Taproot: TxoutType::WITNESS_V1_TAPROOT destination (P2TR address)
+ *  * WitnessUnknown: TxoutType::WITNESS_UNKNOWN destination (P2W??? address)
  *  A CTxDestination is the internal data type encoded in a bitcoin address
  */
 using CTxDestination = std::variant<CNoDestination, PKHash, ScriptHash, WitnessV0ScriptHash, WitnessV0KeyHash, WitnessV1Taproot, WitnessUnknown>;
@@ -109,9 +117,14 @@ using CTxDestination = std::variant<CNoDestination, PKHash, ScriptHash, WitnessV
 bool IsValidDestination(const CTxDestination& dest);
 
 /**
- * Parse a standard scriptPubKey for the destination address. Assigns result to
- * the addressRet parameter and returns true if successful. Currently only works for P2PK,
- * P2PKH, P2SH, P2WPKH, and P2WSH scripts.
+ * Parse a scriptPubKey for the destination.
+ *
+ * For standard scripts that have addresses (and P2PK as an exception), a corresponding CTxDestination
+ * 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.
  */
 bool ExtractDestination(const CScript& scriptPubKey, CTxDestination& addressRet);
 

From 07d3bdf4ebc06825ea24ab6f7c87aef6a22238c6 Mon Sep 17 00:00:00 2001
From: Andrew Chow <github@achow101.com>
Date: Wed, 9 Aug 2023 09:46:25 -0400
Subject: [PATCH 05/17] 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 78df1c13a4ebd..f199d1b479446 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 @@ class CScriptVisitor
         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 @@ class CScriptVisitor
         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 a41d3bda9ef9f..d3422c68130f3 100644
--- a/src/addresstype.h
+++ b/src/addresstype.h
@@ -27,6 +27,19 @@ class CNoDestination {
     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<uint160>
 {
     PKHash() : BaseHash() {}
@@ -103,6 +116,7 @@ struct WitnessUnknown
 /**
  * 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 @@ struct WitnessUnknown
  *  * WitnessUnknown: TxoutType::WITNESS_UNKNOWN destination (P2W??? address)
  *  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. */
+/** 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 96dc01550c0ba..1a43b5846f667 100644
--- a/src/key_io.cpp
+++ b/src/key_io.cpp
@@ -77,6 +77,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
index 4dd424fa1474b..f9343f48a897e 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<PubKeyDestination>(&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 e5ee6d7496272..9a941be181624 100644
--- 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
index acc82f55f6c13..fe41a8c6ae4c6 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<PubKeyDestination>(&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 201495a47e4dc..87ca2f6aede71 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 c3d5990e01b4c..58bdb37b7c8c7 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<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
index c1b99a4f97e45..e9b93afc30b6c 100644
--- 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
index 750b6c100b758..5d2c299a69c1d 100644
--- a/src/wallet/spend.cpp
+++ b/src/wallet/spend.cpp
@@ -490,8 +490,15 @@ 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)) {
+                // For backwards compatibility, we convert P2PK output scripts into PKHash destinations
+                if (auto pk_dest = std::get_if<PubKeyDestination>(&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 e96b6bda90613..64994d6bb3086 100755
--- a/test/functional/rpc_deriveaddresses.py
+++ b/test/functional/rpc_deriveaddresses.py
@@ -42,7 +42,10 @@ 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"])
+
+        # 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

From ad0c469d98c51931b98b7fd937c6ac3eeaed024e Mon Sep 17 00:00:00 2001
From: Andrew Chow <github@achow101.com>
Date: Wed, 9 Aug 2023 09:42:04 -0400
Subject: [PATCH 06/17] wallet: Use CTxDestination in CRecipient rather than
 scriptPubKey

---
 src/wallet/feebumper.cpp         | 10 +++++-----
 src/wallet/rpc/spend.cpp         |  3 +--
 src/wallet/spend.cpp             |  6 ++++--
 src/wallet/test/spend_tests.cpp  |  2 +-
 src/wallet/test/wallet_tests.cpp |  2 +-
 src/wallet/wallet.cpp            | 10 ++++------
 src/wallet/wallet.h              |  2 +-
 7 files changed, 17 insertions(+), 18 deletions(-)

diff --git a/src/wallet/feebumper.cpp b/src/wallet/feebumper.cpp
index 3720d144ebef5..d5ad638aa83e8 100644
--- a/src/wallet/feebumper.cpp
+++ b/src/wallet/feebumper.cpp
@@ -247,12 +247,12 @@ Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCo
     const auto& txouts = outputs.empty() ? wtx.tx->vout : outputs;
     for (size_t i = 0; i < txouts.size(); ++i) {
         const CTxOut& output = txouts.at(i);
+        CTxDestination dest;
+        ExtractDestination(output.scriptPubKey, dest);
         if (reduce_output.has_value() ?  reduce_output.value() == i : OutputIsChange(wallet, output)) {
-            CTxDestination change_dest;
-            ExtractDestination(output.scriptPubKey, change_dest);
-            new_coin_control.destChange = change_dest;
+            new_coin_control.destChange = dest;
         } else {
-            CRecipient recipient = {output.scriptPubKey, output.nValue, false};
+            CRecipient recipient = {dest, output.nValue, false};
             recipients.push_back(recipient);
         }
         new_outputs_value += output.nValue;
@@ -268,7 +268,7 @@ Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCo
 
         // Add change as recipient with SFFO flag enabled, so fees are deduced from it.
         // If the output differs from the original tx output (because the user customized it) a new change output will be created.
-        recipients.emplace_back(CRecipient{GetScriptForDestination(new_coin_control.destChange), new_outputs_value, /*fSubtractFeeFromAmount=*/true});
+        recipients.emplace_back(CRecipient{new_coin_control.destChange, new_outputs_value, /*fSubtractFeeFromAmount=*/true});
         new_coin_control.destChange = CNoDestination();
     }
 
diff --git a/src/wallet/rpc/spend.cpp b/src/wallet/rpc/spend.cpp
index 0c2be26ddfa1c..a82d560a5a36d 100644
--- a/src/wallet/rpc/spend.cpp
+++ b/src/wallet/rpc/spend.cpp
@@ -39,7 +39,6 @@ static void ParseRecipients(const UniValue& address_amounts, const UniValue& sub
         }
         destinations.insert(dest);
 
-        CScript script_pub_key = GetScriptForDestination(dest);
         CAmount amount = AmountFromValue(address_amounts[i++]);
 
         bool subtract_fee = false;
@@ -50,7 +49,7 @@ static void ParseRecipients(const UniValue& address_amounts, const UniValue& sub
             }
         }
 
-        CRecipient recipient = {script_pub_key, amount, subtract_fee};
+        CRecipient recipient = {dest, amount, subtract_fee};
         recipients.push_back(recipient);
     }
 }
diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp
index 5d2c299a69c1d..45f0ece860f58 100644
--- a/src/wallet/spend.cpp
+++ b/src/wallet/spend.cpp
@@ -1044,7 +1044,7 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal(
     // vouts to the payees
     for (const auto& recipient : vecSend)
     {
-        CTxOut txout(recipient.nAmount, recipient.scriptPubKey);
+        CTxOut txout(recipient.nAmount, GetScriptForDestination(recipient.dest));
 
         // Include the fee cost for outputs.
         coin_selection_params.tx_noinputs_size += ::GetSerializeSize(txout, PROTOCOL_VERSION);
@@ -1292,7 +1292,9 @@ bool FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& nFeeRet,
     // Turn the txout set into a CRecipient vector.
     for (size_t idx = 0; idx < tx.vout.size(); idx++) {
         const CTxOut& txOut = tx.vout[idx];
-        CRecipient recipient = {txOut.scriptPubKey, txOut.nValue, setSubtractFeeFromOutputs.count(idx) == 1};
+        CTxDestination dest;
+        ExtractDestination(txOut.scriptPubKey, dest);
+        CRecipient recipient = {dest, txOut.nValue, setSubtractFeeFromOutputs.count(idx) == 1};
         vecSend.push_back(recipient);
     }
 
diff --git a/src/wallet/test/spend_tests.cpp b/src/wallet/test/spend_tests.cpp
index eca1d74cf637b..68c98ae6b9ebe 100644
--- a/src/wallet/test/spend_tests.cpp
+++ b/src/wallet/test/spend_tests.cpp
@@ -27,7 +27,7 @@ BOOST_FIXTURE_TEST_CASE(SubtractFee, TestChain100Setup)
     // leftover input amount which would have been change to the recipient
     // instead of the miner.
     auto check_tx = [&wallet](CAmount leftover_input_amount) {
-        CRecipient recipient{GetScriptForRawPubKey({}), 50 * COIN - leftover_input_amount, /*subtract_fee=*/true};
+        CRecipient recipient{PubKeyDestination({}), 50 * COIN - leftover_input_amount, /*subtract_fee=*/true};
         constexpr int RANDOM_CHANGE_POSITION = -1;
         CCoinControl coin_control;
         coin_control.m_feerate.emplace(10000);
diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp
index 5c297d76e4881..dac6e87983e54 100644
--- a/src/wallet/test/wallet_tests.cpp
+++ b/src/wallet/test/wallet_tests.cpp
@@ -645,7 +645,7 @@ void TestCoinsResult(ListCoinsTest& context, OutputType out_type, CAmount amount
 {
     LOCK(context.wallet->cs_wallet);
     util::Result<CTxDestination> dest = Assert(context.wallet->GetNewDestination(out_type, ""));
-    CWalletTx& wtx = context.AddTx(CRecipient{{GetScriptForDestination(*dest)}, amount, /*fSubtractFeeFromAmount=*/true});
+    CWalletTx& wtx = context.AddTx(CRecipient{*dest, amount, /*fSubtractFeeFromAmount=*/true});
     CoinFilterParams filter;
     filter.skip_locked = false;
     CoinsResult available_coins = AvailableCoins(*context.wallet, nullptr, std::nullopt, filter);
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 6f5248efafbd2..9f65db6c7b9ec 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -2213,15 +2213,13 @@ OutputType CWallet::TransactionChangeType(const std::optional<OutputType>& chang
     bool any_pkh{false};
 
     for (const auto& recipient : vecSend) {
-        std::vector<std::vector<uint8_t>> dummy;
-        const TxoutType type{Solver(recipient.scriptPubKey, dummy)};
-        if (type == TxoutType::WITNESS_V1_TAPROOT) {
+        if (std::get_if<WitnessV1Taproot>(&recipient.dest)) {
             any_tr = true;
-        } else if (type == TxoutType::WITNESS_V0_KEYHASH) {
+        } else if (std::get_if<WitnessV0KeyHash>(&recipient.dest)) {
             any_wpkh = true;
-        } else if (type == TxoutType::SCRIPTHASH) {
+        } else if (std::get_if<ScriptHash>(&recipient.dest)) {
             any_sh = true;
-        } else if (type == TxoutType::PUBKEYHASH) {
+        } else if (std::get_if<PKHash>(&recipient.dest)) {
             any_pkh = true;
         }
     }
diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
index 091a573151e59..97d06641a7343 100644
--- a/src/wallet/wallet.h
+++ b/src/wallet/wallet.h
@@ -288,7 +288,7 @@ inline std::optional<AddressPurpose> PurposeFromString(std::string_view s)
 
 struct CRecipient
 {
-    CScript scriptPubKey;
+    CTxDestination dest;
     CAmount nAmount;
     bool fSubtractFeeFromAmount;
 };

From 3fcd7fc7ff563bdc0e2bba66b4cbe72d898c876e Mon Sep 17 00:00:00 2001
From: Pieter Wuille <pieter@wuille.net>
Date: Mon, 11 Sep 2023 13:54:32 -0400
Subject: [PATCH 07/17] Do not use std::vector = {} to release memory

---
 src/headerssync.cpp     |  5 +++--
 src/net.cpp             | 23 +++++++++++------------
 src/test/util_tests.cpp | 25 +++++++++++++++++++++++++
 src/util/vector.h       | 18 ++++++++++++++++++
 4 files changed, 57 insertions(+), 14 deletions(-)

diff --git a/src/headerssync.cpp b/src/headerssync.cpp
index a3adfb4f701d9..f891063cd23b1 100644
--- a/src/headerssync.cpp
+++ b/src/headerssync.cpp
@@ -7,6 +7,7 @@
 #include <pow.h>
 #include <timedata.h>
 #include <util/check.h>
+#include <util/vector.h>
 
 // The two constants below are computed using the simulation script on
 // https://gist.github.com/sipa/016ae445c132cdf65a2791534dfb7ae1
@@ -51,9 +52,9 @@ HeadersSyncState::HeadersSyncState(NodeId id, const Consensus::Params& consensus
 void HeadersSyncState::Finalize()
 {
     Assume(m_download_state != State::FINAL);
-    m_header_commitments = {};
+    ClearShrink(m_header_commitments);
     m_last_header_received.SetNull();
-    m_redownloaded_headers = {};
+    ClearShrink(m_redownloaded_headers);
     m_redownload_buffer_last_hash.SetNull();
     m_redownload_buffer_first_prev_hash.SetNull();
     m_process_all_remaining_headers = false;
diff --git a/src/net.cpp b/src/net.cpp
index af2855932decb..13397c3d81964 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -35,6 +35,7 @@
 #include <util/threadinterrupt.h>
 #include <util/trace.h>
 #include <util/translation.h>
+#include <util/vector.h>
 
 #ifdef WIN32
 #include <string.h>
@@ -899,8 +900,7 @@ void V1Transport::MarkBytesSent(size_t bytes_sent) noexcept
         m_bytes_sent = 0;
     } else if (!m_sending_header && m_bytes_sent == m_message_to_send.data.size()) {
         // We're done sending a message's data. Wipe the data vector to reduce memory consumption.
-        m_message_to_send.data.clear();
-        m_message_to_send.data.shrink_to_fit();
+        ClearShrink(m_message_to_send.data);
         m_bytes_sent = 0;
     }
 }
@@ -1123,8 +1123,8 @@ void V2Transport::ProcessReceivedMaybeV1Bytes() noexcept
         SetReceiveState(RecvState::V1);
         SetSendState(SendState::V1);
         // Reset v2 transport buffers to save memory.
-        m_recv_buffer = {};
-        m_send_buffer = {};
+        ClearShrink(m_recv_buffer);
+        ClearShrink(m_send_buffer);
     } else {
         // We have not received enough to distinguish v1 from v2 yet. Wait until more bytes come.
     }
@@ -1184,8 +1184,7 @@ bool V2Transport::ProcessReceivedKeyBytes() noexcept
             /*ignore=*/false,
             /*output=*/MakeWritableByteSpan(m_send_buffer).last(BIP324Cipher::EXPANSION));
         // We no longer need the garbage.
-        m_send_garbage.clear();
-        m_send_garbage.shrink_to_fit();
+        ClearShrink(m_send_garbage);
 
         // Construct version packet in the send buffer.
         m_send_buffer.resize(m_send_buffer.size() + BIP324Cipher::EXPANSION + VERSION_CONTENTS.size());
@@ -1275,7 +1274,7 @@ bool V2Transport::ProcessReceivedPacketBytes() noexcept
             // Ignore flag does not matter for garbage authentication. Any valid packet functions
             // as authentication. Receive and process the version packet next.
             SetReceiveState(RecvState::VERSION);
-            m_recv_garbage = {};
+            ClearShrink(m_recv_garbage);
             break;
         case RecvState::VERSION:
             if (!ignore) {
@@ -1295,9 +1294,9 @@ bool V2Transport::ProcessReceivedPacketBytes() noexcept
             Assume(false);
         }
         // Wipe the receive buffer where the next packet will be received into.
-        m_recv_buffer = {};
+        ClearShrink(m_recv_buffer);
         // In all but APP_READY state, we can wipe the decoded contents.
-        if (m_recv_state != RecvState::APP_READY) m_recv_decode_buffer = {};
+        if (m_recv_state != RecvState::APP_READY) ClearShrink(m_recv_decode_buffer);
     } else {
         // We either have less than 3 bytes, so we don't know the packet's length yet, or more
         // than 3 bytes but less than the packet's full ciphertext. Wait until those arrive.
@@ -1511,7 +1510,7 @@ CNetMessage V2Transport::GetReceivedMessage(std::chrono::microseconds time, bool
         LogPrint(BCLog::NET, "V2 transport error: invalid message type (%u bytes contents), peer=%d\n", m_recv_decode_buffer.size(), m_nodeid);
         reject_message = true;
     }
-    m_recv_decode_buffer = {};
+    ClearShrink(m_recv_decode_buffer);
     SetReceiveState(RecvState::APP);
 
     return msg;
@@ -1545,7 +1544,7 @@ bool V2Transport::SetMessageToSend(CSerializedNetMsg& msg) noexcept
     m_cipher.Encrypt(MakeByteSpan(contents), {}, false, MakeWritableByteSpan(m_send_buffer));
     m_send_type = msg.m_type;
     // Release memory
-    msg.data = {};
+    ClearShrink(msg.data);
     return true;
 }
 
@@ -1577,7 +1576,7 @@ void V2Transport::MarkBytesSent(size_t bytes_sent) noexcept
     // Wipe the buffer when everything is sent.
     if (m_send_pos == m_send_buffer.size()) {
         m_send_pos = 0;
-        m_send_buffer = {};
+        ClearShrink(m_send_buffer);
     }
 }
 
diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp
index 26677bfa55ee6..67f71bd266dd0 100644
--- a/src/test/util_tests.cpp
+++ b/src/test/util_tests.cpp
@@ -1791,4 +1791,29 @@ BOOST_AUTO_TEST_CASE(util_WriteBinaryFile)
     BOOST_CHECK(valid);
     BOOST_CHECK_EQUAL(actual_text, expected_text);
 }
+
+BOOST_AUTO_TEST_CASE(clearshrink_test)
+{
+    {
+        std::vector<uint8_t> v = {1, 2, 3};
+        ClearShrink(v);
+        BOOST_CHECK_EQUAL(v.size(), 0);
+        BOOST_CHECK_EQUAL(v.capacity(), 0);
+    }
+
+    {
+        std::vector<bool> v = {false, true, false, false, true, true};
+        ClearShrink(v);
+        BOOST_CHECK_EQUAL(v.size(), 0);
+        BOOST_CHECK_EQUAL(v.capacity(), 0);
+    }
+
+    {
+        std::deque<int> v = {1, 3, 3, 7};
+        ClearShrink(v);
+        BOOST_CHECK_EQUAL(v.size(), 0);
+        // std::deque has no capacity() we can observe.
+    }
+}
+
 BOOST_AUTO_TEST_SUITE_END()
diff --git a/src/util/vector.h b/src/util/vector.h
index 9b9218e54ff7e..40ff73c293c17 100644
--- a/src/util/vector.h
+++ b/src/util/vector.h
@@ -49,4 +49,22 @@ inline V Cat(V v1, const V& v2)
     return v1;
 }
 
+/** Clear a vector (or std::deque) and release its allocated memory. */
+template<typename V>
+inline void ClearShrink(V& v) noexcept
+{
+    // There are various ways to clear a vector and release its memory:
+    //
+    // 1. V{}.swap(v)
+    // 2. v = V{}
+    // 3. v = {}; v.shrink_to_fit();
+    // 4. v.clear(); v.shrink_to_fit();
+    //
+    // (2) does not appear to release memory in glibc debug mode, even if v.shrink_to_fit()
+    // follows. (3) and (4) rely on std::vector::shrink_to_fit, which is only a non-binding
+    // request. Therefore, we use method (1).
+
+    V{}.swap(v);
+}
+
 #endif // BITCOIN_UTIL_VECTOR_H

From bf147bfffa1afb11721f30e83eec1fa829f64d5f Mon Sep 17 00:00:00 2001
From: Anthony Towns <aj@erisian.com.au>
Date: Thu, 14 Sep 2023 10:00:45 +1000
Subject: [PATCH 08/17] serialize: move ser_action functions out of global
 namespace

---
 src/serialize.h | 106 ++++++++++++++++++++++++------------------------
 1 file changed, 54 insertions(+), 52 deletions(-)

diff --git a/src/serialize.h b/src/serialize.h
index 2d790190a0be1..de37fddf29208 100644
--- a/src/serialize.h
+++ b/src/serialize.h
@@ -167,9 +167,9 @@ const Out& AsBase(const In& x)
     return x;
 }
 
-#define READWRITE(...) (::SerReadWriteMany(s, ser_action, __VA_ARGS__))
-#define SER_READ(obj, code) ::SerRead(s, ser_action, obj, [&](Stream& s, typename std::remove_const<Type>::type& obj) { code; })
-#define SER_WRITE(obj, code) ::SerWrite(s, ser_action, obj, [&](Stream& s, const Type& obj) { code; })
+#define READWRITE(...) (ser_action.SerReadWriteMany(s, __VA_ARGS__))
+#define SER_READ(obj, code) ser_action.SerRead(s, obj, [&](Stream& s, typename std::remove_const<Type>::type& obj) { code; })
+#define SER_WRITE(obj, code) ser_action.SerWrite(s, obj, [&](Stream& s, const Type& obj) { code; })
 
 /**
  * Implement the Ser and Unser methods needed for implementing a formatter (see Using below).
@@ -1006,17 +1006,65 @@ void Unserialize(Stream& is, std::shared_ptr<const T>& p)
     p = std::make_shared<const T>(deserialize, is);
 }
 
+/**
+ * Support for (un)serializing many things at once
+ */
+
+template <typename Stream, typename... Args>
+void SerializeMany(Stream& s, const Args&... args)
+{
+    (::Serialize(s, args), ...);
+}
+
+template <typename Stream, typename... Args>
+inline void UnserializeMany(Stream& s, Args&&... args)
+{
+    (::Unserialize(s, args), ...);
+}
 
 /**
  * Support for all macros providing or using the ser_action parameter of the SerializationOps method.
  */
 struct ActionSerialize {
-    constexpr bool ForRead() const { return false; }
+    static constexpr bool ForRead() { return false; }
+
+    template<typename Stream, typename... Args>
+    static void SerReadWriteMany(Stream& s, const Args&... args)
+    {
+        ::SerializeMany(s, args...);
+    }
+
+    template<typename Stream, typename Type, typename Fn>
+    static void SerRead(Stream& s, Type&&, Fn&&)
+    {
+    }
+
+    template<typename Stream, typename Type, typename Fn>
+    static void SerWrite(Stream& s, Type&& obj, Fn&& fn)
+    {
+        fn(s, std::forward<Type>(obj));
+    }
 };
 struct ActionUnserialize {
-    constexpr bool ForRead() const { return true; }
-};
+    static constexpr bool ForRead() { return true; }
 
+    template<typename Stream, typename... Args>
+    static void SerReadWriteMany(Stream& s, Args&&... args)
+    {
+        ::UnserializeMany(s, args...);
+    }
+
+    template<typename Stream, typename Type, typename Fn>
+    static void SerRead(Stream& s, Type&& obj, Fn&& fn)
+    {
+        fn(s, std::forward<Type>(obj));
+    }
+
+    template<typename Stream, typename Type, typename Fn>
+    static void SerWrite(Stream& s, Type&&, Fn&&)
+    {
+    }
+};
 
 /* ::GetSerializeSize implementations
  *
@@ -1063,52 +1111,6 @@ class CSizeComputer
     int GetVersion() const { return nVersion; }
 };
 
-template <typename Stream, typename... Args>
-void SerializeMany(Stream& s, const Args&... args)
-{
-    (::Serialize(s, args), ...);
-}
-
-template <typename Stream, typename... Args>
-inline void UnserializeMany(Stream& s, Args&&... args)
-{
-    (::Unserialize(s, args), ...);
-}
-
-template<typename Stream, typename... Args>
-inline void SerReadWriteMany(Stream& s, ActionSerialize ser_action, const Args&... args)
-{
-    ::SerializeMany(s, args...);
-}
-
-template<typename Stream, typename... Args>
-inline void SerReadWriteMany(Stream& s, ActionUnserialize ser_action, Args&&... args)
-{
-    ::UnserializeMany(s, args...);
-}
-
-template<typename Stream, typename Type, typename Fn>
-inline void SerRead(Stream& s, ActionSerialize ser_action, Type&&, Fn&&)
-{
-}
-
-template<typename Stream, typename Type, typename Fn>
-inline void SerRead(Stream& s, ActionUnserialize ser_action, Type&& obj, Fn&& fn)
-{
-    fn(s, std::forward<Type>(obj));
-}
-
-template<typename Stream, typename Type, typename Fn>
-inline void SerWrite(Stream& s, ActionSerialize ser_action, Type&& obj, Fn&& fn)
-{
-    fn(s, std::forward<Type>(obj));
-}
-
-template<typename Stream, typename Type, typename Fn>
-inline void SerWrite(Stream& s, ActionUnserialize ser_action, Type&&, Fn&&)
-{
-}
-
 template<typename I>
 inline void WriteVarInt(CSizeComputer &s, I n)
 {

From 33203f59b482bddfe0bbe7d497cb8731ce8334a4 Mon Sep 17 00:00:00 2001
From: Anthony Towns <aj@erisian.com.au>
Date: Thu, 14 Sep 2023 10:05:23 +1000
Subject: [PATCH 09/17] serialize: specify type for ParamsWrapper not ref

---
 src/serialize.h | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/src/serialize.h b/src/serialize.h
index de37fddf29208..7e4788a5ca2b0 100644
--- a/src/serialize.h
+++ b/src/serialize.h
@@ -1161,12 +1161,11 @@ class ParamsStream
 template <typename Params, typename T>
 class ParamsWrapper
 {
-    static_assert(std::is_lvalue_reference<T>::value, "ParamsWrapper needs an lvalue reference type T");
     const Params& m_params;
-    T m_object;
+    T& m_object;
 
 public:
-    explicit ParamsWrapper(const Params& params, T obj) : m_params{params}, m_object{obj} {}
+    explicit ParamsWrapper(const Params& params, T& obj) : m_params{params}, m_object{obj} {}
 
     template <typename Stream>
     void Serialize(Stream& s) const
@@ -1190,7 +1189,7 @@ class ParamsWrapper
 template <typename Params, typename T>
 static auto WithParams(const Params& params, T&& t)
 {
-    return ParamsWrapper<Params, T&>{params, t};
+    return ParamsWrapper<Params, T>{params, t};
 }
 
 #endif // BITCOIN_SERIALIZE_H

From 5e5c8f86b60a8018e8801fb44bbe56ce97d9deef Mon Sep 17 00:00:00 2001
From: Anthony Towns <aj@erisian.com.au>
Date: Mon, 11 Sep 2023 18:36:11 +1000
Subject: [PATCH 10/17] serialize: add SER_PARAMS_OPFUNC

---
 src/netaddress.h |  1 +
 src/protocol.h   |  1 +
 src/serialize.h  | 13 +++++++++++++
 3 files changed, 15 insertions(+)

diff --git a/src/netaddress.h b/src/netaddress.h
index a0944c886fc68..7d3659a11af55 100644
--- a/src/netaddress.h
+++ b/src/netaddress.h
@@ -218,6 +218,7 @@ class CNetAddr
     };
     struct SerParams {
         const Encoding enc;
+        SER_PARAMS_OPFUNC
     };
     static constexpr SerParams V1{Encoding::V1};
     static constexpr SerParams V2{Encoding::V2};
diff --git a/src/protocol.h b/src/protocol.h
index a7ca0c6f3e359..e1caba766ff6e 100644
--- a/src/protocol.h
+++ b/src/protocol.h
@@ -396,6 +396,7 @@ class CAddress : public CService
     };
     struct SerParams : CNetAddr::SerParams {
         const Format fmt;
+        SER_PARAMS_OPFUNC
     };
     static constexpr SerParams V1_NETWORK{{CNetAddr::Encoding::V1}, Format::Network};
     static constexpr SerParams V2_NETWORK{{CNetAddr::Encoding::V2}, Format::Network};
diff --git a/src/serialize.h b/src/serialize.h
index 7e4788a5ca2b0..04b29d1ded2fc 100644
--- a/src/serialize.h
+++ b/src/serialize.h
@@ -1192,4 +1192,17 @@ static auto WithParams(const Params& params, T&& t)
     return ParamsWrapper<Params, T>{params, t};
 }
 
+/**
+ * Helper macro for SerParams structs
+ *
+ * Allows you define SerParams instances and then apply them directly
+ * to an object via function call syntax, eg:
+ *
+ *   constexpr SerParams FOO{....};
+ *   ss << FOO(obj);
+ */
+#define SER_PARAMS_OPFUNC \
+    template <typename T> \
+    auto operator()(T&& t) const { return WithParams(*this, t); }
+
 #endif // BITCOIN_SERIALIZE_H

From fb6a2ab63e310d8b600352ef41aab6dafccfbff0 Mon Sep 17 00:00:00 2001
From: Anthony Towns <aj@erisian.com.au>
Date: Thu, 14 Sep 2023 10:20:49 +1000
Subject: [PATCH 11/17] scripted-diff: use SER_PARAMS_OPFUNC

-BEGIN VERIFY SCRIPT-
sed -i 's/WithParams(\(CAddress::V[12]_[A-Z]*\) *, */\1(/g' $(git grep -l 'WithParams' src/)
sed -i 's/WithParams(\(CNetAddr::V[12]\) *, */\1(/g' $(git grep -l 'WithParams' src/)
sed -i 's@\(CNetAddr::V1.CService{}.*\)    //@\1                //@' src/test/util/net.cpp
-END VERIFY SCRIPT-
---
 src/addrdb.cpp             | 4 ++--
 src/net_processing.cpp     | 6 +++---
 src/test/addrman_tests.cpp | 2 +-
 src/test/fuzz/addrman.cpp  | 2 +-
 src/test/net_tests.cpp     | 4 ++--
 src/test/netbase_tests.cpp | 8 ++++----
 src/test/util/net.cpp      | 4 ++--
 7 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/src/addrdb.cpp b/src/addrdb.cpp
index c5b474339b922..b5e35bf8f6abe 100644
--- a/src/addrdb.cpp
+++ b/src/addrdb.cpp
@@ -216,14 +216,14 @@ util::Result<std::unique_ptr<AddrMan>> LoadAddrman(const NetGroupManager& netgro
 void DumpAnchors(const fs::path& anchors_db_path, const std::vector<CAddress>& anchors)
 {
     LOG_TIME_SECONDS(strprintf("Flush %d outbound block-relay-only peer addresses to anchors.dat", anchors.size()));
-    SerializeFileDB("anchors", anchors_db_path, WithParams(CAddress::V2_DISK, anchors));
+    SerializeFileDB("anchors", anchors_db_path, CAddress::V2_DISK(anchors));
 }
 
 std::vector<CAddress> ReadAnchors(const fs::path& anchors_db_path)
 {
     std::vector<CAddress> anchors;
     try {
-        DeserializeFileDB(anchors_db_path, WithParams(CAddress::V2_DISK, anchors));
+        DeserializeFileDB(anchors_db_path, CAddress::V2_DISK(anchors));
         LogPrintf("Loaded %i addresses from %s\n", anchors.size(), fs::quoted(fs::PathToString(anchors_db_path.filename())));
     } catch (const std::exception&) {
         anchors.clear();
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 6b415b3a1ea63..b046b3ac168c7 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -1415,8 +1415,8 @@ void PeerManagerImpl::PushNodeVersion(CNode& pnode, const Peer& peer)
 
     const bool tx_relay{!RejectIncomingTxs(pnode)};
     m_connman.PushMessage(&pnode, CNetMsgMaker(INIT_PROTO_VERSION).Make(NetMsgType::VERSION, PROTOCOL_VERSION, my_services, nTime,
-            your_services, WithParams(CNetAddr::V1, addr_you), // Together the pre-version-31402 serialization of CAddress "addrYou" (without nTime)
-            my_services, WithParams(CNetAddr::V1, CService{}), // Together the pre-version-31402 serialization of CAddress "addrMe" (without nTime)
+            your_services, CNetAddr::V1(addr_you), // Together the pre-version-31402 serialization of CAddress "addrYou" (without nTime)
+            my_services, CNetAddr::V1(CService{}), // Together the pre-version-31402 serialization of CAddress "addrMe" (without nTime)
             nonce, strSubVersion, nNodeStartingHeight, tx_relay));
 
     if (fLogIPs) {
@@ -3293,7 +3293,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
             nTime = 0;
         }
         vRecv.ignore(8); // Ignore the addrMe service bits sent by the peer
-        vRecv >> WithParams(CNetAddr::V1, addrMe);
+        vRecv >> CNetAddr::V1(addrMe);
         if (!pfrom.IsInboundConn())
         {
             m_addrman.SetServices(pfrom.addr, nServices);
diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp
index 941018a8206cb..b01ba81c5f119 100644
--- a/src/test/addrman_tests.cpp
+++ b/src/test/addrman_tests.cpp
@@ -1019,7 +1019,7 @@ static auto MakeCorruptPeersDat()
     std::optional<CNetAddr> resolved{LookupHost("252.2.2.2", false)};
     BOOST_REQUIRE(resolved.has_value());
     AddrInfo info = AddrInfo(addr, resolved.value());
-    s << WithParams(CAddress::V1_DISK, info);
+    s << CAddress::V1_DISK(info);
 
     return s;
 }
diff --git a/src/test/fuzz/addrman.cpp b/src/test/fuzz/addrman.cpp
index 9611a872ec4f9..1b11ff6fdf3f0 100644
--- a/src/test/fuzz/addrman.cpp
+++ b/src/test/fuzz/addrman.cpp
@@ -83,7 +83,7 @@ CNetAddr RandAddr(FuzzedDataProvider& fuzzed_data_provider, FastRandomContext& f
         s << net;
         s << fast_random_context.randbytes(net_len_map.at(net));
 
-        s >> WithParams(CAddress::V2_NETWORK, addr);
+        s >> CAddress::V2_NETWORK(addr);
     }
 
     // Return a dummy IPv4 5.5.5.5 if we generated an invalid address.
diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp
index 3eb7bdec62f3e..036355b5f1a73 100644
--- a/src/test/net_tests.cpp
+++ b/src/test/net_tests.cpp
@@ -850,7 +850,7 @@ BOOST_AUTO_TEST_CASE(initial_advertise_from_version_message)
     std::chrono::microseconds time_received_dummy{0};
 
     const auto msg_version =
-        msg_maker.Make(NetMsgType::VERSION, PROTOCOL_VERSION, services, time, services, WithParams(CAddress::V1_NETWORK, peer_us));
+        msg_maker.Make(NetMsgType::VERSION, PROTOCOL_VERSION, services, time, services, CAddress::V1_NETWORK(peer_us));
     CDataStream msg_version_stream{msg_version.data, SER_NETWORK, PROTOCOL_VERSION};
 
     m_node.peerman->ProcessMessage(
@@ -876,7 +876,7 @@ BOOST_AUTO_TEST_CASE(initial_advertise_from_version_message)
             DataStream s{data};
             std::vector<CAddress> addresses;
 
-            s >> WithParams(CAddress::V1_NETWORK, addresses);
+            s >> CAddress::V1_NETWORK(addresses);
 
             for (const auto& addr : addresses) {
                 if (addr == expected) {
diff --git a/src/test/netbase_tests.cpp b/src/test/netbase_tests.cpp
index e22bf7e7c0231..74ff531cd9476 100644
--- a/src/test/netbase_tests.cpp
+++ b/src/test/netbase_tests.cpp
@@ -561,7 +561,7 @@ BOOST_AUTO_TEST_CASE(caddress_serialize_v1)
 {
     DataStream s{};
 
-    s << WithParams(CAddress::V1_NETWORK, fixture_addresses);
+    s << CAddress::V1_NETWORK(fixture_addresses);
     BOOST_CHECK_EQUAL(HexStr(s), stream_addrv1_hex);
 }
 
@@ -570,7 +570,7 @@ BOOST_AUTO_TEST_CASE(caddress_unserialize_v1)
     DataStream s{ParseHex(stream_addrv1_hex)};
     std::vector<CAddress> addresses_unserialized;
 
-    s >> WithParams(CAddress::V1_NETWORK, addresses_unserialized);
+    s >> CAddress::V1_NETWORK(addresses_unserialized);
     BOOST_CHECK(fixture_addresses == addresses_unserialized);
 }
 
@@ -578,7 +578,7 @@ BOOST_AUTO_TEST_CASE(caddress_serialize_v2)
 {
     DataStream s{};
 
-    s << WithParams(CAddress::V2_NETWORK, fixture_addresses);
+    s << CAddress::V2_NETWORK(fixture_addresses);
     BOOST_CHECK_EQUAL(HexStr(s), stream_addrv2_hex);
 }
 
@@ -587,7 +587,7 @@ BOOST_AUTO_TEST_CASE(caddress_unserialize_v2)
     DataStream s{ParseHex(stream_addrv2_hex)};
     std::vector<CAddress> addresses_unserialized;
 
-    s >> WithParams(CAddress::V2_NETWORK, addresses_unserialized);
+    s >> CAddress::V2_NETWORK(addresses_unserialized);
     BOOST_CHECK(fixture_addresses == addresses_unserialized);
 }
 
diff --git a/src/test/util/net.cpp b/src/test/util/net.cpp
index dc64c0b4c1bbc..bf5a653090b3d 100644
--- a/src/test/util/net.cpp
+++ b/src/test/util/net.cpp
@@ -33,9 +33,9 @@ void ConnmanTestMsg::Handshake(CNode& node,
                 Using<CustomUintFormatter<8>>(remote_services), //
                 int64_t{},                                      // dummy time
                 int64_t{},                                      // ignored service bits
-                WithParams(CNetAddr::V1, CService{}),           // dummy
+                CNetAddr::V1(CService{}),                       // dummy
                 int64_t{},                                      // ignored service bits
-                WithParams(CNetAddr::V1, CService{}),           // ignored
+                CNetAddr::V1(CService{}),                       // ignored
                 uint64_t{1},                                    // dummy nonce
                 std::string{},                                  // dummy subver
                 int32_t{},                                      // dummy starting_height

From a241d6069cf0542acdd8ec6be63724da19f10720 Mon Sep 17 00:00:00 2001
From: fanquake <fanquake@gmail.com>
Date: Wed, 13 Sep 2023 16:19:14 +0100
Subject: [PATCH 12/17] ci: use LLVM 17.0.0 in MSAN jobs

See https://libcxx.llvm.org/Hardening.html as well as
https://discourse.llvm.org/t/rfc-removing-the-legacy-debug-mode-from-libc/71026.
---
 ci/test/01_base_install.sh | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/ci/test/01_base_install.sh b/ci/test/01_base_install.sh
index d8faae8340b0f..b05d7547c8dfd 100755
--- a/ci/test/01_base_install.sh
+++ b/ci/test/01_base_install.sh
@@ -42,7 +42,7 @@ if [ -n "$PIP_PACKAGES" ]; then
 fi
 
 if [[ ${USE_MEMORY_SANITIZER} == "true" ]]; then
-  git clone --depth=1 https://github.com/llvm/llvm-project -b llvmorg-16.0.6 /msan/llvm-project
+  git clone --depth=1 https://github.com/llvm/llvm-project -b "llvmorg-17.0.0-rc4" /msan/llvm-project
 
   cmake -G Ninja -B /msan/clang_build/ \
     -DLLVM_ENABLE_PROJECTS="clang" \
@@ -66,8 +66,7 @@ if [[ ${USE_MEMORY_SANITIZER} == "true" ]]; then
     -DCMAKE_CXX_COMPILER=clang++ \
     -DLLVM_TARGETS_TO_BUILD=Native \
     -DLLVM_ENABLE_PER_TARGET_RUNTIME_DIR=OFF \
-    -DLIBCXX_ENABLE_DEBUG_MODE=ON \
-    -DLIBCXX_ENABLE_ASSERTIONS=ON \
+    -DLIBCXX_HARDENING_MODE=debug \
     -S /msan/llvm-project/runtimes
 
   ninja -C /msan/cxx_build/ "$MAKEJOBS"

From 4a825039a509c43ba20b2cd7aab448b3be16bcc3 Mon Sep 17 00:00:00 2001
From: fanquake <fanquake@gmail.com>
Date: Thu, 14 Sep 2023 13:12:14 +0100
Subject: [PATCH 13/17] build: use _LIBCPP_ENABLE_DEBUG_MODE over
 ENABLE_ASSERTIONS

`_LIBCPP_ENABLE_ASSERTIONS` is deprecated, and will be removed. [See (from libc++ __config in main)](https://github.com/llvm/llvm-project/blob/b57df9fe9a1a230f277d671bfa0884bbda9fc1c5/libcxx/include/__config#L205-L209):

> TODO(hardening): remove this in LLVM 19.
> This is for backward compatibility -- make enabling `_LIBCPP_ENABLE_ASSERTIONS` (which predates hardening modes)
> equivalent to setting the safe mode.
> ifdef _LIBCPP_ENABLE_ASSERTIONS
> warning "_LIBCPP_ENABLE_ASSERTIONS is deprecated, please use _LIBCPP_ENABLE_SAFE_MODE instead."

From LLVM 17, `_LIBCPP_ENABLE_DEBUG_MODE` can be used instead.

See https://libcxx.llvm.org/Hardening.html.

Related to #28476.
---
 depends/hosts/linux.mk | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/depends/hosts/linux.mk b/depends/hosts/linux.mk
index 0e2496174e302..1f33640c66bee 100644
--- a/depends/hosts/linux.mk
+++ b/depends/hosts/linux.mk
@@ -17,7 +17,7 @@ linux_release_CXXFLAGS=$(linux_release_CFLAGS)
 linux_debug_CFLAGS=-O1
 linux_debug_CXXFLAGS=$(linux_debug_CFLAGS)
 
-linux_debug_CPPFLAGS=-D_GLIBCXX_DEBUG -D_GLIBCXX_DEBUG_PEDANTIC -D_LIBCPP_ENABLE_ASSERTIONS=1
+linux_debug_CPPFLAGS=-D_GLIBCXX_DEBUG -D_GLIBCXX_DEBUG_PEDANTIC -D_LIBCPP_ENABLE_DEBUG_MODE=1
 
 ifeq (86,$(findstring 86,$(build_arch)))
 i686_linux_CC=gcc -m32

From 508d05f8a7b511dd53f543df8899813487eb03e5 Mon Sep 17 00:00:00 2001
From: dergoegge <n.goeggi@gmail.com>
Date: Wed, 13 Sep 2023 16:27:45 +0100
Subject: [PATCH 14/17] [fuzz] Don't use afl++ deferred forkserver mode

Deferring the forkserver initialization doesn't make sense for some of
our targets since they involve state that can't be forked (e.g.
threads). We therefore remove the use of __AFL_INIT entirely.

We also increase the __AFL_LOOP count to 100000. Our fuzz targets are
meant to all be deterministic and stateless therefore this should be
fine.
---
 src/test/fuzz/fuzz.cpp | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/src/test/fuzz/fuzz.cpp b/src/test/fuzz/fuzz.cpp
index f5697f14b101a..32bd00ec03ef7 100644
--- a/src/test/fuzz/fuzz.cpp
+++ b/src/test/fuzz/fuzz.cpp
@@ -192,17 +192,11 @@ int main(int argc, char** argv)
 {
     initialize();
     static const auto& test_one_input = *Assert(g_test_one_input);
-#ifdef __AFL_HAVE_MANUAL_CONTROL
-    // Enable AFL deferred forkserver mode. Requires compilation using
-    // afl-clang-fast++. See fuzzing.md for details.
-    __AFL_INIT();
-#endif
-
 #ifdef __AFL_LOOP
     // Enable AFL persistent mode. Requires compilation using afl-clang-fast++.
     // See fuzzing.md for details.
     const uint8_t* buffer = __AFL_FUZZ_TESTCASE_BUF;
-    while (__AFL_LOOP(1000)) {
+    while (__AFL_LOOP(100000)) {
         size_t buffer_len = __AFL_FUZZ_TESTCASE_LEN;
         test_one_input({buffer, buffer_len});
     }

From 3f4e1bb9ae5ee43da9503da37b9894037d613c6d Mon Sep 17 00:00:00 2001
From: Pieter Wuille <pieter@wuille.net>
Date: Fri, 15 Sep 2023 07:17:50 -0400
Subject: [PATCH 15/17] tests: fix incorrect assumption in v2transport_test

---
 src/test/net_tests.cpp | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp
index 34d7867079329..22e1fe7c5593e 100644
--- a/src/test/net_tests.cpp
+++ b/src/test/net_tests.cpp
@@ -1357,11 +1357,19 @@ BOOST_AUTO_TEST_CASE(v2transport_test)
         BOOST_CHECK(!(*ret)[1]);
         BOOST_CHECK((*ret)[2] && (*ret)[2]->m_type == "tx" && Span{(*ret)[2]->m_recv} == MakeByteSpan(msg_data_2));
 
-        // Then send a message with a bit error, expecting failure.
+        // Then send a message with a bit error, expecting failure. It's possible this failure does
+        // not occur immediately (when the length descriptor was modified), but it should come
+        // eventually, and no messages can be delivered anymore.
         tester.SendMessage("bad", msg_data_1);
         tester.Damage();
-        ret = tester.Interact();
-        BOOST_CHECK(!ret);
+        while (true) {
+            ret = tester.Interact();
+            if (!ret) break; // failure
+            BOOST_CHECK(ret->size() == 0); // no message can be delivered
+            // Send another message.
+            auto msg_data_3 = g_insecure_rand_ctx.randbytes<uint8_t>(InsecureRandRange(10000));
+            tester.SendMessage(uint8_t(12), msg_data_3); // getheaders short id
+        }
     }
 
     // Normal scenario, with a transport in responder node.

From 27b636a92199d2d47db5e6049de3c924d1f634f9 Mon Sep 17 00:00:00 2001
From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
Date: Tue, 19 Sep 2023 09:36:53 +0100
Subject: [PATCH 16/17] ci: Reintroduce fixed "test-each-commit" job

The new job checks at most 6 commits of a pull request, excluding the
top one.
---
 .github/workflows/ci.yml | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index 1bdaf8523d514..0ef705250d7ec 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -23,6 +23,28 @@ env:
   MAKEJOBS: '-j10'
 
 jobs:
+  test-each-commit:
+    name: 'test each commit'
+    runs-on: ubuntu-22.04
+    if: github.event_name == 'pull_request' && github.event.pull_request.commits != 1
+    timeout-minutes: 360  # Use maximum time, see https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idtimeout-minutes. Assuming a worst case time of 1 hour per commit, this leads to a --max-count=6 below.
+    env:
+      MAX_COUNT: 6
+    steps:
+      - run: echo "FETCH_DEPTH=$((${{ github.event.pull_request.commits }} + 2))" >> "$GITHUB_ENV"
+      - uses: actions/checkout@v4
+        with:
+          ref: ${{ github.event.pull_request.head.sha }}
+          fetch-depth: ${{ env.FETCH_DEPTH }}
+      - run: |
+          git checkout HEAD~
+          echo "COMMIT_AFTER_LAST_MERGE=$(git log $(git log --merges -1 --format=%H)..HEAD --format=%H --max-count=${{ env.MAX_COUNT }} | tail -1)" >> "$GITHUB_ENV"
+      - run: sudo apt install clang ccache build-essential libtool autotools-dev automake pkg-config bsdmainutils python3-zmq libevent-dev libboost-dev libsqlite3-dev libdb++-dev systemtap-sdt-dev libminiupnpc-dev libnatpmp-dev libqt5gui5 libqt5core5a libqt5dbus5 qttools5-dev qttools5-dev-tools qtwayland5 libqrencode-dev -y
+      - name: Compile and run tests
+        run: |
+          # Use clang++, because it is a bit faster and uses less memory than g++
+          git rebase --exec "echo Running test-one-commit on \$( git log -1 ) && ./autogen.sh && CC=clang CXX=clang++ ./configure && make clean && make -j $(nproc) check && ./test/functional/test_runner.py -j $(( $(nproc) * 2 ))" ${{ env.COMMIT_AFTER_LAST_MERGE }}~1
+
   macos-native-x86_64:
     name: 'macOS 13 native, x86_64, no depends, sqlite only, gui'
     # Use latest image, but hardcode version to avoid silent upgrades (and breaks).

From fa33b2c889b90bd44f188ba5f0fafe31d7d7fad7 Mon Sep 17 00:00:00 2001
From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
Date: Tue, 19 Sep 2023 13:46:37 +0200
Subject: [PATCH 17/17] fuzz: Add missing PROVIDE_FUZZ_MAIN_FUNCTION guard to
 __AFL_FUZZ_INIT

---
 src/test/fuzz/fuzz.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/test/fuzz/fuzz.cpp b/src/test/fuzz/fuzz.cpp
index 32bd00ec03ef7..5245b4607bf50 100644
--- a/src/test/fuzz/fuzz.cpp
+++ b/src/test/fuzz/fuzz.cpp
@@ -29,7 +29,7 @@
 #include <utility>
 #include <vector>
 
-#ifdef __AFL_FUZZ_INIT
+#if defined(PROVIDE_FUZZ_MAIN_FUNCTION) && defined(__AFL_FUZZ_INIT)
 __AFL_FUZZ_INIT();
 #endif