From af1d2ff88344e1545ac8d9ad09f8e37e264da712 Mon Sep 17 00:00:00 2001 From: dergoegge Date: Wed, 1 Nov 2023 14:04:44 +0000 Subject: [PATCH 01/19] [primitives] Precompute result of CTransaction::HasWitness --- src/primitives/transaction.cpp | 12 ++++++++++-- src/primitives/transaction.h | 13 ++++--------- 2 files changed, 14 insertions(+), 11 deletions(-) diff --git a/src/primitives/transaction.cpp b/src/primitives/transaction.cpp index 1ad8345fcb481..77a363f7b6e68 100644 --- a/src/primitives/transaction.cpp +++ b/src/primitives/transaction.cpp @@ -15,6 +15,7 @@ #include #include +#include #include #include @@ -71,6 +72,13 @@ Txid CMutableTransaction::GetHash() const return Txid::FromUint256((CHashWriter{SERIALIZE_TRANSACTION_NO_WITNESS} << *this).GetHash()); } +bool CTransaction::ComputeHasWitness() const +{ + return std::any_of(vin.begin(), vin.end(), [](const auto& input) { + return !input.scriptWitness.IsNull(); + }); +} + Txid CTransaction::ComputeHash() const { return Txid::FromUint256((CHashWriter{SERIALIZE_TRANSACTION_NO_WITNESS} << *this).GetHash()); @@ -85,8 +93,8 @@ Wtxid CTransaction::ComputeWitnessHash() const return Wtxid::FromUint256((CHashWriter{0} << *this).GetHash()); } -CTransaction::CTransaction(const CMutableTransaction& tx) : vin(tx.vin), vout(tx.vout), nVersion(tx.nVersion), nLockTime(tx.nLockTime), hash{ComputeHash()}, m_witness_hash{ComputeWitnessHash()} {} -CTransaction::CTransaction(CMutableTransaction&& tx) : vin(std::move(tx.vin)), vout(std::move(tx.vout)), nVersion(tx.nVersion), nLockTime(tx.nLockTime), hash{ComputeHash()}, m_witness_hash{ComputeWitnessHash()} {} +CTransaction::CTransaction(const CMutableTransaction& tx) : vin(tx.vin), vout(tx.vout), nVersion(tx.nVersion), nLockTime(tx.nLockTime), m_has_witness{ComputeHasWitness()}, hash{ComputeHash()}, m_witness_hash{ComputeWitnessHash()} {} +CTransaction::CTransaction(CMutableTransaction&& tx) : vin(std::move(tx.vin)), vout(std::move(tx.vout)), nVersion(tx.nVersion), nLockTime(tx.nLockTime), m_has_witness{ComputeHasWitness()}, hash{ComputeHash()}, m_witness_hash{ComputeWitnessHash()} {} CAmount CTransaction::GetValueOut() const { diff --git a/src/primitives/transaction.h b/src/primitives/transaction.h index 89deb9de4d87e..594168bbccdbb 100644 --- a/src/primitives/transaction.h +++ b/src/primitives/transaction.h @@ -310,12 +310,15 @@ class CTransaction private: /** Memory only. */ + const bool m_has_witness; const Txid hash; const Wtxid m_witness_hash; Txid ComputeHash() const; Wtxid ComputeWitnessHash() const; + bool ComputeHasWitness() const; + public: /** Convert a CMutableTransaction into a CTransaction. */ explicit CTransaction(const CMutableTransaction& tx); @@ -365,15 +368,7 @@ class CTransaction std::string ToString() const; - bool HasWitness() const - { - for (size_t i = 0; i < vin.size(); i++) { - if (!vin[i].scriptWitness.IsNull()) { - return true; - } - } - return false; - } + bool HasWitness() const { return m_has_witness; } }; /** A mutable version of CTransaction. */ From fa0ed0794161d937d2d3385963c1aa5624b60d17 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Thu, 16 Nov 2023 12:59:43 +0100 Subject: [PATCH 02/19] refactor: VectorWriter without nVersion The field is unused, so remove it. This is also required for future commits. --- src/blockfilter.cpp | 4 ++-- src/net.cpp | 2 +- src/netmessagemaker.h | 9 +++------ src/psbt.h | 6 +++--- src/signet.cpp | 2 +- src/streams.h | 19 ++++++------------- src/test/fuzz/golomb_rice.cpp | 4 ++-- src/test/streams_tests.cpp | 28 ++++++++++++++-------------- src/wallet/test/wallet_tests.cpp | 2 +- 9 files changed, 33 insertions(+), 43 deletions(-) diff --git a/src/blockfilter.cpp b/src/blockfilter.cpp index dd3824fb1c597..404c9ecc4fd62 100644 --- a/src/blockfilter.cpp +++ b/src/blockfilter.cpp @@ -81,7 +81,7 @@ GCSFilter::GCSFilter(const Params& params, const ElementSet& elements) } m_F = static_cast(m_N) * static_cast(m_params.m_M); - CVectorWriter stream(GCS_SER_VERSION, m_encoded, 0); + VectorWriter stream{m_encoded, 0}; WriteCompactSize(stream, m_N); @@ -89,7 +89,7 @@ GCSFilter::GCSFilter(const Params& params, const ElementSet& elements) return; } - BitStreamWriter bitwriter(stream); + BitStreamWriter bitwriter{stream}; uint64_t last_value = 0; for (uint64_t value : BuildHashedSet(elements)) { diff --git a/src/net.cpp b/src/net.cpp index a2f80cbcf7605..075a7d9839d3f 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -818,7 +818,7 @@ bool V1Transport::SetMessageToSend(CSerializedNetMsg& msg) noexcept // serialize header m_header_to_send.clear(); - CVectorWriter{INIT_PROTO_VERSION, m_header_to_send, 0, hdr}; + VectorWriter{m_header_to_send, 0, hdr}; // update state m_message_to_send = std::move(msg); diff --git a/src/netmessagemaker.h b/src/netmessagemaker.h index a121183aabe24..60b8e579d66ba 100644 --- a/src/netmessagemaker.h +++ b/src/netmessagemaker.h @@ -12,14 +12,14 @@ class CNetMsgMaker { public: - explicit CNetMsgMaker(int nVersionIn) : nVersion(nVersionIn){} + explicit CNetMsgMaker(int /*unused*/) {} template - CSerializedNetMsg Make(int nFlags, std::string msg_type, Args&&... args) const + CSerializedNetMsg Make(int /*unused*/, std::string msg_type, Args&&... args) const { CSerializedNetMsg msg; msg.m_type = std::move(msg_type); - CVectorWriter{nFlags | nVersion, msg.data, 0, std::forward(args)...}; + VectorWriter{msg.data, 0, std::forward(args)...}; return msg; } @@ -28,9 +28,6 @@ class CNetMsgMaker { return Make(0, std::move(msg_type), std::forward(args)...); } - -private: - const int nVersion; }; #endif // BITCOIN_NETMESSAGEMAKER_H diff --git a/src/psbt.h b/src/psbt.h index a14df03837811..ac2d246a27a85 100644 --- a/src/psbt.h +++ b/src/psbt.h @@ -316,7 +316,7 @@ struct PSBTInput const auto& [leaf_hashes, origin] = leaf_origin; SerializeToVector(s, PSBT_IN_TAP_BIP32_DERIVATION, xonly); std::vector value; - CVectorWriter s_value{s.GetVersion(), value, 0}; + VectorWriter s_value{value, 0}; s_value << leaf_hashes; SerializeKeyOrigin(s_value, origin); s << value; @@ -757,7 +757,7 @@ struct PSBTOutput if (!m_tap_tree.empty()) { SerializeToVector(s, PSBT_OUT_TAP_TREE); std::vector value; - CVectorWriter s_value{s.GetVersion(), value, 0}; + VectorWriter s_value{value, 0}; for (const auto& [depth, leaf_ver, script] : m_tap_tree) { s_value << depth; s_value << leaf_ver; @@ -771,7 +771,7 @@ struct PSBTOutput const auto& [leaf_hashes, origin] = leaf; SerializeToVector(s, PSBT_OUT_TAP_BIP32_DERIVATION, xonly); std::vector value; - CVectorWriter s_value{s.GetVersion(), value, 0}; + VectorWriter s_value{value, 0}; s_value << leaf_hashes; SerializeKeyOrigin(s_value, origin); s << value; diff --git a/src/signet.cpp b/src/signet.cpp index d6a3a44d913c7..ef9913218a1f4 100644 --- a/src/signet.cpp +++ b/src/signet.cpp @@ -110,7 +110,7 @@ std::optional SignetTxs::Create(const CBlock& block, const CScript& c uint256 signet_merkle = ComputeModifiedMerkleRoot(modified_cb, block); std::vector block_data; - CVectorWriter writer{INIT_PROTO_VERSION, block_data, 0}; + VectorWriter writer{block_data, 0}; writer << block.nVersion; writer << block.hashPrevBlock; writer << signet_merkle; diff --git a/src/streams.h b/src/streams.h index a3f8028da7868..88a5f3da54144 100644 --- a/src/streams.h +++ b/src/streams.h @@ -49,17 +49,15 @@ inline void Xor(Span write, Span key, size_t key_off * * The referenced vector will grow as necessary */ -class CVectorWriter +class VectorWriter { - public: - +public: /* - * @param[in] nVersionIn Serialization Version (including any flags) * @param[in] vchDataIn Referenced byte vector to overwrite/append * @param[in] nPosIn Starting position. Vector index where writes should start. The vector will initially * grow as necessary to max(nPosIn, vec.size()). So to append, use vec.size(). */ - CVectorWriter(int nVersionIn, std::vector& vchDataIn, size_t nPosIn) : nVersion{nVersionIn}, vchData{vchDataIn}, nPos{nPosIn} + VectorWriter(std::vector& vchDataIn, size_t nPosIn) : vchData{vchDataIn}, nPos{nPosIn} { if(nPos > vchData.size()) vchData.resize(nPos); @@ -69,7 +67,7 @@ class CVectorWriter * @param[in] args A list of items to serialize starting at nPosIn. */ template - CVectorWriter(int nVersionIn, std::vector& vchDataIn, size_t nPosIn, Args&&... args) : CVectorWriter{nVersionIn, vchDataIn, nPosIn} + VectorWriter(std::vector& vchDataIn, size_t nPosIn, Args&&... args) : VectorWriter{vchDataIn, nPosIn} { ::SerializeMany(*this, std::forward(args)...); } @@ -85,19 +83,14 @@ class CVectorWriter } nPos += src.size(); } - template - CVectorWriter& operator<<(const T& obj) + template + VectorWriter& operator<<(const T& obj) { ::Serialize(*this, obj); return (*this); } - int GetVersion() const - { - return nVersion; - } private: - const int nVersion; std::vector& vchData; size_t nPos; }; diff --git a/src/test/fuzz/golomb_rice.cpp b/src/test/fuzz/golomb_rice.cpp index f3073c5c973b8..8b395f3ea9589 100644 --- a/src/test/fuzz/golomb_rice.cpp +++ b/src/test/fuzz/golomb_rice.cpp @@ -51,9 +51,9 @@ FUZZ_TARGET(golomb_rice) for (int i = 0; i < n; ++i) { elements.insert(ConsumeRandomLengthByteVector(fuzzed_data_provider, 16)); } - CVectorWriter stream{0, golomb_rice_data, 0}; + VectorWriter stream{golomb_rice_data, 0}; WriteCompactSize(stream, static_cast(elements.size())); - BitStreamWriter bitwriter(stream); + BitStreamWriter bitwriter{stream}; if (!elements.empty()) { uint64_t last_value = 0; for (const uint64_t value : BuildHashedSet(elements, static_cast(elements.size()) * static_cast(BASIC_FILTER_M))) { diff --git a/src/test/streams_tests.cpp b/src/test/streams_tests.cpp index f03f7c1da2415..208993a76db00 100644 --- a/src/test/streams_tests.cpp +++ b/src/test/streams_tests.cpp @@ -74,49 +74,49 @@ BOOST_AUTO_TEST_CASE(streams_vector_writer) // point should yield the same results, even if the first test grew the // vector. - CVectorWriter{INIT_PROTO_VERSION, vch, 0, a, b}; + VectorWriter{vch, 0, a, b}; BOOST_CHECK((vch == std::vector{{1, 2}})); - CVectorWriter{INIT_PROTO_VERSION, vch, 0, a, b}; + VectorWriter{vch, 0, a, b}; BOOST_CHECK((vch == std::vector{{1, 2}})); vch.clear(); - CVectorWriter{INIT_PROTO_VERSION, vch, 2, a, b}; + VectorWriter{vch, 2, a, b}; BOOST_CHECK((vch == std::vector{{0, 0, 1, 2}})); - CVectorWriter{INIT_PROTO_VERSION, vch, 2, a, b}; + VectorWriter{vch, 2, a, b}; BOOST_CHECK((vch == std::vector{{0, 0, 1, 2}})); vch.clear(); vch.resize(5, 0); - CVectorWriter{INIT_PROTO_VERSION, vch, 2, a, b}; + VectorWriter{vch, 2, a, b}; BOOST_CHECK((vch == std::vector{{0, 0, 1, 2, 0}})); - CVectorWriter{INIT_PROTO_VERSION, vch, 2, a, b}; + VectorWriter{vch, 2, a, b}; BOOST_CHECK((vch == std::vector{{0, 0, 1, 2, 0}})); vch.clear(); vch.resize(4, 0); - CVectorWriter{INIT_PROTO_VERSION, vch, 3, a, b}; + VectorWriter{vch, 3, a, b}; BOOST_CHECK((vch == std::vector{{0, 0, 0, 1, 2}})); - CVectorWriter{INIT_PROTO_VERSION, vch, 3, a, b}; + VectorWriter{vch, 3, a, b}; BOOST_CHECK((vch == std::vector{{0, 0, 0, 1, 2}})); vch.clear(); vch.resize(4, 0); - CVectorWriter{INIT_PROTO_VERSION, vch, 4, a, b}; + VectorWriter{vch, 4, a, b}; BOOST_CHECK((vch == std::vector{{0, 0, 0, 0, 1, 2}})); - CVectorWriter{INIT_PROTO_VERSION, vch, 4, a, b}; + VectorWriter{vch, 4, a, b}; BOOST_CHECK((vch == std::vector{{0, 0, 0, 0, 1, 2}})); vch.clear(); - CVectorWriter{INIT_PROTO_VERSION, vch, 0, bytes}; + VectorWriter{vch, 0, bytes}; BOOST_CHECK((vch == std::vector{{3, 4, 5, 6}})); - CVectorWriter{INIT_PROTO_VERSION, vch, 0, bytes}; + VectorWriter{vch, 0, bytes}; BOOST_CHECK((vch == std::vector{{3, 4, 5, 6}})); vch.clear(); vch.resize(4, 8); - CVectorWriter{INIT_PROTO_VERSION, vch, 2, a, bytes, b}; + VectorWriter{vch, 2, a, bytes, b}; BOOST_CHECK((vch == std::vector{{8, 8, 1, 3, 4, 5, 6, 2}})); - CVectorWriter{INIT_PROTO_VERSION, vch, 2, a, bytes, b}; + VectorWriter{vch, 2, a, bytes, b}; BOOST_CHECK((vch == std::vector{{8, 8, 1, 3, 4, 5, 6, 2}})); vch.clear(); } diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index bcbc31ed3eff2..c5452c3b0d5f4 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -752,7 +752,7 @@ bool malformed_descriptor(std::ios_base::failure e) BOOST_FIXTURE_TEST_CASE(wallet_descriptor_test, BasicTestingSetup) { std::vector malformed_record; - CVectorWriter vw{0, malformed_record, 0}; + VectorWriter vw{malformed_record, 0}; vw << std::string("notadescriptor"); vw << uint64_t{0}; vw << int32_t{0}; From 66669da4a5ca9edf2a40d20879d9a8aaf2b9e2ee Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Thu, 16 Nov 2023 14:46:51 +0100 Subject: [PATCH 03/19] Remove unused Make() overload in netmessagemaker.h --- src/netmessagemaker.h | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/src/netmessagemaker.h b/src/netmessagemaker.h index 60b8e579d66ba..ede03aa204c40 100644 --- a/src/netmessagemaker.h +++ b/src/netmessagemaker.h @@ -15,19 +15,13 @@ class CNetMsgMaker explicit CNetMsgMaker(int /*unused*/) {} template - CSerializedNetMsg Make(int /*unused*/, std::string msg_type, Args&&... args) const + CSerializedNetMsg Make(std::string msg_type, Args&&... args) const { CSerializedNetMsg msg; msg.m_type = std::move(msg_type); VectorWriter{msg.data, 0, std::forward(args)...}; return msg; } - - template - CSerializedNetMsg Make(std::string msg_type, Args&&... args) const - { - return Make(0, std::move(msg_type), std::forward(args)...); - } }; #endif // BITCOIN_NETMESSAGEMAKER_H From fa9b5f4fe32c0cfe2e477bb11912756f84a52cfe Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Thu, 16 Nov 2023 15:43:15 +0100 Subject: [PATCH 04/19] refactor: NetMsg::Make() without nVersion The nVersion field is unused, so remove it. This is also required for future commits. Also, add PushMessage aliases in PeerManagerImpl to make calling code less verbose. Co-Authored-By: Anthony Towns --- src/net_processing.cpp | 149 ++++++++---------- src/netmessagemaker.h | 10 +- src/test/fuzz/p2p_transport_serialization.cpp | 2 +- src/test/net_tests.cpp | 5 +- src/test/util/net.cpp | 5 +- 5 files changed, 74 insertions(+), 97 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 1556dc7e6753e..0cd20680b0df3 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -51,6 +51,7 @@ #include #include #include +#include /** Headers download timeout. * Timeout = base + per_header * (expected number of headers) */ @@ -669,6 +670,14 @@ class PeerManagerImpl final : public PeerManager void AddTxAnnouncement(const CNode& node, const GenTxid& gtxid, std::chrono::microseconds current_time) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); + /** Send a message to a peer */ + void PushMessage(CNode& node, CSerializedNetMsg&& msg) const { m_connman.PushMessage(&node, std::move(msg)); } + template + void MakeAndPushMessage(CNode& node, std::string msg_type, Args&&... args) const + { + m_connman.PushMessage(&node, NetMsg::Make(std::move(msg_type), std::forward(args)...)); + } + /** Send a version message to a peer */ void PushNodeVersion(CNode& pnode, const Peer& peer); @@ -1277,14 +1286,14 @@ void PeerManagerImpl::MaybeSetPeerAsAnnouncingHeaderAndIDs(NodeId nodeid) // As per BIP152, we only get 3 of our peers to announce // blocks using compact encodings. m_connman.ForNode(lNodesAnnouncingHeaderAndIDs.front(), [this](CNode* pnodeStop){ - m_connman.PushMessage(pnodeStop, CNetMsgMaker(pnodeStop->GetCommonVersion()).Make(NetMsgType::SENDCMPCT, /*high_bandwidth=*/false, /*version=*/CMPCTBLOCKS_VERSION)); + MakeAndPushMessage(*pnodeStop, NetMsgType::SENDCMPCT, /*high_bandwidth=*/false, /*version=*/CMPCTBLOCKS_VERSION); // save BIP152 bandwidth state: we select peer to be low-bandwidth pnodeStop->m_bip152_highbandwidth_to = false; return true; }); lNodesAnnouncingHeaderAndIDs.pop_front(); } - m_connman.PushMessage(pfrom, CNetMsgMaker(pfrom->GetCommonVersion()).Make(NetMsgType::SENDCMPCT, /*high_bandwidth=*/true, /*version=*/CMPCTBLOCKS_VERSION)); + MakeAndPushMessage(*pfrom, NetMsgType::SENDCMPCT, /*high_bandwidth=*/true, /*version=*/CMPCTBLOCKS_VERSION); // save BIP152 bandwidth state: we select peer to be high-bandwidth pfrom->m_bip152_highbandwidth_to = true; lNodesAnnouncingHeaderAndIDs.push_back(pfrom->GetId()); @@ -1487,10 +1496,10 @@ void PeerManagerImpl::PushNodeVersion(CNode& pnode, const Peer& peer) uint64_t your_services{addr.nServices}; const bool tx_relay{!RejectIncomingTxs(pnode)}; - m_connman.PushMessage(&pnode, CNetMsgMaker(INIT_PROTO_VERSION).Make(NetMsgType::VERSION, PROTOCOL_VERSION, my_services, nTime, + MakeAndPushMessage(pnode, NetMsgType::VERSION, PROTOCOL_VERSION, my_services, 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)); + nonce, strSubVersion, nNodeStartingHeight, tx_relay); if (fLogIPs) { LogPrint(BCLog::NET, "send version message: version %d, blocks=%d, them=%s, txrelay=%d, peer=%d\n", PROTOCOL_VERSION, nNodeStartingHeight, addr_you.ToStringAddrPort(), tx_relay, nodeid); @@ -1861,8 +1870,7 @@ std::optional PeerManagerImpl::FetchBlock(NodeId peer_id, const CBl // Send block request message to the peer bool success = m_connman.ForNode(peer_id, [this, &invs](CNode* node) { - const CNetMsgMaker msgMaker(node->GetCommonVersion()); - this->m_connman.PushMessage(node, msgMaker.Make(NetMsgType::GETDATA, invs)); + this->MakeAndPushMessage(*node, NetMsgType::GETDATA, invs); return true; }); @@ -1985,7 +1993,6 @@ void PeerManagerImpl::BlockDisconnected(const std::shared_ptr &blo void PeerManagerImpl::NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr& pblock) { auto pcmpctblock = std::make_shared(*pblock); - const CNetMsgMaker msgMaker(PROTOCOL_VERSION); LOCK(cs_main); @@ -1997,7 +2004,7 @@ void PeerManagerImpl::NewPoWValidBlock(const CBlockIndex *pindex, const std::sha uint256 hashBlock(pblock->GetHash()); const std::shared_future lazy_ser{ - std::async(std::launch::deferred, [&] { return msgMaker.Make(NetMsgType::CMPCTBLOCK, *pcmpctblock); })}; + std::async(std::launch::deferred, [&] { return NetMsg::Make(NetMsgType::CMPCTBLOCK, *pcmpctblock); })}; { auto most_recent_block_txs = std::make_unique>(); @@ -2028,7 +2035,7 @@ void PeerManagerImpl::NewPoWValidBlock(const CBlockIndex *pindex, const std::sha hashBlock.ToString(), pnode->GetId()); const CSerializedNetMsg& ser_cmpctblock{lazy_ser.get()}; - m_connman.PushMessage(pnode, ser_cmpctblock.Copy()); + PushMessage(*pnode, ser_cmpctblock.Copy()); state.pindexBestHeaderSent = pindex; } }); @@ -2262,7 +2269,6 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv& LogPrint(BCLog::NET, "%s: ignoring request from peer=%i for old block that isn't in the main chain\n", __func__, pfrom.GetId()); return; } - const CNetMsgMaker msgMaker(pfrom.GetCommonVersion()); // disconnect node in case we have reached the outbound limit for serving historical blocks if (m_connman.OutboundTargetReached(true) && (((m_chainman.m_best_header != nullptr) && (m_chainman.m_best_header->GetBlockTime() - pindex->GetBlockTime() > HISTORICAL_BLOCK_AGE)) || inv.IsMsgFilteredBlk()) && @@ -2296,7 +2302,7 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv& if (!m_chainman.m_blockman.ReadRawBlockFromDisk(block_data, pindex->GetBlockPos())) { assert(!"cannot load block from disk"); } - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::BLOCK, Span{block_data})); + MakeAndPushMessage(pfrom, NetMsgType::BLOCK, Span{block_data}); // Don't set pblock as we've sent the block } else { // Send block from disk @@ -2308,9 +2314,9 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv& } if (pblock) { if (inv.IsMsgBlk()) { - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::BLOCK, TX_NO_WITNESS(*pblock))); + MakeAndPushMessage(pfrom, NetMsgType::BLOCK, TX_NO_WITNESS(*pblock)); } else if (inv.IsMsgWitnessBlk()) { - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::BLOCK, TX_WITH_WITNESS(*pblock))); + MakeAndPushMessage(pfrom, NetMsgType::BLOCK, TX_WITH_WITNESS(*pblock)); } else if (inv.IsMsgFilteredBlk()) { bool sendMerkleBlock = false; CMerkleBlock merkleBlock; @@ -2322,7 +2328,7 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv& } } if (sendMerkleBlock) { - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::MERKLEBLOCK, merkleBlock)); + MakeAndPushMessage(pfrom, NetMsgType::MERKLEBLOCK, merkleBlock); // CMerkleBlock just contains hashes, so also push any transactions in the block the client did not see // This avoids hurting performance by pointlessly requiring a round-trip // Note that there is currently no way for a node to request any single transactions we didn't send here - @@ -2331,7 +2337,7 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv& // however we MUST always provide at least what the remote peer needs typedef std::pair PairType; for (PairType& pair : merkleBlock.vMatchedTxn) - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::TX, TX_NO_WITNESS(*pblock->vtx[pair.first]))); + MakeAndPushMessage(pfrom, NetMsgType::TX, TX_NO_WITNESS(*pblock->vtx[pair.first])); } // else // no response @@ -2342,13 +2348,13 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv& // instead we respond with the full, non-compact block. if (CanDirectFetch() && pindex->nHeight >= m_chainman.ActiveChain().Height() - MAX_CMPCTBLOCK_DEPTH) { if (a_recent_compact_block && a_recent_compact_block->header.GetHash() == pindex->GetBlockHash()) { - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::CMPCTBLOCK, *a_recent_compact_block)); + MakeAndPushMessage(pfrom, NetMsgType::CMPCTBLOCK, *a_recent_compact_block); } else { CBlockHeaderAndShortTxIDs cmpctblock{*pblock}; - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::CMPCTBLOCK, cmpctblock)); + MakeAndPushMessage(pfrom, NetMsgType::CMPCTBLOCK, cmpctblock); } } else { - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::BLOCK, TX_WITH_WITNESS(*pblock))); + MakeAndPushMessage(pfrom, NetMsgType::BLOCK, TX_WITH_WITNESS(*pblock)); } } } @@ -2362,7 +2368,7 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv& // wait for other stuff first. std::vector vInv; vInv.emplace_back(MSG_BLOCK, m_chainman.ActiveChain().Tip()->GetBlockHash()); - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::INV, vInv)); + MakeAndPushMessage(pfrom, NetMsgType::INV, vInv); peer.m_continuation_block.SetNull(); } } @@ -2396,7 +2402,6 @@ void PeerManagerImpl::ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic std::deque::iterator it = peer.m_getdata_requests.begin(); std::vector vNotFound; - const CNetMsgMaker msgMaker(pfrom.GetCommonVersion()); // Process as many TX items from the front of the getdata queue as // possible, since they're common and it's efficient to batch process @@ -2419,7 +2424,7 @@ void PeerManagerImpl::ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic if (tx) { // WTX and WITNESS_TX imply we serialize with witness const auto maybe_with_witness = (inv.IsMsgTx() ? TX_NO_WITNESS : TX_WITH_WITNESS); - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::TX, maybe_with_witness(*tx))); + MakeAndPushMessage(pfrom, NetMsgType::TX, maybe_with_witness(*tx)); m_mempool.RemoveUnbroadcastTx(tx->GetHash()); } else { vNotFound.push_back(inv); @@ -2454,7 +2459,7 @@ void PeerManagerImpl::ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic // In normal operation, we often send NOTFOUND messages for parents of // transactions that we relay; if a peer is missing a parent, they may // assume we have them and request the parents from us. - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::NOTFOUND, vNotFound)); + MakeAndPushMessage(pfrom, NetMsgType::NOTFOUND, vNotFound); } } @@ -2478,8 +2483,7 @@ void PeerManagerImpl::SendBlockTransactions(CNode& pfrom, Peer& peer, const CBlo resp.txn[i] = block.vtx[req.indexes[i]]; } - const CNetMsgMaker msgMaker(pfrom.GetCommonVersion()); - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::BLOCKTXN, resp)); + MakeAndPushMessage(pfrom, NetMsgType::BLOCKTXN, resp); } bool PeerManagerImpl::CheckHeadersPoW(const std::vector& headers, const Consensus::Params& consensusParams, Peer& peer) @@ -2707,14 +2711,12 @@ bool PeerManagerImpl::IsAncestorOfBestHeaderOrTip(const CBlockIndex* header) bool PeerManagerImpl::MaybeSendGetHeaders(CNode& pfrom, const CBlockLocator& locator, Peer& peer) { - const CNetMsgMaker msgMaker(pfrom.GetCommonVersion()); - const auto current_time = NodeClock::now(); // Only allow a new getheaders message to go out if we don't have a recent // one already in-flight if (current_time - peer.m_last_getheaders_timestamp > HEADERS_RESPONSE_TIME) { - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::GETHEADERS, locator, uint256())); + MakeAndPushMessage(pfrom, NetMsgType::GETHEADERS, locator, uint256()); peer.m_last_getheaders_timestamp = current_time; return true; } @@ -2728,8 +2730,6 @@ bool PeerManagerImpl::MaybeSendGetHeaders(CNode& pfrom, const CBlockLocator& loc */ void PeerManagerImpl::HeadersDirectFetchBlocks(CNode& pfrom, const Peer& peer, const CBlockIndex& last_header) { - const CNetMsgMaker msgMaker(pfrom.GetCommonVersion()); - LOCK(cs_main); CNodeState *nodestate = State(pfrom.GetId()); @@ -2782,7 +2782,7 @@ void PeerManagerImpl::HeadersDirectFetchBlocks(CNode& pfrom, const Peer& peer, c // In any case, we want to download using a compact block, not a regular one vGetData[0] = CInv(MSG_CMPCT_BLOCK, vGetData[0].hash); } - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::GETDATA, vGetData)); + MakeAndPushMessage(pfrom, NetMsgType::GETDATA, vGetData); } } } @@ -3155,9 +3155,7 @@ void PeerManagerImpl::ProcessGetCFilters(CNode& node,Peer& peer, CDataStream& vR } for (const auto& filter : filters) { - CSerializedNetMsg msg = CNetMsgMaker(node.GetCommonVersion()) - .Make(NetMsgType::CFILTER, filter); - m_connman.PushMessage(&node, std::move(msg)); + MakeAndPushMessage(node, NetMsgType::CFILTER, filter); } } @@ -3196,13 +3194,11 @@ void PeerManagerImpl::ProcessGetCFHeaders(CNode& node, Peer& peer, CDataStream& return; } - CSerializedNetMsg msg = CNetMsgMaker(node.GetCommonVersion()) - .Make(NetMsgType::CFHEADERS, + MakeAndPushMessage(node, NetMsgType::CFHEADERS, filter_type_ser, stop_index->GetBlockHash(), prev_header, filter_hashes); - m_connman.PushMessage(&node, std::move(msg)); } void PeerManagerImpl::ProcessGetCFCheckPt(CNode& node, Peer& peer, CDataStream& vRecv) @@ -3237,12 +3233,10 @@ void PeerManagerImpl::ProcessGetCFCheckPt(CNode& node, Peer& peer, CDataStream& } } - CSerializedNetMsg msg = CNetMsgMaker(node.GetCommonVersion()) - .Make(NetMsgType::CFCHECKPT, + MakeAndPushMessage(node, NetMsgType::CFCHECKPT, filter_type_ser, stop_index->GetBlockHash(), headers); - m_connman.PushMessage(&node, std::move(msg)); } void PeerManagerImpl::ProcessBlock(CNode& node, const std::shared_ptr& block, bool force_processing, bool min_pow_checked) @@ -3266,7 +3260,6 @@ void PeerManagerImpl::ProcessCompactBlockTxns(CNode& pfrom, Peer& peer, const Bl { std::shared_ptr pblock = std::make_shared(); bool fBlockRead{false}; - const CNetMsgMaker msgMaker(pfrom.GetCommonVersion()); { LOCK(cs_main); @@ -3302,7 +3295,7 @@ void PeerManagerImpl::ProcessCompactBlockTxns(CNode& pfrom, Peer& peer, const Bl // Might have collided, fall back to getdata now :( std::vector invs; invs.emplace_back(MSG_BLOCK | GetFetchFlags(peer), block_transactions.blockhash); - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::GETDATA, invs)); + MakeAndPushMessage(pfrom, NetMsgType::GETDATA, invs); } else { RemoveBlockRequest(block_transactions.blockhash, pfrom.GetId()); LogPrint(BCLog::NET, "Peer %d sent us a compact block but it failed to reconstruct, waiting on first download to complete\n", pfrom.GetId()); @@ -3441,10 +3434,8 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, pfrom.SetCommonVersion(greatest_common_version); pfrom.nVersion = nVersion; - const CNetMsgMaker msg_maker(greatest_common_version); - if (greatest_common_version >= WTXID_RELAY_VERSION) { - m_connman.PushMessage(&pfrom, msg_maker.Make(NetMsgType::WTXIDRELAY)); + MakeAndPushMessage(pfrom, NetMsgType::WTXIDRELAY); } // Signal ADDRv2 support (BIP155). @@ -3453,7 +3444,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, // implementations reject messages they don't know. As a courtesy, don't send // it to nodes with a version before 70016, as no software is known to support // BIP155 that doesn't announce at least that protocol version number. - m_connman.PushMessage(&pfrom, msg_maker.Make(NetMsgType::SENDADDRV2)); + MakeAndPushMessage(pfrom, NetMsgType::SENDADDRV2); } pfrom.m_has_all_wanted_services = HasAllDesirableServiceFlags(nServices); @@ -3493,12 +3484,12 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, if (tx_relay && WITH_LOCK(tx_relay->m_bloom_filter_mutex, return tx_relay->m_relay_txs) && !pfrom.IsAddrFetchConn() && !m_opts.ignore_incoming_txs) { const uint64_t recon_salt = m_txreconciliation->PreRegisterPeer(pfrom.GetId()); - m_connman.PushMessage(&pfrom, msg_maker.Make(NetMsgType::SENDTXRCNCL, - TXRECONCILIATION_VERSION, recon_salt)); + MakeAndPushMessage(pfrom, NetMsgType::SENDTXRCNCL, + TXRECONCILIATION_VERSION, recon_salt); } } - m_connman.PushMessage(&pfrom, msg_maker.Make(NetMsgType::VERACK)); + MakeAndPushMessage(pfrom, NetMsgType::VERACK); // Potentially mark this peer as a preferred download peer. { @@ -3522,7 +3513,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, // We skip this for block-relay-only peers. We want to avoid // potentially leaking addr information and we do not want to // indicate to the peer that we will participate in addr relay. - m_connman.PushMessage(&pfrom, CNetMsgMaker(greatest_common_version).Make(NetMsgType::GETADDR)); + MakeAndPushMessage(pfrom, NetMsgType::GETADDR); peer->m_getaddr_sent = true; // When requesting a getaddr, accept an additional MAX_ADDR_TO_SEND addresses in response // (bypassing the MAX_ADDR_PROCESSING_TOKEN_BUCKET limit). @@ -3568,7 +3559,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, // If the peer is old enough to have the old alert system, send it the final alert. if (greatest_common_version <= 70012) { const auto finalAlert{ParseHex("60010000000000000000000000ffffff7f00000000ffffff7ffeffff7f01ffffff7f00000000ffffff7f00ffffff7f002f555247454e543a20416c657274206b657920636f6d70726f6d697365642c2075706772616465207265717569726564004630440220653febd6410f470f6bae11cad19c48413becb1ac2c17f908fd0fd53bdc3abd5202206d0e9c96fe88d4a0f01ed9dedae2b6f9e00da94cad0fecaae66ecf689bf71b50")}; - m_connman.PushMessage(&pfrom, CNetMsgMaker(greatest_common_version).Make("alert", Span{finalAlert})); + MakeAndPushMessage(pfrom, "alert", Span{finalAlert}); } // Feeler connections exist only to verify if address is online. @@ -3585,9 +3576,6 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, return; } - // At this point, the outgoing message serialization version can't change. - const CNetMsgMaker msgMaker(pfrom.GetCommonVersion()); - if (msg_type == NetMsgType::VERACK) { if (pfrom.fSuccessfullyConnected) { LogPrint(BCLog::NET, "ignoring redundant verack message from peer=%d\n", pfrom.GetId()); @@ -3612,7 +3600,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, // cmpctblock messages. // We send this to non-NODE NETWORK peers as well, because // they may wish to request compact blocks from us - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::SENDCMPCT, /*high_bandwidth=*/false, /*version=*/CMPCTBLOCKS_VERSION)); + MakeAndPushMessage(pfrom, NetMsgType::SENDCMPCT, /*high_bandwidth=*/false, /*version=*/CMPCTBLOCKS_VERSION); } if (m_txreconciliation) { @@ -4119,7 +4107,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, LogPrint(BCLog::NET, "Ignoring getheaders from peer=%d because active chain has too little work; sending empty response\n", pfrom.GetId()); // Just respond with an empty headers message, to tell the peer to // go away but not treat us as unresponsive. - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::HEADERS, std::vector())); + MakeAndPushMessage(pfrom, NetMsgType::HEADERS, std::vector()); return; } @@ -4169,7 +4157,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, // will re-announce the new block via headers (or compact blocks again) // in the SendMessages logic. nodestate->pindexBestHeaderSent = pindex ? pindex : m_chainman.ActiveChain().Tip(); - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::HEADERS, TX_WITH_WITNESS(vHeaders))); + MakeAndPushMessage(pfrom, NetMsgType::HEADERS, TX_WITH_WITNESS(vHeaders)); return; } @@ -4471,7 +4459,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, // so we just grab the block via normal getdata std::vector vInv(1); vInv[0] = CInv(MSG_BLOCK | GetFetchFlags(*peer), blockhash); - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::GETDATA, vInv)); + MakeAndPushMessage(pfrom, NetMsgType::GETDATA, vInv); } return; } @@ -4508,7 +4496,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, // Duplicate txindexes, the block is now in-flight, so just request it std::vector vInv(1); vInv[0] = CInv(MSG_BLOCK | GetFetchFlags(*peer), blockhash); - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::GETDATA, vInv)); + MakeAndPushMessage(pfrom, NetMsgType::GETDATA, vInv); } else { // Give up for this peer and wait for other peer(s) RemoveBlockRequest(pindex->GetBlockHash(), pfrom.GetId()); @@ -4527,7 +4515,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, // We will try to round-trip any compact blocks we get on failure, // as long as it's first... req.blockhash = pindex->GetBlockHash(); - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::GETBLOCKTXN, req)); + MakeAndPushMessage(pfrom, NetMsgType::GETBLOCKTXN, req); } else if (pfrom.m_bip152_highbandwidth_to && (!pfrom.IsInboundConn() || IsBlockRequestedFromOutbound(blockhash) || @@ -4537,7 +4525,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, // - we already have an outbound attempt in flight(so we'll take what we can get), or // - it's not the final parallel download slot (which we may reserve for first outbound) req.blockhash = pindex->GetBlockHash(); - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::GETBLOCKTXN, req)); + MakeAndPushMessage(pfrom, NetMsgType::GETBLOCKTXN, req); } else { // Give up for this peer and wait for other peer(s) RemoveBlockRequest(pindex->GetBlockHash(), pfrom.GetId()); @@ -4566,7 +4554,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, // mempool will probably be useless - request the block normally std::vector vInv(1); vInv[0] = CInv(MSG_BLOCK | GetFetchFlags(*peer), blockhash); - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::GETDATA, vInv)); + MakeAndPushMessage(pfrom, NetMsgType::GETDATA, vInv); return; } else { // If this was an announce-cmpctblock, we want the same treatment as a header message @@ -4796,7 +4784,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, // it, if the remote node sends a ping once per second and this node takes 5 // seconds to respond to each, the 5th ping the remote sends would appear to // return very quickly. - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::PONG, nonce)); + MakeAndPushMessage(pfrom, NetMsgType::PONG, nonce); } return; } @@ -5297,7 +5285,6 @@ void PeerManagerImpl::MaybeSendPing(CNode& node_to, Peer& peer, std::chrono::mic return; } - const CNetMsgMaker msgMaker(node_to.GetCommonVersion()); bool pingSend = false; if (peer.m_ping_queued) { @@ -5319,11 +5306,11 @@ void PeerManagerImpl::MaybeSendPing(CNode& node_to, Peer& peer, std::chrono::mic peer.m_ping_start = now; if (node_to.GetCommonVersion() > BIP0031_VERSION) { peer.m_ping_nonce_sent = nonce; - m_connman.PushMessage(&node_to, msgMaker.Make(NetMsgType::PING, nonce)); + MakeAndPushMessage(node_to, NetMsgType::PING, nonce); } else { // Peer is too old to support ping command with nonce, pong will never arrive. peer.m_ping_nonce_sent = 0; - m_connman.PushMessage(&node_to, msgMaker.Make(NetMsgType::PING)); + MakeAndPushMessage(node_to, NetMsgType::PING); } } } @@ -5377,11 +5364,10 @@ void PeerManagerImpl::MaybeSendAddr(CNode& node, Peer& peer, std::chrono::micros // No addr messages to send if (peer.m_addrs_to_send.empty()) return; - CNetMsgMaker mm(node.GetCommonVersion()); if (peer.m_wants_addrv2) { - m_connman.PushMessage(&node, mm.Make(NetMsgType::ADDRV2, CAddress::V2_NETWORK(peer.m_addrs_to_send))); + MakeAndPushMessage(node, NetMsgType::ADDRV2, CAddress::V2_NETWORK(peer.m_addrs_to_send)); } else { - m_connman.PushMessage(&node, mm.Make(NetMsgType::ADDR, CAddress::V1_NETWORK(peer.m_addrs_to_send))); + MakeAndPushMessage(node, NetMsgType::ADDR, CAddress::V1_NETWORK(peer.m_addrs_to_send)); } peer.m_addrs_to_send.clear(); @@ -5406,7 +5392,7 @@ void PeerManagerImpl::MaybeSendSendHeaders(CNode& node, Peer& peer) // We send this to non-NODE NETWORK peers as well, because even // non-NODE NETWORK peers can announce blocks (such as pruning // nodes) - m_connman.PushMessage(&node, CNetMsgMaker(node.GetCommonVersion()).Make(NetMsgType::SENDHEADERS)); + MakeAndPushMessage(node, NetMsgType::SENDHEADERS); peer.m_sent_sendheaders = true; } } @@ -5441,7 +5427,7 @@ void PeerManagerImpl::MaybeSendFeefilter(CNode& pto, Peer& peer, std::chrono::mi // We always have a fee filter of at least the min relay fee filterToSend = std::max(filterToSend, m_mempool.m_min_relay_feerate.GetFeePerK()); if (filterToSend != peer.m_fee_filter_sent) { - m_connman.PushMessage(&pto, CNetMsgMaker(pto.GetCommonVersion()).Make(NetMsgType::FEEFILTER, filterToSend)); + MakeAndPushMessage(pto, NetMsgType::FEEFILTER, filterToSend); peer.m_fee_filter_sent = filterToSend; } peer.m_next_send_feefilter = GetExponentialRand(current_time, AVG_FEEFILTER_BROADCAST_INTERVAL); @@ -5518,9 +5504,6 @@ bool PeerManagerImpl::SendMessages(CNode* pto) if (!pto->fSuccessfullyConnected || pto->fDisconnect) return true; - // If we get here, the outgoing message serialization version is set and can't change. - const CNetMsgMaker msgMaker(pto->GetCommonVersion()); - const auto current_time{GetTime()}; if (pto->IsAddrFetchConn() && current_time - pto->m_connected > 10 * AVG_ADDRESS_BROADCAST_INTERVAL) { @@ -5675,17 +5658,17 @@ bool PeerManagerImpl::SendMessages(CNode* pto) { LOCK(m_most_recent_block_mutex); if (m_most_recent_block_hash == pBestIndex->GetBlockHash()) { - cached_cmpctblock_msg = msgMaker.Make(NetMsgType::CMPCTBLOCK, *m_most_recent_compact_block); + cached_cmpctblock_msg = NetMsg::Make(NetMsgType::CMPCTBLOCK, *m_most_recent_compact_block); } } if (cached_cmpctblock_msg.has_value()) { - m_connman.PushMessage(pto, std::move(cached_cmpctblock_msg.value())); + PushMessage(*pto, std::move(cached_cmpctblock_msg.value())); } else { CBlock block; const bool ret{m_chainman.m_blockman.ReadBlockFromDisk(block, *pBestIndex)}; assert(ret); CBlockHeaderAndShortTxIDs cmpctblock{block}; - m_connman.PushMessage(pto, msgMaker.Make(NetMsgType::CMPCTBLOCK, cmpctblock)); + MakeAndPushMessage(*pto, NetMsgType::CMPCTBLOCK, cmpctblock); } state.pindexBestHeaderSent = pBestIndex; } else if (peer->m_prefers_headers) { @@ -5698,7 +5681,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto) LogPrint(BCLog::NET, "%s: sending header %s to peer=%d\n", __func__, vHeaders.front().GetHash().ToString(), pto->GetId()); } - m_connman.PushMessage(pto, msgMaker.Make(NetMsgType::HEADERS, TX_WITH_WITNESS(vHeaders))); + MakeAndPushMessage(*pto, NetMsgType::HEADERS, TX_WITH_WITNESS(vHeaders)); state.pindexBestHeaderSent = pBestIndex; } else fRevertToInv = true; @@ -5743,7 +5726,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto) for (const uint256& hash : peer->m_blocks_for_inv_relay) { vInv.emplace_back(MSG_BLOCK, hash); if (vInv.size() == MAX_INV_SZ) { - m_connman.PushMessage(pto, msgMaker.Make(NetMsgType::INV, vInv)); + MakeAndPushMessage(*pto, NetMsgType::INV, vInv); vInv.clear(); } } @@ -5796,7 +5779,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto) tx_relay->m_tx_inventory_known_filter.insert(inv.hash); vInv.push_back(inv); if (vInv.size() == MAX_INV_SZ) { - m_connman.PushMessage(pto, msgMaker.Make(NetMsgType::INV, vInv)); + MakeAndPushMessage(*pto, NetMsgType::INV, vInv); vInv.clear(); } } @@ -5848,7 +5831,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto) vInv.push_back(inv); nRelayedTransactions++; if (vInv.size() == MAX_INV_SZ) { - m_connman.PushMessage(pto, msgMaker.Make(NetMsgType::INV, vInv)); + MakeAndPushMessage(*pto, NetMsgType::INV, vInv); vInv.clear(); } tx_relay->m_tx_inventory_known_filter.insert(hash); @@ -5860,7 +5843,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto) } } if (!vInv.empty()) - m_connman.PushMessage(pto, msgMaker.Make(NetMsgType::INV, vInv)); + MakeAndPushMessage(*pto, NetMsgType::INV, vInv); // Detect whether we're stalling auto stalling_timeout = m_block_stalling_timeout.load(); @@ -5980,7 +5963,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto) gtxid.GetHash().ToString(), pto->GetId()); vGetData.emplace_back(gtxid.IsWtxid() ? MSG_WTX : (MSG_TX | GetFetchFlags(*peer)), gtxid.GetHash()); if (vGetData.size() >= MAX_GETDATA_SZ) { - m_connman.PushMessage(pto, msgMaker.Make(NetMsgType::GETDATA, vGetData)); + MakeAndPushMessage(*pto, NetMsgType::GETDATA, vGetData); vGetData.clear(); } m_txrequest.RequestedTx(pto->GetId(), gtxid.GetHash(), current_time + GETDATA_TX_INTERVAL); @@ -5993,7 +5976,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto) if (!vGetData.empty()) - m_connman.PushMessage(pto, msgMaker.Make(NetMsgType::GETDATA, vGetData)); + MakeAndPushMessage(*pto, NetMsgType::GETDATA, vGetData); } // release cs_main MaybeSendFeefilter(*pto, *peer, current_time); return true; diff --git a/src/netmessagemaker.h b/src/netmessagemaker.h index ede03aa204c40..d04867e45e99b 100644 --- a/src/netmessagemaker.h +++ b/src/netmessagemaker.h @@ -9,19 +9,15 @@ #include #include -class CNetMsgMaker -{ -public: - explicit CNetMsgMaker(int /*unused*/) {} - +namespace NetMsg { template - CSerializedNetMsg Make(std::string msg_type, Args&&... args) const + CSerializedNetMsg Make(std::string msg_type, Args&&... args) { CSerializedNetMsg msg; msg.m_type = std::move(msg_type); VectorWriter{msg.data, 0, std::forward(args)...}; return msg; } -}; +} // namespace NetMsg #endif // BITCOIN_NETMESSAGEMAKER_H diff --git a/src/test/fuzz/p2p_transport_serialization.cpp b/src/test/fuzz/p2p_transport_serialization.cpp index 21d8dab53605e..6af6aa1d18939 100644 --- a/src/test/fuzz/p2p_transport_serialization.cpp +++ b/src/test/fuzz/p2p_transport_serialization.cpp @@ -88,7 +88,7 @@ FUZZ_TARGET(p2p_transport_serialization, .init = initialize_p2p_transport_serial assert(msg.m_time == m_time); std::vector header; - auto msg2 = CNetMsgMaker{msg.m_recv.GetVersion()}.Make(msg.m_type, Span{msg.m_recv}); + auto msg2 = NetMsg::Make(msg.m_type, Span{msg.m_recv}); bool queued = send_transport.SetMessageToSend(msg2); assert(queued); std::optional known_more; diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp index 48e0706a53bd0..3c9227cfc41b7 100644 --- a/src/test/net_tests.cpp +++ b/src/test/net_tests.cpp @@ -845,7 +845,6 @@ BOOST_AUTO_TEST_CASE(initial_advertise_from_version_message) const uint64_t services{NODE_NETWORK | NODE_WITNESS}; const int64_t time{0}; - const CNetMsgMaker msg_maker{PROTOCOL_VERSION}; // Force ChainstateManager::IsInitialBlockDownload() to return false. // Otherwise PushAddress() isn't called by PeerManager::ProcessMessage(). @@ -858,13 +857,13 @@ 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, CAddress::V1_NETWORK(peer_us)); + NetMsg::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( peer, NetMsgType::VERSION, msg_version_stream, time_received_dummy, interrupt_dummy); - const auto msg_verack = msg_maker.Make(NetMsgType::VERACK); + const auto msg_verack = NetMsg::Make(NetMsgType::VERACK); CDataStream msg_verack_stream{msg_verack.data, SER_NETWORK, PROTOCOL_VERSION}; // Will set peer.fSuccessfullyConnected to true (necessary in SendMessages()). diff --git a/src/test/util/net.cpp b/src/test/util/net.cpp index e0404e33ed556..9257a4964ac74 100644 --- a/src/test/util/net.cpp +++ b/src/test/util/net.cpp @@ -26,13 +26,12 @@ void ConnmanTestMsg::Handshake(CNode& node, { auto& peerman{static_cast(*m_msgproc)}; auto& connman{*this}; - const CNetMsgMaker mm{0}; peerman.InitializeNode(node, local_services); FlushSendBuffer(node); // Drop the version message added by InitializeNode. CSerializedNetMsg msg_version{ - mm.Make(NetMsgType::VERSION, + NetMsg::Make(NetMsgType::VERSION, version, // Using>(remote_services), // int64_t{}, // dummy time @@ -59,7 +58,7 @@ void ConnmanTestMsg::Handshake(CNode& node, assert(statestats.m_relay_txs == (relay_txs && !node.IsBlockOnlyConn())); assert(statestats.their_services == remote_services); if (successfully_connected) { - CSerializedNetMsg msg_verack{mm.Make(NetMsgType::VERACK)}; + CSerializedNetMsg msg_verack{NetMsg::Make(NetMsgType::VERACK)}; (void)connman.ReceiveMsgFrom(node, std::move(msg_verack)); node.fPauseSend = false; connman.ProcessMessagesOnce(node); From 9e58c5bcd96e7ff2062274868814ccae0626589e Mon Sep 17 00:00:00 2001 From: dergoegge Date: Wed, 11 Oct 2023 14:53:04 +0100 Subject: [PATCH 05/19] Use Txid in COutpoint --- src/bench/disconnected_transactions.cpp | 2 +- src/bench/duplicate_inputs.cpp | 2 +- src/bench/mempool_stress.cpp | 2 +- src/bitcoin-tx.cpp | 4 +-- src/coins.cpp | 4 +-- src/coins.h | 2 +- src/common/bloom.cpp | 4 +-- src/kernel/coinstats.cpp | 4 +-- src/node/transaction.cpp | 2 +- src/primitives/transaction.cpp | 2 +- src/primitives/transaction.h | 9 +++--- src/qt/coincontroldialog.cpp | 8 +++--- src/rest.cpp | 4 +-- src/rpc/blockchain.cpp | 4 +-- src/rpc/mempool.cpp | 2 +- src/rpc/rawtransaction_util.cpp | 4 +-- src/rpc/txoutproof.cpp | 2 +- src/test/blockencodings_tests.cpp | 4 +-- src/test/bloom_tests.cpp | 16 +++++------ src/test/coins_tests.cpp | 14 +++++----- src/test/fuzz/coinscache_sim.cpp | 4 ++- src/test/fuzz/mini_miner.cpp | 2 +- src/test/fuzz/package_eval.cpp | 4 +-- src/test/fuzz/tx_pool.cpp | 12 ++++---- src/test/fuzz/txorphan.cpp | 2 +- src/test/fuzz/util.cpp | 4 +-- src/test/fuzz/util.h | 2 +- src/test/miner_tests.cpp | 28 +++++++++---------- src/test/miniminer_tests.cpp | 20 +++++++++---- src/test/orphanage_tests.cpp | 2 +- src/test/sighash_tests.cpp | 2 +- src/test/transaction_tests.cpp | 9 +++--- src/test/txpackage_tests.cpp | 6 ++-- src/test/util/coins.cpp | 3 +- src/test/util/setup_common.cpp | 2 +- .../validation_chainstatemanager_tests.cpp | 2 +- src/txmempool.cpp | 2 +- src/util/transaction_identifier.h | 5 ++++ src/validation.cpp | 4 +-- src/wallet/receive.cpp | 4 +-- src/wallet/rpc/coins.cpp | 2 +- src/wallet/rpc/spend.cpp | 2 +- src/wallet/spend.cpp | 4 +-- src/wallet/test/feebumper_tests.cpp | 2 +- src/wallet/test/fuzz/notifications.cpp | 2 +- src/wallet/test/wallet_tests.cpp | 6 ++-- src/wallet/wallet.cpp | 4 +-- src/wallet/walletdb.cpp | 2 +- 48 files changed, 124 insertions(+), 114 deletions(-) diff --git a/src/bench/disconnected_transactions.cpp b/src/bench/disconnected_transactions.cpp index 91ce904dd9415..f48175d4720e9 100644 --- a/src/bench/disconnected_transactions.cpp +++ b/src/bench/disconnected_transactions.cpp @@ -28,7 +28,7 @@ struct ReorgTxns { static BlockTxns CreateRandomTransactions(size_t num_txns) { // Ensure every transaction has a different txid by having each one spend the previous one. - static uint256 prevout_hash{uint256::ZERO}; + static Txid prevout_hash{}; BlockTxns txns; txns.reserve(num_txns); diff --git a/src/bench/duplicate_inputs.cpp b/src/bench/duplicate_inputs.cpp index a1c8d4d942023..b56054ae89d95 100644 --- a/src/bench/duplicate_inputs.cpp +++ b/src/bench/duplicate_inputs.cpp @@ -47,7 +47,7 @@ static void DuplicateInputs(benchmark::Bench& bench) uint64_t n_inputs = (((MAX_BLOCK_SERIALIZED_SIZE / WITNESS_SCALE_FACTOR) - (CTransaction(coinbaseTx).GetTotalSize() + CTransaction(naughtyTx).GetTotalSize())) / 41) - 100; for (uint64_t x = 0; x < (n_inputs - 1); ++x) { - naughtyTx.vin.emplace_back(GetRandHash(), 0, CScript(), 0); + naughtyTx.vin.emplace_back(Txid::FromUint256(GetRandHash()), 0, CScript(), 0); } naughtyTx.vin.emplace_back(naughtyTx.vin.back()); diff --git a/src/bench/mempool_stress.cpp b/src/bench/mempool_stress.cpp index 993db666531ef..fe3e204fb3f4e 100644 --- a/src/bench/mempool_stress.cpp +++ b/src/bench/mempool_stress.cpp @@ -56,7 +56,7 @@ static std::vector CreateOrderedCoins(FastRandomContext& det_ra for (size_t ancestor = 0; ancestor < n_ancestors && !available_coins.empty(); ++ancestor){ size_t idx = det_rand.randrange(available_coins.size()); Available coin = available_coins[idx]; - uint256 hash = coin.ref->GetHash(); + Txid hash = coin.ref->GetHash(); // biased towards taking min_ancestors parents, but maybe more size_t n_to_take = det_rand.randrange(2) == 0 ? min_ancestors : diff --git a/src/bitcoin-tx.cpp b/src/bitcoin-tx.cpp index 103d8885db327..8fe2881f6f2f6 100644 --- a/src/bitcoin-tx.cpp +++ b/src/bitcoin-tx.cpp @@ -280,7 +280,7 @@ static void MutateTxAddInput(CMutableTransaction& tx, const std::string& strInpu } // append to transaction input list - CTxIn txin(txid, vout, CScript(), nSequenceIn); + CTxIn txin(Txid::FromUint256(txid), vout, CScript(), nSequenceIn); tx.vin.push_back(txin); } @@ -629,7 +629,7 @@ static void MutateTxSign(CMutableTransaction& tx, const std::string& flagStr) if (nOut < 0) throw std::runtime_error("vout cannot be negative"); - COutPoint out(txid, nOut); + COutPoint out(Txid::FromUint256(txid), nOut); std::vector pkData(ParseHexUV(prevOut["scriptPubKey"], "scriptPubKey")); CScript scriptPubKey(pkData.begin(), pkData.end()); diff --git a/src/coins.cpp b/src/coins.cpp index f576fae748800..b62653e0de961 100644 --- a/src/coins.cpp +++ b/src/coins.cpp @@ -116,7 +116,7 @@ void CCoinsViewCache::EmplaceCoinInternalDANGER(COutPoint&& outpoint, Coin&& coi void AddCoins(CCoinsViewCache& cache, const CTransaction &tx, int nHeight, bool check_for_overwrite) { bool fCoinbase = tx.IsCoinBase(); - const uint256& txid = tx.GetHash(); + const Txid& txid = tx.GetHash(); for (size_t i = 0; i < tx.vout.size(); ++i) { bool overwrite = check_for_overwrite ? cache.HaveCoin(COutPoint(txid, i)) : fCoinbase; // Coinbase transactions can always be overwritten, in order to correctly @@ -341,7 +341,7 @@ void CCoinsViewCache::SanityCheck() const static const size_t MIN_TRANSACTION_OUTPUT_WEIGHT = WITNESS_SCALE_FACTOR * ::GetSerializeSize(CTxOut()); static const size_t MAX_OUTPUTS_PER_BLOCK = MAX_BLOCK_WEIGHT / MIN_TRANSACTION_OUTPUT_WEIGHT; -const Coin& AccessByTxid(const CCoinsViewCache& view, const uint256& txid) +const Coin& AccessByTxid(const CCoinsViewCache& view, const Txid& txid) { COutPoint iter(txid, 0); while (iter.n < MAX_OUTPUTS_PER_BLOCK) { diff --git a/src/coins.h b/src/coins.h index a6cbb0313393c..63a27fe012ad2 100644 --- a/src/coins.h +++ b/src/coins.h @@ -364,7 +364,7 @@ void AddCoins(CCoinsViewCache& cache, const CTransaction& tx, int nHeight, bool //! This function can be quite expensive because in the event of a transaction //! which is not found in the cache, it can cause up to MAX_OUTPUTS_PER_BLOCK //! lookups to database, so it should be used with care. -const Coin& AccessByTxid(const CCoinsViewCache& cache, const uint256& txid); +const Coin& AccessByTxid(const CCoinsViewCache& cache, const Txid& txid); /** * This is a minimally invasive approach to shutdown on LevelDB read errors from the diff --git a/src/common/bloom.cpp b/src/common/bloom.cpp index 5c3ad882a1c47..ca6af90b7601d 100644 --- a/src/common/bloom.cpp +++ b/src/common/bloom.cpp @@ -98,8 +98,8 @@ bool CBloomFilter::IsRelevantAndUpdate(const CTransaction& tx) // for finding tx when they appear in a block if (vData.empty()) // zero-size = "match-all" filter return true; - const uint256& hash = tx.GetHash(); - if (contains(hash)) + const Txid& hash = tx.GetHash(); + if (contains(hash.ToUint256())) fFound = true; for (unsigned int i = 0; i < tx.vout.size(); i++) diff --git a/src/kernel/coinstats.cpp b/src/kernel/coinstats.cpp index 302638cdbea8b..ff8a33e80479b 100644 --- a/src/kernel/coinstats.cpp +++ b/src/kernel/coinstats.cpp @@ -89,7 +89,7 @@ static void ApplyCoinHash(std::nullptr_t, const COutPoint& outpoint, const Coin& //! construction could cause a previously invalid (and potentially malicious) //! UTXO snapshot to be considered valid. template -static void ApplyHash(T& hash_obj, const uint256& hash, const std::map& outputs) +static void ApplyHash(T& hash_obj, const Txid& hash, const std::map& outputs) { for (auto it = outputs.begin(); it != outputs.end(); ++it) { COutPoint outpoint = COutPoint(hash, it->first); @@ -118,7 +118,7 @@ static bool ComputeUTXOStats(CCoinsView* view, CCoinsStats& stats, T hash_obj, c std::unique_ptr pcursor(view->Cursor()); assert(pcursor); - uint256 prevkey; + Txid prevkey; std::map outputs; while (pcursor->Valid()) { if (interruption_point) interruption_point(); diff --git a/src/node/transaction.cpp b/src/node/transaction.cpp index 026c8084dd75f..e8ab2326c1466 100644 --- a/src/node/transaction.cpp +++ b/src/node/transaction.cpp @@ -40,7 +40,7 @@ TransactionError BroadcastTransaction(NodeContext& node, const CTransactionRef t assert(node.peerman); std::promise promise; - uint256 txid = tx->GetHash(); + Txid txid = tx->GetHash(); uint256 wtxid = tx->GetWitnessHash(); bool callback_set = false; diff --git a/src/primitives/transaction.cpp b/src/primitives/transaction.cpp index 82fb59d3494d1..002c922c0de7b 100644 --- a/src/primitives/transaction.cpp +++ b/src/primitives/transaction.cpp @@ -29,7 +29,7 @@ CTxIn::CTxIn(COutPoint prevoutIn, CScript scriptSigIn, uint32_t nSequenceIn) nSequence = nSequenceIn; } -CTxIn::CTxIn(uint256 hashPrevTx, uint32_t nOut, CScript scriptSigIn, uint32_t nSequenceIn) +CTxIn::CTxIn(Txid hashPrevTx, uint32_t nOut, CScript scriptSigIn, uint32_t nSequenceIn) { prevout = COutPoint(hashPrevTx, nOut); scriptSig = scriptSigIn; diff --git a/src/primitives/transaction.h b/src/primitives/transaction.h index ad190f7d7f443..e0c0c860c6a9b 100644 --- a/src/primitives/transaction.h +++ b/src/primitives/transaction.h @@ -28,13 +28,13 @@ class COutPoint { public: - uint256 hash; + Txid hash; uint32_t n; static constexpr uint32_t NULL_INDEX = std::numeric_limits::max(); COutPoint(): n(NULL_INDEX) { } - COutPoint(const uint256& hashIn, uint32_t nIn): hash(hashIn), n(nIn) { } + COutPoint(const Txid& hashIn, uint32_t nIn): hash(hashIn), n(nIn) { } SERIALIZE_METHODS(COutPoint, obj) { READWRITE(obj.hash, obj.n); } @@ -43,8 +43,7 @@ class COutPoint friend bool operator<(const COutPoint& a, const COutPoint& b) { - int cmp = a.hash.Compare(b.hash); - return cmp < 0 || (cmp == 0 && a.n < b.n); + return std::tie(a.hash, a.n) < std::tie(b.hash, b.n); } friend bool operator==(const COutPoint& a, const COutPoint& b) @@ -125,7 +124,7 @@ class CTxIn } explicit CTxIn(COutPoint prevoutIn, CScript scriptSigIn=CScript(), uint32_t nSequenceIn=SEQUENCE_FINAL); - CTxIn(uint256 hashPrevTx, uint32_t nOut, CScript scriptSigIn=CScript(), uint32_t nSequenceIn=SEQUENCE_FINAL); + CTxIn(Txid hashPrevTx, uint32_t nOut, CScript scriptSigIn=CScript(), uint32_t nSequenceIn=SEQUENCE_FINAL); SERIALIZE_METHODS(CTxIn, obj) { READWRITE(obj.prevout, obj.scriptSig, obj.nSequence); } diff --git a/src/qt/coincontroldialog.cpp b/src/qt/coincontroldialog.cpp index 70aa9bc8da9ea..9e49fd87fb8e3 100644 --- a/src/qt/coincontroldialog.cpp +++ b/src/qt/coincontroldialog.cpp @@ -181,7 +181,7 @@ void CoinControlDialog::showMenu(const QPoint &point) if (item->data(COLUMN_ADDRESS, TxHashRole).toString().length() == 64) // transaction hash is 64 characters (this means it is a child node, so it is not a parent node in tree mode) { m_copy_transaction_outpoint_action->setEnabled(true); - if (model->wallet().isLockedCoin(COutPoint(uint256S(item->data(COLUMN_ADDRESS, TxHashRole).toString().toStdString()), item->data(COLUMN_ADDRESS, VOutRole).toUInt()))) + if (model->wallet().isLockedCoin(COutPoint(TxidFromString(item->data(COLUMN_ADDRESS, TxHashRole).toString().toStdString()), item->data(COLUMN_ADDRESS, VOutRole).toUInt()))) { lockAction->setEnabled(false); unlockAction->setEnabled(true); @@ -244,7 +244,7 @@ void CoinControlDialog::lockCoin() if (contextMenuItem->checkState(COLUMN_CHECKBOX) == Qt::Checked) contextMenuItem->setCheckState(COLUMN_CHECKBOX, Qt::Unchecked); - COutPoint outpt(uint256S(contextMenuItem->data(COLUMN_ADDRESS, TxHashRole).toString().toStdString()), contextMenuItem->data(COLUMN_ADDRESS, VOutRole).toUInt()); + COutPoint outpt(TxidFromString(contextMenuItem->data(COLUMN_ADDRESS, TxHashRole).toString().toStdString()), contextMenuItem->data(COLUMN_ADDRESS, VOutRole).toUInt()); model->wallet().lockCoin(outpt, /* write_to_db = */ true); contextMenuItem->setDisabled(true); contextMenuItem->setIcon(COLUMN_CHECKBOX, platformStyle->SingleColorIcon(":/icons/lock_closed")); @@ -254,7 +254,7 @@ void CoinControlDialog::lockCoin() // context menu action: unlock coin void CoinControlDialog::unlockCoin() { - COutPoint outpt(uint256S(contextMenuItem->data(COLUMN_ADDRESS, TxHashRole).toString().toStdString()), contextMenuItem->data(COLUMN_ADDRESS, VOutRole).toUInt()); + COutPoint outpt(TxidFromString(contextMenuItem->data(COLUMN_ADDRESS, TxHashRole).toString().toStdString()), contextMenuItem->data(COLUMN_ADDRESS, VOutRole).toUInt()); model->wallet().unlockCoin(outpt); contextMenuItem->setDisabled(false); contextMenuItem->setIcon(COLUMN_CHECKBOX, QIcon()); @@ -346,7 +346,7 @@ void CoinControlDialog::viewItemChanged(QTreeWidgetItem* item, int column) { if (column == COLUMN_CHECKBOX && item->data(COLUMN_ADDRESS, TxHashRole).toString().length() == 64) // transaction hash is 64 characters (this means it is a child node, so it is not a parent node in tree mode) { - COutPoint outpt(uint256S(item->data(COLUMN_ADDRESS, TxHashRole).toString().toStdString()), item->data(COLUMN_ADDRESS, VOutRole).toUInt()); + COutPoint outpt(TxidFromString(item->data(COLUMN_ADDRESS, TxHashRole).toString().toStdString()), item->data(COLUMN_ADDRESS, VOutRole).toUInt()); if (item->checkState(COLUMN_CHECKBOX) == Qt::Unchecked) m_coin_control.UnSelect(outpt); diff --git a/src/rest.cpp b/src/rest.cpp index 3894b5141d5a7..bb54e780b241b 100644 --- a/src/rest.cpp +++ b/src/rest.cpp @@ -789,7 +789,6 @@ static bool rest_getutxos(const std::any& context, HTTPRequest* req, const std:: for (size_t i = (fCheckMemPool) ? 1 : 0; i < uriParts.size(); i++) { - uint256 txid; int32_t nOutput; std::string strTxid = uriParts[i].substr(0, uriParts[i].find('-')); std::string strOutput = uriParts[i].substr(uriParts[i].find('-')+1); @@ -797,8 +796,7 @@ static bool rest_getutxos(const std::any& context, HTTPRequest* req, const std:: if (!ParseInt32(strOutput, &nOutput) || !IsHex(strTxid)) return RESTERR(req, HTTP_BAD_REQUEST, "Parse error"); - txid.SetHex(strTxid); - vOutPoints.emplace_back(txid, (uint32_t)nOutput); + vOutPoints.emplace_back(TxidFromString(strTxid), (uint32_t)nOutput); } if (vOutPoints.size() > 0) diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index 6a3aa397d917d..be6a8c47fd333 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -1057,7 +1057,7 @@ static RPCHelpMan gettxout() UniValue ret(UniValue::VOBJ); - uint256 hash(ParseHashV(request.params[0], "txid")); + auto hash{Txid::FromUint256(ParseHashV(request.params[0], "txid"))}; COutPoint out{hash, request.params[1].getInt()}; bool fMempool = true; if (!request.params[2].isNull()) @@ -2014,7 +2014,7 @@ bool FindScriptPubKey(std::atomic& scan_progress, const std::atomic& } if (count % 256 == 0) { // update progress reference every 256 item - uint32_t high = 0x100 * *key.hash.begin() + *(key.hash.begin() + 1); + uint32_t high = 0x100 * *UCharCast(key.hash.begin()) + *(UCharCast(key.hash.begin()) + 1); scan_progress = (int)(high * 100.0 / 65536.0 + 0.5); } if (needles.count(coin.out.scriptPubKey)) { diff --git a/src/rpc/mempool.cpp b/src/rpc/mempool.cpp index bf60eae433232..6cbc96c5ec3b6 100644 --- a/src/rpc/mempool.cpp +++ b/src/rpc/mempool.cpp @@ -633,7 +633,7 @@ static RPCHelpMan gettxspendingprevout() {"vout", UniValueType(UniValue::VNUM)}, }, /*fAllowNull=*/false, /*fStrict=*/true); - const uint256 txid(ParseHashO(o, "txid")); + const Txid txid = Txid::FromUint256(ParseHashO(o, "txid")); const int nOutput{o.find_value("vout").getInt()}; if (nOutput < 0) { throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, vout cannot be negative"); diff --git a/src/rpc/rawtransaction_util.cpp b/src/rpc/rawtransaction_util.cpp index 3a6fa39e4dc70..c471986a44810 100644 --- a/src/rpc/rawtransaction_util.cpp +++ b/src/rpc/rawtransaction_util.cpp @@ -34,7 +34,7 @@ void AddInputs(CMutableTransaction& rawTx, const UniValue& inputs_in, std::optio const UniValue& input = inputs[idx]; const UniValue& o = input.get_obj(); - uint256 txid = ParseHashO(o, "txid"); + Txid txid = Txid::FromUint256(ParseHashO(o, "txid")); const UniValue& vout_v = o.find_value("vout"); if (!vout_v.isNum()) @@ -185,7 +185,7 @@ void ParsePrevouts(const UniValue& prevTxsUnival, FillableSigningProvider* keyst {"scriptPubKey", UniValueType(UniValue::VSTR)}, }); - uint256 txid = ParseHashO(prevOut, "txid"); + Txid txid = Txid::FromUint256(ParseHashO(prevOut, "txid")); int nOut = prevOut.find_value("vout").getInt(); if (nOut < 0) { diff --git a/src/rpc/txoutproof.cpp b/src/rpc/txoutproof.cpp index d74959cecce91..46a22e9338843 100644 --- a/src/rpc/txoutproof.cpp +++ b/src/rpc/txoutproof.cpp @@ -69,7 +69,7 @@ static RPCHelpMan gettxoutproof() // Loop through txids and try to find which block they're in. Exit loop once a block is found. for (const auto& tx : setTxids) { - const Coin& coin = AccessByTxid(active_chainstate.CoinsTip(), tx); + const Coin& coin = AccessByTxid(active_chainstate.CoinsTip(), Txid::FromUint256(tx)); if (!coin.IsSpent()) { pblockindex = active_chainstate.m_chain[coin.nHeight]; break; diff --git a/src/test/blockencodings_tests.cpp b/src/test/blockencodings_tests.cpp index e4ef019daf60f..7968966303728 100644 --- a/src/test/blockencodings_tests.cpp +++ b/src/test/blockencodings_tests.cpp @@ -32,13 +32,13 @@ static CBlock BuildBlockTestCase() { block.hashPrevBlock = InsecureRand256(); block.nBits = 0x207fffff; - tx.vin[0].prevout.hash = InsecureRand256(); + tx.vin[0].prevout.hash = Txid::FromUint256(InsecureRand256()); tx.vin[0].prevout.n = 0; block.vtx[1] = MakeTransactionRef(tx); tx.vin.resize(10); for (size_t i = 0; i < tx.vin.size(); i++) { - tx.vin[i].prevout.hash = InsecureRand256(); + tx.vin[i].prevout.hash = Txid::FromUint256(InsecureRand256()); tx.vin[i].prevout.n = 0; } block.vtx[2] = MakeTransactionRef(tx); diff --git a/src/test/bloom_tests.cpp b/src/test/bloom_tests.cpp index 23cbe921ba4a0..daa5a56a14e30 100644 --- a/src/test/bloom_tests.cpp +++ b/src/test/bloom_tests.cpp @@ -136,11 +136,11 @@ BOOST_AUTO_TEST_CASE(bloom_match) BOOST_CHECK_MESSAGE(filter.IsRelevantAndUpdate(tx), "Simple Bloom filter didn't match output address"); filter = CBloomFilter(10, 0.000001, 0, BLOOM_UPDATE_ALL); - filter.insert(COutPoint(uint256S("0x90c122d70786e899529d71dbeba91ba216982fb6ba58f3bdaab65e73b7e9260b"), 0)); + filter.insert(COutPoint(TxidFromString("0x90c122d70786e899529d71dbeba91ba216982fb6ba58f3bdaab65e73b7e9260b"), 0)); BOOST_CHECK_MESSAGE(filter.IsRelevantAndUpdate(tx), "Simple Bloom filter didn't match COutPoint"); filter = CBloomFilter(10, 0.000001, 0, BLOOM_UPDATE_ALL); - COutPoint prevOutPoint(uint256S("0x90c122d70786e899529d71dbeba91ba216982fb6ba58f3bdaab65e73b7e9260b"), 0); + COutPoint prevOutPoint(TxidFromString("0x90c122d70786e899529d71dbeba91ba216982fb6ba58f3bdaab65e73b7e9260b"), 0); { std::vector data(32 + sizeof(unsigned int)); memcpy(data.data(), prevOutPoint.hash.begin(), 32); @@ -158,11 +158,11 @@ BOOST_AUTO_TEST_CASE(bloom_match) BOOST_CHECK_MESSAGE(!filter.IsRelevantAndUpdate(tx), "Simple Bloom filter matched random address"); filter = CBloomFilter(10, 0.000001, 0, BLOOM_UPDATE_ALL); - filter.insert(COutPoint(uint256S("0x90c122d70786e899529d71dbeba91ba216982fb6ba58f3bdaab65e73b7e9260b"), 1)); + filter.insert(COutPoint(TxidFromString("0x90c122d70786e899529d71dbeba91ba216982fb6ba58f3bdaab65e73b7e9260b"), 1)); BOOST_CHECK_MESSAGE(!filter.IsRelevantAndUpdate(tx), "Simple Bloom filter matched COutPoint for an output we didn't care about"); filter = CBloomFilter(10, 0.000001, 0, BLOOM_UPDATE_ALL); - filter.insert(COutPoint(uint256S("0x000000d70786e899529d71dbeba91ba216982fb6ba58f3bdaab65e73b7e9260b"), 0)); + filter.insert(COutPoint(TxidFromString("0x000000d70786e899529d71dbeba91ba216982fb6ba58f3bdaab65e73b7e9260b"), 0)); BOOST_CHECK_MESSAGE(!filter.IsRelevantAndUpdate(tx), "Simple Bloom filter matched COutPoint for an output we didn't care about"); } @@ -414,9 +414,9 @@ BOOST_AUTO_TEST_CASE(merkle_block_4_test_p2pubkey_only) BOOST_CHECK(merkleBlock.header.GetHash() == block.GetHash()); // We should match the generation outpoint - BOOST_CHECK(filter.contains(COutPoint(uint256S("0x147caa76786596590baa4e98f5d9f48b86c7765e489f7a6ff3360fe5c674360b"), 0))); + BOOST_CHECK(filter.contains(COutPoint(TxidFromString("0x147caa76786596590baa4e98f5d9f48b86c7765e489f7a6ff3360fe5c674360b"), 0))); // ... but not the 4th transaction's output (its not pay-2-pubkey) - BOOST_CHECK(!filter.contains(COutPoint(uint256S("0x02981fa052f0481dbc5868f4fc2166035a10f27a03cfd2de67326471df5bc041"), 0))); + BOOST_CHECK(!filter.contains(COutPoint(TxidFromString("0x02981fa052f0481dbc5868f4fc2166035a10f27a03cfd2de67326471df5bc041"), 0))); } BOOST_AUTO_TEST_CASE(merkle_block_4_test_update_none) @@ -437,8 +437,8 @@ BOOST_AUTO_TEST_CASE(merkle_block_4_test_update_none) BOOST_CHECK(merkleBlock.header.GetHash() == block.GetHash()); // We shouldn't match any outpoints (UPDATE_NONE) - BOOST_CHECK(!filter.contains(COutPoint(uint256S("0x147caa76786596590baa4e98f5d9f48b86c7765e489f7a6ff3360fe5c674360b"), 0))); - BOOST_CHECK(!filter.contains(COutPoint(uint256S("0x02981fa052f0481dbc5868f4fc2166035a10f27a03cfd2de67326471df5bc041"), 0))); + BOOST_CHECK(!filter.contains(COutPoint(TxidFromString("0x147caa76786596590baa4e98f5d9f48b86c7765e489f7a6ff3360fe5c674360b"), 0))); + BOOST_CHECK(!filter.contains(COutPoint(TxidFromString("0x02981fa052f0481dbc5868f4fc2166035a10f27a03cfd2de67326471df5bc041"), 0))); } static std::vector RandomData() diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp index 12dc4d1ccc478..b6d3e7d567671 100644 --- a/src/test/coins_tests.cpp +++ b/src/test/coins_tests.cpp @@ -137,16 +137,16 @@ void SimulationTest(CCoinsView* base, bool fake_best_block) stack.push_back(std::make_unique(base)); // Start with one cache. // Use a limited set of random transaction ids, so we do test overwriting entries. - std::vector txids; + std::vector txids; txids.resize(NUM_SIMULATION_ITERATIONS / 8); for (unsigned int i = 0; i < txids.size(); i++) { - txids[i] = InsecureRand256(); + txids[i] = Txid::FromUint256(InsecureRand256()); } for (unsigned int i = 0; i < NUM_SIMULATION_ITERATIONS; i++) { // Do a random modification. { - uint256 txid = txids[InsecureRandRange(txids.size())]; // txid we're going to modify in this iteration. + auto txid = txids[InsecureRandRange(txids.size())]; // txid we're going to modify in this iteration. Coin& coin = result[COutPoint(txid, 0)]; // Determine whether to test HaveCoin before or after Access* (or both). As these functions @@ -290,7 +290,7 @@ UtxoData utxoData; UtxoData::iterator FindRandomFrom(const std::set &utxoSet) { assert(utxoSet.size()); - auto utxoSetIt = utxoSet.lower_bound(COutPoint(InsecureRand256(), 0)); + auto utxoSetIt = utxoSet.lower_bound(COutPoint(Txid::FromUint256(InsecureRand256()), 0)); if (utxoSetIt == utxoSet.end()) { utxoSetIt = utxoSet.begin(); } @@ -926,7 +926,7 @@ void TestFlushBehavior( } }; - uint256 txid = InsecureRand256(); + Txid txid = Txid::FromUint256(InsecureRand256()); COutPoint outp = COutPoint(txid, 0); Coin coin = MakeCoin(); // Ensure the coins views haven't seen this coin before. @@ -1017,7 +1017,7 @@ void TestFlushBehavior( // --- Bonus check: ensure that a coin added to the base view via one cache // can be spent by another cache which has never seen it. // - txid = InsecureRand256(); + txid = Txid::FromUint256(InsecureRand256()); outp = COutPoint(txid, 0); coin = MakeCoin(); BOOST_CHECK(!base.HaveCoin(outp)); @@ -1040,7 +1040,7 @@ void TestFlushBehavior( // --- Bonus check 2: ensure that a FRESH, spent coin is deleted by Sync() // - txid = InsecureRand256(); + txid = Txid::FromUint256(InsecureRand256()); outp = COutPoint(txid, 0); coin = MakeCoin(); CAmount coin_val = coin.out.nValue; diff --git a/src/test/fuzz/coinscache_sim.cpp b/src/test/fuzz/coinscache_sim.cpp index f350c9d032d1b..648e96b4a05b2 100644 --- a/src/test/fuzz/coinscache_sim.cpp +++ b/src/test/fuzz/coinscache_sim.cpp @@ -43,7 +43,9 @@ struct PrecomputedData for (uint32_t i = 0; i < NUM_OUTPOINTS; ++i) { uint32_t idx = (i * 1200U) >> 12; /* Map 3 or 4 entries to same txid. */ const uint8_t ser[4] = {uint8_t(idx), uint8_t(idx >> 8), uint8_t(idx >> 16), uint8_t(idx >> 24)}; - CSHA256().Write(PREFIX_O, 1).Write(ser, sizeof(ser)).Finalize(outpoints[i].hash.begin()); + uint256 txid; + CSHA256().Write(PREFIX_O, 1).Write(ser, sizeof(ser)).Finalize(txid.begin()); + outpoints[i].hash = Txid::FromUint256(txid); outpoints[i].n = i; } diff --git a/src/test/fuzz/mini_miner.cpp b/src/test/fuzz/mini_miner.cpp index 2f53943c3174f..84f9bb4ad0597 100644 --- a/src/test/fuzz/mini_miner.cpp +++ b/src/test/fuzz/mini_miner.cpp @@ -25,7 +25,7 @@ void initialize_miner() static const auto testing_setup = MakeNoLogFileContext(); g_setup = testing_setup.get(); for (uint32_t i = 0; i < uint32_t{100}; ++i) { - g_available_coins.emplace_back(uint256::ZERO, i); + g_available_coins.emplace_back(Txid::FromUint256(uint256::ZERO), i); } } diff --git a/src/test/fuzz/package_eval.cpp b/src/test/fuzz/package_eval.cpp index 8658c0b45a7b8..064930c5aa623 100644 --- a/src/test/fuzz/package_eval.cpp +++ b/src/test/fuzz/package_eval.cpp @@ -252,10 +252,10 @@ FUZZ_TARGET(tx_package_eval, .init = initialize_tx_pool) } if (fuzzed_data_provider.ConsumeBool()) { const auto& txid = fuzzed_data_provider.ConsumeBool() ? - txs.back()->GetHash().ToUint256() : + txs.back()->GetHash() : PickValue(fuzzed_data_provider, mempool_outpoints).hash; const auto delta = fuzzed_data_provider.ConsumeIntegralInRange(-50 * COIN, +50 * COIN); - tx_pool.PrioritiseTransaction(txid, delta); + tx_pool.PrioritiseTransaction(txid.ToUint256(), delta); } // Remember all added transactions diff --git a/src/test/fuzz/tx_pool.cpp b/src/test/fuzz/tx_pool.cpp index 96095539ece8d..ffa0c1216ef00 100644 --- a/src/test/fuzz/tx_pool.cpp +++ b/src/test/fuzz/tx_pool.cpp @@ -277,10 +277,10 @@ FUZZ_TARGET(tx_pool_standard, .init = initialize_tx_pool) } if (fuzzed_data_provider.ConsumeBool()) { const auto& txid = fuzzed_data_provider.ConsumeBool() ? - tx->GetHash().ToUint256() : + tx->GetHash() : PickValue(fuzzed_data_provider, outpoints_rbf).hash; const auto delta = fuzzed_data_provider.ConsumeIntegralInRange(-50 * COIN, +50 * COIN); - tx_pool.PrioritiseTransaction(txid, delta); + tx_pool.PrioritiseTransaction(txid.ToUint256(), delta); } // Remember all removed and added transactions @@ -367,7 +367,7 @@ FUZZ_TARGET(tx_pool, .init = initialize_tx_pool) MockTime(fuzzed_data_provider, chainstate); - std::vector txids; + std::vector txids; txids.reserve(g_outpoints_coinbase_init_mature.size()); for (const auto& outpoint : g_outpoints_coinbase_init_mature) { txids.push_back(outpoint.hash); @@ -375,7 +375,7 @@ FUZZ_TARGET(tx_pool, .init = initialize_tx_pool) for (int i{0}; i <= 3; ++i) { // Add some immature and non-existent outpoints txids.push_back(g_outpoints_coinbase_init_immature.at(i).hash); - txids.push_back(ConsumeUInt256(fuzzed_data_provider)); + txids.push_back(Txid::FromUint256(ConsumeUInt256(fuzzed_data_provider))); } SetMempoolConstraints(*node.args, fuzzed_data_provider); @@ -396,10 +396,10 @@ FUZZ_TARGET(tx_pool, .init = initialize_tx_pool) } if (fuzzed_data_provider.ConsumeBool()) { const auto txid = fuzzed_data_provider.ConsumeBool() ? - mut_tx.GetHash().ToUint256() : + mut_tx.GetHash() : PickValue(fuzzed_data_provider, txids); const auto delta = fuzzed_data_provider.ConsumeIntegralInRange(-50 * COIN, +50 * COIN); - tx_pool.PrioritiseTransaction(txid, delta); + tx_pool.PrioritiseTransaction(txid.ToUint256(), delta); } const auto tx = MakeTransactionRef(mut_tx); diff --git a/src/test/fuzz/txorphan.cpp b/src/test/fuzz/txorphan.cpp index a84dc951fc142..e9ceb299fed9b 100644 --- a/src/test/fuzz/txorphan.cpp +++ b/src/test/fuzz/txorphan.cpp @@ -39,7 +39,7 @@ FUZZ_TARGET(txorphan, .init = initialize_orphanage) std::vector outpoints; // initial outpoints used to construct transactions later for (uint8_t i = 0; i < 4; i++) { - outpoints.emplace_back(uint256{i}, 0); + outpoints.emplace_back(Txid::FromUint256(uint256{i}), 0); } // if true, allow duplicate input when constructing tx const bool duplicate_input = fuzzed_data_provider.ConsumeBool(); diff --git a/src/test/fuzz/util.cpp b/src/test/fuzz/util.cpp index 87ca2f6aede71..79bffa8369303 100644 --- a/src/test/fuzz/util.cpp +++ b/src/test/fuzz/util.cpp @@ -40,7 +40,7 @@ int64_t ConsumeTime(FuzzedDataProvider& fuzzed_data_provider, const std::optiona return fuzzed_data_provider.ConsumeIntegralInRange(min.value_or(time_min), max.value_or(time_max)); } -CMutableTransaction ConsumeTransaction(FuzzedDataProvider& fuzzed_data_provider, const std::optional>& prevout_txids, const int max_num_in, const int max_num_out) noexcept +CMutableTransaction ConsumeTransaction(FuzzedDataProvider& fuzzed_data_provider, const std::optional>& prevout_txids, const int max_num_in, const int max_num_out) noexcept { CMutableTransaction tx_mut; const auto p2wsh_op_true = fuzzed_data_provider.ConsumeBool(); @@ -53,7 +53,7 @@ CMutableTransaction ConsumeTransaction(FuzzedDataProvider& fuzzed_data_provider, for (int i = 0; i < num_in; ++i) { const auto& txid_prev = prevout_txids ? PickValue(fuzzed_data_provider, *prevout_txids) : - ConsumeUInt256(fuzzed_data_provider); + Txid::FromUint256(ConsumeUInt256(fuzzed_data_provider)); const auto index_out = fuzzed_data_provider.ConsumeIntegralInRange(0, max_num_out); const auto sequence = ConsumeSequence(fuzzed_data_provider); const auto script_sig = p2wsh_op_true ? CScript{} : ConsumeScript(fuzzed_data_provider); diff --git a/src/test/fuzz/util.h b/src/test/fuzz/util.h index 0ad2ed61289c4..fb566fcf22c86 100644 --- a/src/test/fuzz/util.h +++ b/src/test/fuzz/util.h @@ -145,7 +145,7 @@ template [[nodiscard]] int64_t ConsumeTime(FuzzedDataProvider& fuzzed_data_provider, const std::optional& min = std::nullopt, const std::optional& max = std::nullopt) noexcept; -[[nodiscard]] CMutableTransaction ConsumeTransaction(FuzzedDataProvider& fuzzed_data_provider, const std::optional>& prevout_txids, const int max_num_in = 10, const int max_num_out = 10) noexcept; +[[nodiscard]] CMutableTransaction ConsumeTransaction(FuzzedDataProvider& fuzzed_data_provider, const std::optional>& prevout_txids, const int max_num_in = 10, const int max_num_out = 10) noexcept; [[nodiscard]] CScriptWitness ConsumeScriptWitness(FuzzedDataProvider& fuzzed_data_provider, const size_t max_stack_elem_size = 32) noexcept; diff --git a/src/test/miner_tests.cpp b/src/test/miner_tests.cpp index 96acbea31742d..342d2514ed00a 100644 --- a/src/test/miner_tests.cpp +++ b/src/test/miner_tests.cpp @@ -118,19 +118,19 @@ void MinerTestingSetup::TestPackageSelection(const CScript& scriptPubKey, const tx.vout.resize(1); tx.vout[0].nValue = 5000000000LL - 1000; // This tx has a low fee: 1000 satoshis - uint256 hashParentTx = tx.GetHash(); // save this txid for later use + Txid hashParentTx = tx.GetHash(); // save this txid for later use tx_mempool.addUnchecked(entry.Fee(1000).Time(Now()).SpendsCoinbase(true).FromTx(tx)); // This tx has a medium fee: 10000 satoshis tx.vin[0].prevout.hash = txFirst[1]->GetHash(); tx.vout[0].nValue = 5000000000LL - 10000; - uint256 hashMediumFeeTx = tx.GetHash(); + Txid hashMediumFeeTx = tx.GetHash(); tx_mempool.addUnchecked(entry.Fee(10000).Time(Now()).SpendsCoinbase(true).FromTx(tx)); // This tx has a high fee, but depends on the first transaction tx.vin[0].prevout.hash = hashParentTx; tx.vout[0].nValue = 5000000000LL - 1000 - 50000; // 50k satoshi fee - uint256 hashHighFeeTx = tx.GetHash(); + Txid hashHighFeeTx = tx.GetHash(); tx_mempool.addUnchecked(entry.Fee(50000).Time(Now()).SpendsCoinbase(false).FromTx(tx)); std::unique_ptr pblocktemplate = AssemblerForTest(tx_mempool).CreateNewBlock(scriptPubKey); @@ -142,7 +142,7 @@ void MinerTestingSetup::TestPackageSelection(const CScript& scriptPubKey, const // Test that a package below the block min tx fee doesn't get included tx.vin[0].prevout.hash = hashHighFeeTx; tx.vout[0].nValue = 5000000000LL - 1000 - 50000; // 0 fee - uint256 hashFreeTx = tx.GetHash(); + Txid hashFreeTx = tx.GetHash(); tx_mempool.addUnchecked(entry.Fee(0).FromTx(tx)); size_t freeTxSize = ::GetSerializeSize(TX_WITH_WITNESS(tx)); @@ -152,7 +152,7 @@ void MinerTestingSetup::TestPackageSelection(const CScript& scriptPubKey, const tx.vin[0].prevout.hash = hashFreeTx; tx.vout[0].nValue = 5000000000LL - 1000 - 50000 - feeToUse; - uint256 hashLowFeeTx = tx.GetHash(); + Txid hashLowFeeTx = tx.GetHash(); tx_mempool.addUnchecked(entry.Fee(feeToUse).FromTx(tx)); pblocktemplate = AssemblerForTest(tx_mempool).CreateNewBlock(scriptPubKey); // Verify that the free tx and the low fee tx didn't get selected @@ -180,7 +180,7 @@ void MinerTestingSetup::TestPackageSelection(const CScript& scriptPubKey, const tx.vout.resize(2); tx.vout[0].nValue = 5000000000LL - 100000000; tx.vout[1].nValue = 100000000; // 1BTC output - uint256 hashFreeTx2 = tx.GetHash(); + Txid hashFreeTx2 = tx.GetHash(); tx_mempool.addUnchecked(entry.Fee(0).SpendsCoinbase(true).FromTx(tx)); // This tx can't be mined by itself @@ -188,7 +188,7 @@ void MinerTestingSetup::TestPackageSelection(const CScript& scriptPubKey, const tx.vout.resize(1); feeToUse = blockMinFeeRate.GetFee(freeTxSize); tx.vout[0].nValue = 5000000000LL - 100000000 - feeToUse; - uint256 hashLowFeeTx2 = tx.GetHash(); + Txid hashLowFeeTx2 = tx.GetHash(); tx_mempool.addUnchecked(entry.Fee(feeToUse).SpendsCoinbase(false).FromTx(tx)); pblocktemplate = AssemblerForTest(tx_mempool).CreateNewBlock(scriptPubKey); @@ -210,7 +210,7 @@ void MinerTestingSetup::TestPackageSelection(const CScript& scriptPubKey, const void MinerTestingSetup::TestBasicMining(const CScript& scriptPubKey, const std::vector& txFirst, int baseheight) { - uint256 hash; + Txid hash; CMutableTransaction tx; TestMemPoolEntryHelper entry; entry.nFee = 11; @@ -545,20 +545,20 @@ void MinerTestingSetup::TestPrioritisedMining(const CScript& scriptPubKey, const tx.vin[0].prevout.n = 0; tx.vout[0].nValue = 5000000000LL - 1000; // This tx has a low fee: 1000 satoshis - uint256 hashParentTx = tx.GetHash(); // save this txid for later use + Txid hashParentTx = tx.GetHash(); // save this txid for later use tx_mempool.addUnchecked(entry.Fee(1000).Time(Now()).SpendsCoinbase(true).FromTx(tx)); // This tx has a medium fee: 10000 satoshis tx.vin[0].prevout.hash = txFirst[2]->GetHash(); tx.vout[0].nValue = 5000000000LL - 10000; - uint256 hashMediumFeeTx = tx.GetHash(); + Txid hashMediumFeeTx = tx.GetHash(); tx_mempool.addUnchecked(entry.Fee(10000).Time(Now()).SpendsCoinbase(true).FromTx(tx)); tx_mempool.PrioritiseTransaction(hashMediumFeeTx, -5 * COIN); // This tx also has a low fee, but is prioritised tx.vin[0].prevout.hash = hashParentTx; tx.vout[0].nValue = 5000000000LL - 1000 - 1000; // 1000 satoshi fee - uint256 hashPrioritsedChild = tx.GetHash(); + Txid hashPrioritsedChild = tx.GetHash(); tx_mempool.addUnchecked(entry.Fee(1000).Time(Now()).SpendsCoinbase(false).FromTx(tx)); tx_mempool.PrioritiseTransaction(hashPrioritsedChild, 2 * COIN); @@ -570,19 +570,19 @@ void MinerTestingSetup::TestPrioritisedMining(const CScript& scriptPubKey, const // When FreeChild is included, FreeChild's prioritisation should also not be included. tx.vin[0].prevout.hash = txFirst[3]->GetHash(); tx.vout[0].nValue = 5000000000LL; // 0 fee - uint256 hashFreeParent = tx.GetHash(); + Txid hashFreeParent = tx.GetHash(); tx_mempool.addUnchecked(entry.Fee(0).SpendsCoinbase(true).FromTx(tx)); tx_mempool.PrioritiseTransaction(hashFreeParent, 10 * COIN); tx.vin[0].prevout.hash = hashFreeParent; tx.vout[0].nValue = 5000000000LL; // 0 fee - uint256 hashFreeChild = tx.GetHash(); + Txid hashFreeChild = tx.GetHash(); tx_mempool.addUnchecked(entry.Fee(0).SpendsCoinbase(false).FromTx(tx)); tx_mempool.PrioritiseTransaction(hashFreeChild, 1 * COIN); tx.vin[0].prevout.hash = hashFreeChild; tx.vout[0].nValue = 5000000000LL; // 0 fee - uint256 hashFreeGrandchild = tx.GetHash(); + Txid hashFreeGrandchild = tx.GetHash(); tx_mempool.addUnchecked(entry.Fee(0).SpendsCoinbase(false).FromTx(tx)); auto pblocktemplate = AssemblerForTest(tx_mempool).CreateNewBlock(scriptPubKey); diff --git a/src/test/miniminer_tests.cpp b/src/test/miniminer_tests.cpp index 311e402e3e5b9..5b007997b96b5 100644 --- a/src/test/miniminer_tests.cpp +++ b/src/test/miniminer_tests.cpp @@ -190,7 +190,7 @@ BOOST_FIXTURE_TEST_CASE(miniminer_1p1c, TestChain100Setup) CFeeRate(23330), CFeeRate(50000), CFeeRate(5*CENT)}); // All nonexistent entries have a bumpfee of zero, regardless of feerate - std::vector nonexistent_outpoints({ COutPoint{GetRandHash(), 0}, COutPoint{GetRandHash(), 3} }); + std::vector nonexistent_outpoints({ COutPoint{Txid::FromUint256(GetRandHash()), 0}, COutPoint{Txid::FromUint256(GetRandHash()), 3} }); for (const auto& outpoint : nonexistent_outpoints) BOOST_CHECK(!pool.isSpent(outpoint)); for (const auto& feerate : various_normal_feerates) { node::MiniMiner mini_miner(pool, nonexistent_outpoints); @@ -590,9 +590,17 @@ BOOST_FIXTURE_TEST_CASE(calculate_cluster, TestChain100Setup) CTxMemPool& pool = *Assert(m_node.mempool); LOCK2(cs_main, pool.cs); + // TODO this can be removed once the mempool interface uses Txid, Wtxid + auto convert_to_uint256_vec = [](const std::vector& vec) -> std::vector { + std::vector out; + std::transform(vec.begin(), vec.end(), std::back_inserter(out), + [](const Txid& txid) { return txid.ToUint256(); }); + return out; + }; + // Add chain of size 500 TestMemPoolEntryHelper entry; - std::vector chain_txids; + std::vector chain_txids; auto& lasttx = m_coinbase_txns[0]; for (auto i{0}; i < 500; ++i) { const auto tx = make_tx({COutPoint{lasttx->GetHash(), 0}}, /*num_outputs=*/1); @@ -603,7 +611,7 @@ BOOST_FIXTURE_TEST_CASE(calculate_cluster, TestChain100Setup) const auto cluster_500tx = pool.GatherClusters({lasttx->GetHash()}); CTxMemPool::setEntries cluster_500tx_set{cluster_500tx.begin(), cluster_500tx.end()}; BOOST_CHECK_EQUAL(cluster_500tx.size(), cluster_500tx_set.size()); - const auto vec_iters_500 = pool.GetIterVec(chain_txids); + const auto vec_iters_500 = pool.GetIterVec(convert_to_uint256_vec(chain_txids)); for (const auto& iter : vec_iters_500) BOOST_CHECK(cluster_500tx_set.count(iter)); // GatherClusters stops at 500 transactions. @@ -618,9 +626,9 @@ BOOST_FIXTURE_TEST_CASE(calculate_cluster, TestChain100Setup) * txc0 txc1 txc2 ... txc48 * Note that each transaction's ancestor size is 1 or 3, and each descendant size is 1, 2 or 3. * However, all of these transactions are in the same cluster. */ - std::vector zigzag_txids; + std::vector zigzag_txids; for (auto p{0}; p < 50; ++p) { - const auto txp = make_tx({COutPoint{GetRandHash(), 0}}, /*num_outputs=*/2); + const auto txp = make_tx({COutPoint{Txid::FromUint256(GetRandHash()), 0}}, /*num_outputs=*/2); pool.addUnchecked(entry.Fee(CENT).FromTx(txp)); zigzag_txids.push_back(txp->GetHash()); } @@ -629,7 +637,7 @@ BOOST_FIXTURE_TEST_CASE(calculate_cluster, TestChain100Setup) pool.addUnchecked(entry.Fee(CENT).FromTx(txc)); zigzag_txids.push_back(txc->GetHash()); } - const auto vec_iters_zigzag = pool.GetIterVec(zigzag_txids); + const auto vec_iters_zigzag = pool.GetIterVec(convert_to_uint256_vec(zigzag_txids)); // It doesn't matter which tx we calculate cluster for, everybody is in it. const std::vector indices{0, 22, 72, zigzag_txids.size() - 1}; for (const auto index : indices) { diff --git a/src/test/orphanage_tests.cpp b/src/test/orphanage_tests.cpp index b51de30cb2bf2..bf465c0c642af 100644 --- a/src/test/orphanage_tests.cpp +++ b/src/test/orphanage_tests.cpp @@ -68,7 +68,7 @@ BOOST_AUTO_TEST_CASE(DoS_mapOrphans) CMutableTransaction tx; tx.vin.resize(1); tx.vin[0].prevout.n = 0; - tx.vin[0].prevout.hash = InsecureRand256(); + tx.vin[0].prevout.hash = Txid::FromUint256(InsecureRand256()); tx.vin[0].scriptSig << OP_1; tx.vout.resize(1); tx.vout[0].nValue = 1*CENT; diff --git a/src/test/sighash_tests.cpp b/src/test/sighash_tests.cpp index bc2e3ca45ea17..0da9283fa826b 100644 --- a/src/test/sighash_tests.cpp +++ b/src/test/sighash_tests.cpp @@ -102,7 +102,7 @@ void static RandomTransaction(CMutableTransaction& tx, bool fSingle) for (int in = 0; in < ins; in++) { tx.vin.emplace_back(); CTxIn &txin = tx.vin.back(); - txin.prevout.hash = InsecureRand256(); + txin.prevout.hash = Txid::FromUint256(InsecureRand256()); txin.prevout.n = InsecureRandBits(2); RandomScript(txin.scriptSig); txin.nSequence = (InsecureRandBool()) ? InsecureRand32() : std::numeric_limits::max(); diff --git a/src/test/transaction_tests.cpp b/src/test/transaction_tests.cpp index 220b61f7e7e69..0a7ef3f7807f3 100644 --- a/src/test/transaction_tests.cpp +++ b/src/test/transaction_tests.cpp @@ -27,6 +27,7 @@ #include #include #include +#include #include #include @@ -219,7 +220,7 @@ BOOST_AUTO_TEST_CASE(tx_valid) fValid = false; break; } - COutPoint outpoint{uint256S(vinput[0].get_str()), uint32_t(vinput[1].getInt())}; + COutPoint outpoint{TxidFromString(vinput[0].get_str()), uint32_t(vinput[1].getInt())}; mapprevOutScriptPubKeys[outpoint] = ParseScript(vinput[2].get_str()); if (vinput.size() >= 4) { @@ -307,7 +308,7 @@ BOOST_AUTO_TEST_CASE(tx_invalid) fValid = false; break; } - COutPoint outpoint{uint256S(vinput[0].get_str()), uint32_t(vinput[1].getInt())}; + COutPoint outpoint{TxidFromString(vinput[0].get_str()), uint32_t(vinput[1].getInt())}; mapprevOutScriptPubKeys[outpoint] = ParseScript(vinput[2].get_str()); if (vinput.size() >= 4) { @@ -504,9 +505,7 @@ BOOST_AUTO_TEST_CASE(test_big_witness_transaction) // create a big transaction of 4500 inputs signed by the same key for(uint32_t ij = 0; ij < 4500; ij++) { uint32_t i = mtx.vin.size(); - uint256 prevId; - prevId.SetHex("0000000000000000000000000000000000000000000000000000000000000100"); - COutPoint outpoint(prevId, i); + COutPoint outpoint(TxidFromString("0000000000000000000000000000000000000000000000000000000000000100"), i); mtx.vin.resize(mtx.vin.size() + 1); mtx.vin[i].prevout = outpoint; diff --git a/src/test/txpackage_tests.cpp b/src/test/txpackage_tests.cpp index 84c9ecc3d1e6f..637f92bd0fcdf 100644 --- a/src/test/txpackage_tests.cpp +++ b/src/test/txpackage_tests.cpp @@ -29,7 +29,7 @@ inline CTransactionRef create_placeholder_tx(size_t num_inputs, size_t num_outpu mtx.vout.resize(num_outputs); auto random_script = CScript() << ToByteVector(InsecureRand256()) << ToByteVector(InsecureRand256()); for (size_t i{0}; i < num_inputs; ++i) { - mtx.vin[i].prevout.hash = InsecureRand256(); + mtx.vin[i].prevout.hash = Txid::FromUint256(InsecureRand256()); mtx.vin[i].prevout.n = 0; mtx.vin[i].scriptSig = random_script; } @@ -83,7 +83,7 @@ BOOST_FIXTURE_TEST_CASE(package_sanitization_tests, TestChain100Setup) // Packages can't have transactions spending the same prevout CMutableTransaction tx_zero_1; CMutableTransaction tx_zero_2; - COutPoint same_prevout{InsecureRand256(), 0}; + COutPoint same_prevout{Txid::FromUint256(InsecureRand256()), 0}; tx_zero_1.vin.emplace_back(same_prevout); tx_zero_2.vin.emplace_back(same_prevout); // Different vouts (not the same tx) @@ -101,7 +101,7 @@ BOOST_FIXTURE_TEST_CASE(package_sanitization_tests, TestChain100Setup) // IsConsistentPackage only cares about conflicts between transactions, not about a transaction // conflicting with itself (i.e. duplicate prevouts in vin). CMutableTransaction dup_tx; - const COutPoint rand_prevout{InsecureRand256(), 0}; + const COutPoint rand_prevout{Txid::FromUint256(InsecureRand256()), 0}; dup_tx.vin.emplace_back(rand_prevout); dup_tx.vin.emplace_back(rand_prevout); Package package_with_dup_tx{MakeTransactionRef(dup_tx)}; diff --git a/src/test/util/coins.cpp b/src/test/util/coins.cpp index 9b6c5535c5ed1..742dbc04d127d 100644 --- a/src/test/util/coins.cpp +++ b/src/test/util/coins.cpp @@ -16,8 +16,7 @@ COutPoint AddTestCoin(CCoinsViewCache& coins_view) { Coin new_coin; - const uint256 txid{InsecureRand256()}; - COutPoint outpoint{txid, /*nIn=*/0}; + COutPoint outpoint{Txid::FromUint256(InsecureRand256()), /*nIn=*/0}; new_coin.nHeight = 1; new_coin.out.nValue = InsecureRandMoneyAmount(); new_coin.out.scriptPubKey.assign(uint32_t{56}, 1); diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index 9979b75444746..22fdf132c72cb 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -501,7 +501,7 @@ void TestChain100Setup::MockMempoolMinFee(const CFeeRate& target_feerate) // Manually create an invalid transaction. Manually set the fee in the CTxMemPoolEntry to // achieve the exact target feerate. CMutableTransaction mtx = CMutableTransaction(); - mtx.vin.emplace_back(COutPoint{g_insecure_rand_ctx.rand256(), 0}); + mtx.vin.emplace_back(COutPoint{Txid::FromUint256(g_insecure_rand_ctx.rand256()), 0}); mtx.vout.emplace_back(1 * COIN, GetScriptForDestination(WitnessV0ScriptHash(CScript() << OP_TRUE))); const auto tx{MakeTransactionRef(mtx)}; LockPoints lp; diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp index e2541a74fd644..6969822ad754c 100644 --- a/src/test/validation_chainstatemanager_tests.cpp +++ b/src/test/validation_chainstatemanager_tests.cpp @@ -724,7 +724,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_snapshot_completion_hash_mismatch, Sna badcoin.out.nValue = InsecureRand32(); badcoin.nHeight = 1; badcoin.out.scriptPubKey.assign(InsecureRandBits(6), 0); - uint256 txid = InsecureRand256(); + Txid txid = Txid::FromUint256(InsecureRand256()); ibd_coins.AddCoin(COutPoint(txid, 0), std::move(badcoin), false); fs::path snapshot_chainstate_dir = gArgs.GetDataDirNet() / "chainstate_snapshot"; diff --git a/src/txmempool.cpp b/src/txmempool.cpp index e057d7ece1ccd..b6be395d74342 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -124,7 +124,7 @@ void CTxMemPool::UpdateTransactionsFromBlock(const std::vector& vHashes if (it == mapTx.end()) { continue; } - auto iter = mapNextTx.lower_bound(COutPoint(hash, 0)); + auto iter = mapNextTx.lower_bound(COutPoint(Txid::FromUint256(hash), 0)); // First calculate the children, and update CTxMemPoolEntry::m_children to // include them, and update their CTxMemPoolEntry::m_parents to include this tx. // we cache the in-mempool children to avoid duplicate updates diff --git a/src/util/transaction_identifier.h b/src/util/transaction_identifier.h index 4fb9b49966b18..89e10dee0175a 100644 --- a/src/util/transaction_identifier.h +++ b/src/util/transaction_identifier.h @@ -65,4 +65,9 @@ using Txid = transaction_identifier; /** Wtxid commits to all transaction fields including the witness. */ using Wtxid = transaction_identifier; +inline Txid TxidFromString(std::string_view str) +{ + return Txid::FromUint256(uint256S(str.data())); +} + #endif // BITCOIN_UTIL_TRANSACTION_IDENTIFIER_H diff --git a/src/validation.cpp b/src/validation.cpp index ed72a7c97a692..56ea9363b2843 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -696,7 +696,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) AssertLockHeld(m_pool.cs); const CTransactionRef& ptx = ws.m_ptx; const CTransaction& tx = *ws.m_ptx; - const uint256& hash = ws.m_hash; + const Txid& hash = ws.m_hash; // Copy/alias what we need out of args const int64_t nAcceptTime = args.m_accept_time; @@ -2036,7 +2036,7 @@ DisconnectResult Chainstate::DisconnectBlock(const CBlock& block, const CBlockIn // undo transactions in reverse order for (int i = block.vtx.size() - 1; i >= 0; i--) { const CTransaction &tx = *(block.vtx[i]); - uint256 hash = tx.GetHash(); + Txid hash = tx.GetHash(); bool is_coinbase = tx.IsCoinBase(); bool is_bip30_exception = (is_coinbase && !fEnforceBIP30); diff --git a/src/wallet/receive.cpp b/src/wallet/receive.cpp index 0a75bb6d92fd9..b9d8d9abc92dc 100644 --- a/src/wallet/receive.cpp +++ b/src/wallet/receive.cpp @@ -173,7 +173,7 @@ CAmount CachedTxGetAvailableCredit(const CWallet& wallet, const CWalletTx& wtx, bool allow_used_addresses = (filter & ISMINE_USED) || !wallet.IsWalletFlagSet(WALLET_FLAG_AVOID_REUSE); CAmount nCredit = 0; - uint256 hashTx = wtx.GetHash(); + Txid hashTx = wtx.GetHash(); for (unsigned int i = 0; i < wtx.tx->vout.size(); i++) { const CTxOut& txout = wtx.tx->vout[i]; if (!wallet.IsSpent(COutPoint(hashTx, i)) && (allow_used_addresses || !wallet.IsSpentKey(txout.scriptPubKey))) { @@ -348,7 +348,7 @@ std::map GetAddressBalances(const CWallet& wallet) if(!ExtractDestination(output.scriptPubKey, addr)) continue; - CAmount n = wallet.IsSpent(COutPoint(walletEntry.first, i)) ? 0 : output.nValue; + CAmount n = wallet.IsSpent(COutPoint(Txid::FromUint256(walletEntry.first), i)) ? 0 : output.nValue; balances[addr] += n; } } diff --git a/src/wallet/rpc/coins.cpp b/src/wallet/rpc/coins.cpp index fdc6ee055d57e..0cb0891141bd0 100644 --- a/src/wallet/rpc/coins.cpp +++ b/src/wallet/rpc/coins.cpp @@ -320,7 +320,7 @@ RPCHelpMan lockunspent() {"vout", UniValueType(UniValue::VNUM)}, }); - const uint256 txid(ParseHashO(o, "txid")); + const Txid txid = Txid::FromUint256(ParseHashO(o, "txid")); const int nOutput = o.find_value("vout").getInt(); if (nOutput < 0) { throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, vout cannot be negative"); diff --git a/src/wallet/rpc/spend.cpp b/src/wallet/rpc/spend.cpp index 65195046185a2..84e617c140b9a 100644 --- a/src/wallet/rpc/spend.cpp +++ b/src/wallet/rpc/spend.cpp @@ -672,7 +672,7 @@ void FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& fee_out, if (options.exists("input_weights")) { for (const UniValue& input : options["input_weights"].get_array().getValues()) { - uint256 txid = ParseHashO(input, "txid"); + Txid txid = Txid::FromUint256(ParseHashO(input, "txid")); const UniValue& vout_v = input.find_value("vout"); if (!vout_v.isNum()) { diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index d586f6d4aa5e5..35583642a5ae9 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -321,7 +321,7 @@ CoinsResult AvailableCoins(const CWallet& wallet, std::set trusted_parents; for (const auto& entry : wallet.mapWallet) { - const uint256& wtxid = entry.first; + const uint256& txid = entry.first; const CWalletTx& wtx = entry.second; if (wallet.IsTxImmatureCoinBase(wtx) && !params.include_immature_coinbase) @@ -381,7 +381,7 @@ CoinsResult AvailableCoins(const CWallet& wallet, for (unsigned int i = 0; i < wtx.tx->vout.size(); i++) { const CTxOut& output = wtx.tx->vout[i]; - const COutPoint outpoint(wtxid, i); + const COutPoint outpoint(Txid::FromUint256(txid), i); if (output.nValue < params.min_amount || output.nValue > params.max_amount) continue; diff --git a/src/wallet/test/feebumper_tests.cpp b/src/wallet/test/feebumper_tests.cpp index 2480a9b4e17fa..80fd7784192be 100644 --- a/src/wallet/test/feebumper_tests.cpp +++ b/src/wallet/test/feebumper_tests.cpp @@ -21,7 +21,7 @@ static void CheckMaxWeightComputation(const std::string& script_str, const std:: { std::vector script_data(ParseHex(script_str)); CScript script(script_data.begin(), script_data.end()); - CTxIn input(uint256(), 0, script); + CTxIn input(Txid{}, 0, script); for (const auto& s : witness_str_stack) { input.scriptWitness.stack.push_back(ParseHex(s)); diff --git a/src/wallet/test/fuzz/notifications.cpp b/src/wallet/test/fuzz/notifications.cpp index abd788f96fbe9..b243d5f471072 100644 --- a/src/wallet/test/fuzz/notifications.cpp +++ b/src/wallet/test/fuzz/notifications.cpp @@ -100,7 +100,7 @@ FUZZ_TARGET(wallet_notifications, .init = initialize_setup) // Add the initial entry chain.emplace_back(); auto& [coins, block]{chain.back()}; - coins.emplace(total_amount, COutPoint{uint256::ONE, 1}); + coins.emplace(total_amount, COutPoint{Txid::FromUint256(uint256::ONE), 1}); } LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 200) { diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index bcbc31ed3eff2..63932c643075a 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -946,7 +946,7 @@ BOOST_FIXTURE_TEST_CASE(wallet_sync_tx_invalid_state_test, TestingSetup) CMutableTransaction mtx; mtx.vout.emplace_back(COIN, GetScriptForDestination(op_dest)); - mtx.vin.emplace_back(g_insecure_rand_ctx.rand256(), 0); + mtx.vin.emplace_back(Txid::FromUint256(g_insecure_rand_ctx.rand256()), 0); const auto& tx_id_to_spend = wallet.AddToWallet(MakeTransactionRef(mtx), TxStateInMempool{})->GetHash(); { @@ -963,12 +963,12 @@ BOOST_FIXTURE_TEST_CASE(wallet_sync_tx_invalid_state_test, TestingSetup) mtx.vin.clear(); mtx.vin.emplace_back(tx_id_to_spend, 0); wallet.transactionAddedToMempool(MakeTransactionRef(mtx)); - const auto good_tx_id{mtx.GetHash().ToUint256()}; + const auto good_tx_id{mtx.GetHash()}; { // Verify balance update for the new tx and the old one LOCK(wallet.cs_wallet); - const CWalletTx* new_wtx = wallet.GetWalletTx(good_tx_id); + const CWalletTx* new_wtx = wallet.GetWalletTx(good_tx_id.ToUint256()); BOOST_CHECK_EQUAL(CachedTxGetAvailableCredit(wallet, *new_wtx), 1 * COIN); // Now the old wtx diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index ecf18fbe7836a..c34bf96b5ec00 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -681,7 +681,7 @@ std::set CWallet::GetConflicts(const uint256& txid) const bool CWallet::HasWalletSpend(const CTransactionRef& tx) const { AssertLockHeld(cs_wallet); - const uint256& txid = tx->GetHash(); + const Txid& txid = tx->GetHash(); for (unsigned int i = 0; i < tx->vout.size(); ++i) { if (IsSpent(COutPoint(txid, i))) { return true; @@ -1373,7 +1373,7 @@ void CWallet::RecursiveUpdateTxState(const uint256& tx_hash, const TryUpdatingSt batch.WriteTx(wtx); // Iterate over all its outputs, and update those tx states as well (if applicable) for (unsigned int i = 0; i < wtx.tx->vout.size(); ++i) { - std::pair range = mapTxSpends.equal_range(COutPoint(now, i)); + std::pair range = mapTxSpends.equal_range(COutPoint(Txid::FromUint256(now), i)); for (TxSpends::const_iterator iter = range.first; iter != range.second; ++iter) { if (!done.count(iter->second)) { todo.insert(iter->second); diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index 92eca46f051ec..66da5c1dc44ac 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -1073,7 +1073,7 @@ static DBErrors LoadTxRecords(CWallet* pwallet, DatabaseBatch& batch, std::vecto // Load locked utxo record LoadResult locked_utxo_res = LoadRecords(pwallet, batch, DBKeys::LOCKED_UTXO, [] (CWallet* pwallet, DataStream& key, CDataStream& value, std::string& err) EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet) { - uint256 hash; + Txid hash; uint32_t n; key >> hash; key >> n; From fa79a881ce0537e1d74da296a7513730438d2a02 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Thu, 16 Nov 2023 12:53:31 +0100 Subject: [PATCH 06/19] refactor: P2P transport without serialize version and type --- src/net.cpp | 29 +++++++++---------- src/net.h | 28 ++++++------------ src/net_processing.cpp | 18 +++++------- src/net_processing.h | 2 +- src/test/fuzz/p2p_transport_serialization.cpp | 8 ++--- src/test/net_tests.cpp | 8 ++--- 6 files changed, 40 insertions(+), 53 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 075a7d9839d3f..e1772233b2987 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -683,8 +683,8 @@ bool CNode::ReceiveMsgBytes(Span msg_bytes, bool& complete) return true; } -V1Transport::V1Transport(const NodeId node_id, int nTypeIn, int nVersionIn) noexcept : - m_magic_bytes{Params().MessageStart()}, m_node_id(node_id), hdrbuf(nTypeIn, nVersionIn), vRecv(nTypeIn, nVersionIn) +V1Transport::V1Transport(const NodeId node_id) noexcept + : m_magic_bytes{Params().MessageStart()}, m_node_id{node_id} { LOCK(m_recv_mutex); Reset(); @@ -968,12 +968,12 @@ void V2Transport::StartSendingHandshake() noexcept // We cannot wipe m_send_garbage as it will still be used as AAD later in the handshake. } -V2Transport::V2Transport(NodeId nodeid, bool initiating, int type_in, int version_in, const CKey& key, Span ent32, std::vector garbage) noexcept : - m_cipher{key, ent32}, m_initiating{initiating}, m_nodeid{nodeid}, - m_v1_fallback{nodeid, type_in, version_in}, m_recv_type{type_in}, m_recv_version{version_in}, - m_recv_state{initiating ? RecvState::KEY : RecvState::KEY_MAYBE_V1}, - m_send_garbage{std::move(garbage)}, - m_send_state{initiating ? SendState::AWAITING_KEY : SendState::MAYBE_V1} +V2Transport::V2Transport(NodeId nodeid, bool initiating, const CKey& key, Span ent32, std::vector garbage) noexcept + : m_cipher{key, ent32}, m_initiating{initiating}, m_nodeid{nodeid}, + m_v1_fallback{nodeid}, + m_recv_state{initiating ? RecvState::KEY : RecvState::KEY_MAYBE_V1}, + m_send_garbage{std::move(garbage)}, + m_send_state{initiating ? SendState::AWAITING_KEY : SendState::MAYBE_V1} { Assume(m_send_garbage.size() <= MAX_GARBAGE_LEN); // Start sending immediately if we're the initiator of the connection. @@ -983,9 +983,9 @@ V2Transport::V2Transport(NodeId nodeid, bool initiating, int type_in, int versio } } -V2Transport::V2Transport(NodeId nodeid, bool initiating, int type_in, int version_in) noexcept : - V2Transport{nodeid, initiating, type_in, version_in, GenerateRandomKey(), - MakeByteSpan(GetRandHash()), GenerateRandomGarbage()} { } +V2Transport::V2Transport(NodeId nodeid, bool initiating) noexcept + : V2Transport{nodeid, initiating, GenerateRandomKey(), + MakeByteSpan(GetRandHash()), GenerateRandomGarbage()} {} void V2Transport::SetReceiveState(RecvState recv_state) noexcept { @@ -1429,8 +1429,7 @@ CNetMessage V2Transport::GetReceivedMessage(std::chrono::microseconds time, bool Assume(m_recv_state == RecvState::APP_READY); Span contents{m_recv_decode_buffer}; auto msg_type = GetMessageType(contents); - CDataStream ret(m_recv_type, m_recv_version); - CNetMessage msg{std::move(ret)}; + CNetMessage msg{DataStream{}}; // Note that BIP324Cipher::EXPANSION also includes the length descriptor size. msg.m_raw_message_size = m_recv_decode_buffer.size() + BIP324Cipher::EXPANSION; if (msg_type) { @@ -3638,9 +3637,9 @@ ServiceFlags CConnman::GetLocalServices() const static std::unique_ptr MakeTransport(NodeId id, bool use_v2transport, bool inbound) noexcept { if (use_v2transport) { - return std::make_unique(id, /*initiating=*/!inbound, SER_NETWORK, INIT_PROTO_VERSION); + return std::make_unique(id, /*initiating=*/!inbound); } else { - return std::make_unique(id, SER_NETWORK, INIT_PROTO_VERSION); + return std::make_unique(id); } } diff --git a/src/net.h b/src/net.h index aa75df075d728..b673df69b75b2 100644 --- a/src/net.h +++ b/src/net.h @@ -232,15 +232,16 @@ class CNodeStats * Ideally it should only contain receive time, payload, * type and size. */ -class CNetMessage { +class CNetMessage +{ public: - CDataStream m_recv; //!< received message data + DataStream m_recv; //!< received message data std::chrono::microseconds m_time{0}; //!< time of message receipt uint32_t m_message_size{0}; //!< size of the payload uint32_t m_raw_message_size{0}; //!< used wire size of the message (including header/checksum) std::string m_type; - CNetMessage(CDataStream&& recv_in) : m_recv(std::move(recv_in)) {} + explicit CNetMessage(DataStream&& recv_in) : m_recv(std::move(recv_in)) {} // Only one CNetMessage object will exist for the same message on either // the receive or processing queue. For performance reasons we therefore // delete the copy constructor and assignment operator to avoid the @@ -249,11 +250,6 @@ class CNetMessage { CNetMessage(const CNetMessage&) = delete; CNetMessage& operator=(CNetMessage&&) = default; CNetMessage& operator=(const CNetMessage&) = delete; - - void SetVersion(int nVersionIn) - { - m_recv.SetVersion(nVersionIn); - } }; /** The Transport converts one connection's sent messages to wire bytes, and received bytes back. */ @@ -379,9 +375,9 @@ class V1Transport final : public Transport mutable CHash256 hasher GUARDED_BY(m_recv_mutex); mutable uint256 data_hash GUARDED_BY(m_recv_mutex); bool in_data GUARDED_BY(m_recv_mutex); // parsing header (false) or data (true) - CDataStream hdrbuf GUARDED_BY(m_recv_mutex); // partially received header + DataStream hdrbuf GUARDED_BY(m_recv_mutex){}; // partially received header CMessageHeader hdr GUARDED_BY(m_recv_mutex); // complete header - CDataStream vRecv GUARDED_BY(m_recv_mutex); // received message data + DataStream vRecv GUARDED_BY(m_recv_mutex){}; // received message data unsigned int nHdrPos GUARDED_BY(m_recv_mutex); unsigned int nDataPos GUARDED_BY(m_recv_mutex); @@ -420,7 +416,7 @@ class V1Transport final : public Transport size_t m_bytes_sent GUARDED_BY(m_send_mutex) {0}; public: - V1Transport(const NodeId node_id, int nTypeIn, int nVersionIn) noexcept; + explicit V1Transport(const NodeId node_id) noexcept; bool ReceivedMessageComplete() const override EXCLUSIVE_LOCKS_REQUIRED(!m_recv_mutex) { @@ -598,10 +594,6 @@ class V2Transport final : public Transport std::vector m_recv_aad GUARDED_BY(m_recv_mutex); /** Buffer to put decrypted contents in, for converting to CNetMessage. */ std::vector m_recv_decode_buffer GUARDED_BY(m_recv_mutex); - /** Deserialization type. */ - const int m_recv_type; - /** Deserialization version number. */ - const int m_recv_version; /** Current receiver state. */ RecvState m_recv_state GUARDED_BY(m_recv_mutex); @@ -647,13 +639,11 @@ class V2Transport final : public Transport * * @param[in] nodeid the node's NodeId (only for debug log output). * @param[in] initiating whether we are the initiator side. - * @param[in] type_in the serialization type of returned CNetMessages. - * @param[in] version_in the serialization version of returned CNetMessages. */ - V2Transport(NodeId nodeid, bool initiating, int type_in, int version_in) noexcept; + V2Transport(NodeId nodeid, bool initiating) noexcept; /** Construct a V2 transport with specified keys and garbage (test use only). */ - V2Transport(NodeId nodeid, bool initiating, int type_in, int version_in, const CKey& key, Span ent32, std::vector garbage) noexcept; + V2Transport(NodeId nodeid, bool initiating, const CKey& key, Span ent32, std::vector garbage) noexcept; // Receive side functions. bool ReceivedMessageComplete() const noexcept override EXCLUSIVE_LOCKS_REQUIRED(!m_recv_mutex); diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 0cd20680b0df3..533346ac81162 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -515,7 +515,7 @@ class PeerManagerImpl final : public PeerManager void RelayTransaction(const uint256& txid, const uint256& wtxid) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); void SetBestHeight(int height) override { m_best_height = height; }; void UnitTestMisbehaving(NodeId peer_id, int howmuch) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex) { Misbehaving(*Assert(GetPeerRef(peer_id)), howmuch, ""); }; - void ProcessMessage(CNode& pfrom, const std::string& msg_type, CDataStream& vRecv, + void ProcessMessage(CNode& pfrom, const std::string& msg_type, DataStream& vRecv, const std::chrono::microseconds time_received, const std::atomic& interruptMsgProc) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, !m_recent_confirmed_transactions_mutex, !m_most_recent_block_mutex, !m_headers_presync_mutex, g_msgproc_mutex); void UpdateLastBlockAnnounceTime(NodeId node, int64_t time_in_seconds) override; @@ -1033,7 +1033,7 @@ class PeerManagerImpl final : public PeerManager * @param[in] peer The peer that we received the request from * @param[in] vRecv The raw message received */ - void ProcessGetCFilters(CNode& node, Peer& peer, CDataStream& vRecv); + void ProcessGetCFilters(CNode& node, Peer& peer, DataStream& vRecv); /** * Handle a cfheaders request. @@ -1044,7 +1044,7 @@ class PeerManagerImpl final : public PeerManager * @param[in] peer The peer that we received the request from * @param[in] vRecv The raw message received */ - void ProcessGetCFHeaders(CNode& node, Peer& peer, CDataStream& vRecv); + void ProcessGetCFHeaders(CNode& node, Peer& peer, DataStream& vRecv); /** * Handle a getcfcheckpt request. @@ -1055,7 +1055,7 @@ class PeerManagerImpl final : public PeerManager * @param[in] peer The peer that we received the request from * @param[in] vRecv The raw message received */ - void ProcessGetCFCheckPt(CNode& node, Peer& peer, CDataStream& vRecv); + void ProcessGetCFCheckPt(CNode& node, Peer& peer, DataStream& vRecv); /** Checks if address relay is permitted with peer. If needed, initializes * the m_addr_known bloom filter and sets m_addr_relay_enabled to true. @@ -3130,7 +3130,7 @@ bool PeerManagerImpl::PrepareBlockFilterRequest(CNode& node, Peer& peer, return true; } -void PeerManagerImpl::ProcessGetCFilters(CNode& node,Peer& peer, CDataStream& vRecv) +void PeerManagerImpl::ProcessGetCFilters(CNode& node, Peer& peer, DataStream& vRecv) { uint8_t filter_type_ser; uint32_t start_height; @@ -3159,7 +3159,7 @@ void PeerManagerImpl::ProcessGetCFilters(CNode& node,Peer& peer, CDataStream& vR } } -void PeerManagerImpl::ProcessGetCFHeaders(CNode& node, Peer& peer, CDataStream& vRecv) +void PeerManagerImpl::ProcessGetCFHeaders(CNode& node, Peer& peer, DataStream& vRecv) { uint8_t filter_type_ser; uint32_t start_height; @@ -3201,7 +3201,7 @@ void PeerManagerImpl::ProcessGetCFHeaders(CNode& node, Peer& peer, CDataStream& filter_hashes); } -void PeerManagerImpl::ProcessGetCFCheckPt(CNode& node, Peer& peer, CDataStream& vRecv) +void PeerManagerImpl::ProcessGetCFCheckPt(CNode& node, Peer& peer, DataStream& vRecv) { uint8_t filter_type_ser; uint256 stop_hash; @@ -3342,7 +3342,7 @@ void PeerManagerImpl::ProcessCompactBlockTxns(CNode& pfrom, Peer& peer, const Bl return; } -void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDataStream& vRecv, +void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, DataStream& vRecv, const std::chrono::microseconds time_received, const std::atomic& interruptMsgProc) { @@ -5056,8 +5056,6 @@ bool PeerManagerImpl::ProcessMessages(CNode* pfrom, std::atomic& interrupt CaptureMessage(pfrom->addr, msg.m_type, MakeUCharSpan(msg.m_recv), /*is_incoming=*/true); } - msg.SetVersion(pfrom->GetCommonVersion()); - try { ProcessMessage(*pfrom, msg.m_type, msg.m_recv, msg.m_time, interruptMsgProc); if (interruptMsgProc) return false; diff --git a/src/net_processing.h b/src/net_processing.h index 80d07648a48ef..afdef00067492 100644 --- a/src/net_processing.h +++ b/src/net_processing.h @@ -105,7 +105,7 @@ class PeerManager : public CValidationInterface, public NetEventsInterface virtual void CheckForStaleTipAndEvictPeers() = 0; /** Process a single message from a peer. Public for fuzz testing */ - virtual void ProcessMessage(CNode& pfrom, const std::string& msg_type, CDataStream& vRecv, + virtual void ProcessMessage(CNode& pfrom, const std::string& msg_type, DataStream& vRecv, const std::chrono::microseconds time_received, const std::atomic& interruptMsgProc) EXCLUSIVE_LOCKS_REQUIRED(g_msgproc_mutex) = 0; /** This function is used for testing the stale tip eviction logic, see denialofservice_tests.cpp */ diff --git a/src/test/fuzz/p2p_transport_serialization.cpp b/src/test/fuzz/p2p_transport_serialization.cpp index 6af6aa1d18939..a205ce19f4a3a 100644 --- a/src/test/fuzz/p2p_transport_serialization.cpp +++ b/src/test/fuzz/p2p_transport_serialization.cpp @@ -36,8 +36,8 @@ void initialize_p2p_transport_serialization() FUZZ_TARGET(p2p_transport_serialization, .init = initialize_p2p_transport_serialization) { // Construct transports for both sides, with dummy NodeIds. - V1Transport recv_transport{NodeId{0}, SER_NETWORK, INIT_PROTO_VERSION}; - V1Transport send_transport{NodeId{1}, SER_NETWORK, INIT_PROTO_VERSION}; + V1Transport recv_transport{NodeId{0}}; + V1Transport send_transport{NodeId{1}}; FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()}; @@ -335,7 +335,7 @@ void SimulationTest(Transport& initiator, Transport& responder, R& rng, FuzzedDa std::unique_ptr MakeV1Transport(NodeId nodeid) noexcept { - return std::make_unique(nodeid, SER_NETWORK, INIT_PROTO_VERSION); + return std::make_unique(nodeid); } template @@ -369,7 +369,7 @@ std::unique_ptr MakeV2Transport(NodeId nodeid, bool initiator, RNG& r .Write(garb.data(), garb.size()) .Finalize(UCharCast(ent.data())); - return std::make_unique(nodeid, initiator, SER_NETWORK, INIT_PROTO_VERSION, key, ent, std::move(garb)); + return std::make_unique(nodeid, initiator, key, ent, std::move(garb)); } } // namespace diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp index 3c9227cfc41b7..508c42cabc927 100644 --- a/src/test/net_tests.cpp +++ b/src/test/net_tests.cpp @@ -1046,10 +1046,10 @@ class V2TransportTester public: /** Construct a tester object. test_initiator: whether the tested transport is initiator. */ - V2TransportTester(bool test_initiator) : - m_transport(0, test_initiator, SER_NETWORK, INIT_PROTO_VERSION), - m_cipher{GenerateRandomTestKey(), MakeByteSpan(InsecureRand256())}, - m_test_initiator(test_initiator) {} + explicit V2TransportTester(bool test_initiator) + : m_transport{0, test_initiator}, + m_cipher{GenerateRandomTestKey(), MakeByteSpan(InsecureRand256())}, + m_test_initiator(test_initiator) {} /** Data type returned by Interact: * From fa6b87b9ee661d8ef4ec244d230ebdeb7d1841a0 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Thu, 23 Nov 2023 17:49:25 +0100 Subject: [PATCH 07/19] fuzz: CDataStream -> DataStream in script_flags --- src/test/fuzz/script_flags.cpp | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/src/test/fuzz/script_flags.cpp b/src/test/fuzz/script_flags.cpp index 3b8f5c068d79e..3f7b4faac978d 100644 --- a/src/test/fuzz/script_flags.cpp +++ b/src/test/fuzz/script_flags.cpp @@ -7,21 +7,12 @@ #include