foster/bitcoin-bitcoin-core
0
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2025-02-02 09:46:52 -05:00
Code Issues Activity

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

Browse source 
fb56d37612 p2p: ensure inv is GenMsgTx before ToGenTxid in inv processing (John Newbery)
aa3621385e test: use CInv::MSG_WITNESS_TX flag in p2p_segwit (Jon Atack)
24ee4f01ea p2p: make gtxid(.hash) and fAlreadyHave localvars const (Jon Atack)
b1c855453b p2p: use CInv block message helpers in net_processing.cpp (Jon Atack)
acd6642167 [net processing] Change AlreadyHaveTx() to take a GenTxid (John Newbery)
5fdfb80b86 [net processing] Change AlreadyHaveBlock() to take block_hash argument (John Newbery)
430e183b89 [net processing] Remove mempool argument from AlreadyHaveBlock() (John Newbery)
42ca5618ca [net processing] Split AlreadyHave() into separate block and tx functions (John Newbery)
39f1dc9445 p2p: remove nFetchFlags from NetMsgType TX and INV processing (Jon Atack)
471714e1f0 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 fb56d37612
  jnewbery:
    utACK fb56d37612
  vasild:
    ACK fb56d3761

Tree-SHA512: ba39b58e6aaf850880a842fe5f6295e9f1870906ef690206acfc17140aae2ac854981e1066dbcd4238062478762fbd040ef772fdc2c50eea6869997c583e6a6d
This commit is contained in:
Wladimir J. van der Laan 2020-09-02 12:54:19 +02:00
commit 505b39e72b
No known key found for this signature in database
GPG key ID: 1E4AED62986CD25D
4 changed files with 74 additions and 74 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

124
src/net_processing.cpp
View file

@ -1465,47 +1465,40 @@ 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 GenTxid& gtxid, 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;
const uint256& hash = gtxid.GetHash();
{
LOCK(g_cs_orphans);
if (!gtxid.IsWtxid() && mapOrphanTransactions.count(hash)) {
return true;
} else if (gtxid.IsWtxid() && g_orphans_by_wtxid.count(hash)) {
return true;
}
}
{
LOCK(g_cs_recent_confirmed_transactions);
if (g_recent_confirmed_transactions->contains(hash)) return true;
}
return recentRejects->contains(hash) || mempool.exists(gtxid);
}
bool static AlreadyHaveBlock(const uint256& block_hash) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
{
return LookupBlockIndex(block_hash) != nullptr;
}
void RelayTransaction(const uint256& txid, const uint256& wtxid, const CConnman& connman)
@ -1608,7 +1601,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());
@ -1634,7 +1627,7 @@ void static ProcessGetBlockData(CNode& pfrom, const CChainParams& chainparams, c
std::shared_ptr<const CBlock> 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<uint8_t> block_data;
@ -1651,12 +1644,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) {
@ -1680,9 +1672,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
@ -1810,7 +1800,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
@ -2692,14 +2682,11 @@ void PeerLogicValidation::ProcessMessage(CNode& pfrom, const std::string& msg_ty
LOCK(cs_main);
uint32_t nFetchFlags = GetFetchFlags(pfrom);
const auto current_time = GetTime<std::chrono::microseconds>();
uint256* best_block{nullptr};
for (CInv &inv : vInv)
{
if (interruptMsgProc)
return;
for (CInv& inv : vInv) {
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.
@ -2710,14 +2697,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.IsMsgBlk()) {
const bool fAlreadyHave = AlreadyHaveBlock(inv.hash);
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)) {
// Headers-first is the primary method of announcement on
@ -2727,15 +2710,21 @@ 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());
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());
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);
}
} else {
LogPrint(BCLog::NET, "Unknown inv type \"%s\" received from peer=%d\n", inv.ToString(), pfrom.GetId());
}
}
@ -3006,7 +2995,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(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);
@ -3050,7 +3039,6 @@ void PeerLogicValidation::ProcessMessage(CNode& pfrom, const std::string& msg_ty
}
}
if (!fRejectedParents) {
uint32_t nFetchFlags = GetFetchFlags(pfrom);
const auto current_time = GetTime<std::chrono::microseconds>();
for (const uint256& parent_txid : unique_parents) {
@ -3059,9 +3047,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 | nFetchFlags, parent_txid);
const GenTxid gtxid{/* is_wtxid=*/false, parent_txid};
pfrom.AddKnownTx(parent_txid);
if (!AlreadyHave(_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());
@ -4611,7 +4599,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(ToGenTxid(inv), m_mempool)) {
// If this transaction was last requested more than 1 minute ago,
// then request.
const auto last_request_time = GetTxRequestTime(gtxid);

20
src/protocol.h
View file

@ -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;

3
test/functional/p2p_segwit.py
View file

@ -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,
@ -2157,7 +2158,7 @@ class SegWitTest(BitcoinTestFramework):
self.wtx_node.wait_for_getdata([tx.sha256], 60)
with p2p_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)

1
test/functional/test_framework/messages.py
View file

@ -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