test: Extends wait_for_getheaders so a specific block hash can be checked

[sr-gi]

Fixes #18614

Previously, wait_for_getheaders would check whether a node had received any getheaders message. This implied that, if a test needed to check for a specific block hash within a headers message, it had to make sure that it was checking the desired message. This normally involved having to manually clear last_message. This method, apart from being too verbose, was error-prone, given an undesired getheaders would make tests pass.

This adds the ability to check for a specific block_hash within the last getheaders message.

[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	BrandonOdiwuor, cbergqvist, stratospher, achow101
Concept ACK	kevkevinpal
Stale ACK	brunoerg

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:

• #29748 (test: Makes wait_for_getdata delete data on checks, plus allows to check the getdata message type by sr-gi)
• #29575 (net_processing: make any misbehavior trigger immediate discouragement by sipa)

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.

[sr-gi]

I've added a default case for block_hash=None, for the cases in which we may care about a message being received (for flow control), but not really about the content of it. FullBlockTest::bootstrap_p2p in feature_block.py is a good example of this.

[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/23110398265}

[sr-gi]

Something that may be worth discussing is whether we want to assert that the desired block hash is within the hashes reported on the getheaders message (current approach), or if we want to make sure it matches the first one (i.e. the closest to the tip).

For the patched tests, I've made sure that it was the case it matched the closest to the tip, but its parent would also have made the test pass.

[sr-gi]

@maflcko @stratospher

[kevkevinpal]

Concept ACK 3520d28

just an observation, It seems like we only use block_hash=None in feature_block.py

Something that may be worth discussing is whether we want to assert that the desired block hash is within the hashes reported on the getheaders message (current approach), or if we want to make sure it matches the first one (i.e. the closest to the tip).

If we're looking for the block_hash as the tip value is there a chance we can miss the correct block_hash being the tip causing the test to fail?

[maflcko]

lgtm

Partially fixes #18614

I think this fully solves the remainder, or is something left to be done afterwards?

[sr-gi]

lgtm

Partially fixes #18614

I think this fully solves the remainder, or is something left to be done afterwards?

You're right, I thought there may be other methods that also needed patching (and didn't check that was not the case 😅 ).

I'm working on a cleanup PR for things that were left out after #18690, but that should be unrelated

[brunoerg]

Concept ACK

[sr-gi]

Concept ACK 3520d28

just an observation, It seems like we only use block_hash=None in feature_block.py

Yeah, that's done on purpose. The getheaders received in feature_block.py is part of the reconnection. The test is not really expecting that a getheaders with a specific block hash is received, but that the message is received as part of the connection flow. Trying to make it assert an specific block hash would make the test more verbose with no apparent benefits

Something that may be worth discussing is whether we want to assert that the desired block hash is within the hashes reported on the getheaders message (current approach), or if we want to make sure it matches the first one (i.e. the closest to the tip).

If we're looking for the block_hash as the tip value is there a chance we can miss the correct block_hash being the tip causing the test to fail?

That could imply that the test is wrong. The current approach is more permissive, but it means that a test could pass unexpectedly. e.g; Imagine our current chain looks like this:

Genesis - B1 - ... - BN - BN+1 - BN+2 (tip)

We wait to receive a getheaders containing BN, and we receive:

[BN+1, BN, ..., Genesis]

In this case, the test will pass because our expected hash is within the received hashes, but likely the test should have waited for BN+1.

It really depends on what we want to check, and the flexibility we want to give to the method though

[brunoerg]

crACK fb9eb7a

[kevkevinpal]

I just tried with return block_hash == last_getheaders.locator.vHave[0] and the tests seemed to have run fine for me. So I have no problem just checking the last block_hash

[sr-gi]

I just tried with return block_hash == last_getheaders.locator.vHave[0] and the tests seemed to have run fine for me. So I have no problem just checking the last block_hash

Yeah, for all patched tests I"ve made sure that was the case. The question was more if generally that would be the expected case

[stratospher]

yay, nice! just 1 question, will ACK after that.

[stratospher]

Something that may be worth discussing is whether we want to assert that the desired block hash is within the hashes reported on the getheaders message (current approach), or if we want to make sure it matches the first one (i.e. the closest to the tip).

For the patched tests, I've made sure that it was the case it matched the closest to the tip, but its parent would also have made the test pass.

i prefer the stricter option since i'd want tests written in the future to be aware of those unexpected passes. if we don't care about the exact content of header, there is the block_hash=None variant anyways.

[stratospher]

looks like this wait_until got forgotten. maybe update that to use wait_for_getheaders here too?

[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/23298959570}

[sr-gi]

Thanks for the thorough review @stratospher

looks like this wait_until got forgotten. maybe update that to use wait_for_getheaders here too?

I've done it in 80ec483. Given we are reusing the same test node throughout the whole file and the state of what block may be expected is not clean, I waited for the message to be seen, but without any expected block check (this should be safe now that we pop the content of the dict).

Also:

• I replaced get by pop so we don't need to manually pop to make sure we are not checking an old message
• I have strengthened the membership check so the expected block needs to match the tip of vHave instead of just being part of it

[sr-gi]

Rebased to include the changes from #29750 and cover #29736 (comment)

[sr-gi]

I wonder if it may be worth splitting the wait_for_getheaders function so the internal function can also be used externally, so we can use that to check for whether a certain message exists without waiting for them (e.g. checking for the negative case) in a similar fashion as done in bc844fd

This way we would get rid of a bunch of manual checks like:

with p2p_lock:
    assert "getheaders" not in peer.last_message

cc/ @maflcko @stratospher

[BrandonOdiwuor]

crACK c4f857c

[cbergqvist]

ACK c4f857c

Nice to see the TODO in p2p.py resolved!

Searched through source for "getheaders" to make sure the function had been applied when suitable.

Reasoned through the modified test functions.

Modifications look good taking into account the new behavior of wait_for_getheaders() popping off the message and requiring the block hash to match the top one if provided.

Built and ran functional tests with --extended --exclude=feature_dbcrash with no failures (after cherry-picking 49c0b8b from #29791 on top of this PR to bump timeouts in 2 tests not touched in this PR).

Nit: Commit message subject is 73 chars long

CONTRIBUTING.md links https://chris.beams.io/posts/git-commit/ which states "consider 72 the hard limit". It further suggests using an "Imperative mood", so you could just change "Extends" -> "Extend" and be done with it.

[stratospher]

tested ACK c4f857c. went through all getheaders messages sent in the tests and checked that it's the one we want.

[achow101]

ACK c4f857c

[sr-gi]

I wonder if it may be worth splitting the wait_for_getheaders function so the internal function can also be used externally, so we can use that to check for whether a certain message exists without waiting for them (e.g. checking for the negative case) in a similar fashion as done in bc844fd

This way we would get rid of a bunch of manual checks like:

with p2p_lock:
    assert "getheaders" not in peer.last_message

cc/ @maflcko @stratospher

This was never discussed btw, and I wonder if it is worth a followup