Heh, this one might be controversial. In theory it shouldn't be confusing if there is only exactly one p2p connection. If this pull gets Concept ACKs, that is fine. If not, it might be good to replace the assert self.p2ps with assert len(self.p2ps)==1.

@MarcoFalke indeed. You put this idea in my head 😂 but assert len == 1 is a good way too. I don't see any advantages to having this though.

Yes, I remember that. It looks like that was in reply to a performance bottleneck. Is that still observable?

@MarcoFalke I think the performance problems were addressed elsewhere (comment). We were talking about p2p_invalid_messages.py which has a lot of subtests that needed to wait for disconnect_p2ps before moving on.

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• test: Remove or explain syncwithvalidationinterfacequeue #19893 (test: Remove or explain syncwithvalidationinterfacequeue by MarcoFalke)
• test: check for all possible OP_CLTV fail reasons in feature_cltv.py (BIP 65) #19801 (test: check for all possible OP_CLTV fail reasons in feature_cltv.py (BIP 65) by theStack)
• test: Add more tests for orphan tx handling #19393 (test: Add more tests for orphan tx handling by hebasto)
• [tests] Allow outbound & block-relay-only connections in functional tests.  #19315 ([tests] Allow outbound & block-relay-only connections in functional tests. by amitiuttarwar)

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.

Concept ACK
I agree that the p2p property can be quite confusing, as it is a bit too much of "implicit magic" going on.

node.add_p2p_connection(P2PInterface())
node.p2p.do_something_fancy()

is less clear than the explicit

p2p_conn = node.add_p2p_connection(P2PInterface())
p2p_conn.do_something_fancy()

in terms of readability, in my opinion. Generally, if you have a function to create an object, the handle returned by that function should be the preferred way to access that object, and not some other implicit way. (.p2ps[0] suffers from the same problem, but at least it is more obvious that it is the first connection.)

That said, I also agree to MarcoFalke that this might be controversial... if it doesn't get enough Concept ACKs, I'm also happy to ACK the variant with assert len(self.p2ps)==1 in the p2p property.

thanks for Concept ACK @theStack!

Agreed that's more clear, I could also just apply the explicit p2p_conn = node.add_p2p_connection(...) to all the places where p2p is used (just couldn't be a scripted-diff).

I'll wait for more people to weigh in :)

Yes, agreed that this seems controversial :) there was even discussion of this in the original PR: #11182 (comment)

I don't have a strong preference for p2p vs p2ps[0]. I have a mild preference for not reversing an existing style decision without new information. Maybe you can see if the original author says "I regret my decision to introduce the p2p property; let's change it"? :)

But I'm definitely in favor of assert len(self.p2ps) == 1 to avoid incorrect/confusing use of the p2p property!

That is true, I should have asked - @jnewbery how do you feel about this?

@jnewbery how do you feel about this?

I can see how TestNode.p2p could introduce subtle bugs if tests get extended and the wrong connection is used by mistake, so we should either remove the property or add an assert when it gets used.

The approach in this PR (removing the property entirely) seems fine to me. Concept ACK.

Concept ACK

And thanks for the motivation to learn about scripted-diff :)

Are you also on MacOS? I had to install gsed and use that in place of sed in order to run commit-script-check.sh locally, but once I did this, I was able to reproduce the CI error.

A few thoughts:

• Do you need to append a final /g to the scripted diff to avoid the "unterminated `s command" CI error at https://travis-ci.org/github/bitcoin/bitcoin/jobs/721079379?
• Should the regex be \.p2p\b (end on a word boundary instead of a dot)? e.g. it looks like test/functional/feature_versionbits_warning.py uses node.p2p without a trailing dot
• Would it make sense to restrict the git grep to the test/functional directory?

Here's a verify script that takes all of the above into account:

sed -i 's/\.p2p\b/.p2ps[0]/g' $(git grep -l "\.p2p\b" test/functional)

• Should the regex be \.p2p\b (end on a word boundary instead of a dot)? e.g. it looks like test/functional/feature_versionbits_warning.py uses node.p2p without a trailing dot

@robot-dreams really good point about feature_versionbits_warning, thanks for catching that! I think the problem with using \.p2p\b is we'll also be replacing all of the test_framework.p2p imports. I'm going through and refactoring the tests to explicitly call the p2p objects returned from add_p2p_connection though, so the scripted diff will be pretty minimal.

Oops, nice catch! I forgot to actually test the script myself 😅

It's a moot point at this point since your proposed change sounds better, but I think this would've worked (don't do the replace on lines that start with from)

sed -i '/^from/!s/\.p2p\b/.p2ps[0]/g' $(git grep -l "\.p2p\b" test/functional)

One day I will get the scripted-diff right on the first try... 😂

I figured 3 Concept ACKs was enough for me to put some work into this. Last push replaces p2p property with the p2p object returned from node.add_p2p_connection() instead (except for two places where it'd be a nontrivial refactor). As stated previously in the description and by @theStack, this is the clearest way to do it. It makes reviewing the PR slightly more involved, but I think it's a bigger improvement as well :) @MarcoFalke please let me know what you think?

Did a little more refactoring to make feature_block.py and feature_csv_activation.py more clear, addressed @MarcoFalke's comments. Just left p2p_invalid_tx.py for the scripted-diff since it seems to want a variable number of connections in bootstrap_p2p, maybe future additions to the test could need it. Also added some stuff to the functional test README. Ready for another look :)

utACK 10d6150

Thanks for improving the documentation as well!

utACK 10d6150