Merge #19610: p2p: refactor AlreadyHave(), CInv::type, INV/TX processing

fb56d37612dea6666e7da73d671311a697570dae p2p: ensure inv is GenMsgTx before ToGenTxid in inv processing (John Newbery)
aa3621385ee66c9dde5c632c0a79fba3a6ea2d62 test: use CInv::MSG_WITNESS_TX flag in p2p_segwit (Jon Atack)
24ee4f01eadb870435712950a1364cf0def06e9f p2p: make gtxid(.hash) and fAlreadyHave localvars const (Jon Atack)
b1c855453bf2634e7fd9b53c4a76a8536fc9865d p2p: use CInv block message helpers in net_processing.cpp (Jon Atack)
acd66421671e42a58e8e067868e1ab86268e3231 [net processing] Change AlreadyHaveTx() to take a GenTxid (John Newbery)
5fdfb80b861e0de3fcf8a57163b3f52af4b2df3b [net processing] Change AlreadyHaveBlock() to take block_hash argument (John Newbery)
430e183b89d00b4148f0b77a6fcacca2cd948202 [net processing] Remove mempool argument from AlreadyHaveBlock() (John Newbery)
42ca5618cae0fd9ef97d2006b17d896bc58cc17c [net processing] Split AlreadyHave() into separate block and tx functions (John Newbery)
39f1dc944554218911b0945fff7e6d06f3dab284 p2p: remove nFetchFlags from NetMsgType TX and INV processing (Jon Atack)
471714e1f024fb3b4892a7a8b34a76b83a13fa19 p2p: add CInv block message helper methods (Jon Atack)

Pull request description:

  Building on #19590 and the recent `wtxid` and `GenTxid` changes, this is a refactoring and cleanup PR to simplify and improve some of the net processing code.

  Some of the diffs are best reviewed with `-w` to ignore spacing.

  Co-authored by John Newbery.

ACKs for top commit:
  laanwj:
    Code review ACK fb56d37612dea6666e7da73d671311a697570dae
  jnewbery:
    utACK fb56d37612dea6666e7da73d671311a697570dae
  vasild:
    ACK fb56d3761

Tree-SHA512: ba39b58e6aaf850880a842fe5f6295e9f1870906ef690206acfc17140aae2ac854981e1066dbcd4238062478762fbd040ef772fdc2c50eea6869997c583e6a6d
This commit is contained in:
Wladimir J. van der Laan 2020-09-02 12:54:19 +02:00 committed by pasta
parent 8e29d1dda6
commit 11d166d609
No known key found for this signature in database
GPG Key ID: 52527BEDABE87984
2 changed files with 33 additions and 18 deletions

View File

@ -353,6 +353,7 @@ private:
* we fully-validated them at some point. * we fully-validated them at some point.
*/ */
bool BlockRequestAllowed(const CBlockIndex* pindex, const Consensus::Params& consensusParams) EXCLUSIVE_LOCKS_REQUIRED(cs_main); bool BlockRequestAllowed(const CBlockIndex* pindex, const Consensus::Params& consensusParams) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
bool AlreadyHaveBlock(const uint256& block_hash) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
void ProcessGetBlockData(CNode& pfrom, const CChainParams& chainparams, const CInv& inv, CConnman& connman, llmq::CInstantSendManager& isman); void ProcessGetBlockData(CNode& pfrom, const CChainParams& chainparams, const CInv& inv, CConnman& connman, llmq::CInstantSendManager& isman);
/** /**
@ -1845,9 +1846,6 @@ bool PeerManagerImpl::AlreadyHave(const CInv& inv)
(g_txindex != nullptr && g_txindex->HasTx(inv.hash)); (g_txindex != nullptr && g_txindex->HasTx(inv.hash));
} }
case MSG_BLOCK:
return m_chainman.m_blockman.LookupBlockIndex(inv.hash) != nullptr;
/* /*
Dash Related Inventory Messages Dash Related Inventory Messages
@ -1886,6 +1884,11 @@ bool PeerManagerImpl::AlreadyHave(const CInv& inv)
return true; return true;
} }
bool PeerManagerImpl::AlreadyHaveBlock(const uint256& block_hash)
{
return m_chainman.m_blockman.LookupBlockIndex(block_hash) != nullptr;
}
void PeerManagerImpl::RelayTransaction(const uint256& txid) void PeerManagerImpl::RelayTransaction(const uint256& txid)
{ {
CInv inv(::dstxManager->GetDSTX(txid) ? MSG_DSTX : MSG_TX, txid); CInv inv(::dstxManager->GetDSTX(txid) ? MSG_DSTX : MSG_TX, txid);
@ -1981,7 +1984,7 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, const CChainParams& chai
// disconnect node in case we have reached the outbound limit for serving historical blocks // disconnect node in case we have reached the outbound limit for serving historical blocks
if (send && if (send &&
connman.OutboundTargetReached(true) && 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 !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()); LogPrint(BCLog::NET, "historical block serving limit reached, disconnect peer=%d\n", pfrom.GetId());
@ -2015,9 +2018,9 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, const CChainParams& chai
pblock = pblockRead; pblock = pblockRead;
} }
if (pblock) { if (pblock) {
if (inv.type == MSG_BLOCK) if (inv.IsMsgBlk()) {
connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::BLOCK, *pblock)); connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::BLOCK, *pblock));
else if (inv.type == MSG_FILTERED_BLOCK) { } else if (inv.IsMsgFilteredBlk()) {
bool sendMerkleBlock = false; bool sendMerkleBlock = false;
CMerkleBlock merkleBlock; CMerkleBlock merkleBlock;
if (pfrom.RelayAddrsWithConn()) { if (pfrom.RelayAddrsWithConn()) {
@ -2047,8 +2050,8 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, const CChainParams& chai
} }
} }
// else // else
// no response // 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 // 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, // 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 // and we don't feel like constructing the object for them, so
@ -2280,7 +2283,7 @@ void PeerManagerImpl::ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic
// expensive to process. // expensive to process.
if (it != peer.m_getdata_requests.end() && !pfrom.fPauseSend) { if (it != peer.m_getdata_requests.end() && !pfrom.fPauseSend) {
const CInv &inv = *it++; const CInv &inv = *it++;
if (inv.type == MSG_BLOCK || inv.type == MSG_FILTERED_BLOCK || inv.type == MSG_CMPCT_BLOCK) { if (inv.IsGenBlkMsg()) {
ProcessGetBlockData(pfrom, m_chainparams, inv, m_connman, *m_llmq_ctx->isman); ProcessGetBlockData(pfrom, m_chainparams, inv, m_connman, *m_llmq_ctx->isman);
} }
// else: If the first item on the queue is an unknown type, we erase it // else: If the first item on the queue is an unknown type, we erase it
@ -3247,21 +3250,19 @@ void PeerManagerImpl::ProcessMessage(
const auto current_time = GetTime<std::chrono::microseconds>(); const auto current_time = GetTime<std::chrono::microseconds>();
uint256* best_block{nullptr}; uint256* best_block{nullptr};
for (CInv &inv : vInv) for (CInv& inv : vInv) {
{
if(!inv.IsKnownType()) { if(!inv.IsKnownType()) {
LogPrint(BCLog::NET, "got inv of unknown type %d: %s peer=%d\n", inv.type, inv.hash.ToString(), pfrom.GetId()); LogPrint(BCLog::NET, "got inv of unknown type %d: %s peer=%d\n", inv.type, inv.hash.ToString(), pfrom.GetId());
continue; continue;
} }
if (interruptMsgProc) if (interruptMsgProc) return;
return;
bool fAlreadyHave = AlreadyHave(inv); if (inv.IsMsgBlk()) {
LogPrint(BCLog::NET, "got inv: %s %s peer=%d\n", inv.ToString(), fAlreadyHave ? "have" : "new", pfrom.GetId()); const bool fAlreadyHave = AlreadyHaveBlock(inv.hash);
statsClient.inc(strprintf("message.received.inv_%s", inv.GetCommand()), 1.0f); LogPrint(BCLog::NET, "got inv: %s %s peer=%d\n", inv.ToString(), fAlreadyHave ? "have" : "new", pfrom.GetId());
statsClient.inc(strprintf("message.received.inv_%s", inv.GetCommand()), 1.0f);
if (inv.type == MSG_BLOCK) {
UpdateBlockAvailability(pfrom.GetId(), inv.hash); UpdateBlockAvailability(pfrom.GetId(), inv.hash);
if (fAlreadyHave || fImporting || fReindex || mapBlocksInFlight.count(inv.hash)) { if (fAlreadyHave || fImporting || fReindex || mapBlocksInFlight.count(inv.hash)) {
@ -3289,6 +3290,10 @@ void PeerManagerImpl::ProcessMessage(
best_block = &inv.hash; best_block = &inv.hash;
} }
} else { } else {
const bool fAlreadyHave = AlreadyHave(inv);
LogPrint(BCLog::NET, "got inv: %s %s peer=%d\n", inv.ToString(), fAlreadyHave ? "have" : "new", pfrom.GetId());
statsClient.inc(strprintf("message.received.inv_%s", inv.GetCommand()), 1.0f);
static std::set<int> allowWhileInIBDObjs = { static std::set<int> allowWhileInIBDObjs = {
MSG_SPORK MSG_SPORK
}; };

View File

@ -546,10 +546,20 @@ public:
// Single-message helper methods // Single-message helper methods
bool IsMsgTx() const { return type == MSG_TX; } bool IsMsgTx() const { return type == MSG_TX; }
bool IsMsgBlk() const { return type == MSG_BLOCK; }
bool IsMsgDstx() const { return type == MSG_DSTX; } bool IsMsgDstx() const { return type == MSG_DSTX; }
bool IsMsgFilteredBlk() const { return type == MSG_FILTERED_BLOCK; }
bool IsMsgCmpctBlk() const { return type == MSG_CMPCT_BLOCK; }
// Combined-message helper methods // Combined-message helper methods
bool IsGenTxMsg() const { return type == MSG_TX || type == MSG_DSTX; } bool IsGenTxMsg() const
{
return type == MSG_TX || type == MSG_DSTX;
}
bool IsGenBlkMsg() const
{
return type == MSG_BLOCK || type == MSG_FILTERED_BLOCK || type == MSG_CMPCT_BLOCK;
}
private: private:
const char* GetCommandInternal() const; const char* GetCommandInternal() const;