test: enable opening partial p2p connections where useful #18498

As a follow-up to PR #18247, which added sync_with_ping by default in add_p2p_connection(), this PR allows optionally passing an argument to add_p2p_connection to avoid running sync_with_ping for partial connections.

The second commit passes that argument in cases where add_p2p_connection is followed closely by a call to sync_with_ping or send_with_ping, or where the test integrity appears better served with the previous behavior of opening a partial connection.

As this PR maintains previous behavior WRT a few tests, there should be no downside here.

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #18726 (test: check misbehavior more independently in p2p_filter.py by robot-visions)

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.

Rebased.

Thanks for updating this! One question before I finish reviewing: am I correct in understanding that you looked through every use of add_p2p_connection and individually decided whether the special sync_with_ping=False behavior is needed?

Slightly tend to NACK. This is making the tests harder to read (a reader will ask themselves why some calls have the ping disabled explicitly and the call in the very next line does not have it disabled). Also, it is making the tests harder to write or change, because forgetting to update one of the disabled pings to be enabled, when needed, can easily be forgotten. There is no infrastructure in place to catch such mistakes (other than review).

I think we should make tests easy to read and easy to write, not the opposite.

Thanks for the feeback. Below I'll try to summarize the different aspects as I (correctly or not) see them.

Full connections by default instead of partial connections

add_p2p_connection was added in March 2017 with 5e5725cc2b5, which means that for the past 3 years the functional test suite using this method was running without forcing full p2p connections (which were made by calling sync_with_ping). If I understand correctly, the motivation for fa9064704524a0fd1fa9ea73e was to hopefully reduce spurious test failures in the CI. Nevertheless, ISTM we would ideally prefer to avoid sync_with_ping everywhere by default and could verify regularly if it can be reverted without increasing the number of false test negatives.

Allowing the previous default of partial connections

It seems we still need a way for tests to make partial connections. One reason is speed/performance: Waste not; gains may appear minor, but the functional suite will continue growing and its run time continue to increase, long-term discouraging people from running it and increasing our dependance on centralized third parties. A more immediately interesting reason is, are there tests that might now be false positives, because their partial connection was changed to a full one to reduce false negatives?

Downside of allowing partial connections

For 3 years, connections were partial by default and required an additional sync_with_ping to become full. Now, connections are full by default and would only require an additional argument be partial. The complexity is roughly equivalent with only the default state changed, but inevitably we need to be able to test various kinds of connections.

In the worst case, a test that was using a partial connection might be modified to not longer need it without the author and reviewers noticing (if the changed test required a full connection, it would fail and be noticed/updated). In that worse case, the connection would function the way the functional test suite did for the previous 3 years. Only if the new test fails would it become an issue needing to be addressed (and the fix is trivial).

Implementation of connection types

I like @robot-visions' suggestion combining the states into one argument. Yes, I looked for cases where a partial connection is a better test or more practical e.g. followed by a sync already. There are likely more of them.

Question: what changed since March 2017? An increasing number of spurious (maddening, to be sure) CI failures?

Yes, we have a lot of spurious ci failures. No one reports them as bugs or even tries to fix them (except me). Everyone blindly hits the restart button on travis and it is up to me to beat them and save the logs locally. We have bugs reported by travis that crash the whole node, when a miner submits a block: #18742

I don't think making a handful of tests faster by a few microseconds is going to help in any notiecable way. And with the risk of making the test suite more fragile and missing actual failures, I don't think the unspecified gain [1] is worth the risk.

[1] Performance improvements need benchmarks to be useful

Btw, calling it "partial connection" implies that this is a different kind of connection or something that the caller can exploit for a test in some way. This is not true. The verack is already in flight and it can't be erased from the wire or changed in any way. If the caller wanted to exploit that intermittent state for a test, the result would be racy. Races are the number one reason that our tests fail.

If a test needs the verack to be not send (and requires a "partial connection" or whatever the right term for that is), it can use CNodeNoVerackIdle.

Thanks for the feedback. I empathize with the burden of the CI failures you're dealing with and the need for robust tests and benchmarks. Robust > speed. Will think about this more.