From 9efd86a908cf09d9ddbadd3195f202635117d505 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Wed, 22 Jul 2020 17:17:00 -0700 Subject: [PATCH] refactor: add GenTxid (=txid or wtxid) type and use it for tx request logic --- src/net_processing.cpp | 34 +++++++++++++++++----------------- src/primitives/transaction.h | 15 +++++++++++++++ src/protocol.cpp | 6 ++++++ src/protocol.h | 4 ++++ 4 files changed, 42 insertions(+), 17 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index fc5b4a8e7f..5f8a73cece 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -398,7 +398,7 @@ struct CNodeState { /* Track when to attempt download of announced transactions (process * time in micros -> txid) */ - std::multimap m_tx_process_time; + std::multimap m_tx_process_time; //! Store all the transactions a peer has recently announced std::set m_tx_announced; @@ -797,23 +797,23 @@ std::chrono::microseconds CalculateTxGetDataTime(const uint256& txid, std::chron return process_time; } -void RequestTx(CNodeState* state, const uint256& txid, std::chrono::microseconds current_time) EXCLUSIVE_LOCKS_REQUIRED(cs_main) +void RequestTx(CNodeState* state, const GenTxid& gtxid, std::chrono::microseconds current_time) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { CNodeState::TxDownloadState& peer_download_state = state->m_tx_download; if (peer_download_state.m_tx_announced.size() >= MAX_PEER_TX_ANNOUNCEMENTS || peer_download_state.m_tx_process_time.size() >= MAX_PEER_TX_ANNOUNCEMENTS || - peer_download_state.m_tx_announced.count(txid)) { + peer_download_state.m_tx_announced.count(gtxid.GetHash())) { // Too many queued announcements from this peer, or we already have // this announcement return; } - peer_download_state.m_tx_announced.insert(txid); + peer_download_state.m_tx_announced.insert(gtxid.GetHash()); // Calculate the time to try requesting this transaction. Use // fPreferredDownload as a proxy for outbound peers. - const auto process_time = CalculateTxGetDataTime(txid, current_time, !state->fPreferredDownload, !state->m_wtxid_relay && g_wtxid_relay_peers > 0); + const auto process_time = CalculateTxGetDataTime(gtxid.GetHash(), current_time, !state->fPreferredDownload, !state->m_wtxid_relay && g_wtxid_relay_peers > 0); - peer_download_state.m_tx_process_time.emplace(process_time, txid); + peer_download_state.m_tx_process_time.emplace(process_time, gtxid); } } // namespace @@ -2678,7 +2678,7 @@ void ProcessMessage( pfrom.fDisconnect = true; return; } else if (!fAlreadyHave && !chainman.ActiveChainstate().IsInitialBlockDownload()) { - RequestTx(State(pfrom.GetId()), inv.hash, current_time); + RequestTx(State(pfrom.GetId()), ToGenTxid(inv), current_time); } } } @@ -2994,7 +2994,7 @@ void ProcessMessage( // protocol for getting all unconfirmed parents. CInv _inv(MSG_TX | nFetchFlags, txin.prevout.hash); pfrom.AddKnownTx(txin.prevout.hash); - if (!AlreadyHave(_inv, mempool)) RequestTx(State(pfrom.GetId()), _inv.hash, current_time); + if (!AlreadyHave(_inv, mempool)) RequestTx(State(pfrom.GetId()), ToGenTxid(_inv), current_time); } } AddOrphanTx(ptx, pfrom.GetId()); @@ -4529,15 +4529,15 @@ bool PeerLogicValidation::SendMessages(CNode* pto) auto& tx_process_time = state.m_tx_download.m_tx_process_time; while (!tx_process_time.empty() && tx_process_time.begin()->first <= current_time && state.m_tx_download.m_tx_in_flight.size() < MAX_PEER_TX_IN_FLIGHT) { - const uint256 txid = tx_process_time.begin()->second; + const GenTxid gtxid = tx_process_time.begin()->second; // Erase this entry from tx_process_time (it may be added back for // processing at a later time, see below) tx_process_time.erase(tx_process_time.begin()); - CInv inv(state.m_wtxid_relay ? MSG_WTX : (MSG_TX | GetFetchFlags(*pto)), txid); + CInv inv(gtxid.IsWtxid() ? MSG_WTX : (MSG_TX | GetFetchFlags(*pto)), gtxid.GetHash()); if (!AlreadyHave(inv, m_mempool)) { // If this transaction was last requested more than 1 minute ago, // then request. - const auto last_request_time = GetTxRequestTime(inv.hash); + const auto last_request_time = GetTxRequestTime(gtxid.GetHash()); if (last_request_time <= current_time - GETDATA_TX_INTERVAL) { LogPrint(BCLog::NET, "Requesting %s peer=%d\n", inv.ToString(), pto->GetId()); vGetData.push_back(inv); @@ -4545,8 +4545,8 @@ bool PeerLogicValidation::SendMessages(CNode* pto) connman->PushMessage(pto, msgMaker.Make(NetMsgType::GETDATA, vGetData)); vGetData.clear(); } - UpdateTxRequestTime(inv.hash, current_time); - state.m_tx_download.m_tx_in_flight.emplace(inv.hash, current_time); + UpdateTxRequestTime(gtxid.GetHash(), current_time); + state.m_tx_download.m_tx_in_flight.emplace(gtxid.GetHash(), current_time); } else { // This transaction is in flight from someone else; queue // up processing to happen after the download times out @@ -4560,13 +4560,13 @@ bool PeerLogicValidation::SendMessages(CNode* pto) // would open us up to an attacker using inbound // wtxid-relay to prevent us from requesting transactions // from outbound txid-relay peers). - const auto next_process_time = CalculateTxGetDataTime(txid, current_time, !state.fPreferredDownload, false); - tx_process_time.emplace(next_process_time, txid); + const auto next_process_time = CalculateTxGetDataTime(gtxid.GetHash(), current_time, !state.fPreferredDownload, false); + tx_process_time.emplace(next_process_time, gtxid); } } else { // We have already seen this transaction, no need to download. - state.m_tx_download.m_tx_announced.erase(inv.hash); - state.m_tx_download.m_tx_in_flight.erase(inv.hash); + state.m_tx_download.m_tx_announced.erase(gtxid.GetHash()); + state.m_tx_download.m_tx_in_flight.erase(gtxid.GetHash()); } } diff --git a/src/primitives/transaction.h b/src/primitives/transaction.h index 4514db578a..544bab6d9b 100644 --- a/src/primitives/transaction.h +++ b/src/primitives/transaction.h @@ -12,6 +12,8 @@ #include #include +#include + static const int SERIALIZE_TRANSACTION_NO_WITNESS = 0x40000000; /** An outpoint - a combination of a transaction hash and an index n into its vout */ @@ -388,4 +390,17 @@ typedef std::shared_ptr CTransactionRef; static inline CTransactionRef MakeTransactionRef() { return std::make_shared(); } template static inline CTransactionRef MakeTransactionRef(Tx&& txIn) { return std::make_shared(std::forward(txIn)); } +/** A generic txid reference (txid or wtxid). */ +class GenTxid +{ + const bool m_is_wtxid; + const uint256 m_hash; +public: + GenTxid(bool is_wtxid, const uint256& hash) : m_is_wtxid(is_wtxid), m_hash(hash) {} + bool IsWtxid() const { return m_is_wtxid; } + const uint256& GetHash() const { return m_hash; } + friend bool operator==(const GenTxid& a, const GenTxid& b) { return a.m_is_wtxid == b.m_is_wtxid && a.m_hash == b.m_hash; } + friend bool operator<(const GenTxid& a, const GenTxid& b) { return std::tie(a.m_is_wtxid, a.m_hash) < std::tie(b.m_is_wtxid, b.m_hash); } +}; + #endif // BITCOIN_PRIMITIVES_TRANSACTION_H diff --git a/src/protocol.cpp b/src/protocol.cpp index ee77ca3b94..5a91acee0f 100644 --- a/src/protocol.cpp +++ b/src/protocol.cpp @@ -241,3 +241,9 @@ std::vector serviceFlagsToStr(uint64_t flags) return str_flags; } + +GenTxid ToGenTxid(const CInv& inv) +{ + assert(inv.IsGenTxMsg()); + return {inv.IsMsgWtx(), inv.hash}; +} diff --git a/src/protocol.h b/src/protocol.h index 26e64b0009..d6767cc3e0 100644 --- a/src/protocol.h +++ b/src/protocol.h @@ -11,6 +11,7 @@ #define BITCOIN_PROTOCOL_H #include +#include #include #include #include @@ -442,4 +443,7 @@ public: uint256 hash; }; +/** Convert a TX/WITNESS_TX/WTX CInv to a GenTxid. */ +GenTxid ToGenTxid(const CInv& inv); + #endif // BITCOIN_PROTOCOL_H