Merge #19914: refactor: Do not pass chain params to CheckForStaleTipAndEvictPeers twice

fa7e407b504bc60c77341f02636ed9d6a4b53d79 Do not pass chain params to CheckForStaleTipAndEvictPeers twice (MarcoFalke)

Pull request description:

  `PeerManager` already keeps a reference to the chain params as a member variable. No need to pass it in once again as a function parameter.

ACKs for top commit:
  naumenkogs:
    utACK fa7e407b504bc60c77341f02636ed9d6a4b53d79
  jnewbery:
    code review ACK fa7e407b504bc60c77341f02636ed9d6a4b53d79
  epson121:
    Code review ACK fa7e407b504bc60c77341f02636ed9d6a4b53d79

Tree-SHA512: 640c2d8adf9f1d54d0bfbdf81989064be2f5ba4b534d07d42258b372dc130f7b9c3fd087c7d28f0439678d124127f5d6f82f3139b1766f59f5ed661e7ac2a923
This commit is contained in:
MarcoFalke 2020-09-08 22:13:32 +02:00 committed by pasta
parent 5492191904
commit 48016a3fba
No known key found for this signature in database
GPG Key ID: 52527BEDABE87984
3 changed files with 12 additions and 14 deletions

View File

@ -240,7 +240,7 @@ public:
bool SendMessages(CNode* pto) override EXCLUSIVE_LOCKS_REQUIRED(pto->cs_sendProcessing);
/** Implement PeerManager */
void CheckForStaleTipAndEvictPeers(const Consensus::Params &consensusParams) override;
void CheckForStaleTipAndEvictPeers() override;
bool GetNodeStateStats(NodeId nodeid, CNodeStateStats& stats) override;
bool IgnoresIncomingTxs() override { return m_ignore_incoming_txs; }
void RelayTransaction(const uint256& txid) override;
@ -1582,13 +1582,12 @@ PeerManagerImpl::PeerManagerImpl(const CChainParams& chainparams, CConnman& conn
m_ignore_incoming_txs(ignore_incoming_txs)
{
assert(std::addressof(g_chainman) == std::addressof(m_chainman));
const Consensus::Params& consensusParams = Params().GetConsensus();
// 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)
// timer.
static_assert(EXTRA_PEER_CHECK_INTERVAL < STALE_CHECK_INTERVAL, "peer eviction timer should be less than stale tip check timer");
scheduler.scheduleEvery([this, consensusParams] { this->CheckForStaleTipAndEvictPeers(consensusParams); }, std::chrono::seconds{EXTRA_PEER_CHECK_INTERVAL});
scheduler.scheduleEvery([this] { this->CheckForStaleTipAndEvictPeers(); }, std::chrono::seconds{EXTRA_PEER_CHECK_INTERVAL});
// schedule next run for 10-15 minutes in the future
const std::chrono::milliseconds delta = std::chrono::minutes{10} + GetRandMillis(std::chrono::minutes{5});
@ -4611,7 +4610,7 @@ void PeerManagerImpl::EvictExtraOutboundPeers(int64_t time_in_seconds)
}
}
void PeerManagerImpl::CheckForStaleTipAndEvictPeers(const Consensus::Params &consensusParams)
void PeerManagerImpl::CheckForStaleTipAndEvictPeers()
{
LOCK(cs_main);
@ -4655,7 +4654,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto)
{
assert(m_llmq_ctx);
const Consensus::Params& consensusParams = Params().GetConsensus();
const Consensus::Params& consensusParams = m_chainparams.GetConsensus();
// We must call MaybeDiscourageAndDisconnect first, to ensure that we'll
// disconnect misbehaving peers even before the version handshake is complete.

View File

@ -68,7 +68,7 @@ public:
* Evict extra outbound peers. If we think our tip may be stale, connect to an extra outbound.
* Public for unit testing.
*/
virtual void CheckForStaleTipAndEvictPeers(const Consensus::Params &consensusParams) = 0;
virtual void CheckForStaleTipAndEvictPeers() = 0;
/** Process a single message from a peer. Public for fuzz testing */
virtual void ProcessMessage(CNode& pfrom, const std::string& msg_type, CDataStream& vRecv,

View File

@ -156,7 +156,6 @@ BOOST_AUTO_TEST_CASE(stale_tip_peer_management)
*m_node.chainman, *m_node.mempool, *governance, m_node.cj_ctx,
m_node.llmq_ctx, false);
const Consensus::Params& consensusParams = Params().GetConsensus();
constexpr int max_outbound_full_relay = MAX_OUTBOUND_FULL_RELAY_CONNECTIONS;
CConnman::Options options;
options.nMaxConnections = DEFAULT_MAX_PEER_CONNECTIONS;
@ -171,18 +170,18 @@ BOOST_AUTO_TEST_CASE(stale_tip_peer_management)
AddRandomOutboundPeer(vNodes, *peerLogic, connman.get());
}
peerLogic->CheckForStaleTipAndEvictPeers(consensusParams);
peerLogic->CheckForStaleTipAndEvictPeers();
// No nodes should be marked for disconnection while we have no extra peers
for (const CNode *node : vNodes) {
BOOST_CHECK(node->fDisconnect == false);
}
SetMockTime(GetTime() + 3*consensusParams.nPowTargetSpacing + 1);
SetMockTime(GetTime() + 3 * chainparams.GetConsensus().nPowTargetSpacing + 1);
// Now tip should definitely be stale, and we should look for an extra
// outbound peer
peerLogic->CheckForStaleTipAndEvictPeers(consensusParams);
peerLogic->CheckForStaleTipAndEvictPeers();
BOOST_CHECK(connman->GetTryNewOutboundPeer());
// Still no peers should be marked for disconnection
@ -195,8 +194,8 @@ BOOST_AUTO_TEST_CASE(stale_tip_peer_management)
// required time connected check should be satisfied).
AddRandomOutboundPeer(vNodes, *peerLogic, connman.get());
peerLogic->CheckForStaleTipAndEvictPeers(consensusParams);
for (int i=0; i<max_outbound_full_relay; ++i) {
peerLogic->CheckForStaleTipAndEvictPeers();
for (int i = 0; i < max_outbound_full_relay; ++i) {
BOOST_CHECK(vNodes[i]->fDisconnect == false);
}
// Last added node should get marked for eviction
@ -208,8 +207,8 @@ BOOST_AUTO_TEST_CASE(stale_tip_peer_management)
// peer, and check that the next newest node gets evicted.
UpdateLastBlockAnnounceTime(vNodes.back()->GetId(), GetTime());
peerLogic->CheckForStaleTipAndEvictPeers(consensusParams);
for (int i=0; i<max_outbound_full_relay-1; ++i) {
peerLogic->CheckForStaleTipAndEvictPeers();
for (int i = 0; i < max_outbound_full_relay - 1; ++i) {
BOOST_CHECK(vNodes[i]->fDisconnect == false);
}
BOOST_CHECK(vNodes[max_outbound_full_relay-1]->fDisconnect == true);