diff --git a/src/flat-database.h b/src/flat-database.h index a55231900545..2c484e3c37e7 100644 --- a/src/flat-database.h +++ b/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()); diff --git a/src/governance/net_governance.cpp b/src/governance/net_governance.cpp index 2e8b5da79140..56d193d9ad62 100644 --- a/src/governance/net_governance.cpp +++ b/src/governance/net_governance.cpp @@ -288,7 +288,7 @@ 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) { @@ -296,7 +296,7 @@ bool NetGovernance::ProcessGetData(CNode& pfrom, const CInv& inv, const CNetMsgM 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; diff --git a/src/llmq/dkgsessionmgr.cpp b/src/llmq/dkgsessionmgr.cpp index 0749ff23395c..9a8e5705a527 100644 --- a/src/llmq/dkgsessionmgr.cpp +++ b/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) diff --git a/src/llmq/net_quorum.cpp b/src/llmq/net_quorum.cpp index 0f26ae924fc9..193e959f91f1 100644 --- a/src/llmq/net_quorum.cpp +++ b/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; }; diff --git a/src/llmq/quorums.cpp b/src/llmq/quorums.cpp index 69b2100bd778..4d804c115d8f 100644 --- a/src/llmq/quorums.cpp +++ b/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); diff --git a/src/llmq/quorumsman.cpp b/src/llmq/quorumsman.cpp index f494c9673249..6a0f13d67b52 100644 --- a/src/llmq/quorumsman.cpp +++ b/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)) { diff --git a/src/streams.h b/src/streams.h index b1d261b29935..87b16ff996ca 100644 --- a/src/streams.h +++ b/src/streams.h @@ -316,14 +316,6 @@ class DataStream vch.insert(vch.end(), src.begin(), src.end()); } - template - void Serialize(Stream& s) const - { - // Special case: stream << stream concatenates like stream += stream - if (!vch.empty()) - s.write(MakeByteSpan(vch)); - } - template DataStream& operator<<(const T& obj) { diff --git a/src/wallet/dump.cpp b/src/wallet/dump.cpp index 3344cfd0be7e..b55b64859c09 100644 --- a/src/wallet/dump.cpp +++ b/src/wallet/dump.cpp @@ -252,11 +252,7 @@ bool CreateFromDump(const ArgsManager& args, const std::string& name, const fs:: std::vector k = ParseHex(key); std::vector 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; diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 9817d69eda55..a5b6dd8ccbd0 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -1873,8 +1873,23 @@ std::unordered_set 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 LegacyScriptPubKeyMan::GetNotMineScriptPubKeys() const +{ + LOCK(cs_KeyStore); + std::unordered_set 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 11ad0a558b10..04e78b5e7292 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -535,6 +535,12 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv std::set GetKeys() const override; std::unordered_set 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 GetNotMineScriptPubKeys() const; + /** Get the DescriptorScriptPubKeyMans (with private keys) that have the same scriptPubKeys as this LegacyScriptPubKeyMan. * Does not modify this ScriptPubKeyMan. */ std::optional MigrateToDescriptor(); diff --git a/src/wallet/test/util.cpp b/src/wallet/test/util.cpp index 84382cb59efa..4f4dc8c11fe5 100644 --- a/src/wallet/test/util.cpp +++ b/src/wallet/test/util.cpp @@ -75,7 +75,7 @@ std::unique_ptr 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; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 467bb4ba432c..d6265a49b881 100644 --- a/src/wallet/wallet.cpp +++ b/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 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; diff --git a/test/functional/p2p_leak_tx.py b/test/functional/p2p_leak_tx.py index 2ebfec9c8b86..69b6246c30c2 100755 --- a/test/functional/p2p_leak_tx.py +++ b/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))) diff --git a/test/functional/wallet_fundrawtransaction.py b/test/functional/wallet_fundrawtransaction.py index 5686c015b25b..5afdf33a5153 100755 --- a/test/functional/wallet_fundrawtransaction.py +++ b/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 diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py index 8def79c7ac0c..33c8d27cf2e4 100755 --- a/test/functional/wallet_migration.py +++ b/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"])