Merge #19590: p2p, refactor: add CInv transaction message helpers; use in net processing

c251d710a4 p2p, refactoring: use CInv helpers in net_processing.cpp (Jon Atack)
4254cd9f8f p2p: add CInv transaction message helper methods (Jon Atack)

Pull request description:

  Following the merge of wtxid relay in #18044, this is the first of three refactoring PRs (this one, #19610, and #19611) with no change in behavior, tightly scoped to ease review, to simplify the net processing code and improve encapsulation:

  - add `CInv` transaction message helper methods, defined in the class

  - use the new helpers in `net_processing.cpp` to simplify the code and improve encapsulation

  Test coverage is provided by the functional p2p tests, notably (from seeing which tests failed when breaking things to test coverage) `p2p_segwit`, `p2p_tx_download`, `p2p_feefilter`, and `p2p_permissions`.

ACKs for top commit:
  fjahr:
    Code review ACK c251d710a4
  laanwj:
    Code review ACK c251d710a4
  vasild:
    ACK c251d71
  theStack:
    Code-Review ACK c251d710a4
  hebasto:
    ACK c251d710a4, I have reviewed the code and it looks OK, I agree it can be merged.

Tree-SHA512: ead034b3c9e438909b4c5010c570d7930e69063c114290b051b7cebfd9bd5b19f573218bebe8a521256d32e830797f997adad3d85b4539c64ac5762b698e656d
This commit is contained in:
Wladimir J. van der Laan 2020-07-30 16:03:06 +02:00
commit 17de75b028
No known key found for this signature in database
GPG key ID: 1E4AED62986CD25D
2 changed files with 18 additions and 11 deletions

View file

@ -1441,9 +1441,9 @@ bool static AlreadyHave(const CInv& inv, const CTxMemPool& mempool) EXCLUSIVE_LO
{
LOCK(g_cs_orphans);
if (inv.type != MSG_WTX && mapOrphanTransactions.count(inv.hash)) {
if (!inv.IsMsgWtx() && mapOrphanTransactions.count(inv.hash)) {
return true;
} else if (inv.type == MSG_WTX && g_orphans_by_wtxid.count(inv.hash)) {
} else if (inv.IsMsgWtx() && g_orphans_by_wtxid.count(inv.hash)) {
return true;
}
}
@ -1453,8 +1453,7 @@ bool static AlreadyHave(const CInv& inv, const CTxMemPool& mempool) EXCLUSIVE_LO
if (g_recent_confirmed_transactions->contains(inv.hash)) return true;
}
const bool by_wtxid = (inv.type == MSG_WTX);
return recentRejects->contains(inv.hash) || mempool.exists(inv.hash, by_wtxid);
return recentRejects->contains(inv.hash) || mempool.exists(inv.hash, inv.IsMsgWtx());
}
case MSG_BLOCK:
case MSG_WITNESS_BLOCK:
@ -1715,7 +1714,7 @@ void static ProcessGetData(CNode& pfrom, const CChainParams& chainparams, CConnm
// 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
// them.
while (it != pfrom.vRecvGetData.end() && (it->type == MSG_TX || it->type == MSG_WITNESS_TX || it->type == MSG_WTX)) {
while (it != pfrom.vRecvGetData.end() && it->IsGenTxMsg()) {
if (interruptMsgProc) return;
// The send buffer provides backpressure. If there's no space in
// the buffer, pause processing until the next call.
@ -1728,10 +1727,10 @@ void static ProcessGetData(CNode& pfrom, const CChainParams& chainparams, CConnm
continue;
}
CTransactionRef tx = FindTxForGetData(pfrom, inv.hash, inv.type == MSG_WTX, mempool_req, now);
CTransactionRef tx = FindTxForGetData(pfrom, inv.hash, inv.IsMsgWtx(), mempool_req, now);
if (tx) {
// WTX and WITNESS_TX imply we serialize with witness
int nSendFlags = (inv.type == MSG_TX ? SERIALIZE_TRANSACTION_NO_WITNESS : 0);
int nSendFlags = (inv.IsMsgTx() ? SERIALIZE_TRANSACTION_NO_WITNESS : 0);
connman.PushMessage(&pfrom, msgMaker.Make(nSendFlags, NetMsgType::TX, *tx));
mempool.RemoveUnbroadcastTx(tx->GetHash());
// As we're going to send tx, make sure its unconfirmed parents are made requestable.
@ -2650,15 +2649,15 @@ void ProcessMessage(
// ignore INVs that don't match wtxidrelay setting
if (State(pfrom.GetId())->m_wtxid_relay) {
if (inv.type == MSG_TX) continue;
if (inv.IsMsgTx()) continue;
} else {
if (inv.type == MSG_WTX) continue;
if (inv.IsMsgWtx()) continue;
}
bool fAlreadyHave = AlreadyHave(inv, mempool);
LogPrint(BCLog::NET, "got inv: %s %s peer=%d\n", inv.ToString(), fAlreadyHave ? "have" : "new", pfrom.GetId());
if (inv.type == MSG_TX) {
if (inv.IsMsgTx()) {
inv.type |= nFetchFlags;
}
@ -3690,7 +3689,7 @@ void ProcessMessage(
vRecv >> vInv;
if (vInv.size() <= MAX_PEER_TX_IN_FLIGHT + MAX_BLOCKS_IN_TRANSIT_PER_PEER) {
for (CInv &inv : vInv) {
if (inv.type == MSG_TX || inv.type == MSG_WITNESS_TX || inv.type == MSG_WTX) {
if (inv.IsGenTxMsg()) {
// If we receive a NOTFOUND message for a txid we requested, erase
// it from our data structures for this peer.
auto in_flight_it = state->m_tx_download.m_tx_in_flight.find(inv.hash);

View file

@ -430,6 +430,14 @@ public:
std::string GetCommand() const;
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; }
// Combined-message helper methods
bool IsGenTxMsg() const { return type == MSG_TX || type == MSG_WTX || type == MSG_WITNESS_TX; }
int type;
uint256 hash;
};