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 6cb65165e795..5b68ffb0059d 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 98bd4441def3..3a901b69e8d0 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 985aa583bbc8..f529e15664fb 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 95b669f9382e..2b3ac280f31d 100644 --- a/src/wallet/dump.cpp +++ b/src/wallet/dump.cpp @@ -251,11 +251,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 ac7d41afa17a..7a99e075fa4d 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -1872,8 +1872,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 b40bdfaabb63..e074f545e1fd 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -526,6 +526,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 59e397f0a81b..d7402f73f283 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 9dc23d31ea7f..c3f765b92df1 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4655,9 +4655,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()); @@ -4694,6 +4692,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."); @@ -4817,6 +4822,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 f1e6749dccec..e024cc455efe 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 @@ -547,11 +548,22 @@ def test_spend_2of2(self): def test_locked_wallet(self): self.log.info("Test fundrawtxn with locked wallet and hardened derivation") - self.nodes[1].encryptwallet("test") + 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()]) + self.generate(self.nodes[1], 1) + + # Encrypt wallet and import descriptors + wallet.encryptwallet("test") if self.options.descriptors: - self.nodes[1].walletpassphrase('test', 999000) - self.nodes[1].importdescriptors([{ + wallet.walletpassphrase('test', 999000) + wallet.importdescriptors([{ 'desc': descsum_create('pkh(tprv8ZgxMBicQKsPdYeeZbPSKd2KYLmeVKtcFA7kqCxDvDR13MQ6us8HopUR2wLcS2ZKPhLyKsqpDL2FtL73LMHcgoCL7DXsciA8eX8nbjCR2eG/0h/*h)'), 'timestamp': 'now', 'active': True @@ -562,48 +574,58 @@ def test_locked_wallet(self): 'active': True, 'internal': True }]) - self.nodes[1].walletlock() + wallet.walletlock() # Drain the keypool. - self.nodes[1].getnewaddress() + wallet.getnewaddress() + + # Choose input + inputs = wallet.listunspent() + + # 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) - # Choose 2 inputs - # bnb doesn't work same way as in bitcoin, so, `value` is also calculated by different way - inputs = self.nodes[1].listunspent() - value = sum(inp["amount"] for inp in inputs) - Decimal("0.00002200") outputs = {self.nodes[0].getnewaddress():value} - rawtx = self.nodes[1].createrawtransaction(inputs, outputs) + rawtx = wallet.createrawtransaction(inputs, outputs) # fund a transaction that does not require a new key for the change output - self.nodes[1].fundrawtransaction(rawtx) + funded_tx = wallet.fundrawtransaction(rawtx) + assert_equal(funded_tx["changepos"], -1) # fund a transaction that requires a new key for the change output # creating the key must be impossible because the wallet is locked outputs = {self.nodes[0].getnewaddress():value - Decimal("0.1")} - rawtx = self.nodes[1].createrawtransaction(inputs, outputs) - assert_raises_rpc_error(-4, "Transaction needs a change address, but we can't generate it.", self.nodes[1].fundrawtransaction, rawtx) + rawtx = wallet.createrawtransaction(inputs, outputs) + assert_raises_rpc_error(-4, "Transaction needs a change address, but we can't generate it.", wallet.fundrawtransaction, rawtx) # Refill the keypool. - self.nodes[1].walletpassphrase("test", 999000) - self.nodes[1].walletlock() + wallet.walletpassphrase("test", 999000) + wallet.walletlock() - assert_raises_rpc_error(-13, "walletpassphrase", self.nodes[1].sendtoaddress, self.nodes[0].getnewaddress(), 12) + assert_raises_rpc_error(-13, "walletpassphrase", wallet.sendtoaddress, self.nodes[0].getnewaddress(), 12) oldBalance = self.nodes[0].getbalance() inputs = [] outputs = {self.nodes[0].getnewaddress():11} - rawtx = self.nodes[1].createrawtransaction(inputs, outputs) - fundedTx = self.nodes[1].fundrawtransaction(rawtx) + rawtx = wallet.createrawtransaction(inputs, outputs) + fundedTx = wallet.fundrawtransaction(rawtx) + assert fundedTx["changepos"] != -1 # Now we need to unlock. - self.nodes[1].walletpassphrase("test", 999000) - signedTx = self.nodes[1].signrawtransactionwithwallet(fundedTx['hex']) - self.nodes[1].sendrawtransaction(signedTx['hex']) + wallet.walletpassphrase("test", 999000) + signedTx = wallet.signrawtransactionwithwallet(fundedTx['hex']) + wallet.sendrawtransaction(signedTx['hex']) self.generate(self.nodes[1], 1) # Make sure funds are received at node1. assert_equal(oldBalance+Decimal('511.0000000'), self.nodes[0].getbalance()) + # Restore pre-test wallet state + wallet.sendall(recipients=[df_wallet.getnewaddress(), df_wallet.getnewaddress(), df_wallet.getnewaddress()]) + wallet.unloadwallet() + self.generate(self.nodes[1], 1) + def test_many_inputs_fee(self): """Multiple (~19) inputs tx test | Compare fee.""" self.log.info("Test fundrawtxn fee with many inputs") diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py index af516448b2b5..f1716b0aa103 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 @@ -611,6 +611,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"]) @@ -620,6 +635,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"])