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.

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.

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.

Rebased to fix CI

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.

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)

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

🚧 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}

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.

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.

Updated to address #29122 (comment)

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.

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.

ACK d53d848