From cd9902ac5054c01228d52616bf85f7196364d4ff Mon Sep 17 00:00:00 2001 From: John Newbery Date: Wed, 31 Mar 2021 18:40:47 +0100 Subject: [PATCH] [net processing] Default initialize recentRejects Now that recentRejects is owned by PeerManagerImpl, and PeerManagerImpl's lifetime is managed by the node context, we can just default initialize recentRejects during object initialization. We can also remove the unique_ptr indirection. --- src/net_processing.cpp | 26 ++++++++++---------------- 1 file changed, 10 insertions(+), 16 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 6a1cd7f0222..0f5f18aada5 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -470,7 +470,7 @@ private: * * Memory used: 1.3 MB */ - std::unique_ptr recentRejects GUARDED_BY(cs_main); + CRollingBloomFilter recentRejects GUARDED_BY(::cs_main){120'000, 0.000'001}; uint256 hashRecentRejectsChainTip GUARDED_BY(cs_main); /* @@ -1396,9 +1396,6 @@ PeerManagerImpl::PeerManagerImpl(const CChainParams& chainparams, CConnman& conn m_mempool(pool), m_ignore_incoming_txs(ignore_incoming_txs) { - // Initialize global variables that cannot be constructed at startup. - recentRejects.reset(new CRollingBloomFilter(120000, 0.000001)); - // Blocks don't typically have more than 4000 transactions, so this should // be at least six blocks (~1 hr) worth of transactions that we can store, // inserting both a txid and wtxid for every observed transaction. @@ -1601,14 +1598,13 @@ void PeerManagerImpl::BlockChecked(const CBlock& block, const BlockValidationSta bool PeerManagerImpl::AlreadyHaveTx(const GenTxid& gtxid) { - assert(recentRejects); if (m_chainman.ActiveChain().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 = m_chainman.ActiveChain().Tip()->GetBlockHash(); - recentRejects->reset(); + recentRejects.reset(); } const uint256& hash = gtxid.GetHash(); @@ -1620,7 +1616,7 @@ bool PeerManagerImpl::AlreadyHaveTx(const GenTxid& gtxid) if (m_recent_confirmed_transactions->contains(hash)) return true; } - return recentRejects->contains(hash) || m_mempool.exists(gtxid); + return recentRejects.contains(hash) || m_mempool.exists(gtxid); } bool PeerManagerImpl::AlreadyHaveBlock(const uint256& block_hash) @@ -2239,8 +2235,7 @@ void PeerManagerImpl::ProcessOrphanTx(std::set& orphan_work_set) // See also comments in https://github.com/bitcoin/bitcoin/pull/18044#discussion_r443419034 // for concerns around weakening security of unupgraded nodes // if we start doing this too early. - assert(recentRejects); - recentRejects->insert(porphanTx->GetWitnessHash()); + recentRejects.insert(porphanTx->GetWitnessHash()); // If the transaction failed for TX_INPUTS_NOT_STANDARD, // then we know that the witness was irrelevant to the policy // failure, since this check depends only on the txid @@ -2252,7 +2247,7 @@ void PeerManagerImpl::ProcessOrphanTx(std::set& orphan_work_set) if (state.GetResult() == TxValidationResult::TX_INPUTS_NOT_STANDARD && porphanTx->GetWitnessHash() != porphanTx->GetHash()) { // We only add the txid if it differs from the wtxid, to // avoid wasting entries in the rolling bloom filter. - recentRejects->insert(porphanTx->GetHash()); + recentRejects.insert(porphanTx->GetHash()); } } m_orphanage.EraseTx(orphanHash); @@ -3255,7 +3250,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, std::sort(unique_parents.begin(), unique_parents.end()); unique_parents.erase(std::unique(unique_parents.begin(), unique_parents.end()), unique_parents.end()); for (const uint256& parent_txid : unique_parents) { - if (recentRejects->contains(parent_txid)) { + if (recentRejects.contains(parent_txid)) { fRejectedParents = true; break; } @@ -3296,8 +3291,8 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, // regardless of what witness is provided, we will not accept // this, so we don't need to allow for redownload of this txid // from any of our non-wtxidrelay peers. - recentRejects->insert(tx.GetHash()); - recentRejects->insert(tx.GetWitnessHash()); + recentRejects.insert(tx.GetHash()); + recentRejects.insert(tx.GetWitnessHash()); m_txrequest.ForgetTxHash(tx.GetHash()); m_txrequest.ForgetTxHash(tx.GetWitnessHash()); } @@ -3316,8 +3311,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, // See also comments in https://github.com/bitcoin/bitcoin/pull/18044#discussion_r443419034 // for concerns around weakening security of unupgraded nodes // if we start doing this too early. - assert(recentRejects); - recentRejects->insert(tx.GetWitnessHash()); + recentRejects.insert(tx.GetWitnessHash()); m_txrequest.ForgetTxHash(tx.GetWitnessHash()); // If the transaction failed for TX_INPUTS_NOT_STANDARD, // then we know that the witness was irrelevant to the policy @@ -3328,7 +3322,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, // transactions are later received (resulting in // parent-fetching by txid via the orphan-handling logic). if (state.GetResult() == TxValidationResult::TX_INPUTS_NOT_STANDARD && tx.GetWitnessHash() != tx.GetHash()) { - recentRejects->insert(tx.GetHash()); + recentRejects.insert(tx.GetHash()); m_txrequest.ForgetTxHash(tx.GetHash()); } if (RecursiveDynamicUsage(*ptx) < 100000) {