test: Extend stale_tip_peer_management test #23352

pull maflcko wants to merge 1 commits into bitcoin:master from maflcko:2110-testDos changing 4 files +80 −8

maflcko commented at 10:08 AM on October 25, 2021: member

Add coverage for the case where a peer is protected when it pretends to have a block.

Also, add some ASSERT_DEBUG_LOG to document the tests for the reader.
DrahtBot added the label Tests on Oct 25, 2021
DrahtBot commented at 6:31 PM on October 25, 2021: contributor


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



Code Coverage

For detailed information about the code coverage, see the test coverage report.



Reviews

See the guideline for information on the review process.

Type Reviewers

Concept ACK jamesob, jonatack, mzumsande

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #28464 (net: improve max-connection limits code by mzumsande)
- #28463 (p2p: Increase inbound capacity for block-relay only connections by mzumsande)
- #28155 (net: improves addnode / m_added_nodes logic by sr-gi)
- #26812 (test: add end-to-end tests for CConnman and PeerManager 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.
DrahtBot added the label Needs rebase on Nov 18, 2021
maflcko force-pushed on Nov 29, 2021
DrahtBot removed the label Needs rebase on Nov 29, 2021
DrahtBot added the label Needs rebase on Dec 6, 2021
jamesob commented at 4:18 PM on December 6, 2021: contributor

Concept ACK, will review soon
maflcko force-pushed on Mar 21, 2022
DrahtBot removed the label Needs rebase on Mar 21, 2022
maflcko force-pushed on Mar 22, 2022
jonatack commented at 5:00 PM on April 15, 2022: member

Concept ACK
maflcko force-pushed on Apr 15, 2022
DrahtBot added the label Needs rebase on Apr 26, 2022
maflcko force-pushed on Jul 22, 2022
DrahtBot removed the label Needs rebase on Jul 22, 2022
DrahtBot added the label Needs rebase on Aug 30, 2022
achow101 commented at 6:43 PM on October 12, 2022: member

Are you still working on this?
achow101 commented at 5:12 PM on November 10, 2022: member

Closing this as it has not had any activity in a while. If you are interested in continuing work on this, please leave a comment so that it can be reopened.
achow101 closed this on Nov 10, 2022
maflcko commented at 11:40 AM on November 11, 2022: member

Ok, I'll rebase
maflcko reopened this on Nov 11, 2022
maflcko force-pushed on Nov 11, 2022
DrahtBot removed the label Needs rebase on Nov 11, 2022

Type	Reviewers
Concept ACK	jamesob, jonatack, mzumsande

in src/test/denialofservice_tests.cpp:221 in fa9dded81d outdated

 217 |      BOOST_CHECK(vNodes.back()->fDisconnect == false);
 218 |  
 219 | +    vNodes[max_outbound_full_relay - 1]->fDisconnect = false;
 220 | +    peerLogic->UpdateLastBlockAnnounceTime(vNodes.back()->GetId(), 0);
 221 | +
 222 | +    // Set CanDirectFetch() to true by generating a block

mzumsande commented at 2:07 AM on November 12, 2022:

Why is this necessary, do we care if CanDirectFetch is true? The tests passes without this block (if we set nVersion on dummy below).

in src/test/denialofservice_tests.cpp:229 in fa9dded81d outdated

 225 | +        block->nTime = count_seconds(time_later);
 226 | +        SolvePow(*block);
 227 | +        m_node.chainman->ProcessNewBlock(block, true, true, nullptr);
 228 | +    }
 229 | +    // Last node pretends to send a block
 230 | +    std::vector<CBlock> headers;

mzumsande commented at 2:40 AM on November 12, 2022:

could be moved inside the following block

mzumsande commented at 3:22 AM on November 12, 2022: contributor

Concept ACK.

I wonder if it's possible (and worth it) to tweak the test such that it would have caught #26172. Sending the block via p2p like this test does also updates m_last_block_announcement, however I think we'd need to add a getter to check this directly. Or maybe it could work to have 5 nodes send new block headers via p2p instead of 1, such that only MAX_OUTBOUND_PEERS_TO_PROTECT_FROM_DISCONNECT=4 of them get protected, and the fifth also gets saved from disconnect via m_last_block_announcement.
LarryRuane commented at 3:27 AM on November 13, 2022: contributor

I wonder if it's possible (and worth it) to tweak the test such that it would have caught #26172.

I have a test for that but haven't made the PR yet (I'll do that this week). I'll compare what I have to this PR since there's likely some overlap.
maflcko force-pushed on Nov 14, 2022
maflcko force-pushed on Nov 14, 2022
DrahtBot added the label CI failed on Apr 25, 2023
achow101 requested review from jamesob on Apr 25, 2023
theuni commented at 3:31 PM on April 25, 2023: member

Ping @MarcoFalke, apparently :)
DrahtBot added the label Needs rebase on May 6, 2023
jonatack commented at 2:19 AM on July 28, 2023: member

I wonder if it's possible (and worth it) to tweak the test such that it would have caught #26172.

I have a test for that but haven't made the PR yet (I'll do that this week). I'll compare what I have to this PR since there's likely some overlap. @LarryRuane any update on the test?
maflcko force-pushed on Sep 20, 2023
maflcko force-pushed on Sep 20, 2023
DrahtBot removed the label Needs rebase on Sep 20, 2023
DrahtBot renamed this:
~~test: Extend stale_tip_peer_management test~~
test: Extend stale_tip_peer_management test
on Sep 20, 2023
DrahtBot added the label Needs rebase on Nov 7, 2023
maflcko force-pushed on Feb 6, 2024
DrahtBot removed the label Needs rebase on Feb 6, 2024
test: Extend stale_tip_peer_management test d6e170ede4
maflcko force-pushed on Feb 6, 2024
maflcko closed this on Feb 6, 2024
maflcko deleted the branch on Feb 6, 2024
bitcoin locked this on Feb 5, 2025