test: Remove unused classes from p2p

dhruv commented at 5:53 AM on August 15, 2020: member

Removes CNodeNoVersionMisbehavior per recommendation at #19657 (comment)
Removes CNodeNoVersionIdle because it is similarly unnecessary
As someone new to the codebase, I found it easier to understand it if no_version_disconnect_node tries to overwhelm the peer with any message that is not version/verack.
Per recommendation at #19727#pullrequestreview-468093555, made a clear distinction between nodes(bitcoind) and peers(mininode interface implementations)

fanquake added the label Tests on Aug 15, 2020

adamjonas commented at 12:59 AM on August 16, 2020: member

@dhruv welcome! Looks like a great first contribution.

Ran tests and everything LGTM, but @jnewbery is probably the guy who you'd want to take a look at this since it was his suggestion.

jnewbery commented at 8:28 AM on August 16, 2020: member

Concept ACK. Thanks @dhruv . I'll review soon.

dhruv commented at 4:23 PM on August 16, 2020: member

@dhruv welcome! Looks like a great first contribution.

Thank you for the warm welcome and review, @adamjonas. I am excited to be here.

in test/functional/p2p_leak.py:125 in 43986d9a78 outdated

 119 | @@ -125,7 +120,7 @@ def run_test(self):
 120 |          wait_until(lambda: no_verack_idlenode.version_received, timeout=10, lock=mininode_lock)
 121 |  
 122 |          # Mine a block and make sure that it's not sent to the connected nodes
 123 | -        self.nodes[0].generatetoaddress(1, self.nodes[0].get_deterministic_priv_key().address)
 124 | +        self.nodes[0].generate(nblocks=1)
 125 |  
 126 |          #Give the node enough time to possibly leak out a message
 127 |          time.sleep(5)

glozow commented at 8:47 PM on August 16, 2020:

Unrelated to this PR, but is there a better way to sync here? wait_until RPC shows something that would happen after a block announcement?

dhruv commented at 12:11 AM on August 17, 2020:

Going to take a look at this and follow-up in a separate PR.

in test/functional/p2p_leak.py:112 in 43986d9a78 outdated

 113 | +        # Send enough ping messages (any non-version message will do) prior to sending
 114 | +        # version to reach the peer discouragement threshold. This should get us disconnected.
 115 |          for _ in range(DISCOURAGEMENT_THRESHOLD):
 116 | -            no_version_disconnect_node.send_message(msg_verack())
 117 | +            no_version_disconnect_node.send_message(msg_ping())
 118 |

glozow commented at 8:55 PM on August 16, 2020:

Should we wait until all the pings are received by the node? e.g. through

`getpeerinfo` -> peer['bytesrecv_per_msg'].get('ping', 0)

dhruv commented at 12:12 AM on August 17, 2020:

Great suggestion - I did my best but please let me know if there's a better way

dhruv commented at 5:29 AM on August 18, 2020:

@gzhao408 After I added this assertion, the test sporadically failed. You and I discussed it and thought it was perhaps the timeout. But increasing it didn't seem to solve the problem. @amitiuttarwar gave me hints here. It turns out that occasionally, the peer has been dropped prior to the test making getpeerinfo RPC. And at that point, the misbehaving peer isn't even in the peer list.

Since this particular peer does not do anything else (not even a version call) except overwhelm the node with pings, I think the assertion that makes sure the connection is dropped suffices. Would you agree?

in test/functional/p2p_leak.py:99 in 43986d9a78 outdated

  93 | @@ -106,17 +94,24 @@ def set_test_params(self):
  94 |          self.num_nodes = 1
  95 |  
  96 |      def run_test(self):
  97 | +        # Peer that never sends a version. We will send a bunch of messages
  98 | +        # from this node anyway and verify eventual disconnection.
  99 |          no_version_disconnect_node = self.nodes[0].add_p2p_connection(

glozow commented at 8:57 PM on August 16, 2020:

nit: I think we should take this opportunity to rename the mininodes to "peers" instead of nodes, because they aren't actually bitcoind nodes. (nit but I'm sure jnewbery would agree)

        no_version_disconnect_peer = self.nodes[0].add_p2p_connection(

dhruv commented at 12:12 AM on August 17, 2020:

Made this change across the test

in test/functional/p2p_leak.py:114 in 43986d9a78 outdated

 115 |          for _ in range(DISCOURAGEMENT_THRESHOLD):
 116 | -            no_version_disconnect_node.send_message(msg_verack())
 117 | +            no_version_disconnect_node.send_message(msg_ping())
 118 |  
 119 | -        # Wait until we got the verack in response to the version. Though, don't wait for the other node to receive the
 120 | +        # Wait until we got the verack in response to the version. Though, don't wait for self.nodes[0] to receive the

glozow commented at 8:58 PM on August 16, 2020:

        # Wait until we got the verack in response to the version. Though, don't wait for the node to receive the

in test/functional/p2p_leak.py:104 in 43986d9a78 outdated

 101 | -        no_version_idlenode = self.nodes[0].add_p2p_connection(CNodeNoVersionIdle(), send_version=False, wait_for_verack=False)
 102 | +            CLazyNode(), send_version=False, wait_for_verack=False)
 103 | +
 104 | +        # Another peer that never sends a version. Just sits idle and hopes to receive
 105 | +        # any message (it shouldn't!)
 106 | +        no_version_idlenode = self.nodes[0].add_p2p_connection(CLazyNode(), send_version=False, wait_for_verack=False)

glozow commented at 9:04 PM on August 16, 2020:

nit: same

        no_version_idle_peer = self.nodes[0].add_p2p_connection(CLazyNode(), send_version=False, wait_for_verack=False)

in test/functional/p2p_leak.py:107 in 43986d9a78 outdated

 104 | +        # Another peer that never sends a version. Just sits idle and hopes to receive
 105 | +        # any message (it shouldn't!)
 106 | +        no_version_idlenode = self.nodes[0].add_p2p_connection(CLazyNode(), send_version=False, wait_for_verack=False)
 107 | +
 108 | +        # Peer that sends a version but not a verack.
 109 |          no_verack_idlenode = self.nodes[0].add_p2p_connection(CNodeNoVerackIdle(), wait_for_verack=False)

glozow commented at 9:04 PM on August 16, 2020:

nit: same again

        no_verack_idle_peer = self.nodes[0].add_p2p_connection(CNodeNoVerackIdle(), wait_for_verack=False)

in test/functional/p2p_leak.py:102 in 43986d9a78 outdated

  99 |          no_version_disconnect_node = self.nodes[0].add_p2p_connection(
 100 | -            CNodeNoVersionMisbehavior(), send_version=False, wait_for_verack=False)
 101 | -        no_version_idlenode = self.nodes[0].add_p2p_connection(CNodeNoVersionIdle(), send_version=False, wait_for_verack=False)
 102 | +            CLazyNode(), send_version=False, wait_for_verack=False)
 103 | +
 104 | +        # Another peer that never sends a version. Just sits idle and hopes to receive

glozow commented at 9:12 PM on August 16, 2020:

I like the documentation you've added! nit: I think "hopes" kind of sounds like it's doing something actively

        # Another peer that never sends a version, nor any other messages. It shouldn't receive anything from the node.

glozow commented at 9:18 PM on August 16, 2020: member

Concept ACK! Welcome Dhruv :D

I was heavy-handed with the nits because I wanted to give as much feedback as possible (please forgive me). Mostly, I think you should have a clear distinction between nodes and mininodes in your naming and comments.

bitcoind nodes (i.e. TestNodes) = I would call them "nodes" Mininodes = not bitcoind nodes, they just implement the P2P interface. So I'd prefer to call them "peers"

Also left a few food-for-thought questions about synchronization.

dhruv referenced this in commit 6c5733c796 on Aug 17, 2020

dhruv force-pushed on Aug 17, 2020

promag commented at 12:28 AM on August 17, 2020: member

Peers = not bitcoind nodes, but "mininodes" that just implement the P2P interface

A node is a peer, a peer can be a node, a peer is not a beer 🍻

glozow commented at 1:23 AM on August 17, 2020: member

A node is a peer, a peer can be a node

Oops ya that looks wrong out of context. I meant we shouldn't call mininodes nodes, and "peer" is what I recommend as a better name. I edited my comment thank you @promag 🥂

in test/functional/p2p_leak.py:76 in 6c5733c796 outdated

  87 |          super().__init__()
  88 |  
  89 |      def on_verack(self, message): pass
  90 |      # When version is received, don't reply with a verack. Instead, see if the
  91 | -    # node will give us a message that it shouldn't. This is not an exhaustive
  92 | +    # peer will give us a message that it shouldn't. This is not an exhaustive

jnewbery commented at 8:21 AM on August 17, 2020:

This is actually waiting to see if the bitcoind node sends an unexpected messaged, so the word 'node' is correct here.

dhruv commented at 6:12 PM on August 17, 2020:

Nice catch. Thank you!

in test/functional/p2p_leak.py:32 in 6c5733c796 outdated

  25 | @@ -27,9 +26,10 @@
  26 |  )
  27 |  
  28 |  DISCOURAGEMENT_THRESHOLD = 100
  29 | +PING_MESSAGE_SIZE = 32
  30 |  
  31 |  
  32 | -class CLazyNode(P2PInterface):
  33 | +class CLazyPeer(P2PInterface):

jnewbery commented at 8:22 AM on August 17, 2020:

Since you're renaming these classes, you should drop the C prefix, which isn't the current code style.

dhruv commented at 6:12 PM on August 17, 2020:

Done

jnewbery commented at 8:25 AM on August 17, 2020: member

Looks good @dhruv . Thanks for your contribution! I've left a couple of comments inline.

You should also make a couple of small changes to the commit logs. The summary line should be in the indicative mood (ie "remove CNodeNoVersionMisbehaving" instead of "removing CNodeNoVersionMisbehaving"), and all lines should be wrapped at 80 chars (see https://chris.beams.io/posts/git-commit/ for some tips on writing commit logs).

dhruv referenced this in commit 4fb89912fe on Aug 17, 2020

dhruv referenced this in commit 90cfde8a0a on Aug 17, 2020

dhruv force-pushed on Aug 17, 2020

dhruv renamed this:
~~test: Removing unused classes from p2p_leak.py~~
test: Remove unused classes from p2p_leak.py
on Aug 17, 2020

dhruv commented at 6:23 PM on August 17, 2020: member

@jnewbery Comments addressed. I also increased a wait_until timeout in this commit to address this. Please let me know if there is a better way.

dhruv commented at 9:19 PM on August 17, 2020: member

The test is now failing on a different architecture but passing on the one it failed at the last time (before raising the timeout) which I think confirms that it's a Travis CI timeout issue. However, I feel uncertain that just continuing to raise the timeout to reduce test fragility is the path forward. I'm looking for guidance from more experienced contributors.

in test/functional/p2p_leak.py:117 in 23d6ccfaaf outdated

 122 | +        # Wait until we got the verack in response to the version. Though, don't wait for the node to receive the
 123 |          # verack, since we never sent one
 124 | -        no_verack_idlenode.wait_for_verack()
 125 | +        no_verack_idle_peer.wait_for_verack()
 126 | +
 127 | +        wait_until(lambda: no_version_disconnect_peer.ever_connected, timeout=10, lock=mininode_lock)

amitiuttarwar commented at 9:43 PM on August 17, 2020:

is there a reason this is a wait_until? won't this peer start with ever_connected as false & here we want to check that it is still false?

dhruv commented at 5:37 AM on August 18, 2020:

The peer does start with ever_connected set to False. Then, if I understand things correctly, when the tcp connection is established, mininode.py::P2PConnection::connection_made() will invoke p2p_leak.py::LazyPeer::on_open() which will set no_version_disconnect_peer.ever_connected to True. So I think this wait_until is waiting for the transport connection to be established prior to generating a block below. This way it can verify that the peers do not receive leaked information despite an established tcp connection, and not because of connection problems.

Am I missing something?

amitiuttarwar commented at 7:38 PM on August 18, 2020:

oh I'm sorry, I mixed up true and false 😅 yes this makes sense- we want to check that ever_connected has been switched to true, and allow some buffer time for that to occur.

in test/functional/p2p_leak.py:122 in 23d6ccfaaf outdated

 126 | +
 127 | +        wait_until(lambda: no_version_disconnect_peer.ever_connected, timeout=10, lock=mininode_lock)
 128 | +
 129 | +        # Wait until the node receives at least as many pings from no_version_disconnect_peer (peer 0)
 130 | +        # as we sent in the test
 131 | +        wait_until(lambda: self.nodes[0].getpeerinfo()[0]['bytesrecv_per_msg'].get('ping', 0) >=

amitiuttarwar commented at 9:48 PM on August 17, 2020:

what is the intent behind this assertion? wouldn't another way to check if this was successful to be checked they are no long connected? (which you do below). is the purpose of this wait_until to allow for sync to occur before checking they are disconnected, or something else?

dhruv commented at 5:40 AM on August 18, 2020:

Removed this assertion. Thank you for the hints. I learnt that occasionally the misbehaving peer is removed before this assertion even runs, and so the assertion fails. Since this peer does nothing else except send a 100 pings, i think it suffices to check they are no longer connected (we also check that they did connect at first).

amitiuttarwar commented at 10:36 PM on August 17, 2020: contributor

concept ACK! welcome! 🎈

I've figured out the root cause of the sporadic test failure. in order to suggest a fix I want to better understand the intention of the wait_until...bytesrecv_per_msg assertion.

I'm also hoping to help you figure out the root cause of the failure yourself. Two big hints from my debugging process:

Running it locally on repeat & seeing that even if I decrease the timeout to 0.1, I wasn't able to get (even a sporadic) failure.
Reading the failure logs on Travis, and seeing that the node received all the ping messages, disconnected the peer, and then later is failing the bytesrecv_per_msg condition. Why could that be?

Incase you haven't seen it, ./combine_logs is a very helpful tool for introspecting on what's going on during tests. You could try comparing the logs from the failure to the logs you see in your ordinary success cases.

other meta notes:

no need to refer to review comments in your commit messages. the context is best left for github convo & the commit messages focused on the code changes :) its fine for the message to just be 1 line when the commit seems self explanatory.
I think two of these commits (removing C prefix & distinguishing nodes from peers) can probably be done via a scripted diff.

dhruv force-pushed on Aug 18, 2020

dhruv commented at 5:44 AM on August 18, 2020: member

Thanks for the great review, @amitiuttarwar. I appreciate you taking the time to teach me with hints rather than giving out the answer. I've removed the assertion and made the changes to the commit messages. I had some trouble running test/lint/commit-script-check.sh locally so I am going to look more into that in the morning.

dhruv force-pushed on Aug 18, 2020

practicalswift commented at 7:36 AM on August 18, 2020: contributor

Concept ACK

Warm welcome as a contributor @dhruv! Hope to see more contributions from you in the future :)

in test/functional/p2p_leak.py:29 in 54ebddd547 outdated

  25 | @@ -27,9 +26,10 @@
  26 |  )
  27 |  
  28 |  DISCOURAGEMENT_THRESHOLD = 100
  29 | +PING_MESSAGE_SIZE = 32

jnewbery commented at 10:51 AM on August 18, 2020:

No longer used.

dhruv commented at 4:11 PM on August 18, 2020:

Thank you for the catch.

jnewbery commented at 10:53 AM on August 18, 2020: member

Looks good. One comment inline.

I think the last two commits can be squashed. No need to rename classes in one commit and then rename them again in a subsequent commit!

dhruv force-pushed on Aug 18, 2020

dhruv commented at 4:11 PM on August 18, 2020: member

@jnewbery commits squashed

in test/functional/p2p_leak.py:117 in 5187e84784 outdated

 124 | +        no_verack_idle_peer.wait_for_verack()
 125 |  
 126 | -        wait_until(lambda: no_version_disconnect_node.ever_connected, timeout=10, lock=mininode_lock)
 127 | -        wait_until(lambda: no_version_idlenode.ever_connected, timeout=10, lock=mininode_lock)
 128 | -        wait_until(lambda: no_verack_idlenode.version_received, timeout=10, lock=mininode_lock)
 129 | +        wait_until(lambda: no_version_disconnect_peer.ever_connected, timeout=10, lock=mininode_lock)

jnewbery commented at 7:13 PM on August 18, 2020:

oops. Something seems to have gone wrong with the rebase here. You've changed these lines in the first commit (test: remove CNodeNoVersionMisbehavior from p2p_leak.py _), but the names are only changed above in the third commit (_test: Distinguish between nodes(bitcoind) and peers(mininodes) in p2p_leak.py), which means that the test fails for the first two commits. We try to make all of our commits clean so that tools like git bisect work.

dhruv commented at 8:10 PM on August 18, 2020:

Thank you, @jnewbery. I learnt to:

Not rebase latest master every round of review
Make sure tests pass on every commit

I have pushed up fixed commits.

jnewbery commented at 7:15 PM on August 18, 2020: member

Final code looks good, but something's gone askew with the commits. See comment inline.

When you squash or fixup commits, there's no need to rebase on the lastest master. That makes things a bit more difficult for reviewers, because they can't easily compare what's changed.

test: remove `CNodeNoVersionMisbehavior` from p2p_leak.py

It's also clearer to have `no_version_disconnect_node` send a message
other than version or verack in order to reach the peer discouragement
threshold.

45cf55ccac

test: remove `CNodeNoVersionIdle` from p2p_leak.py f6f082b934

dhruv force-pushed on Aug 18, 2020

test: Distinguish between nodes(bitcoind) and peers(mininodes) in p2p_leak.py

Also, remove "C" prefix from class names to match new style

ed5cd12869

dhruv force-pushed on Aug 18, 2020

jnewbery commented at 8:46 AM on August 19, 2020: member

tested ACK ed5cd12869e0691a785199d2d977ce5879095180

amitiuttarwar commented at 11:02 PM on August 19, 2020: contributor

utACK ed5cd12869

fanquake merged this on Aug 20, 2020

fanquake closed this on Aug 20, 2020

sidhujag referenced this in commit 6a279b7519 on Aug 20, 2020

deadalnix referenced this in commit 5e48e99a4b on Sep 15, 2021

DrahtBot locked this on Feb 15, 2022

test: Remove unused classes from p2p_leak.py #19727