From 9190b01d8dcf03b74e9b9e1653688a97ac171b37 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Wed, 31 Mar 2021 18:34:49 +0100 Subject: [net processing] Add Orphanage empty consistency check When removing the final peer, assert that m_tx_orphanage is empty. --- src/net_processing.cpp | 1 + src/txorphanage.h | 7 +++++++ 2 files changed, 8 insertions(+) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 9c4544df21..c2202c73da 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1194,6 +1194,7 @@ void PeerManagerImpl::FinalizeNode(const CNode& node) assert(m_outbound_peers_with_protect_from_disconnect == 0); assert(m_wtxid_relay_peers == 0); assert(m_txrequest.Size() == 0); + assert(m_orphanage.Size() == 0); } } // cs_main if (node.fSuccessfullyConnected && misbehavior == 0 && diff --git a/src/txorphanage.h b/src/txorphanage.h index e4266e470a..24c8318f36 100644 --- a/src/txorphanage.h +++ b/src/txorphanage.h @@ -47,6 +47,13 @@ public: * (ie orphans that may have found their final missing parent, and so should be reconsidered for the mempool) */ void AddChildrenToWorkSet(const CTransaction& tx, std::set& orphan_work_set) const EXCLUSIVE_LOCKS_REQUIRED(g_cs_orphans); + /** Return how many entries exist in the orphange */ + size_t Size() LOCKS_EXCLUDED(::g_cs_orphans) + { + LOCK(::g_cs_orphans); + return m_orphans.size(); + } + protected: struct OrphanTx { CTransactionRef tx; -- cgit v1.2.3 From a28bfd1d4cfa523a6abf3832dbfd6183cd546944 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Wed, 31 Mar 2021 18:37:52 +0100 Subject: [net processing] Default initialize m_stale_tip_check_time --- 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 c2202c73da..6a1cd7f022 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -396,7 +396,8 @@ private: /** The height of the best chain */ std::atomic m_best_height{-1}; - int64_t m_stale_tip_check_time; //!< Next time to check for stale tip + /** Next time to check for stale tip */ + int64_t m_stale_tip_check_time{0}; /** Whether this node is running in blocks only mode */ const bool m_ignore_incoming_txs; @@ -1393,7 +1394,6 @@ PeerManagerImpl::PeerManagerImpl(const CChainParams& chainparams, CConnman& conn m_banman(banman), m_chainman(chainman), m_mempool(pool), - m_stale_tip_check_time(0), m_ignore_incoming_txs(ignore_incoming_txs) { // Initialize global variables that cannot be constructed at startup. -- cgit v1.2.3 From cd9902ac5054c01228d52616bf85f7196364d4ff Mon Sep 17 00:00:00 2001 From: John Newbery Date: Wed, 31 Mar 2021 18:40:47 +0100 Subject: [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 6a1cd7f022..0f5f18aada 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) { -- cgit v1.2.3 From 37dcd12d539e4a875581fa049aa0f7fafeb932a4 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Wed, 31 Mar 2021 18:44:18 +0100 Subject: scripted-diff: Rename recentRejects -BEGIN VERIFY SCRIPT- ren() { sed -i "s:\<$1\>:$2:g" $(git grep -l "\<$1\>" ./src ./test); } ren recentRejects m_recent_rejects -END VERIFY SCRIPT- --- src/net_processing.cpp | 28 ++++++++++++++-------------- test/functional/mempool_reorg.py | 2 +- test/functional/p2p_permissions.py | 2 +- 3 files changed, 16 insertions(+), 16 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 0f5f18aada..94ecbdf983 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -470,7 +470,7 @@ private: * * Memory used: 1.3 MB */ - CRollingBloomFilter recentRejects GUARDED_BY(::cs_main){120'000, 0.000'001}; + CRollingBloomFilter m_recent_rejects GUARDED_BY(::cs_main){120'000, 0.000'001}; uint256 hashRecentRejectsChainTip GUARDED_BY(cs_main); /* @@ -1604,7 +1604,7 @@ bool PeerManagerImpl::AlreadyHaveTx(const GenTxid& gtxid) // or a double-spend. Reset the rejects filter and give those // txs a second chance. hashRecentRejectsChainTip = m_chainman.ActiveChain().Tip()->GetBlockHash(); - recentRejects.reset(); + m_recent_rejects.reset(); } const uint256& hash = gtxid.GetHash(); @@ -1616,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 m_recent_rejects.contains(hash) || m_mempool.exists(gtxid); } bool PeerManagerImpl::AlreadyHaveBlock(const uint256& block_hash) @@ -2235,7 +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. - recentRejects.insert(porphanTx->GetWitnessHash()); + m_recent_rejects.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 @@ -2247,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()); + m_recent_rejects.insert(porphanTx->GetHash()); } } m_orphanage.EraseTx(orphanHash); @@ -3250,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 (m_recent_rejects.contains(parent_txid)) { fRejectedParents = true; break; } @@ -3291,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()); + m_recent_rejects.insert(tx.GetHash()); + m_recent_rejects.insert(tx.GetWitnessHash()); m_txrequest.ForgetTxHash(tx.GetHash()); m_txrequest.ForgetTxHash(tx.GetWitnessHash()); } @@ -3311,7 +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. - recentRejects.insert(tx.GetWitnessHash()); + m_recent_rejects.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 @@ -3322,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()); + m_recent_rejects.insert(tx.GetHash()); m_txrequest.ForgetTxHash(tx.GetHash()); } if (RecursiveDynamicUsage(*ptx) < 100000) { @@ -3331,21 +3331,21 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, } } - // If a tx has been detected by recentRejects, we will have reached + // If a tx has been detected by m_recent_rejects, we will have reached // this point and the tx will have been ignored. Because we haven't run // the tx through AcceptToMemoryPool, we won't have computed a DoS // score for it or determined exactly why we consider it invalid. // // This means we won't penalize any peer subsequently relaying a DoSy // tx (even if we penalized the first peer who gave it to us) because - // we have to account for recentRejects showing false positives. In + // we have to account for m_recent_rejects showing false positives. In // other words, we shouldn't penalize a peer if we aren't *sure* they // submitted a DoSy tx. // - // Note that recentRejects doesn't just record DoSy or invalid + // Note that m_recent_rejects doesn't just record DoSy or invalid // transactions, but any tx not accepted by the mempool, which may be // due to node policy (vs. consensus). So we can't blanket penalize a - // peer simply for relaying a tx that our recentRejects has caught, + // peer simply for relaying a tx that our m_recent_rejects has caught, // regardless of false positives. if (state.IsInvalid()) { diff --git a/test/functional/mempool_reorg.py b/test/functional/mempool_reorg.py index bcc6aa7bcc..b5086e1df1 100755 --- a/test/functional/mempool_reorg.py +++ b/test/functional/mempool_reorg.py @@ -80,7 +80,7 @@ class MempoolCoinbaseTest(BitcoinTestFramework): self.log.info("Generate a block") last_block = self.nodes[0].generate(1) # Sync blocks, so that peer 1 gets the block before timelock_tx - # Otherwise, peer 1 would put the timelock_tx in recentRejects + # Otherwise, peer 1 would put the timelock_tx in m_recent_rejects self.sync_all() self.log.info("The time-locked transaction can now be spent") diff --git a/test/functional/p2p_permissions.py b/test/functional/p2p_permissions.py index 594a28d662..8b285907c5 100755 --- a/test/functional/p2p_permissions.py +++ b/test/functional/p2p_permissions.py @@ -130,7 +130,7 @@ class P2PPermissionsTests(BitcoinTestFramework): tx.vout[0].nValue += 1 txid = tx.rehash() # Send the transaction twice. The first time, it'll be rejected by ATMP because it conflicts - # with a mempool transaction. The second time, it'll be in the recentRejects filter. + # with a mempool transaction. The second time, it'll be in the m_recent_rejects filter. p2p_rebroadcast_wallet.send_txs_and_test( [tx], self.nodes[1], -- cgit v1.2.3 From fde1bf4f6136638e84cdf9806eedaae08e841bbf Mon Sep 17 00:00:00 2001 From: John Newbery Date: Wed, 31 Mar 2021 18:40:47 +0100 Subject: [net processing] Default initialize m_recent_confirmed_transactions Now that m_recent_confirmed_transactions is owned by PeerManagerImpl, and PeerManagerImpl's lifetime is managed by the node context, we can just default initialize m_recent_confirmed_transactions during object initialization. We can also remove the unique_ptr indirection. --- src/net_processing.cpp | 31 +++++++++++++++---------------- 1 file changed, 15 insertions(+), 16 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 94ecbdf983..c418620859 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -477,9 +477,19 @@ private: * Filter for transactions that have been recently confirmed. * We use this to avoid requesting transactions that have already been * confirnmed. + * + * 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. + * If the number of transactions appearing in a block goes up, or if we are + * seeing getdata requests more than an hour after initial announcement, we + * can increase this number. + * The false positive rate of 1/1M should come out to less than 1 + * transaction per day that would be inadvertently ignored (which is the + * same probability that we have in the reject filter). */ Mutex m_recent_confirmed_transactions_mutex; - std::unique_ptr m_recent_confirmed_transactions GUARDED_BY(m_recent_confirmed_transactions_mutex); + CRollingBloomFilter m_recent_confirmed_transactions GUARDED_BY(m_recent_confirmed_transactions_mutex){48'000, 0.000'001}; /** Have we requested this block from a peer */ bool IsBlockRequested(const uint256& hash) EXCLUSIVE_LOCKS_REQUIRED(cs_main); @@ -1396,17 +1406,6 @@ PeerManagerImpl::PeerManagerImpl(const CChainParams& chainparams, CConnman& conn m_mempool(pool), m_ignore_incoming_txs(ignore_incoming_txs) { - // 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. - // If the number of transactions appearing in a block goes up, or if we are - // seeing getdata requests more than an hour after initial announcement, we - // can increase this number. - // The false positive rate of 1/1M should come out to less than 1 - // transaction per day that would be inadvertently ignored (which is the - // same probability that we have in the reject filter). - m_recent_confirmed_transactions.reset(new CRollingBloomFilter(48000, 0.000001)); - // Stale tip checking and peer eviction are on two different timers, but we // don't want them to get out of sync due to drift in the scheduler, so we // combine them in one function and schedule at the quicker (peer-eviction) @@ -1432,9 +1431,9 @@ void PeerManagerImpl::BlockConnected(const std::shared_ptr& pblock { LOCK(m_recent_confirmed_transactions_mutex); for (const auto& ptx : pblock->vtx) { - m_recent_confirmed_transactions->insert(ptx->GetHash()); + m_recent_confirmed_transactions.insert(ptx->GetHash()); if (ptx->GetHash() != ptx->GetWitnessHash()) { - m_recent_confirmed_transactions->insert(ptx->GetWitnessHash()); + m_recent_confirmed_transactions.insert(ptx->GetWitnessHash()); } } } @@ -1458,7 +1457,7 @@ void PeerManagerImpl::BlockDisconnected(const std::shared_ptr &blo // presumably the most common case of relaying a confirmed transaction // should be just after a new block containing it is found. LOCK(m_recent_confirmed_transactions_mutex); - m_recent_confirmed_transactions->reset(); + m_recent_confirmed_transactions.reset(); } // All of the following cache a recent block, and are protected by cs_most_recent_block @@ -1613,7 +1612,7 @@ bool PeerManagerImpl::AlreadyHaveTx(const GenTxid& gtxid) { LOCK(m_recent_confirmed_transactions_mutex); - if (m_recent_confirmed_transactions->contains(hash)) return true; + if (m_recent_confirmed_transactions.contains(hash)) return true; } return m_recent_rejects.contains(hash) || m_mempool.exists(gtxid); -- cgit v1.2.3