From 82126d3ca527e5c06606a50ee8d8baf792193ef4 Mon Sep 17 00:00:00 2001 From: Gleb Naumenko Date: Wed, 17 Jan 2024 11:28:02 +0200 Subject: [PATCH] p2p: Cache inbound reconciling peers count It helps to avoid recomputing every time we consider a transaction for fanout/reconciliation. --- src/net_processing.cpp | 4 ++-- src/node/txreconciliation.cpp | 35 ++++++++++++++++++----------- src/node/txreconciliation.h | 2 +- src/test/txreconciliation_tests.cpp | 16 ++++++++----- 4 files changed, 35 insertions(+), 22 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index e72b95b46787f6..30fdb3e9ae6663 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1610,7 +1610,7 @@ void PeerManagerImpl::FinalizeNode(const CNode& node) } m_orphanage.EraseForPeer(nodeid); m_txrequest.DisconnectedPeer(nodeid); - if (m_txreconciliation) m_txreconciliation->ForgetPeer(nodeid); + if (m_txreconciliation) m_txreconciliation->ForgetPeer(nodeid, node.IsInboundConn()); m_num_preferred_download_peers -= state->fPreferredDownload; m_peers_downloading_from -= (!state->vBlocksInFlight.empty()); assert(m_peers_downloading_from >= 0); @@ -3623,7 +3623,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, // We could have optimistically pre-registered/registered the peer. In that case, // we should forget about the reconciliation state here if this wasn't followed // by WTXIDRELAY (since WTXIDRELAY can't be announced later). - m_txreconciliation->ForgetPeer(pfrom.GetId()); + m_txreconciliation->ForgetPeer(pfrom.GetId(), pfrom.IsInboundConn()); } } diff --git a/src/node/txreconciliation.cpp b/src/node/txreconciliation.cpp index 011005712956ec..369c36a06c6943 100644 --- a/src/node/txreconciliation.cpp +++ b/src/node/txreconciliation.cpp @@ -95,6 +95,12 @@ class TxReconciliationTracker::Impl */ std::unordered_map> m_states GUARDED_BY(m_txreconciliation_mutex); + /* + * Keeps track of how many of the registered peers are inbound. Updated on registering or + * forgetting peers. + */ + size_t m_inbounds_count GUARDED_BY(m_txreconciliation_mutex){0}; + /* * A least-recently-added cache tracking which peers we should fanout a transaction to. * @@ -170,6 +176,9 @@ class TxReconciliationTracker::Impl m_states.erase(recon_state); m_states.emplace(peer_id, std::move(new_state)); + if (is_peer_inbound && m_inbounds_count < std::numeric_limits::max()) { + ++m_inbounds_count; + } return ReconciliationRegisterResult::SUCCESS; } @@ -214,13 +223,20 @@ class TxReconciliationTracker::Impl return peer_state->m_local_set.erase(wtxid_to_remove) > 0; } - void ForgetPeer(NodeId peer_id) EXCLUSIVE_LOCKS_REQUIRED(!m_txreconciliation_mutex) + void ForgetPeer(NodeId peer_id, bool is_peer_inbound) EXCLUSIVE_LOCKS_REQUIRED(!m_txreconciliation_mutex) { AssertLockNotHeld(m_txreconciliation_mutex); LOCK(m_txreconciliation_mutex); - if (m_states.erase(peer_id)) { - LogPrintLevel(BCLog::TXRECONCILIATION, BCLog::Level::Debug, "Forget txreconciliation state of peer=%d\n", peer_id); + const auto peer = m_states.find(peer_id); + if (peer == m_states.end()) return; + const bool registered = std::holds_alternative(peer->second); + m_states.erase(peer); + + if (registered && is_peer_inbound) { + Assert(m_inbounds_count > 0); + --m_inbounds_count; } + LogPrintLevel(BCLog::TXRECONCILIATION, BCLog::Level::Debug, "Forget txreconciliation state of peer=%d\n", peer_id); } /** @@ -315,16 +331,9 @@ class TxReconciliationTracker::Impl if (peer_state->m_we_initiate) { destinations = OUTBOUND_FANOUT_DESTINATIONS - outbounds_nonrcncl_tx_relay; } else { - const size_t inbound_rcncl_peers = std::count_if(m_states.begin(), m_states.end(), - [](const auto& indexed_state) { - const auto* cur_state = std::get_if(&indexed_state.second); - if (cur_state) return !cur_state->m_we_initiate; - return false; - }); - // Since we use the fraction for inbound peers, we first need to compute the total // number of inbound targets. - const double inbound_targets = (inbounds_nonrcncl_tx_relay + inbound_rcncl_peers) * INBOUND_FANOUT_DESTINATIONS_FRACTION; + const double inbound_targets = (inbounds_nonrcncl_tx_relay + m_inbounds_count) * INBOUND_FANOUT_DESTINATIONS_FRACTION; destinations = inbound_targets - inbounds_nonrcncl_tx_relay; } @@ -363,9 +372,9 @@ bool TxReconciliationTracker::TryRemovingFromSet(NodeId peer_id, const Wtxid& wt return m_impl->TryRemovingFromSet(peer_id, wtxid_to_remove); } -void TxReconciliationTracker::ForgetPeer(NodeId peer_id) +void TxReconciliationTracker::ForgetPeer(NodeId peer_id, bool is_peer_inbound) { - m_impl->ForgetPeer(peer_id); + m_impl->ForgetPeer(peer_id, is_peer_inbound); } bool TxReconciliationTracker::IsPeerRegistered(NodeId peer_id) const diff --git a/src/node/txreconciliation.h b/src/node/txreconciliation.h index d923baff483b98..835a58be680966 100644 --- a/src/node/txreconciliation.h +++ b/src/node/txreconciliation.h @@ -92,7 +92,7 @@ class TxReconciliationTracker * Attempts to forget txreconciliation-related state of the peer (if we previously stored any). * After this, we won't be able to reconcile transactions with the peer. */ - void ForgetPeer(NodeId peer_id); + void ForgetPeer(NodeId peer_id, bool is_peer_inbound); /** * Check if a peer is registered to reconcile transactions with us. diff --git a/src/test/txreconciliation_tests.cpp b/src/test/txreconciliation_tests.cpp index f385749ddd6963..53a8061f2536c6 100644 --- a/src/test/txreconciliation_tests.cpp +++ b/src/test/txreconciliation_tests.cpp @@ -53,9 +53,13 @@ BOOST_AUTO_TEST_CASE(ForgetPeerTest) TxReconciliationTracker tracker(TXRECONCILIATION_VERSION, hasher); NodeId peer_id0 = 0; + // Removing peer which is not there works. + tracker.ForgetPeer(peer_id0, true); + tracker.ForgetPeer(peer_id0, false); + // Removing peer after pre-registring works and does not let to register the peer. tracker.PreRegisterPeer(peer_id0); - tracker.ForgetPeer(peer_id0); + tracker.ForgetPeer(peer_id0, true); BOOST_CHECK_EQUAL(tracker.RegisterPeer(peer_id0, true, 1, 1), ReconciliationRegisterResult::NOT_FOUND); // Removing peer after it is registered works. @@ -63,7 +67,7 @@ BOOST_AUTO_TEST_CASE(ForgetPeerTest) BOOST_REQUIRE(!tracker.IsPeerRegistered(peer_id0)); BOOST_REQUIRE_EQUAL(tracker.RegisterPeer(peer_id0, true, 1, 1), ReconciliationRegisterResult::SUCCESS); BOOST_CHECK(tracker.IsPeerRegistered(peer_id0)); - tracker.ForgetPeer(peer_id0); + tracker.ForgetPeer(peer_id0, true); BOOST_CHECK(!tracker.IsPeerRegistered(peer_id0)); } @@ -80,7 +84,7 @@ BOOST_AUTO_TEST_CASE(IsPeerRegisteredTest) BOOST_REQUIRE_EQUAL(tracker.RegisterPeer(peer_id0, true, 1, 1), ReconciliationRegisterResult::SUCCESS); BOOST_CHECK(tracker.IsPeerRegistered(peer_id0)); - tracker.ForgetPeer(peer_id0); + tracker.ForgetPeer(peer_id0, true); BOOST_CHECK(!tracker.IsPeerRegistered(peer_id0)); } @@ -102,7 +106,7 @@ BOOST_AUTO_TEST_CASE(AddToSetTest) BOOST_REQUIRE(tracker.AddToSet(peer_id0, wtxid)); - tracker.ForgetPeer(peer_id0); + tracker.ForgetPeer(peer_id0, true); Wtxid wtxid2{Wtxid::FromUint256(frc.rand256())}; BOOST_REQUIRE(!tracker.AddToSet(peer_id0, wtxid2)); @@ -138,7 +142,7 @@ BOOST_AUTO_TEST_CASE(TryRemovingFromSetTest) BOOST_REQUIRE(!tracker.TryRemovingFromSet(peer_id0, wtxid)); BOOST_REQUIRE(tracker.AddToSet(peer_id0, wtxid)); - tracker.ForgetPeer(peer_id0); + tracker.ForgetPeer(peer_id0, true); BOOST_REQUIRE(!tracker.TryRemovingFromSet(peer_id0, wtxid)); } @@ -178,7 +182,7 @@ BOOST_AUTO_TEST_CASE(ShouldFanoutToTest) /*inbounds_nonrcncl_tx_relay=*/0, /*outbounds_nonrcncl_tx_relay=*/1)); } - tracker.ForgetPeer(peer_id0); + tracker.ForgetPeer(peer_id0, false); // A forgotten (reconciliation-wise) peer should be always selected for fanout again. for (int i = 0; i < 100; ++i) { BOOST_CHECK(tracker.ShouldFanoutTo(Wtxid::FromUint256(frc.rand256()), peer_id0,