backport: bitcoin#27800, bitcoin#28125, bitcoin#28232, bitcoin#27695

src/flat-database.h

@@ -59,7 +59,7 @@ class CFlatDB
 
         // Write and commit header, data
         try {
-            fileout << ssObj;
+            fileout << Span{ssObj};
         }
         catch (std::exception &e) {
             LogError("%s: Serialize or I/O error - %s\n", __func__, e.what());

src/governance/net_governance.cpp

@@ -288,15 +288,15 @@ bool NetGovernance::ProcessGetData(CNode& pfrom, const CInv& inv, const CNetMsgM
         CDataStream ss(SER_NETWORK, pfrom.GetCommonVersion());
         ss.reserve(1000);
         if (!m_gov_manager.SerializeObjectForHash(inv.hash, ss)) return false;
-        m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::MNGOVERNANCEOBJECT, ss));
+        m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::MNGOVERNANCEOBJECT, Span{ss}));
         return true;
     }
     if (inv.type == MSG_GOVERNANCE_OBJECT_VOTE) {
         if (!m_gov_manager.HaveVoteForHash(inv.hash)) return false;
         CDataStream ss(SER_NETWORK, pfrom.GetCommonVersion());
         ss.reserve(1000);
         if (!m_gov_manager.SerializeVoteForHash(inv.hash, ss)) return false;
-        m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::MNGOVERNANCEOBJECTVOTE, ss));
+        m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::MNGOVERNANCEOBJECTVOTE, Span{ss}));
         return true;
     }
     return false;

src/llmq/dkgsessionmgr.cpp

@@ -129,7 +129,7 @@ void CDKGSessionManager::WriteVerifiedVvecContribution(Consensus::LLMQType llmqT
     for (auto& pubkey : *vvec) {
         s << CBLSPublicKeyVersionWrapper(pubkey, false);
     }
-    db->Write(std::make_tuple(DB_VVEC, llmqType, pQuorumBaseBlockIndex->GetBlockHash(), proTxHash), s);
+    db->Write(std::make_tuple(DB_VVEC, llmqType, pQuorumBaseBlockIndex->GetBlockHash(), proTxHash), Span{s});
 }
 
 void CDKGSessionManager::WriteVerifiedSkContribution(Consensus::LLMQType llmqType, const CBlockIndex* pQuorumBaseBlockIndex, const uint256& proTxHash, const CBLSSecretKey& skContribution)

src/llmq/net_quorum.cpp

@@ -113,8 +113,8 @@ void NetQuorum::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDataS
             }
             request.SetError(nError);
             CDataStream ssResponse{SER_NETWORK, pfrom.GetCommonVersion()};
-            ssResponse << request << body;
-            m_connman.PushMessage(&pfrom, CNetMsgMaker(pfrom.GetCommonVersion()).Make(NetMsgType::QDATA, ssResponse));
+            ssResponse << request << Span{body};
+            m_connman.PushMessage(&pfrom, CNetMsgMaker(pfrom.GetCommonVersion()).Make(NetMsgType::QDATA, Span{ssResponse}));
             return misbehave;
         };
 

src/llmq/quorums.cpp

@@ -200,7 +200,7 @@ void CQuorum::WriteContributions(CDBWrapper& db) const
         for (auto& pubkey : *quorumVvec) {
             s << CBLSPublicKeyVersionWrapper(pubkey, false);
         }
-        db.Write(std::make_pair(DB_QUORUM_QUORUM_VVEC, dbKey), s);
+        db.Write(std::make_pair(DB_QUORUM_QUORUM_VVEC, dbKey), Span{s});
     }
     if (skShare.IsValid()) {
         db.Write(std::make_pair(DB_QUORUM_SK_SHARE, dbKey), skShare);

src/llmq/quorumsman.cpp

@@ -676,7 +676,7 @@ void CQuorumManager::MigrateOldQuorumDB(CEvoDB& evoDb) const
                 if (!evoDb.GetRawDB().ReadDataStream(k, s)) {
                     break;
                 }
-                batch.Write(k, s);
+                batch.Write(k, Span{s});
             }
             if (prefix == DB_QUORUM_SK_SHARE) {
                 if (!pcursor->GetValue(sk)) {

src/streams.h

@@ -316,14 +316,6 @@ class DataStream
         vch.insert(vch.end(), src.begin(), src.end());
     }
 
-    template<typename Stream>
-    void Serialize(Stream& s) const
-    {
-        // Special case: stream << stream concatenates like stream += stream
-        if (!vch.empty())
-            s.write(MakeByteSpan(vch));
-    }
-
     template<typename T>
     DataStream& operator<<(const T& obj)
     {

src/wallet/dump.cpp

@@ -252,11 +252,7 @@ bool CreateFromDump(const ArgsManager& args, const std::string& name, const fs::
 
             std::vector<unsigned char> k = ParseHex(key);
             std::vector<unsigned char> v = ParseHex(value);
-
-            DataStream ss_key{k};
-            DataStream ss_value{v};
-
-            if (!batch->Write(ss_key, ss_value)) {
+            if (!batch->Write(Span{k}, Span{v})) {
                 error = strprintf(_("Error: Unable to write record to new wallet"));
                 ret = false;
                 break;

src/wallet/scriptpubkeyman.cpp

@@ -1873,8 +1873,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;
 }
 

src/wallet/scriptpubkeyman.h

@@ -535,6 +535,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();

src/wallet/test/util.cpp

@@ -75,7 +75,7 @@ std::unique_ptr<WalletDatabase> DuplicateMockDatabase(WalletDatabase& database,
         DatabaseCursor::Status status = cursor->Next(key, value);
         assert(status != DatabaseCursor::Status::FAIL);
         if (status == DatabaseCursor::Status::DONE) break;
-        new_batch->Write(key, value);
+        new_batch->Write(Span{key}, Span{value});
     }
 
     return new_database;

src/wallet/wallet.cpp

@@ -4735,9 +4735,7 @@ bool CWallet::MigrateToSQLite(bilingual_str& error)
     bool began = batch->TxnBegin();
     assert(began); // This is a critical error, the new db could not be written to. The original db exists as a backup, but we should not continue execution.
     for (const auto& [key, value] : records) {
-        DataStream ss_key{key};
-        DataStream ss_value{value};
-        if (!batch->Write(ss_key, ss_value)) {
+        if (!batch->Write(MakeUCharSpan(key), MakeUCharSpan(value))) {
             batch->TxnAbort();
             m_database->Close();
             fs::remove(m_database->Filename());
@@ -4774,6 +4772,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.");
@@ -4898,6 +4903,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;

test/functional/p2p_leak_tx.py

@@ -2,7 +2,7 @@
 # Copyright (c) 2017-2020 The Bitcoin Core developers
 # Distributed under the MIT software license, see the accompanying
 # file COPYING or http://www.opensource.org/licenses/mit-license.php.
-"""Test that we don't leak txs to inbound peers that we haven't yet announced to"""
+"""Test transaction upload"""
 
 from test_framework.messages import msg_getdata, CInv, MSG_TX
 from test_framework.p2p import p2p_lock, P2PDataStore
@@ -23,16 +23,38 @@ def set_test_params(self):
         self.num_nodes = 1
 
     def run_test(self):
-        gen_node = self.nodes[0]  # The block and tx generating node
-        miniwallet = MiniWallet(gen_node)
+        self.gen_node = self.nodes[0]  # The block and tx generating node
+        self.miniwallet = MiniWallet(self.gen_node)
 
-        inbound_peer = self.nodes[0].add_p2p_connection(P2PNode())  # An "attacking" inbound peer
+        self.test_tx_in_block()
+        self.test_notfound_on_unannounced_tx()
+
+    def test_tx_in_block(self):
+        self.log.info("Check that a transaction in the last block is uploaded (beneficial for compact block relay)")
+        inbound_peer = self.gen_node.add_p2p_connection(P2PNode())
+
+        self.log.debug("Generate transaction and block")
+        inbound_peer.last_message.pop("inv", None)
+        txid = self.miniwallet.send_self_transfer(from_node=self.gen_node)["txid"]
+        self.bump_mocktime(60)
+        inbound_peer.wait_until(lambda: "inv" in inbound_peer.last_message and inbound_peer.last_message.get("inv").inv[0].hash == int(txid, 16))
+        want_tx = msg_getdata(inv=inbound_peer.last_message.get("inv").inv)
+        self.generate(self.gen_node, 1)
+
+        self.log.debug("Request transaction")
+        inbound_peer.last_message.pop("tx", None)
+        inbound_peer.send_and_ping(want_tx)
+        assert_equal(inbound_peer.last_message.get("tx").tx.rehash(), txid)
+
+    def test_notfound_on_unannounced_tx(self):
+        self.log.info("Check that we don't leak txs to inbound peers that we haven't yet announced to")
+        inbound_peer = self.gen_node.add_p2p_connection(P2PNode())  # An "attacking" inbound peer
 
         MAX_REPEATS = 100
         self.log.info("Running test up to {} times.".format(MAX_REPEATS))
         for i in range(MAX_REPEATS):
             self.log.info('Run repeat {}'.format(i + 1))
-            txid = miniwallet.send_self_transfer(from_node=gen_node)['txid']
+            txid = self.miniwallet.send_self_transfer(from_node=self.gen_node)["txid"]
 
             want_tx = msg_getdata()
             want_tx.inv.append(CInv(t=MSG_TX, h=int(txid, 16)))

test/functional/wallet_fundrawtransaction.py

@@ -20,6 +20,7 @@
     assert_raises_rpc_error,
     count_bytes,
     find_vout_for_address,
+    get_fee,
     satoshi_round,
 )
 from test_framework.wallet_util import bytes_to_wif
@@ -550,6 +551,8 @@ def test_locked_wallet(self):
         df_wallet = self.nodes[1].get_wallet_rpc(self.default_wallet_name)
         self.nodes[1].createwallet(wallet_name="locked_wallet", descriptors=self.options.descriptors)
         wallet = self.nodes[1].get_wallet_rpc("locked_wallet")
+        # This test is not meant to exercise fee estimation. Making sure all txs are sent at a consistent fee rate.
+        wallet.settxfee(self.min_relay_tx_fee)
 
         # Add some balance to the wallet (this will be reverted at the end of the test)
         df_wallet.sendall(recipients=[wallet.getnewaddress()])
@@ -578,10 +581,11 @@ def test_locked_wallet(self):
 
         # Choose input
         inputs = wallet.listunspent()
-        # Deduce fee to produce a changeless transaction
-        # (500 duffs is above the fee of this 1-in/1-out tx at the test feerate,
-        #  while the remainder stays below the cost of creating a change output)
-        value = inputs[0]["amount"] - Decimal("0.00000500")
+
+        # Deduce exact fee to produce a changeless transaction
+        tx_size = 191  # Total tx size: 191 bytes, p2pkh -> p2pkh. Input 147 bytes + rest of tx is 44 bytes.
+        value = inputs[0]["amount"] - get_fee(tx_size, self.min_relay_tx_fee)
+
         outputs = {self.nodes[0].getnewaddress():value}
         rawtx = wallet.createrawtransaction(inputs, outputs)
         # fund a transaction that does not require a new key for the change output

test/functional/wallet_migration.py

@@ -7,7 +7,7 @@
 import os
 import random
 import shutil
-from test_framework.address import key_to_p2pkh
+from test_framework.address import key_to_p2pkh, script_to_p2sh
 from test_framework.descriptors import descsum_create
 from test_framework.key import ECPubKey
 from test_framework.test_framework import BitcoinTestFramework
@@ -620,6 +620,21 @@ def send_to_script(script, amount):
         wallet.rpc.importaddress(address=script_pkh.hex(), label="raw_spk", rescan=True, p2sh=True)
         assert_equal(wallet.getbalances()['watchonly']['trusted'], 2)
 
+        # 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 = self.migrate_wallet(wallet)
         wallet_wo = self.nodes[0].get_wallet_rpc(info_migration["watchonly_name"])
@@ -629,6 +644,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"])