test: adds outbound eviction functional tests, updates comment in ConsiderEviction

[sr-gi]

Motivation

While checking the outbound eviction code I realized a case was not considered within the comments, which in turn made me realize we had no functional tests for the outbound eviction case (when I went to check/add the test case).

This PR updates the aforementioned comment and adds functional tests to cover the outbound eviction logic, in addition to the existing unit tests found at src/test/denialofservice_tests.cpp.

[DrahtBot]

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
ACK	davidgumberg, cbergqvist, tdb3, achow101
Concept ACK	sipa, stratospher, marcofleon

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

Conflicts

No conflicts as of last run.

[sr-gi]

While the tests seem to run fine, I noticed a warning that appears every now and then:

TestFramework.p2p (WARNING): Connection lost to 0:0 due to [Errno 54] Connection reset by peer

I'm not sure if something unintended is happening under the hood. Disconnections are expected, but it's our node who is supposed to trigger them, so maybe I'm missing something.

[sr-gi]

Rebased to fix CI

[sr-gi]

Rebased. This has failed CI once when waiting for the disconnection after:

cur_mock_time += (HEADERS_RESPONSE_TIME + 1)

I've tried to reproduce it but I cannot in my local setup, the only thing that comes to mind is that mock time may be too close to the timeout (only one second ahead), so I gave it a delta of 10 seconds instead.

PS: For context, you can check the failure here: https://cirrus-ci.com/task/4624744360706048?logs=ci#L6520. See how the peer is given the final notice by sending a getheaders message, but the check is performed before the peer gets disconnected.

[tdb3]

Excellent work finding a gap and filling it by increasing functional test robustness.
ACK for edc14bc9da332cae39f8803db559b532b3c74e16

Checked out the PR branch, built, and ran all functional tests (all passed).

The review was focused on verifying edge case coverage of the tests and that tests will accurately catch both pass and fail conditions.

Did not observe previously reported warning message:

TestFramework.p2p (WARNING): Connection lost to 0:0 due to [Errno 54] Connection reset by peer

[sr-gi]

Thanks for the review @tdb3

[sr-gi]

I redefined how peers are split in p2p_eviction:test_outbound_eviction_protected_mixed to address #29122 (comment). Both commits can be squashed (leaving them as they are for now in case someone does not agree with the approach)

[sr-gi]

Rebased to deal with merge conflicts

[davidgumberg]

reACK 59affd0

[tdb3]

re ACK for 59affd0.
Built and re-ran functional tests locally. All passed.

[cbergqvist]

re ACK 59affd0

Built & ran test/functional/test_runner.py --cachedir=/mnt/tmp/cache --tmpdir=/mnt/tmp successfully.

[marcofleon]

re-ACK 59affd0. Ran all functional tests, no issues.

[sr-gi]

Rebased to fix CI due to #29610

[davidgumberg]

reACK cd5b7a1

$ git range-diff a945f09...59affd0 015ac13...cd5b7a1                                              
1:  a12a4dfa90 = 1:  8fa272dccc test: adds outbound eviction functional tests, updates comment in ConsiderEviction     
2:  59affd0bf9 = 2:  cd5b7a11ee test: adds outbound eviction tests for non outbound-full-relay peers 

$ python ./test/functional/test_runner.py
ALL                                                    | ✓ Passed  | 1350 s (accumulated) 

[cbergqvist]

reACK cd5b7a1

(Diff didn't change). Built & ran functional tests.

[sipa]

Concept ACK

[tdb3]

re ACK for cd5b7a1.
Built and re-ran functional tests locally. All passed.

[marcofleon]

re-ACK cd5b7a1. All functional tests passed.

[stratospher]

cool test! Concept ACK. left some suggestions, nits and 1 problem.

something I found interesting was when generateblock is called - inv, getdata and block messages are exchanged between node and peer. so in the tests (ex: test_outbound_eviction_unprotected), the peer is sending the header of an older block it already knows about! I don't think it affects the behaviour in the tests. but would like to make sure that this is expected.

[sr-gi]

Rebased + addressed comments from @stratospher. I had to slightly reshape test_outbound_eviction_unprotected to fit #29122 (comment) given some time mocks + waits were unnecessary

[tdb3]

reACK for 368389d973a85d21aa9ebf030ef5e7f7ac5343de.
Pulled, built, ran all unit/functional tests (all passed).

[sr-gi]

Rebased on top of #29736. Now this is using wait_for_getheaders to reduce the boilerplate of having to manually pop the results from last_message.

cc/ @stratospher

[DrahtBot]

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

_{Debug: https://github.com/bitcoin/bitcoin/runs/24266342152}

[davidgumberg]

ACK be2c551
Re-reviewed code, and attempted to break tests by making ConsiderEviction misbehave.

It is worth noting that these tests will not catch if outbound eviction logic gets triggered too early. e.g.:

/** How long to wait for a peer to respond to a getheaders request */
static constexpr auto HEADERS_RESPONSE_TIME{0s};
// [...]
/** Timeout for (unprotected) outbound peers to sync to our chainwork */
static constexpr auto CHAIN_SYNC_TIMEOUT{0s};

will pass. That seems fine to me, maybe something for a follow up.

[sr-gi]

ACK be2c551 Re-reviewed code, and attempted to break tests by making ConsiderEviction misbehave.

It is worth noting that these tests will not catch if outbound eviction logic gets triggered too early. e.g.:

/** How long to wait for a peer to respond to a getheaders request */
static constexpr auto HEADERS_RESPONSE_TIME{0s};
// [...]
/** Timeout for (unprotected) outbound peers to sync to our chainwork */
static constexpr auto CHAIN_SYNC_TIMEOUT{0s};

will pass. That seems fine to me, maybe something for a follow-up.

I don't think I follow you here. HEADERS_RESPONSE_TIME and CHAIN_SYNC_TIMEOUT are constants hardcoded in net_processing.cpp (and mapped here). I would expect unit test to fail if those are updated.

[sr-gi]

Updated to address #29122 (comment)

[davidgumberg]

reACK d53d848

I don't think I follow you here. HEADERS_RESPONSE_TIME and CHAIN_SYNC_TIMEOUT are constants hardcoded in net_processing.cpp (and mapped here). I would expect unit test to fail if those are updated.

I didn't mean to say that these tests should be responsible for enforcing the particular values of HEADERS_RESPONSE_TIME and CHAIN_SYNC_TIMEOUT, just that they would not catch a regression where outbound eviction logic gets triggered earlier than expected on peers, which can be demonstrated by setting both values to zero.

Anyways, I think if what I'm saying isn't confused in some way, it is more pedantic than useful.

[cbergqvist]

ACK d53d848

Functional including --extended tests passed (skipped unrelated feature_dbcrash).

[tdb3]

Re ACK for d53d848
Pulled, built, ran all unit/functional tests (all passed).
Performed code review, but didn't see anything noteworthy over previous review.

[marcofleon]

Re ACK. Reviewed the code and the tests look good to me. I ran p2p_outbound_eviction.py individually and along with all the other functional tests and everything looks good.

[achow101]

ACK d53d848