test/refactor: reference p2p objects explicitly and remove confusing Test

glozow commented at 5:22 PM on August 25, 2020: member

The TestNode has a p2p property which is an alias for p2ps[0].

I think this should be removed because it can be confusing and misleading (to both the test writer and reviewer), especially if a TestNode has multiple p2ps connected (which is the case for many tests). Another example is when a test has multiple subtests that connect 1 p2p and use the p2p property to reference it. If the subtests don't completely clean up after themselves, the subtests may affect one another.

The best way to refer to a connected p2p is use the object returned by add_p2p_connection like this:

p2p_conn = node.add_p2p_connection(P2PInterface())

A good example is p2p_invalid_locator.py, which cleans up after itself (waits in both wait_for_disconnect and in disconnect_p2ps) but wouldn't need so much complexity if it just referenced the connections directly.

If there is only one connected, it's not really that tedious to just use node.p2ps[0] instead of node.p2p (and it can always be aliased inside the test itself).

MarcoFalke commented at 5:45 PM on August 25, 2020: member

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.

glozow commented at 5:51 PM on August 25, 2020: member

@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.

MarcoFalke commented at 6:16 PM on August 25, 2020: member

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

DrahtBot added the label Docs on Aug 25, 2020

DrahtBot added the label Tests on Aug 25, 2020

MarcoFalke removed the label Docs on Aug 25, 2020

glozow commented at 7:01 PM on August 25, 2020: member

@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.

DrahtBot commented at 1:12 AM on August 26, 2020: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#19893 (test: Remove or explain syncwithvalidationinterfacequeue by MarcoFalke)
#19801 (test: check for all possible OP_CLTV fail reasons in feature_cltv.py (BIP 65) by theStack)
#19393 (test: Add more tests for orphan tx handling by hebasto)
#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.

DrahtBot added the label Needs rebase on Aug 26, 2020

theStack commented at 10:39 AM on August 28, 2020: member

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.

glozow commented at 3:50 PM on August 28, 2020: member

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 :)

robot-dreams commented at 6:25 AM on September 1, 2020: contributor

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!

glozow commented at 11:45 AM on September 1, 2020: member

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

jnewbery commented at 1:35 PM on September 1, 2020: member

@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.

robot-dreams commented at 5:58 PM on September 1, 2020: contributor

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)

glozow commented at 1:39 AM on September 4, 2020: member

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.

glozow force-pushed on Sep 4, 2020

robot-dreams commented at 2:09 AM on September 4, 2020: contributor

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)

glozow renamed this:
~~test: remove confusing p2p property~~
test/refactor: reference p2p objects explicitly and remove confusing Test_Node.p2p property
on Sep 4, 2020

glozow force-pushed on Sep 4, 2020

glozow commented at 2:25 AM on September 4, 2020: member

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?

in test/functional/feature_block.py:1410 in 9223a40a78 outdated

1406 | @@ -1407,7 +1407,7 @@ def send_blocks(self, blocks, success=True, reject_reason=None, force_send=False
1407 |          """Sends blocks to test node. Syncs and verifies that tip has advanced to most recent block.
1408 |  
1409 |          Call with success = False if the tip shouldn't advance to the most recent block."""
1410 | -        self.nodes[0].p2p.send_blocks_and_test(blocks, self.nodes[0], success=success, reject_reason=reject_reason, force_send=force_send, timeout=timeout, expect_disconnect=reconnect)
1411 | +        self.nodes[0].p2ps[0].send_blocks_and_test(blocks, self.nodes[0], success=success, reject_reason=reject_reason, force_send=force_send, timeout=timeout, expect_disconnect=reconnect)

robot-dreams commented at 2:30 AM on September 4, 2020:

Nit: Would it make sense to incorporate this change into the scripted-diff?

glozow commented at 12:27 PM on September 4, 2020:

yes i should do that!

MarcoFalke commented at 8:41 AM on September 6, 2020:

        self.helper_peer.send_blocks_and_test(blocks, self.nodes[0], success=success, reject_reason=reject_reason, force_send=force_send, timeout=timeout, expect_disconnect=reconnect)

glozow commented at 2:03 PM on September 10, 2020:

🤦‍♀️ I should've just done that

in test/functional/example_test.py:210 in 9223a40a78 outdated

 208 | +        peer_receiving.wait_until(lambda: sorted(blocks) == sorted(list(peer_receiving.block_receive_map.keys())), timeout=5)
 209 |  
 210 |          self.log.info("Check that each block was received only once")
 211 |          # The network thread uses a global lock on data access to the P2PConnection objects when sending and receiving
 212 |          # messages. The test thread should acquire the global lock before accessing any P2PConnection data to avoid locking
 213 |          # and synchronization issues. Note p2p.wait_until() acquires this global lock internally when testing the predicate.

robot-dreams commented at 2:34 AM on September 4, 2020:

Nit: Should p2p.wait_until be replaced with peer_receiving.wait_until in this comment?

glozow commented at 12:27 PM on September 4, 2020:

they are the same, no?

robot-dreams commented at 5:21 PM on September 4, 2020:

Yes, my intent was "for clarity, should the variable name in the comment match the usage on line 205", but I could also see the case for p2p being clearer than peer_receiving.

I'm happy with it the way it is, especially if you think it's clearer as p2p!

glozow commented at 5:50 PM on September 4, 2020:

ohhhh i see what you mean! 🤷 I think since it's in example_test.py, yeah, p2p might be more helpful

DrahtBot removed the label Needs rebase on Sep 4, 2020

robot-dreams commented at 3:59 AM on September 4, 2020: contributor

ACK f1ccb4cc9c4457bed2cc12ff9cedb30d56a31985 aside from nits

Thanks for cleaning this up! Going through and refactoring as you did must've been a nice opportunity to learn about each test / choose reasonable variable names :)

Looked over change manually (note: I didn't think about the choice of variable names)
Did grep for remaining occurrences of p2p, made sure they were all as intended
Ran test/functional/test_runner.py --extended, looks good except for a local failure that didn't look related to your change at all (feature_pruning.py)
Ran commit-script-check.sh locally, looks good

in test/functional/feature_block.py:1389 in f1ccb4cc9c outdated

1385 | @@ -1386,14 +1386,14 @@ def bootstrap_p2p(self, timeout=10):
1386 |          """Add a P2P connection to the node.
1387 |  
1388 |          Helper to connect and wait for version handshake."""
1389 | -        self.nodes[0].add_p2p_connection(P2PDataStore())
1390 | +        helper_peer = self.nodes[0].add_p2p_connection(P2PDataStore())

MarcoFalke commented at 8:41 AM on September 6, 2020:

What about

        self.helper_peer = self.nodes[0].add_p2p_connection(P2PDataStore())

MarcoFalke commented at 8:43 AM on September 6, 2020: member

Seems odd to use a local reference and self.nodes[0].p2ps to access the same p2p connection

[refactor] clarify tests by referencing p2p objects directly

Use object returned from add_p2p_connection to refer to
p2ps. Add a test class attribute if it needs to be used across
many methods. Don't use the p2p property.

784f757994

[doc] sample code for test framework p2p objects 7a0de46aea

scripted-diff: replace p2p with p2ps[0] in p2p_invalid_tx

-BEGIN VERIFY SCRIPT-
sed -i 's/\.p2p\./.p2ps[0]./g' test/functional/p2p_invalid_tx.py
-END VERIFY SCRIPT-

549d30faf0

[test] remove confusing p2p property 10d61505fe

glozow force-pushed on Sep 10, 2020

glozow commented at 4:05 PM on September 10, 2020: member

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 :)

robot-dreams commented at 9:33 PM on September 10, 2020: contributor

utACK 10d61505fe77880d6989115defa5e08417f3de2d

Thanks for improving the documentation as well!

jnewbery commented at 8:41 AM on September 14, 2020: member

utACK 10d61505fe77880d6989115defa5e08417f3de2d

glozow requested review from MarcoFalke on Sep 15, 2020

in test/functional/wallet_resendwallettransactions.py:67 in 784f757994 outdated

  63 | @@ -64,7 +64,7 @@ def run_test(self):
  64 |          # Transaction should be rebroadcast approximately 24 hours in the future,
  65 |          # but can range from 12-36. So bump 36 hours to be sure.
  66 |          node.setmocktime(now + 36 * 60 * 60)
  67 | -        node.p2p.wait_for_broadcast([txid])
  68 | +        peer_second.wait_for_broadcast([txid])

guggero commented at 2:21 PM on September 17, 2020:

6 lines above there's a stray node.p2ps[1] that can be replaced with peer_second.

MarcoFalke commented at 12:22 PM on September 25, 2020:

@guggero Seems like a good suggestion. Mind following up with a patch?

guggero commented at 2:32 PM on September 17, 2020: contributor

Concept ACK 10d61505.

I think the preferred way should indeed be to access the peers through the return value of add_p2p_connection whenever possible.

Perhaps we could make this PR less controversial by replacing all the index-based accesses where possible? The two easiest examples would be:

MarcoFalke merged this on Sep 25, 2020

MarcoFalke closed this on Sep 25, 2020

sidhujag referenced this in commit d6866b9bac on Sep 25, 2020

MarcoFalke referenced this in commit 8aa3a4a498 on Sep 26, 2020

Fabcien referenced this in commit 53018ea73e on Oct 14, 2021

Fabcien referenced this in commit a57f68f3fa on Oct 14, 2021

Fabcien referenced this in commit 3e5bd59365 on Oct 14, 2021

Fabcien referenced this in commit c4c8141ed8 on Oct 14, 2021

Fabcien referenced this in commit 1a94984789 on Oct 14, 2021

DrahtBot locked this on Feb 15, 2022

test/refactor: reference p2p objects explicitly and remove confusing Test_Node.p2p property #19804

Conflicts