From 471714e1f024fb3b4892a7a8b34a76b83a13fa19 Mon Sep 17 00:00:00 2001 From: Jon Atack Date: Mon, 27 Jul 2020 17:31:50 +0200 Subject: [PATCH 01/10] p2p: add CInv block message helper methods --- src/protocol.h | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/src/protocol.h b/src/protocol.h index 2e6c767cdd1..a059bf482ad 100644 --- a/src/protocol.h +++ b/src/protocol.h @@ -247,7 +247,7 @@ extern const char* CFCHECKPT; * txid. * @since protocol version 70016 as described by BIP 339. */ -extern const char *WTXIDRELAY; +extern const char* WTXIDRELAY; }; // namespace NetMsgType /* Get a vector of all valid message types (see above) */ @@ -418,12 +418,22 @@ public: std::string ToString() const; // Single-message helper methods - bool IsMsgTx() const { return type == MSG_TX; } - bool IsMsgWtx() const { return type == MSG_WTX; } - bool IsMsgWitnessTx() const { return type == MSG_WITNESS_TX; } + bool IsMsgTx() const { return type == MSG_TX; } + bool IsMsgBlk() const { return type == MSG_BLOCK; } + bool IsMsgWtx() const { return type == MSG_WTX; } + bool IsMsgFilteredBlk() const { return type == MSG_FILTERED_BLOCK; } + bool IsMsgCmpctBlk() const { return type == MSG_CMPCT_BLOCK; } + bool IsMsgWitnessBlk() const { return type == MSG_WITNESS_BLOCK; } // Combined-message helper methods - bool IsGenTxMsg() const { return type == MSG_TX || type == MSG_WTX || type == MSG_WITNESS_TX; } + bool IsGenTxMsg() const + { + return type == MSG_TX || type == MSG_WTX || type == MSG_WITNESS_TX; + } + bool IsGenBlkMsg() const + { + return type == MSG_BLOCK || type == MSG_FILTERED_BLOCK || type == MSG_CMPCT_BLOCK || type == MSG_WITNESS_BLOCK; + } int type; uint256 hash; From 39f1dc944554218911b0945fff7e6d06f3dab284 Mon Sep 17 00:00:00 2001 From: Jon Atack Date: Wed, 5 Aug 2020 17:46:31 +0200 Subject: [PATCH 02/10] p2p: remove nFetchFlags from NetMsgType TX and INV processing The nFetchFlags code can be removed here because GetFetchFlags() can only add the MSG_WITNESS_FLAG, which is added to the CInv::type field. That CInv is only passed to AlreadyHave() or ToGenTxid(), and neither of those functions do anything different depending on whether the CInv type is MSG_TX or MSG_WITNESS_TX. Co-authored by: John Newbery --- src/net_processing.cpp | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 60bdfbe9f5d..a01514185d1 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -2654,14 +2654,12 @@ void PeerLogicValidation::ProcessMessage(CNode& pfrom, const std::string& msg_ty LOCK(cs_main); - uint32_t nFetchFlags = GetFetchFlags(pfrom); const auto current_time = GetTime(); uint256* best_block{nullptr}; for (CInv &inv : vInv) { - if (interruptMsgProc) - return; + if (interruptMsgProc) return; // Ignore INVs that don't match wtxidrelay setting. // Note that orphan parent fetching always uses MSG_TX GETDATAs regardless of the wtxidrelay setting. @@ -2675,10 +2673,6 @@ void PeerLogicValidation::ProcessMessage(CNode& pfrom, const std::string& msg_ty bool fAlreadyHave = AlreadyHave(inv, m_mempool); LogPrint(BCLog::NET, "got inv: %s %s peer=%d\n", inv.ToString(), fAlreadyHave ? "have" : "new", pfrom.GetId()); - if (inv.IsMsgTx()) { - inv.type |= nFetchFlags; - } - if (inv.type == MSG_BLOCK) { UpdateBlockAvailability(pfrom.GetId(), inv.hash); if (!fAlreadyHave && !fImporting && !fReindex && !mapBlocksInFlight.count(inv.hash)) { @@ -3013,7 +3007,6 @@ void PeerLogicValidation::ProcessMessage(CNode& pfrom, const std::string& msg_ty } } if (!fRejectedParents) { - uint32_t nFetchFlags = GetFetchFlags(pfrom); const auto current_time = GetTime(); for (const uint256& parent_txid : unique_parents) { @@ -3022,7 +3015,7 @@ void PeerLogicValidation::ProcessMessage(CNode& pfrom, const std::string& msg_ty // wtxidrelay peers. // Eventually we should replace this with an improved // protocol for getting all unconfirmed parents. - CInv _inv(MSG_TX | nFetchFlags, parent_txid); + CInv _inv(MSG_TX, parent_txid); pfrom.AddKnownTx(parent_txid); if (!AlreadyHave(_inv, m_mempool)) RequestTx(State(pfrom.GetId()), ToGenTxid(_inv), current_time); } From 42ca5618cae0fd9ef97d2006b17d896bc58cc17c Mon Sep 17 00:00:00 2001 From: John Newbery Date: Thu, 30 Jul 2020 10:30:24 +0100 Subject: [PATCH 03/10] [net processing] Split AlreadyHave() into separate block and tx functions --- src/net_processing.cpp | 84 ++++++++++++++++++++---------------------- 1 file changed, 39 insertions(+), 45 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index a01514185d1..3a0613deeb8 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1422,47 +1422,38 @@ void PeerLogicValidation::BlockChecked(const CBlock& block, const BlockValidatio // -bool static AlreadyHave(const CInv& inv, const CTxMemPool& mempool) EXCLUSIVE_LOCKS_REQUIRED(cs_main) +bool static AlreadyHaveTx(const CInv& inv, const CTxMemPool& mempool) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { - switch (inv.type) - { - case MSG_TX: - case MSG_WITNESS_TX: - case MSG_WTX: - { - assert(recentRejects); - if (::ChainActive().Tip()->GetBlockHash() != hashRecentRejectsChainTip) - { - // If the chain tip has changed previously rejected transactions - // might be now valid, e.g. due to a nLockTime'd tx becoming valid, - // or a double-spend. Reset the rejects filter and give those - // txs a second chance. - hashRecentRejectsChainTip = ::ChainActive().Tip()->GetBlockHash(); - recentRejects->reset(); - } - - { - LOCK(g_cs_orphans); - if (!inv.IsMsgWtx() && mapOrphanTransactions.count(inv.hash)) { - return true; - } else if (inv.IsMsgWtx() && g_orphans_by_wtxid.count(inv.hash)) { - return true; - } - } - - { - LOCK(g_cs_recent_confirmed_transactions); - if (g_recent_confirmed_transactions->contains(inv.hash)) return true; - } - - return recentRejects->contains(inv.hash) || mempool.exists(ToGenTxid(inv)); - } - case MSG_BLOCK: - case MSG_WITNESS_BLOCK: - return LookupBlockIndex(inv.hash) != nullptr; + assert(recentRejects); + if (::ChainActive().Tip()->GetBlockHash() != hashRecentRejectsChainTip) { + // If the chain tip has changed previously rejected transactions + // might be now valid, e.g. due to a nLockTime'd tx becoming valid, + // or a double-spend. Reset the rejects filter and give those + // txs a second chance. + hashRecentRejectsChainTip = ::ChainActive().Tip()->GetBlockHash(); + recentRejects->reset(); } - // Don't know what it is, just say we already got one - return true; + + { + LOCK(g_cs_orphans); + if (!inv.IsMsgWtx() && mapOrphanTransactions.count(inv.hash)) { + return true; + } else if (inv.IsMsgWtx() && g_orphans_by_wtxid.count(inv.hash)) { + return true; + } + } + + { + LOCK(g_cs_recent_confirmed_transactions); + if (g_recent_confirmed_transactions->contains(inv.hash)) return true; + } + + return recentRejects->contains(inv.hash) || mempool.exists(ToGenTxid(inv)); +} + +bool static AlreadyHaveBlock(const CInv& inv, const CTxMemPool& mempool) EXCLUSIVE_LOCKS_REQUIRED(cs_main) +{ + return LookupBlockIndex(inv.hash) != nullptr; } void RelayTransaction(const uint256& txid, const uint256& wtxid, const CConnman& connman) @@ -2670,10 +2661,10 @@ void PeerLogicValidation::ProcessMessage(CNode& pfrom, const std::string& msg_ty if (inv.IsMsgWtx()) continue; } - bool fAlreadyHave = AlreadyHave(inv, m_mempool); - LogPrint(BCLog::NET, "got inv: %s %s peer=%d\n", inv.ToString(), fAlreadyHave ? "have" : "new", pfrom.GetId()); - if (inv.type == MSG_BLOCK) { + bool fAlreadyHave = AlreadyHaveBlock(inv, m_mempool); + LogPrint(BCLog::NET, "got inv: %s %s peer=%d\n", inv.ToString(), fAlreadyHave ? "have" : "new", pfrom.GetId()); + UpdateBlockAvailability(pfrom.GetId(), inv.hash); if (!fAlreadyHave && !fImporting && !fReindex && !mapBlocksInFlight.count(inv.hash)) { // Headers-first is the primary method of announcement on @@ -2684,6 +2675,9 @@ void PeerLogicValidation::ProcessMessage(CNode& pfrom, const std::string& msg_ty best_block = &inv.hash; } } else { + bool fAlreadyHave = AlreadyHaveTx(inv, mempool); + LogPrint(BCLog::NET, "got inv: %s %s peer=%d\n", inv.ToString(), fAlreadyHave ? "have" : "new", pfrom.GetId()); + pfrom.AddKnownTx(inv.hash); if (fBlocksOnly) { LogPrint(BCLog::NET, "transaction (%s) inv sent in violation of protocol, disconnecting peer=%d\n", inv.hash.ToString(), pfrom.GetId()); @@ -2963,7 +2957,7 @@ void PeerLogicValidation::ProcessMessage(CNode& pfrom, const std::string& msg_ty // already; and an adversary can already relay us old transactions // (older than our recency filter) if trying to DoS us, without any need // for witness malleation. - if (!AlreadyHave(CInv(MSG_WTX, wtxid), m_mempool) && + if (!AlreadyHaveTx(CInv(MSG_WTX, wtxid), m_mempool) && AcceptToMemoryPool(m_mempool, state, ptx, &lRemovedTxn, false /* bypass_limits */, 0 /* nAbsurdFee */)) { m_mempool.check(&::ChainstateActive().CoinsTip()); RelayTransaction(tx.GetHash(), tx.GetWitnessHash(), m_connman); @@ -3017,7 +3011,7 @@ void PeerLogicValidation::ProcessMessage(CNode& pfrom, const std::string& msg_ty // protocol for getting all unconfirmed parents. CInv _inv(MSG_TX, parent_txid); pfrom.AddKnownTx(parent_txid); - if (!AlreadyHave(_inv, m_mempool)) RequestTx(State(pfrom.GetId()), ToGenTxid(_inv), current_time); + if (!AlreadyHaveTx(_inv, m_mempool)) RequestTx(State(pfrom.GetId()), ToGenTxid(_inv), current_time); } AddOrphanTx(ptx, pfrom.GetId()); @@ -4568,7 +4562,7 @@ bool PeerLogicValidation::SendMessages(CNode* pto) // processing at a later time, see below) tx_process_time.erase(tx_process_time.begin()); CInv inv(gtxid.IsWtxid() ? MSG_WTX : (MSG_TX | GetFetchFlags(*pto)), gtxid.GetHash()); - if (!AlreadyHave(inv, m_mempool)) { + if (!AlreadyHaveTx(inv, m_mempool)) { // If this transaction was last requested more than 1 minute ago, // then request. const auto last_request_time = GetTxRequestTime(gtxid); From 430e183b89d00b4148f0b77a6fcacca2cd948202 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Thu, 30 Jul 2020 11:01:11 +0100 Subject: [PATCH 04/10] [net processing] Remove mempool argument from AlreadyHaveBlock() --- src/net_processing.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 3a0613deeb8..3fb8907b5e4 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1451,7 +1451,7 @@ bool static AlreadyHaveTx(const CInv& inv, const CTxMemPool& mempool) EXCLUSIVE_ return recentRejects->contains(inv.hash) || mempool.exists(ToGenTxid(inv)); } -bool static AlreadyHaveBlock(const CInv& inv, const CTxMemPool& mempool) EXCLUSIVE_LOCKS_REQUIRED(cs_main) +bool static AlreadyHaveBlock(const CInv& inv) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { return LookupBlockIndex(inv.hash) != nullptr; } @@ -2662,7 +2662,7 @@ void PeerLogicValidation::ProcessMessage(CNode& pfrom, const std::string& msg_ty } if (inv.type == MSG_BLOCK) { - bool fAlreadyHave = AlreadyHaveBlock(inv, m_mempool); + bool fAlreadyHave = AlreadyHaveBlock(inv); LogPrint(BCLog::NET, "got inv: %s %s peer=%d\n", inv.ToString(), fAlreadyHave ? "have" : "new", pfrom.GetId()); UpdateBlockAvailability(pfrom.GetId(), inv.hash); From 5fdfb80b861e0de3fcf8a57163b3f52af4b2df3b Mon Sep 17 00:00:00 2001 From: John Newbery Date: Thu, 30 Jul 2020 11:02:40 +0100 Subject: [PATCH 05/10] [net processing] Change AlreadyHaveBlock() to take block_hash argument --- src/net_processing.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 3fb8907b5e4..82d41d737e1 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1451,9 +1451,9 @@ bool static AlreadyHaveTx(const CInv& inv, const CTxMemPool& mempool) EXCLUSIVE_ return recentRejects->contains(inv.hash) || mempool.exists(ToGenTxid(inv)); } -bool static AlreadyHaveBlock(const CInv& inv) EXCLUSIVE_LOCKS_REQUIRED(cs_main) +bool static AlreadyHaveBlock(const uint256& block_hash) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { - return LookupBlockIndex(inv.hash) != nullptr; + return LookupBlockIndex(block_hash) != nullptr; } void RelayTransaction(const uint256& txid, const uint256& wtxid, const CConnman& connman) @@ -2662,7 +2662,7 @@ void PeerLogicValidation::ProcessMessage(CNode& pfrom, const std::string& msg_ty } if (inv.type == MSG_BLOCK) { - bool fAlreadyHave = AlreadyHaveBlock(inv); + bool fAlreadyHave = AlreadyHaveBlock(inv.hash); LogPrint(BCLog::NET, "got inv: %s %s peer=%d\n", inv.ToString(), fAlreadyHave ? "have" : "new", pfrom.GetId()); UpdateBlockAvailability(pfrom.GetId(), inv.hash); From acd66421671e42a58e8e067868e1ab86268e3231 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Thu, 30 Jul 2020 11:18:45 +0100 Subject: [PATCH 06/10] [net processing] Change AlreadyHaveTx() to take a GenTxid --- src/net_processing.cpp | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 82d41d737e1..49077435872 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1422,7 +1422,7 @@ void PeerLogicValidation::BlockChecked(const CBlock& block, const BlockValidatio // -bool static AlreadyHaveTx(const CInv& inv, const CTxMemPool& mempool) EXCLUSIVE_LOCKS_REQUIRED(cs_main) +bool static AlreadyHaveTx(const GenTxid& gtxid, const CTxMemPool& mempool) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { assert(recentRejects); if (::ChainActive().Tip()->GetBlockHash() != hashRecentRejectsChainTip) { @@ -1436,19 +1436,19 @@ bool static AlreadyHaveTx(const CInv& inv, const CTxMemPool& mempool) EXCLUSIVE_ { LOCK(g_cs_orphans); - if (!inv.IsMsgWtx() && mapOrphanTransactions.count(inv.hash)) { + if (!gtxid.IsWtxid() && mapOrphanTransactions.count(gtxid.GetHash())) { return true; - } else if (inv.IsMsgWtx() && g_orphans_by_wtxid.count(inv.hash)) { + } else if (gtxid.IsWtxid() && g_orphans_by_wtxid.count(gtxid.GetHash())) { return true; } } { LOCK(g_cs_recent_confirmed_transactions); - if (g_recent_confirmed_transactions->contains(inv.hash)) return true; + if (g_recent_confirmed_transactions->contains(gtxid.GetHash())) return true; } - return recentRejects->contains(inv.hash) || mempool.exists(ToGenTxid(inv)); + return recentRejects->contains(gtxid.GetHash()) || mempool.exists(gtxid); } bool static AlreadyHaveBlock(const uint256& block_hash) EXCLUSIVE_LOCKS_REQUIRED(cs_main) @@ -2675,7 +2675,8 @@ void PeerLogicValidation::ProcessMessage(CNode& pfrom, const std::string& msg_ty best_block = &inv.hash; } } else { - bool fAlreadyHave = AlreadyHaveTx(inv, mempool); + GenTxid gtxid = ToGenTxid(inv); + bool fAlreadyHave = AlreadyHaveTx(gtxid, mempool); LogPrint(BCLog::NET, "got inv: %s %s peer=%d\n", inv.ToString(), fAlreadyHave ? "have" : "new", pfrom.GetId()); pfrom.AddKnownTx(inv.hash); @@ -2684,7 +2685,7 @@ void PeerLogicValidation::ProcessMessage(CNode& pfrom, const std::string& msg_ty pfrom.fDisconnect = true; return; } else if (!fAlreadyHave && !m_chainman.ActiveChainstate().IsInitialBlockDownload()) { - RequestTx(State(pfrom.GetId()), ToGenTxid(inv), current_time); + RequestTx(State(pfrom.GetId()), gtxid, current_time); } } } @@ -2957,7 +2958,7 @@ void PeerLogicValidation::ProcessMessage(CNode& pfrom, const std::string& msg_ty // already; and an adversary can already relay us old transactions // (older than our recency filter) if trying to DoS us, without any need // for witness malleation. - if (!AlreadyHaveTx(CInv(MSG_WTX, wtxid), m_mempool) && + if (!AlreadyHaveTx(GenTxid(/* is_wtxid=*/true, wtxid), m_mempool) && AcceptToMemoryPool(m_mempool, state, ptx, &lRemovedTxn, false /* bypass_limits */, 0 /* nAbsurdFee */)) { m_mempool.check(&::ChainstateActive().CoinsTip()); RelayTransaction(tx.GetHash(), tx.GetWitnessHash(), m_connman); @@ -3009,9 +3010,9 @@ void PeerLogicValidation::ProcessMessage(CNode& pfrom, const std::string& msg_ty // wtxidrelay peers. // Eventually we should replace this with an improved // protocol for getting all unconfirmed parents. - CInv _inv(MSG_TX, parent_txid); + GenTxid gtxid{/* is_wtxid=*/false, parent_txid}; pfrom.AddKnownTx(parent_txid); - if (!AlreadyHaveTx(_inv, m_mempool)) RequestTx(State(pfrom.GetId()), ToGenTxid(_inv), current_time); + if (!AlreadyHaveTx(gtxid, m_mempool)) RequestTx(State(pfrom.GetId()), gtxid, current_time); } AddOrphanTx(ptx, pfrom.GetId()); @@ -4562,7 +4563,7 @@ bool PeerLogicValidation::SendMessages(CNode* pto) // processing at a later time, see below) tx_process_time.erase(tx_process_time.begin()); CInv inv(gtxid.IsWtxid() ? MSG_WTX : (MSG_TX | GetFetchFlags(*pto)), gtxid.GetHash()); - if (!AlreadyHaveTx(inv, m_mempool)) { + if (!AlreadyHaveTx(ToGenTxid(inv), m_mempool)) { // If this transaction was last requested more than 1 minute ago, // then request. const auto last_request_time = GetTxRequestTime(gtxid); From b1c855453bf2634e7fd9b53c4a76a8536fc9865d Mon Sep 17 00:00:00 2001 From: Jon Atack Date: Wed, 5 Aug 2020 17:35:05 +0200 Subject: [PATCH 07/10] p2p: use CInv block message helpers in net_processing.cpp --- src/net_processing.cpp | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 49077435872..290853e0a66 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1555,7 +1555,7 @@ void static ProcessGetBlockData(CNode& pfrom, const CChainParams& chainparams, c // disconnect node in case we have reached the outbound limit for serving historical blocks if (send && connman.OutboundTargetReached(true) && - (((pindexBestHeader != nullptr) && (pindexBestHeader->GetBlockTime() - pindex->GetBlockTime() > HISTORICAL_BLOCK_AGE)) || inv.type == MSG_FILTERED_BLOCK) && + (((pindexBestHeader != nullptr) && (pindexBestHeader->GetBlockTime() - pindex->GetBlockTime() > HISTORICAL_BLOCK_AGE)) || inv.IsMsgFilteredBlk()) && !pfrom.HasPermission(PF_DOWNLOAD) // nodes with the download permission may exceed target ) { LogPrint(BCLog::NET, "historical block serving limit reached, disconnect peer=%d\n", pfrom.GetId()); @@ -1581,7 +1581,7 @@ void static ProcessGetBlockData(CNode& pfrom, const CChainParams& chainparams, c std::shared_ptr pblock; if (a_recent_block && a_recent_block->GetHash() == pindex->GetBlockHash()) { pblock = a_recent_block; - } else if (inv.type == MSG_WITNESS_BLOCK) { + } else if (inv.IsMsgWitnessBlk()) { // Fast-path: in this case it is possible to serve the block directly from disk, // as the network format matches the format on disk std::vector block_data; @@ -1598,12 +1598,11 @@ void static ProcessGetBlockData(CNode& pfrom, const CChainParams& chainparams, c pblock = pblockRead; } if (pblock) { - if (inv.type == MSG_BLOCK) + if (inv.IsMsgBlk()) { connman.PushMessage(&pfrom, msgMaker.Make(SERIALIZE_TRANSACTION_NO_WITNESS, NetMsgType::BLOCK, *pblock)); - else if (inv.type == MSG_WITNESS_BLOCK) + } else if (inv.IsMsgWitnessBlk()) { connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::BLOCK, *pblock)); - else if (inv.type == MSG_FILTERED_BLOCK) - { + } else if (inv.IsMsgFilteredBlk()) { bool sendMerkleBlock = false; CMerkleBlock merkleBlock; if (pfrom.m_tx_relay != nullptr) { @@ -1627,9 +1626,7 @@ void static ProcessGetBlockData(CNode& pfrom, const CChainParams& chainparams, c } // else // no response - } - else if (inv.type == MSG_CMPCT_BLOCK) - { + } else if (inv.IsMsgCmpctBlk()) { // If a peer is asking for old blocks, we're almost guaranteed // they won't have a useful mempool to match against a compact block, // and we don't feel like constructing the object for them, so @@ -1757,7 +1754,7 @@ void static ProcessGetData(CNode& pfrom, const CChainParams& chainparams, CConnm // expensive to process. if (it != pfrom.vRecvGetData.end() && !pfrom.fPauseSend) { const CInv &inv = *it++; - if (inv.type == MSG_BLOCK || inv.type == MSG_FILTERED_BLOCK || inv.type == MSG_CMPCT_BLOCK || inv.type == MSG_WITNESS_BLOCK) { + if (inv.IsGenBlkMsg()) { ProcessGetBlockData(pfrom, chainparams, inv, connman); } // else: If the first item on the queue is an unknown type, we erase it @@ -2661,7 +2658,7 @@ void PeerLogicValidation::ProcessMessage(CNode& pfrom, const std::string& msg_ty if (inv.IsMsgWtx()) continue; } - if (inv.type == MSG_BLOCK) { + if (inv.IsMsgBlk()) { bool fAlreadyHave = AlreadyHaveBlock(inv.hash); LogPrint(BCLog::NET, "got inv: %s %s peer=%d\n", inv.ToString(), fAlreadyHave ? "have" : "new", pfrom.GetId()); From 24ee4f01eadb870435712950a1364cf0def06e9f Mon Sep 17 00:00:00 2001 From: Jon Atack Date: Sat, 8 Aug 2020 14:36:26 +0200 Subject: [PATCH 08/10] p2p: make gtxid(.hash) and fAlreadyHave localvars const --- src/net_processing.cpp | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 290853e0a66..b9f0e1f98eb 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1434,21 +1434,23 @@ bool static AlreadyHaveTx(const GenTxid& gtxid, const CTxMemPool& mempool) EXCLU recentRejects->reset(); } + const uint256& hash = gtxid.GetHash(); + { LOCK(g_cs_orphans); - if (!gtxid.IsWtxid() && mapOrphanTransactions.count(gtxid.GetHash())) { + if (!gtxid.IsWtxid() && mapOrphanTransactions.count(hash)) { return true; - } else if (gtxid.IsWtxid() && g_orphans_by_wtxid.count(gtxid.GetHash())) { + } else if (gtxid.IsWtxid() && g_orphans_by_wtxid.count(hash)) { return true; } } { LOCK(g_cs_recent_confirmed_transactions); - if (g_recent_confirmed_transactions->contains(gtxid.GetHash())) return true; + if (g_recent_confirmed_transactions->contains(hash)) return true; } - return recentRejects->contains(gtxid.GetHash()) || mempool.exists(gtxid); + return recentRejects->contains(hash) || mempool.exists(gtxid); } bool static AlreadyHaveBlock(const uint256& block_hash) EXCLUSIVE_LOCKS_REQUIRED(cs_main) @@ -2645,8 +2647,7 @@ void PeerLogicValidation::ProcessMessage(CNode& pfrom, const std::string& msg_ty const auto current_time = GetTime(); uint256* best_block{nullptr}; - for (CInv &inv : vInv) - { + for (CInv& inv : vInv) { if (interruptMsgProc) return; // Ignore INVs that don't match wtxidrelay setting. @@ -2659,7 +2660,7 @@ void PeerLogicValidation::ProcessMessage(CNode& pfrom, const std::string& msg_ty } if (inv.IsMsgBlk()) { - bool fAlreadyHave = AlreadyHaveBlock(inv.hash); + const bool fAlreadyHave = AlreadyHaveBlock(inv.hash); LogPrint(BCLog::NET, "got inv: %s %s peer=%d\n", inv.ToString(), fAlreadyHave ? "have" : "new", pfrom.GetId()); UpdateBlockAvailability(pfrom.GetId(), inv.hash); @@ -2672,8 +2673,8 @@ void PeerLogicValidation::ProcessMessage(CNode& pfrom, const std::string& msg_ty best_block = &inv.hash; } } else { - GenTxid gtxid = ToGenTxid(inv); - bool fAlreadyHave = AlreadyHaveTx(gtxid, mempool); + const GenTxid gtxid = ToGenTxid(inv); + const bool fAlreadyHave = AlreadyHaveTx(gtxid, mempool); LogPrint(BCLog::NET, "got inv: %s %s peer=%d\n", inv.ToString(), fAlreadyHave ? "have" : "new", pfrom.GetId()); pfrom.AddKnownTx(inv.hash); @@ -3007,7 +3008,7 @@ void PeerLogicValidation::ProcessMessage(CNode& pfrom, const std::string& msg_ty // wtxidrelay peers. // Eventually we should replace this with an improved // protocol for getting all unconfirmed parents. - GenTxid gtxid{/* is_wtxid=*/false, parent_txid}; + const GenTxid gtxid{/* is_wtxid=*/false, parent_txid}; pfrom.AddKnownTx(parent_txid); if (!AlreadyHaveTx(gtxid, m_mempool)) RequestTx(State(pfrom.GetId()), gtxid, current_time); } From aa3621385ee66c9dde5c632c0a79fba3a6ea2d62 Mon Sep 17 00:00:00 2001 From: Jon Atack Date: Sat, 8 Aug 2020 13:22:19 +0200 Subject: [PATCH 09/10] test: use CInv::MSG_WITNESS_TX flag in p2p_segwit --- test/functional/p2p_segwit.py | 3 ++- test/functional/test_framework/messages.py | 1 + 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/test/functional/p2p_segwit.py b/test/functional/p2p_segwit.py index 564e49f3d8a..b80e7994be1 100755 --- a/test/functional/p2p_segwit.py +++ b/test/functional/p2p_segwit.py @@ -25,6 +25,7 @@ from test_framework.messages import ( MSG_BLOCK, MSG_TX, MSG_WITNESS_FLAG, + MSG_WITNESS_TX, MSG_WTX, NODE_NETWORK, NODE_WITNESS, @@ -2158,7 +2159,7 @@ class SegWitTest(BitcoinTestFramework): self.wtx_node.wait_for_getdata([tx.sha256], 60) with mininode_lock: lgd = self.wtx_node.lastgetdata[:] - assert_equal(lgd, [CInv(MSG_TX|MSG_WITNESS_FLAG, tx.sha256)]) + assert_equal(lgd, [CInv(MSG_WITNESS_TX, tx.sha256)]) # Send tx through test_transaction_acceptance(self.nodes[0], self.wtx_node, tx, with_witness=False, accepted=True) diff --git a/test/functional/test_framework/messages.py b/test/functional/test_framework/messages.py index 5207b563a1a..5e411e35a82 100755 --- a/test/functional/test_framework/messages.py +++ b/test/functional/test_framework/messages.py @@ -63,6 +63,7 @@ MSG_CMPCT_BLOCK = 4 MSG_WTX = 5 MSG_WITNESS_FLAG = 1 << 30 MSG_TYPE_MASK = 0xffffffff >> 2 +MSG_WITNESS_TX = MSG_TX | MSG_WITNESS_FLAG FILTER_TYPE_BASIC = 0 From fb56d37612dea6666e7da73d671311a697570dae Mon Sep 17 00:00:00 2001 From: John Newbery Date: Fri, 21 Aug 2020 16:10:48 +0200 Subject: [PATCH 10/10] p2p: ensure inv is GenMsgTx before ToGenTxid in inv processing and otherwise log that an unknown INV type was received. In INV processing, when handling transaction type inv messages, ToGenTxid() expects that we constructed the CInv ourselves or that we verified that it is for a transaction type CInv. Therefore, change this `else` branch into an `else if (inv.GenMsgTx())` to make this safer and log any INVs that fall through. --- src/net_processing.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index b9f0e1f98eb..0bd5e56ddd1 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -2672,7 +2672,7 @@ void PeerLogicValidation::ProcessMessage(CNode& pfrom, const std::string& msg_ty // then fetch the blocks we need to catch up. best_block = &inv.hash; } - } else { + } else if (inv.IsGenTxMsg()) { const GenTxid gtxid = ToGenTxid(inv); const bool fAlreadyHave = AlreadyHaveTx(gtxid, mempool); LogPrint(BCLog::NET, "got inv: %s %s peer=%d\n", inv.ToString(), fAlreadyHave ? "have" : "new", pfrom.GetId()); @@ -2685,6 +2685,8 @@ void PeerLogicValidation::ProcessMessage(CNode& pfrom, const std::string& msg_ty } else if (!fAlreadyHave && !m_chainman.ActiveChainstate().IsInitialBlockDownload()) { RequestTx(State(pfrom.GetId()), gtxid, current_time); } + } else { + LogPrint(BCLog::NET, "Unknown inv type \"%s\" received from peer=%d\n", inv.ToString(), pfrom.GetId()); } }