test: add unit test for block-relay-only eviction by mzumsande · Pull Request #23614 · bitcoin/bitcoin

mzumsande · 2021-11-27T17:24:22Z

Adds a unit test for block-relay-only eviction logic added in #19858, which was not covered by any tests before. The added test is very similar to the existing stale_tip_peer_management unit test, which tests the analogous logic for regular outbound peers.

DrahtBot · 2021-11-27T18:36:02Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#23604 (Use Sock in CNode by vasild)
#23352 (test: Extend stale_tip_peer_management test by MarcoFalke)
#21878 (Make all networking code mockable by vasild)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Coverage

Coverage	Change (pull 23614, 67fbae357af4d6553d8574824e2ee144e316ad98)	Reference (master, `d20d6ac`)
Lines	+0.0362 %	84.0190 %
Functions	+0.1206 %	76.2641 %
Branches	-0.0053 %	52.5739 %

^{Updated at: 2021-12-06T11:23:42.381149.}

shaavan

Concept ACK

The added test looks good and seems logically in line with the original block-relay-only eviction logic. However, I shall take another look before ACKing the PR.
In the meantime, I would like to point out two nit suggestions that can be improved upon.

src/test/denialofservice_tests.cpp

glozow

ACK 4c449a5, very clean 👌

src/test/denialofservice_tests.cpp

mzumsande · 2021-11-29T13:02:29Z

Thanks for the reviews - I addressed all comments!

glozow · 2021-11-29T13:03:33Z

reACK e0a214f48ec2b8f45d09f85bf8429d805e6ae411

changes:

rename s/max_outbound_blocksonly/max_outbound_block_relay
braced initialization for constants
delete newline

mzumsande · 2021-11-29T15:22:51Z

e0a214f -> 4740fe8 removed unnecessary call to UpdateLastBlockAnnounceTime (setting nLastBlockTime is sufficient)

rajarshimaitra

tACK 4740fe8

Just one nit.

rajarshimaitra · 2021-11-30T10:33:04Z

src/test/denialofservice_tests.cpp

+{
+    const CChainParams& chainparams = Params();
+    auto connman = std::make_unique<ConnmanTestMsg>(0x1337, 0x1337, *m_node.addrman);
+    auto peerLogic = PeerManager::make(chainparams, *connman, *m_node.addrman, nullptr,


This has been used before too, but can we name it peerman instead, like connman? Sounds more clear.

Yes, will do if I push again, though this should be probably done everywhere in this test per scripted-diff.

shaavan

ACK 4740fe8. Great work @ mzumsande!

glozow · 2021-12-02T10:53:35Z

reACK 4740fe8

LarryRuane

ACK 4740fe8

LarryRuane · 2021-12-01T18:13:19Z

src/test/denialofservice_tests.cpp

+    peerLogic->CheckForStaleTipAndEvictPeers();
+
+    for (int i = 0; i < max_outbound_block_relay; ++i) {
+        BOOST_CHECK(vNodes[i]->fDisconnect == false);


If you have a chance to re-touch

Suggested change

BOOST_CHECK(vNodes[i]->fDisconnect == false);

BOOST_CHECK(!vNodes[i]->fDisconnect);

(and similarly below for both true and false.) It's okay as-is, but it's more standard to not compare against (literal) true and false. You can write boolean_expression instead of boolean_expression == true and !boolean_expression instead of boolean_expression == false.

maflcko · 2021-12-06T12:23:01Z

master: https://drahtbot.space/host_reports/DrahtBot/reports/coverage/bitcoin/bitcoin/d20d6ac545159fb9/total.coverage/src/net_processing.cpp.gcov.html#4191
this: https://drahtbot.space/host_reports/DrahtBot/reports/coverage/bitcoin/bitcoin/23614/total.coverage/src/net_processing.cpp.gcov.html#4191

maflcko · 2021-12-06T12:33:29Z

src/test/denialofservice_tests.cpp

+    options.m_max_outbound_block_relay = max_outbound_block_relay;
+
+    connman->Init(options);
+    std::vector<CNode*> vNodes;


in new code you can just use nodes;

jnewbery

ACK 4740fe8

I've left a couple of nits inline. No need to address unless someone is touching this test again with follow-ups.

There is currently some work being done to overhaul the eviction logic and collect it in a single place. This test will need to be updated to test that new code.

jnewbery · 2021-12-07T10:32:14Z

src/test/denialofservice_tests.cpp

+    }
+    peerLogic->CheckForStaleTipAndEvictPeers();
+
+    for (int i = 0; i < max_outbound_block_relay; ++i) {


Alternatively, use range-based for loops throughout:

index ef18ed1385..c74864bd7f 100644 --- a/src/test/denialofservice_tests.cpp +++ b/src/test/denialofservice_tests.cpp @@ -207,14 +207,17 @@ BOOST_AUTO_TEST_CASE(block_relay_only_eviction) connman->Init(options); std::vector<CNode*> vNodes; + // reset global id counter + id = 0; + // Add block-relay-only peers up to the limit for (int i = 0; i < max_outbound_block_relay; ++i) { AddRandomOutboundPeer(vNodes, *peerLogic, *connman, ConnectionType::BLOCK_RELAY); } peerLogic->CheckForStaleTipAndEvictPeers(); - for (int i = 0; i < max_outbound_block_relay; ++i) { - BOOST_CHECK(vNodes[i]->fDisconnect == false); + for (CNode* node : vNodes) { + BOOST_CHECK(node->fDisconnect == false); } // Add an extra block-relay-only peer breaking the limit (mocks logic in ThreadOpenConnections) @@ -222,17 +225,19 @@ BOOST_AUTO_TEST_CASE(block_relay_only_eviction) peerLogic->CheckForStaleTipAndEvictPeers(); // The extra peer should only get marked for eviction after MINIMUM_CONNECT_TIME - for (int i = 0; i < max_outbound_block_relay; ++i) { - BOOST_CHECK(vNodes[i]->fDisconnect == false); + for (CNode* node : vNodes) { + BOOST_CHECK(node->fDisconnect == false); } - BOOST_CHECK(vNodes.back()->fDisconnect == false); SetMockTime(GetTime() + MINIMUM_CONNECT_TIME + 1); peerLogic->CheckForStaleTipAndEvictPeers(); - for (int i = 0; i < max_outbound_block_relay; ++i) { - BOOST_CHECK(vNodes[i]->fDisconnect == false); + for (CNode* node : vNodes) { + if (node->GetId() == max_outbound_block_relay) { + BOOST_CHECK(node->fDisconnect == true); + } else { + BOOST_CHECK(node->fDisconnect == false); + } } - BOOST_CHECK(vNodes.back()->fDisconnect == true); // Update the last block time for the extra peer, // and check that the next youngest peer gets evicted. @@ -240,11 +245,13 @@ BOOST_AUTO_TEST_CASE(block_relay_only_eviction) vNodes.back()->nLastBlockTime = GetTime(); peerLogic->CheckForStaleTipAndEvictPeers(); - for (int i = 0; i < max_outbound_block_relay - 1; ++i) { - BOOST_CHECK(vNodes[i]->fDisconnect == false); + for (CNode* node : vNodes) { + if (node->GetId() == max_outbound_block_relay - 1) { + BOOST_CHECK(node->fDisconnect == true); + } else { + BOOST_CHECK(node->fDisconnect == false); + } } - BOOST_CHECK(vNodes[max_outbound_block_relay - 1]->fDisconnect == true); - BOOST_CHECK(vNodes.back()->fDisconnect == false); for (const CNode* node : vNodes) { peerLogic->FinalizeNode(*node);

jnewbery · 2021-12-07T10:33:20Z

src/test/denialofservice_tests.cpp

 }

-static void AddRandomOutboundPeer(std::vector<CNode*>& vNodes, PeerManager& peerLogic, ConnmanTestMsg& connman)
+static void AddRandomOutboundPeer(std::vector<CNode*>& vNodes, PeerManager& peerLogic, ConnmanTestMsg& connman, ConnectionType connType)


New parameters/variables should use snake_case:

Suggested change

static void AddRandomOutboundPeer(std::vector<CNode*>& vNodes, PeerManager& peerLogic, ConnmanTestMsg& connman, ConnectionType connType)

static void AddRandomOutboundPeer(std::vector<CNode*>& vNodes, PeerManager& peerLogic, ConnmanTestMsg& connman, ConnectionType connection_type)

4740fe8 test: Add test for block relay only eviction (Martin Zumsande) Pull request description: Adds a unit test for block-relay-only eviction logic added in bitcoin#19858, which was not covered by any tests before. The added test is very similar to the existing `stale_tip_peer_management` unit test, which tests the analogous logic for regular outbound peers. ACKs for top commit: glozow: reACK 4740fe8 rajarshimaitra: tACK bitcoin@4740fe8 shaavan: ACK 4740fe8. Great work @ mzumsande! LarryRuane: ACK 4740fe8 Tree-SHA512: 5985afd7d8f7ae311903dbbf6b7d526e16309c83c88ae6dd6551960c0b186156310a6be0cf6b684f82ac1378d0fc5aa3717f0139e078471013fceb6aebe81bf6

… eviction c62f7e5 test: Add test for block relay only eviction (Martin Zumsande) Pull request description: Adds a unit test for block-relay-only eviction logic added in #19858, which was not covered by any tests before. The added test is very similar to the existing `stale_tip_peer_management` unit test, which tests the analogous logic for regular outbound peers. ACKs for top commit: glozow: reACK c62f7e5 rajarshimaitra: tACK bitcoin/bitcoin@c62f7e5 shaavan: ACK c62f7e5. Great work @ mzumsande! LarryRuane: ACK c62f7e5 Tree-SHA512: 5985afd7d8f7ae311903dbbf6b7d526e16309c83c88ae6dd6551960c0b186156310a6be0cf6b684f82ac1378d0fc5aa3717f0139e078471013fceb6aebe81bf6

DrahtBot added the Tests label Nov 27, 2021

DrahtBot mentioned this pull request Nov 27, 2021

Use Sock in CNode #23604

Merged

shaavan reviewed Nov 28, 2021

View reviewed changes

src/test/denialofservice_tests.cpp Outdated Show resolved Hide resolved

src/test/denialofservice_tests.cpp Outdated Show resolved Hide resolved

This was referenced Nov 28, 2021

Make all networking code mockable #21878

Closed

test: Extend stale_tip_peer_management test #23352

Closed

maflcko reviewed Nov 29, 2021

View reviewed changes

src/test/denialofservice_tests.cpp Outdated Show resolved Hide resolved

glozow reviewed Nov 29, 2021

View reviewed changes

src/test/denialofservice_tests.cpp Outdated Show resolved Hide resolved

mzumsande force-pushed the 202111_test_blocksonly_eviction branch from 4c449a5 to e0a214f Compare November 29, 2021 12:58

test: Add test for block relay only eviction

4740fe8

mzumsande force-pushed the 202111_test_blocksonly_eviction branch from e0a214f to 4740fe8 Compare November 29, 2021 15:21

rajarshimaitra reviewed Nov 30, 2021

View reviewed changes

shaavan approved these changes Nov 30, 2021

View reviewed changes

LarryRuane reviewed Dec 3, 2021

View reviewed changes

maflcko merged commit 8b2c0df into bitcoin:master Dec 6, 2021

maflcko reviewed Dec 6, 2021

View reviewed changes

jnewbery reviewed Dec 7, 2021

View reviewed changes

mzumsande deleted the 202111_test_blocksonly_eviction branch December 8, 2021 15:53

bitcoin locked and limited conversation to collaborators Dec 8, 2022

	BOOST_CHECK(vNodes[i]->fDisconnect == false);
	BOOST_CHECK(!vNodes[i]->fDisconnect);

	static void AddRandomOutboundPeer(std::vector<CNode*>& vNodes, PeerManager& peerLogic, ConnmanTestMsg& connman, ConnectionType connType)
	static void AddRandomOutboundPeer(std::vector<CNode*>& vNodes, PeerManager& peerLogic, ConnmanTestMsg& connman, ConnectionType connection_type)

Conversation

mzumsande commented Nov 27, 2021

Uh oh!

DrahtBot commented Nov 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Coverage

Uh oh!

shaavan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mzumsande commented Nov 29, 2021

Uh oh!

glozow commented Nov 29, 2021

Uh oh!

mzumsande commented Nov 29, 2021

Uh oh!

rajarshimaitra left a comment

Choose a reason for hiding this comment

Uh oh!

rajarshimaitra Nov 30, 2021

Choose a reason for hiding this comment

Uh oh!

mzumsande Dec 2, 2021

Choose a reason for hiding this comment

Uh oh!

shaavan left a comment • edited by maflcko Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glozow commented Dec 2, 2021

Uh oh!

LarryRuane left a comment

Choose a reason for hiding this comment

Uh oh!

LarryRuane Dec 1, 2021

Choose a reason for hiding this comment

Uh oh!

maflcko commented Dec 6, 2021

Uh oh!

maflcko Dec 6, 2021

Choose a reason for hiding this comment

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

jnewbery Dec 7, 2021

Choose a reason for hiding this comment

Uh oh!

jnewbery Dec 7, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

DrahtBot commented Nov 27, 2021 •

edited

Loading

shaavan left a comment •

edited by maflcko

Loading