Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

p2p: adaptive connections services flags #28170

Merged
merged 6 commits into from
Jan 31, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions src/Makefile.test.include
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@ BITCOIN_TESTS =\
test/net_tests.cpp \
test/netbase_tests.cpp \
test/orphanage_tests.cpp \
test/peerman_tests.cpp \
test/pmt_tests.cpp \
test/policy_fee_tests.cpp \
test/policyestimator_tests.cpp \
Expand Down
76 changes: 76 additions & 0 deletions src/test/peerman_tests.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
// Copyright (c) 2024-present The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or https://www.opensource.org/licenses/mit-license.php.

#include <chainparams.h>
#include <node/miner.h>
#include <net_processing.h>
#include <pow.h>
#include <test/util/setup_common.h>
#include <validation.h>

#include <boost/test/unit_test.hpp>

BOOST_FIXTURE_TEST_SUITE(peerman_tests, RegTestingSetup)

/** Window, in blocks, for connecting to NODE_NETWORK_LIMITED peers */
static constexpr int64_t NODE_NETWORK_LIMITED_ALLOW_CONN_BLOCKS = 144;

static void mineBlock(const node::NodeContext& node, std::chrono::seconds block_time)
{
auto curr_time = GetTime<std::chrono::seconds>();
SetMockTime(block_time); // update time so the block is created with it
CBlock block = node::BlockAssembler{node.chainman->ActiveChainstate(), nullptr}.CreateNewBlock(CScript() << OP_TRUE)->block;
while (!CheckProofOfWork(block.GetHash(), block.nBits, node.chainman->GetConsensus())) ++block.nNonce;
block.fChecked = true; // little speedup
SetMockTime(curr_time); // process block at current time
Assert(node.chainman->ProcessNewBlock(std::make_shared<const CBlock>(block), /*force_processing=*/true, /*min_pow_checked=*/true, nullptr));
SyncWithValidationInterfaceQueue(); // drain events queue
}

// Verifying when network-limited peer connections are desirable based on the node's proximity to the tip
BOOST_AUTO_TEST_CASE(connections_desirable_service_flags)
{
std::unique_ptr<PeerManager> peerman = PeerManager::make(*m_node.connman, *m_node.addrman, nullptr, *m_node.chainman, *m_node.mempool, {});
auto consensus = m_node.chainman->GetParams().GetConsensus();

// Check we start connecting to full nodes
ServiceFlags peer_flags{NODE_WITNESS | NODE_NETWORK_LIMITED};
BOOST_CHECK(peerman->GetDesirableServiceFlags(peer_flags) == ServiceFlags(NODE_NETWORK | NODE_WITNESS));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could also have a check for HasAllDesirableServiceFlags each time GetDesirableServiceFlags is checked.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, noted. As it is a nit and would love to move forward, will leave it for a small follow-up. Thanks!


// Make peerman aware of the initial best block and verify we accept limited peers when we start close to the tip time.
auto tip = WITH_LOCK(::cs_main, return m_node.chainman->ActiveChain().Tip());
uint64_t tip_block_time = tip->GetBlockTime();
int tip_block_height = tip->nHeight;
peerman->SetBestBlock(tip_block_height, std::chrono::seconds{tip_block_time});

SetMockTime(tip_block_time + 1); // Set node time to tip time
BOOST_CHECK(peerman->GetDesirableServiceFlags(peer_flags) == ServiceFlags(NODE_NETWORK_LIMITED | NODE_WITNESS));

// Check we don't disallow limited peers connections when we are behind but still recoverable (below the connection safety window)
SetMockTime(GetTime<std::chrono::seconds>() + std::chrono::seconds{consensus.nPowTargetSpacing * (NODE_NETWORK_LIMITED_ALLOW_CONN_BLOCKS - 1)});
BOOST_CHECK(peerman->GetDesirableServiceFlags(peer_flags) == ServiceFlags(NODE_NETWORK_LIMITED | NODE_WITNESS));

// Check we disallow limited peers connections when we are further than the limited peers safety window
SetMockTime(GetTime<std::chrono::seconds>() + std::chrono::seconds{consensus.nPowTargetSpacing * 2});
BOOST_CHECK(peerman->GetDesirableServiceFlags(peer_flags) == ServiceFlags(NODE_NETWORK | NODE_WITNESS));

// By now, we tested that the connections desirable services flags change based on the node's time proximity to the tip.
// Now, perform the same tests for when the node receives a block.
RegisterValidationInterface(peerman.get());

// First, verify a block in the past doesn't enable limited peers connections
// At this point, our time is (NODE_NETWORK_LIMITED_ALLOW_CONN_BLOCKS + 1) * 10 minutes ahead the tip's time.
mineBlock(m_node, /*block_time=*/std::chrono::seconds{tip_block_time + 1});
BOOST_CHECK(peerman->GetDesirableServiceFlags(peer_flags) == ServiceFlags(NODE_NETWORK | NODE_WITNESS));

// Verify a block close to the tip enables limited peers connections
mineBlock(m_node, /*block_time=*/GetTime<std::chrono::seconds>());
BOOST_CHECK(peerman->GetDesirableServiceFlags(peer_flags) == ServiceFlags(NODE_NETWORK_LIMITED | NODE_WITNESS));

// Lastly, verify the stale tip checks can disallow limited peers connections after not receiving blocks for a prolonged period.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As noted in #28170 (comment), stale tip checks don't factor into the current approach. This check seems redundant now with the one on lines 54-56.

Copy link
Member Author

@furszy furszy Jan 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As noted in #28170 (comment), stale tip checks don't factor into the current approach.

Sure. As it is a nit, will update the comment in the follow-up PR. Thanks!.

This check seems redundant now with the one on lines 54-56.

I think it's worth checking whether the services flags can still change after receiving blocks from the network. The lines 54-56 are checking updates prior receiving any block.

SetMockTime(GetTime<std::chrono::seconds>() + std::chrono::seconds{consensus.nPowTargetSpacing * NODE_NETWORK_LIMITED_ALLOW_CONN_BLOCKS + 1});
BOOST_CHECK(peerman->GetDesirableServiceFlags(peer_flags) == ServiceFlags(NODE_NETWORK | NODE_WITNESS));
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aff7d92

would be nice to test reacting to reorgs.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. As it would be really nice to progress on this PR, will leave it follow-up. Thanks.


BOOST_AUTO_TEST_SUITE_END()