test: Connection eviction logic tests

mzumsande commented at 1:52 pm on August 29, 2019: contributor

This adds a functional test for the eviction logic for inbound peers, which is triggered when the number of maximum connections is exceeded.

The functional test covers eviction protection for peers that have sent us blocks or txns recently, or that have faster pings. I couldn’t find a way to test the logic of CConnman::AttemptToEvictConnection that is based on netgroup (see #14210 for related discussion)

Fixes #16660 (at least partially).

[Edit: Earlier, this PR also contained a unit test, which was removed after the discussion]

mzumsande renamed this:
~~test: Connection eviction ligc tests~~
test: Connection eviction logic tests
on Aug 29, 2019

mzumsande force-pushed on Aug 29, 2019

DrahtBot added the label P2P on Aug 29, 2019

DrahtBot added the label Tests on Aug 29, 2019

mzumsande force-pushed on Aug 29, 2019

DrahtBot commented at 4:24 pm on August 29, 2019: contributor

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#17335 (Add test for syncing blocks generated after invalidateblock. by TheBlueMatt)

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.

mzumsande force-pushed on Aug 30, 2019

instagibbs commented at 5:38 pm on August 30, 2019: member

TSan build timing out with last output being:

0Remaining jobs: [feature_block.py, p2p_permissions.py]

mzumsande commented at 6:14 pm on August 30, 2019: contributor

Hmm, do you think this is caused by my changes? I didn’t change the tests that timed out and the changes to net were meant to only expose the eviction helper functions for testing (that shouldn’t be called in these tests because the code had no previous test coverage).

jamesob commented at 6:18 pm on August 30, 2019: member

Concept ACK, will review soon

instagibbs commented at 6:22 pm on August 30, 2019: member

@mzumsande I’ll let someone else weigh in on that, I just didn’t want to blow away that result in case it was interesting. Restarted that build.

mzumsande commented at 7:05 pm on August 30, 2019: contributor

@instagibbs Thanks! It happened again, so it does seem to be caused by the changes here. Will look into it and try to reproduce.

practicalswift commented at 7:12 pm on August 30, 2019: contributor

Concept ACK

Thanks for working on this!

mzumsande commented at 9:01 pm on August 30, 2019: contributor

Found a few recent travis runs from merges where the TSan build timed out in the same way as here: https://travis-ci.org/bitcoin/bitcoin/builds/578612449 https://travis-ci.org/bitcoin/bitcoin/builds/578444718 https://travis-ci.org/bitcoin/bitcoin/builds/577975853

naumenkogs commented at 4:22 pm on September 3, 2019: member

Tested ACK 42a01bf1c15eeb625abdc8bbc0ef9a7852a2b1af

mzumsande commented at 7:09 pm on September 3, 2019: contributor

Thanks for the review! By the way, the functional test will need a small update once #15759 is merged because of the 2 extra outbound connections: self.extra_args = [['-maxconnections=32']] instead of 30

mzumsande force-pushed on Sep 7, 2019

mzumsande commented at 9:54 pm on September 7, 2019: contributor

Rebased and adjusted maxconnections after the merge of #15759 as explained above.

mzumsande commented at 11:09 pm on September 7, 2019: contributor

The AppVeyor build failed - there seem to be intermittent failures with the new functional test. I can’t reproduce so far, but will look into it.

mzumsande force-pushed on Sep 17, 2019

mzumsande commented at 1:08 am on September 17, 2019: contributor

I think that my assumption that the artificially slowed down peers never have the lowest minimum ping can be wrong, leading to the intermittent failure. I changed the functional test to use getpeerinfo RPC to determine what the 8 nodes with the lowest minimum ping actually are. This should hopefully fix this issue.

jonatack commented at 10:00 am on September 18, 2019: contributor

Concept ACK. Nice. Could you mention/describe the proposed changes in net.h/net.cpp in the PR description, and possibly move the combine log/appveyor changes (if they are still needed) to a separate PR.

mzumsande force-pushed on Sep 18, 2019

mzumsande commented at 5:03 pm on September 18, 2019: contributor

I added those commits temporarily, because the AppVeyor build was failing and I couldn’t reproduce this locally and wanted look at the debug.log, which AppVeyor currently doesnt’ show due to #16894. I have removed the commits and fixed my mistake (not giving enough time for ping after adding a new peer). Will update the PR text as suggested.

jonatack commented at 6:20 pm on September 18, 2019: contributor

Thanks, I figured the timing of my review wasn’t ideal. Looking forward to reviewing this.

in test/functional/p2p_eviction.py:18 in 92a73f8af9 outdated

13+
14+
15+""" Test node eviction logic
16+
17+When the number of peers has reached the limit of maximum connections,
18+the next connecting inbound peer will trigger the eviction mechanis.

practicalswift commented at 3:09 pm on September 29, 2019:

Should be “mechanism” :)

in src/test/net_tests.cpp:348 in 92a73f8af9 outdated

343+    BOOST_CHECK_EQUAL(vTestCandidates[0].id, 4);
344+
345+    // CompareNodeTXTime: erase node with largest nLastTXTime (c5)
346+    // in case of a tie, erase node that has fRelayTxes set (c4)
347+    // then erase node with fBloomFilter unset (c2)
348+    // remaining items are orderes by smallest time connected

practicalswift commented at 3:09 pm on September 29, 2019:

Should be “ordered” :)

mzumsande commented at 4:56 pm on September 30, 2019:

Thanks, fixed both typos.

mzumsande force-pushed on Sep 30, 2019

laanwj commented at 5:24 am on October 3, 2019: member

Concept ACK, but I dislike moving internal functionality to net.h just for testing.

mzumsande commented at 5:10 pm on October 3, 2019: contributor

I don’t really see a practical way to avoid that. I haven’t seen any discussions about this in this project, but did some research and the overall consensus on the internet seems to be that internal functionality should not be subject to unit tests (e.g. private members should not be made public just for that).

So would you or others advise that I should just drop the commit with the unit test?

maflcko commented at 1:09 pm on October 17, 2019: member

So would you or others advise that I should just drop the commit with the unit test? @sdaftuar Any conceptual thoughts on this or this pull in general?

sdaftuar commented at 4:49 pm on October 22, 2019: member

I’m a concept ACK on the p2p test – that seems great! However I don’t really have an opinion either way on the unit test.

maflcko commented at 5:22 pm on October 22, 2019: member

@mzumsande Can you split up the functional test, please?

mzumsande force-pushed on Oct 23, 2019

mzumsande commented at 12:43 pm on October 23, 2019: contributor

Ok, I removed the unit test from this PR and edited the PR description accordingly.

in test/functional/p2p_eviction.py:66 in a0780c5ff6 outdated

61+            tip = int(best_block, 16)
62+            best_block_time = node.getblock(best_block)['time']
63+            block = create_block(tip, create_coinbase(node.getblockcount() + 1), best_block_time + 1)
64+            block.solve()
65+            blockpeer.send_blocks_and_test([block], node, success=True)
66+            protected.add(len(node.p2ps))

naumenkogs commented at 10:51 pm on May 18, 2020:

Although I understand how it works, this construction takes a little bit of cognitive effort.

Fine for this one, but I suggest using something explicit for the next PRs. Simple i would work here, and below, well, you’d have to introduce i + N(4) + M(5) or something, but I think it would be easier to comprehend.

mzumsande commented at 10:24 pm on May 28, 2020:

I introduced a variable current_peer, hopefully that makes it easier to read.

naumenkogs commented at 0:40 am on May 19, 2020: member

tested ACK a0780c5ff6818a0a59afd7657bb98c4954ab06b3

maflcko commented at 3:55 pm on May 19, 2020: member

Open-Close to re-run ci. See #15847 (comment)

maflcko closed this on May 19, 2020

maflcko reopened this on May 19, 2020

in test/functional/p2p_eviction.py:27 in a0780c5ff6 outdated

26+3.) Protect 8 nodes with the smallest minimum ping
27+"""
28+
29+class SlowP2PDataStore(P2PDataStore):
30+    def on_ping(self, message):
31+        time.sleep(0.1)

glozow commented at 1:23 am on May 20, 2020:

After #18638 can this use setmocktime instead of sleep?

naumenkogs commented at 1:33 am on May 20, 2020:

I don’t think so. Logically this sleep should happen on the mininode side (of couple particular mininodes, not all). Using mock time would sleep for all mininodes, but that doesn’t satisfy the goal of the test.

Unless the mininodes in the test are created in a very particular order, which would probably complicate things.

glozow commented at 1:37 am on May 20, 2020:

Ah, thanks for the info!

jonatack commented at 1:29 pm on May 20, 2020: contributor

Concept ACK. Tested on the branch and rebased on current master.

Here are some suggestions to improve the test output info/debug logging, move the network setup to a separate function, use self.setup_clean_chain = False if you don’t need a new chain, import order, clearer naming (protected_peers and evicted_peers), improve/clarify the assertions, etc. Feel free to use/adapt/ignore: https://github.com/jonatack/bitcoin/commit/c9e96a1

  0diff --git a/test/functional/p2p_eviction.py b/test/functional/p2p_eviction.py
  1index b82d644ad7..9bd9872672 100755
  2--- a/test/functional/p2p_eviction.py
  3+++ b/test/functional/p2p_eviction.py
  4@@ -2,16 +2,6 @@
  5 # Copyright (c) 2019 The Bitcoin Core developers
  6 # Distributed under the MIT software license, see the accompanying
  7 # file COPYING or http://www.opensource.org/licenses/mit-license.php.
  8-
  9-from test_framework.test_framework import BitcoinTestFramework
 10-from test_framework.mininode import P2PInterface, P2PDataStore
 11-from test_framework.util import assert_equal, wait_until
 12-from test_framework.blocktools import create_block, create_coinbase
 13-from test_framework.messages import CTransaction, FromHex, msg_pong, msg_tx
 14-
 15-import time
 16-
 17-
 18 """ Test node eviction logic
 19 
 20 When the number of peers has reached the limit of maximum connections,
 21@@ -26,6 +16,14 @@ Therefore, this test is limited to the remaining protection criteria:
 22 3.) Protect 8 nodes with the smallest minimum ping
 23 """
 24 
 25+import time
 26+
 27+from test_framework.test_framework import BitcoinTestFramework
 28+from test_framework.mininode import P2PInterface, P2PDataStore
 29+from test_framework.util import assert_equal, wait_until
 30+from test_framework.blocktools import create_block, create_coinbase
 31+from test_framework.messages import CTransaction, FromHex, msg_pong, msg_tx
 32+
 33 class SlowP2PDataStore(P2PDataStore):
 34     def on_ping(self, message):
 35         time.sleep(0.1)
 36@@ -36,53 +34,47 @@ class SlowP2PInterface(P2PInterface):
 37         time.sleep(0.1)
 38         self.send_message(msg_pong(message.nonce))
 39 
 40-
 41 class P2PEvict(BitcoinTestFramework):
 42     def set_test_params(self):
 43-        self.setup_clean_chain = True
 44+        self.setup_clean_chain = False
 45         self.num_nodes = 1
 46-        # The choice of maxconnections results in a maximum of 21 inbound connections
 47-        # (32 - 10 outbound - 1 feeler). 20 inbounds peers are protected from eviction.
 48+        # The choice of 32 maxconnections results in a maximum of 21 inbound connections,
 49+        # e.g. 32 conns - 10 outbound - 1 feeler) = 20 inbound peers protected from eviction.
 50         self.extra_args = [['-maxconnections=32']]
 51 
 52     def setup_network(self):
 53         self.setup_nodes()
 54 
 55-    def run_test(self):
 56-        protected = set()
 57+    def setup_test_peers(self):
 58+        self.protected_peers = set()
 59         node = self.nodes[0]
 60         node.generatetoaddress(101, node.get_deterministic_priv_key().address)
 61 
 62-        #The first 4 peers send us a block, protecting them from eviction
 63-        for i in range(4):
 64-            blockpeer = node.add_p2p_connection(SlowP2PDataStore())
 65-            blockpeer.sync_with_ping(timeout=5)
 66+        # The first 4 peers send us a block, protecting them from eviction
 67+        for _ in range(4):
 68+            block_peer = node.add_p2p_connection(SlowP2PDataStore())
 69+            block_peer.sync_with_ping(timeout=5)
 70             best_block = node.getbestblockhash()
 71             tip = int(best_block, 16)
 72             best_block_time = node.getblock(best_block)['time']
 73             block = create_block(tip, create_coinbase(node.getblockcount() + 1), best_block_time + 1)
 74             block.solve()
 75-            blockpeer.send_blocks_and_test([block], node, success=True)
 76-            protected.add(len(node.p2ps))
 77+            block_peer.send_blocks_and_test([block], node, success=True)
 78+            self.protected_peers.add(len(node.p2ps))
 79 
 80-        #The next 5 nodes are slow-pinging peers, making them eviction candidates
 81-        for i in range(5):
 82+        # The next 5 nodes are slow-pinging peers, making them eviction candidates
 83+        for _ in range(5):
 84             node.add_p2p_connection(SlowP2PInterface())
 85 
 86-        #The next 4 peers send us a tx, protecting them from eviction
 87+        # The next 4 peers send us a tx, protecting them from eviction
 88         for i in range(4):
 89             txpeer= node.add_p2p_connection(SlowP2PInterface())
 90             txpeer.sync_with_ping(timeout=5)
 91 
 92             prevtx = node.getblock(node.getblockhash(i+1), 2)['tx'][0]
 93             rawtx = node.createrawtransaction(
 94-                inputs=[{
 95-                    'txid': prevtx['txid'],
 96-                    'vout': 0
 97-                }],
 98-                outputs=[{
 99-                    node.get_deterministic_priv_key().address: 50 - 0.00125
100-                }],
101+                inputs=[{'txid': prevtx['txid'], 'vout': 0}],
102+                outputs=[{node.get_deterministic_priv_key().address: 50 - 0.00125}],
103             )
104             sigtx = node.signrawtransactionwithkey(
105                 hexstring=rawtx,
106@@ -94,18 +86,18 @@ class P2PEvict(BitcoinTestFramework):
107                 }],
108             )['hex']
109             txpeer.send_message(msg_tx(FromHex(CTransaction(), sigtx)))
110-            protected.add(len(node.p2ps))
111+            self.protected_peers.add(len(node.p2ps))
112 
113         # The next 8 peers have faster pings, which will usually protect them from eviction
114-        for i in range(8):
115+        for _ in range(8):
116             fastpeer = node.add_p2p_connection(P2PInterface())
117             wait_until(lambda: "ping" in fastpeer.last_message, timeout=10)
118 
119         # Make sure by asking the node what the actual min pings are
120         peerinfo = node.getpeerinfo()
121-        pings = [[0 for i in range(2)] for j in range(len(node.p2ps))]
122+        pings = [[0 for __ in range(2)] for _ in range(len(node.p2ps))]
123 
124-        #After adding 21 peers, the next one hits the maxconnection limit and triggers the eviction mechanism
125+        # After adding 21 peers, the next one hits the maxconnection limit and triggers the eviction mechanism
126         node.add_p2p_connection(SlowP2PInterface())
127 
128         # Usually the 8 fast peers are protected. In rare case of unreliable pings,
129@@ -115,21 +107,29 @@ class P2PEvict(BitcoinTestFramework):
130             pings[i][1] = peerinfo[i]['minping'] if 'minping' in peerinfo[i] else 1000000
131 
132         sorted_pings = sorted(pings, key = lambda x:x[1])
133-
134         for i in range(8):
135-            protected.add(sorted_pings[i][0])
136+            self.protected_peers.add(sorted_pings[i][0])
137 
138         # One of the non-protected peers must be evicted. We can't be sure which one because
139         # 4 peers are protected via netgroup, which is identical for all peers,
140-        # and the eviction mechanism doesn't preserve the order of identical elements
141-        nodes_disconn = []
142+        # and the eviction mechanism doesn't preserve the order of identical elements.
143+        self.evicted_peers = []
144         for i in range(len(node.p2ps)):
145             if(node.p2ps[i].is_connected == False ) :
146-                nodes_disconn.append(i)
147-        assert_equal(len(nodes_disconn), 1)
148-        self.log.info("Protected peers:{}".format(protected))
149-        self.log.info("Evicted peer {}".format(nodes_disconn[0]))
150-        assert_equal(nodes_disconn[0] in protected, False)
151+                self.evicted_peers.append(i)
152+
153+    def run_test(self):
154+        self.log.info("Set up test peer network")
155+        self.setup_test_peers()
156+
157+        self.log.info("Test that only one node was evicted")
158+        self.log.debug("{} evicted peers: {}".format(len(self.evicted_peers), set(self.evicted_peers)))
159+        assert_equal(len(self.evicted_peers), 1)
160+
161+        self.log.info("Test that the evicted node is no longer in the set of protected nodes")
162+        self.log.debug("{} protected peers: {}".format(len(self.protected_peers), self.protected_peers))
163+        assert self.evicted_peers[0] not in self.protected_peers

mzumsande commented at 8:06 pm on May 20, 2020: contributor

Thanks for the reviews and the suggestions! @jonatack I’ll take a close look/push an update early next week!

jonatack commented at 6:45 am on May 21, 2020: contributor

No worries, feel free to ignore :)

I wonder if protected_peers is a good name, perhaps just connected_peers.

naumenkogs commented at 6:54 am on May 22, 2020: member

I wonder if protected_peers is a good name, perhaps just connected_peers.

I would say all peers by default are connected, so this is not very helpful. Protected is a subset of connected. In addition, protected terminology was used for a long time, so I’d leave it as is.

jonatack commented at 7:15 am on May 22, 2020: contributor

protected terminology was used for a long time

Thanks @naumenkogs, TIL!

mzumsande force-pushed on May 28, 2020

mzumsande commented at 10:44 pm on May 28, 2020: contributor

@jonatack Thanks again - I took several of your suggestions (some slightly adapted) and pushed an update (just preferred not to split the asserts/logging from the rest)

Yes, protected_peers are the subset of peers that are expected not to be evicted by our node (because they sent us a block, tx, have fast pings) - otherwise the test would fail.

naumenkogs commented at 1:46 pm on May 29, 2020: member

Tested ACK 3758af7

jnewbery added the label Review club on May 29, 2020

jnewbery commented at 2:00 pm on May 29, 2020: contributor

We’ll cover this in review club on 2020-06-03: https://bitcoincore.reviews/16756.html

in test/functional/p2p_eviction.py:103 in 3758af7856 outdated

102+            current_peer += 1
103+            wait_until(lambda: "ping" in fastpeer.last_message, timeout=10)
104+
105+        # Make sure by asking the node what the actual min pings are
106+        peerinfo = node.getpeerinfo()
107+        pings = [[0 for i in range(2)] for j in range(len(node.p2ps))]

rajarshimaitra commented at 9:36 am on June 2, 2020:

Is it possible to combine pings initialization with the actual filling of the list with minping data (happening few lines later) together? Any specific reason why they are separated by the add_p2p_connection?

mzumsande commented at 10:13 pm on June 2, 2020:

This is probably not material, but the reason is that I wanted to trigger the eviction mechanism immediately after the getpeerinfo call, so that the node does not get time to exchange additional pings with its peers while the test does other things.

jnewbery commented at 11:43 pm on June 3, 2020:

I wondered the same thing. It’s confusing that you do some of the processing before adding the new peer (pings = [[...) and some after (for i in range(...). I think you should do all the processing before adding the new connection, since it’ll take very little time, and the chance of there being another ping in between is ~zero.

If you do keep this ordering, I think it’s worth a comment to say why you’re doing it.

mzumsande commented at 8:17 pm on June 6, 2020:

I changed the ordering since the probability of furthers pings is extremely low.

in test/functional/p2p_eviction.py:117 in 3758af7856 outdated

116+            pings[i][1] = peerinfo[i]['minping'] if 'minping' in peerinfo[i] else 1000000
117+
118+        sorted_pings = sorted(pings, key = lambda x:x[1])
119+
120+        for i in range(8):
121+            self.protected_peers.add(sorted_pings[i][0])

rajarshimaitra commented at 10:08 am on June 2, 2020:

At this stage, the total number of protected_peers is coming out to be 15, which actually should be 16 (4 blocks, 4 txs, 8 fast ping)? This is probably happening because the previous protected_peers.add(current_peer) added the peers with index + 1, (current_peer is 1 for peer 0), and protected_peers.add(sorted_pings[i][0]) is adding them by peer index. This is causing an overlap of the set items and thus one less peer is added to the set.

Or I might be completely wrong and this is intended behavior.

mzumsande commented at 5:36 pm on June 2, 2020:

Nice catch, I introduced this bug in the last push! Will fix later today.

mzumsande commented at 0:12 am on June 3, 2020:

I fixed the initialization of current_peer (so that having current_peer=0 for peer 0).

in test/functional/p2p_eviction.py:45 in 3758af7856 outdated

40+class P2PEvict(BitcoinTestFramework):
41+    def set_test_params(self):
42+        self.setup_clean_chain = True
43+        self.num_nodes = 1
44+        # The choice of 32 maxconnections results in a maximum of 21 inbound connections
45+        # (32 - 10 outbound - 1 feeler). 20 inbounds peers are protected from eviction.

rajarshimaitra commented at 10:24 am on June 2, 2020:

“20 inbounds peers are protected from eviction.” I am not being able to figure the rationale behind this number 20.

andrewtoth commented at 4:42 pm on June 2, 2020:

I believe the 20 that are protected are: 4 - netgroup (cannot be tested in this framework) 4 - sent us blocks first 4 - sent us transactions first 8 - lowest ping time

20 - total protected

andrewtoth commented at 4:46 pm on June 2, 2020:

Perhaps this would be helpful to clarify in the comments.

mzumsande commented at 0:13 am on June 3, 2020:

Done, I expanded that comment.

rajarshimaitra commented at 5:27 am on June 3, 2020:

Thanks @andrewtoth, that helped a lot.

jonatack commented at 2:41 pm on June 3, 2020:

For reference, see CConnman::AttemptToEvictConnection() in src/net.cpp.

in test/functional/p2p_eviction.py:113 in 3758af7856 outdated

119+
120+        for i in range(8):
121+            self.protected_peers.add(sorted_pings[i][0])
122+
123+        # One of the non-protected peers must be evicted. We can't be sure which one because
124+        # 4 peers are protected via netgroup, which is identical for all peers,

rajarshimaitra commented at 10:32 am on June 2, 2020:

This is coming a little confusing for me, which 4 peers are protected via netgroup? If i understand correctly for functional testing all the peers are from same netgroup. So there’s no way to differentiate between them via IP addresses. Is this what this comment is trying to convey?

andrewtoth commented at 4:53 pm on June 2, 2020:

I believe these comments above try to convey this behaviour https://github.com/bitcoin/bitcoin/pull/16756/files#diff-96b7763a95e093580bd4507bce07ff22R10-R12

mzumsande commented at 5:32 pm on June 2, 2020:

I think it is impossible to say which peers are protected via netgroup because it can vary from run to run! Since A) this is the first check in AttemptToEvictConnection() B) all test nodes have identical netgroup and C) std::sort does not guarantee to preserve the order of identical elements, this step should protect 4 arbitrary peers in the test. If, for example, all 4 block-sending peers would be protected by chance, then the later EraseLastKElements with CompareNodeBlockTime would operate on identical elements and erase 4 random peers from vEvictionCandidates (protecting them). The assertion at the end only tests that no peer assumed to be protected will be evicted. Its protection might happen in the EraseLastKElements specific to its unique feature, or earlier by chance.

jonatack commented at 2:45 pm on June 3, 2020:

Yes, the only certainty we have regarding the evicted peers in this test is that it will be one of the zero-indexed peers in the set of {4, 5, 6, 7, 8} e.g. the slow-pinging ones.

rajarshimaitra commented at 10:36 am on June 2, 2020: contributor

Concept ACK. Very nice test framework with elaborate comments to understand the logic. Below are few comments for mostly my own understanding and one probable minor bug.

in test/functional/p2p_eviction.py:67 in 3758af7856 outdated

69+
70+        # The next 5 nodes are slow-pinging peers, making them eviction candidates.
71+        for _ in range(5):
72+            node.add_p2p_connection(SlowP2PInterface())
73+            current_peer += 1
74+

fjahr commented at 12:20 pm on June 2, 2020:

nit: double new-line

mzumsande commented at 0:08 am on June 3, 2020:

fixed.

fjahr approved

fjahr commented at 12:46 pm on June 2, 2020: contributor

Code review ACK 3758af78564e9a0e059f38c11e941d68be192fb6

Nice test, thank you for adding it!

in test/functional/p2p_eviction.py:136 in 3758af7856 outdated

131+        self.log.info("Test that one peer was evicted")
132+        assert_equal(len(self.evicted_peers), 1)
133+        self.log.debug("{} evicted peer: {}".format(len(self.evicted_peers), set(self.evicted_peers)))
134+
135+        self.log.info("Test that no peer expected to be protected was evicted")
136+        assert_equal(self.evicted_peers[0] in self.protected_peers, False)

jonatack commented at 1:03 pm on June 2, 2020:

style nit: assertion here is about exclusion, not equality; I think the idiomatic assertion in the tests codebase would be

0        assert self.evicted_peers[0] not in self.protected_peers

mzumsande commented at 11:57 pm on June 2, 2020:

done.

in test/functional/p2p_eviction.py:132 in 3758af7856 outdated

132+        assert_equal(len(self.evicted_peers), 1)
133+        self.log.debug("{} evicted peer: {}".format(len(self.evicted_peers), set(self.evicted_peers)))
134+
135+        self.log.info("Test that no peer expected to be protected was evicted")
136+        assert_equal(self.evicted_peers[0] in self.protected_peers, False)
137+        self.log.debug("{} protected peers: {}".format(len(self.protected_peers), self.protected_peers))

jonatack commented at 1:05 pm on June 2, 2020:

In case of assertion failure, these debug logs would be helpful to see. I’d suggest placing them before the assertions here and line 133.

mzumsande commented at 11:57 pm on June 2, 2020:

done

in test/functional/p2p_eviction.py:50 in 3758af7856 outdated

45+        # (32 - 10 outbound - 1 feeler). 20 inbounds peers are protected from eviction.
46+        self.extra_args = [['-maxconnections=32']]
47+
48+    def setup_network(self):
49+        self.setup_nodes()
50+

jonatack commented at 1:10 pm on June 2, 2020:

Is this needed?

0-    def setup_network(self):
1-        self.setup_nodes()
2-

jonatack commented at 1:14 pm on June 2, 2020:

I think it would be good to separate the lengthy setup from the tests, per https://github.com/jonatack/bitcoin/commit/c9e96a1, and/or at least add logging at the start of the setup:

0    def run_test(self):
1+        self.log.info("Set up test peer network")
2+        self.setup_test_peers()

The test takes a bit of time to get started, and logging provides useful feedback on how much time is spent in your custom setup (adding the logging shows the custom setup takes ~5 seconds on my laptop) which can also potentially help optimise it.

mzumsande commented at 11:59 pm on June 2, 2020:

The setup_network override is not needed imo, removed - thanks!

I added several logging messages for the initial phase (as also suggested below).

I am still a bit hesitant with respect to the setup network / test separation, because I don’t really view most of it as network setup - in the network setup phase, one node is created, and then in the second phase, we simulate inbound peers connecting to it, sending blocks and transactions, until all slots are taken and a peer gets evicted. But I will change it if others also think that this is preferable!

jonatack commented at 2:35 pm on June 3, 2020:

Thanks for adding the additional logging; it’s great.

I’m unsure but suspect one would see connect_node statements in the tests if the override was needed.

If you prefer to keep the setup and the assertions all in one block, then no need for self.protected_peers and self.evicted_peers to be class variables; that was done in order to separate the network setup from the tests/assertions.

mzumsande commented at 8:18 pm on June 6, 2020:

Done, made them local again.

jonatack commented at 1:15 pm on June 2, 2020: contributor

Almost-ACK. Thanks for taking some of the suggestions in https://github.com/jonatack/bitcoin/commit/c9e96a1.

in test/functional/p2p_eviction.py:57 in 3758af7856 outdated

52+        self.protected_peers = set() # peers that we expect to be protected from eviction
53+        current_peer = 0
54+        node = self.nodes[0]
55+        node.generatetoaddress(101, node.get_deterministic_priv_key().address)
56+
57+        # The first 4 peers send us a block, protecting them from eviction.

andrewtoth commented at 4:34 pm on June 2, 2020:

These single line comments above each for block might be more helpful as logs. Rationale https://github.com/bitcoin/bitcoin/blob/master/test/functional/README.md#general-test-writing-advice

0        self.log.info("Create the first 4 peers and protect them from eviction by sending us a block.")

mzumsande commented at 11:59 pm on June 2, 2020:

Done, thanks!

andrewtoth commented at 4:58 pm on June 2, 2020: contributor

Tested ACK 3758af78564e9a0e059f38c11e941d68be192fb6

mzumsande force-pushed on Jun 3, 2020

rajarshimaitra commented at 6:36 am on June 3, 2020: contributor

Not specific to this PR, but related to the eviction logic code. https://github.com/bitcoin/bitcoin/blob/657b82cef0e8e8695fc189d013a4353bdbebb041/src/net.cpp#L932-L935 The naMostConnections is storing the netgroup key with most number of connections. Why the variable name is like that? First i thought its a typo, but then it seems the a is simply added to distinguish it from nMostConnections, or is there a specific meaning to it? IMO feels like not so appropriate name to give to a variable which is not storing number of connections.

instagibbs commented at 2:57 pm on June 3, 2020: member

@rajarshimaitra feel free to open a PR on that if you have ideas on making the code more self-explanatory :+1:

jonatack commented at 3:30 pm on June 3, 2020: contributor

ACK 3758af78564e9a0e059f38c11e941d68be192fb6 thanks for adding useful test coverage. Reviewed/tested, re-checked the coherence with CConnman::AttemptToEvictConnection(), printed the peers at each step, and added additional assertions. I reckon the code could be shorter, but it’s fine for now.

Thanks also for your patience and taking into consideration all the feedback.

jnewbery commented at 3:43 pm on June 3, 2020: contributor

I have a few style nits which I won’t leave inline in order not to distract this PR from functional review:

L49: at least 2 spaces before inline comments (https://www.python.org/dev/peps/pep-0008/#inline-comments)
L78: spaces around operators (https://www.python.org/dev/peps/pep-0008/#other-recommendations)
L114: no spaces around keyword argument (https://www.python.org/dev/peps/pep-0008/#other-recommendations)
L114: space after colon (https://www.python.org/dev/peps/pep-0008/#pet-peeves)
L124: prefer if not expression over if expression == false (https://www.python.org/dev/peps/pep-0008/#programming-recommendations)

Consider installing a python linter like flake8 or black to apply these automatically.

jonatack commented at 3:53 pm on June 3, 2020: contributor

Building on John’s style points, from test/functional/README.md: When calling RPCs with several arguments, consider using named keyword arguments instead of positional arguments to make the intent of the call clear to readers.

The variables used as positional args in this test are sufficiently well-named that I didn’t bring it up, but it’s good to know.

in test/functional/p2p_eviction.py:49 in 32c6ce0be3 outdated

44+        # (32 - 10 outbound - 1 feeler). 20 inbound peers are protected from eviction:
45+        # 4 by netgroup, 4 that sent us blocks, 4 that sent us transactions and 8 via lowest ping time
46+        self.extra_args = [['-maxconnections=32']]
47+
48+    def run_test(self):
49+        self.protected_peers = set() # peers that we expect to be protected from eviction

jnewbery commented at 3:59 pm on June 3, 2020:

nit: protected_peers can be a local variable rather than a class member, since it’s only ever used in this function.

in test/functional/p2p_eviction.py:58 in 32c6ce0be3 outdated

53+
54+        self.log.info("Create 4 peers and protect them from eviction by sending us a block")
55+        for _ in range(4):
56+            block_peer = node.add_p2p_connection(SlowP2PDataStore())
57+            current_peer += 1
58+            block_peer.sync_with_ping(timeout=5)

jnewbery commented at 4:02 pm on June 3, 2020:

nit: no need for custom timeout here.

in test/functional/p2p_eviction.py:76 in 32c6ce0be3 outdated

71+
72+        self.log.info("Create 4 peers and protect them from eviction by sending us a tx")
73+        for i in range(4):
74+            txpeer = node.add_p2p_connection(SlowP2PInterface())
75+            current_peer += 1
76+            txpeer.sync_with_ping(timeout=5)

jnewbery commented at 4:04 pm on June 3, 2020:

nit: as above, no need for custom timeout

in test/functional/p2p_eviction.py:103 in 32c6ce0be3 outdated

 98+            current_peer += 1
 99+            wait_until(lambda: "ping" in fastpeer.last_message, timeout=10)
100+
101+        # Make sure by asking the node what the actual min pings are
102+        peerinfo = node.getpeerinfo()
103+        pings = [[0 for i in range(2)] for j in range(len(node.p2ps))]

jnewbery commented at 11:45 pm on June 3, 2020:

Using a list of lists like this is a slightly confusing structure. Consider using a dictionary, and then sorting the dict.items() items by value.

mzumsande commented at 8:36 pm on June 6, 2020:

Changed it to a dictionary.

in test/functional/p2p_eviction.py:15 in 32c6ce0be3 outdated

10+We cannot currently test the parts of the eviction logic that are based on
11+address/netgroup since in the current framework, all peers are connecting from
12+the same local address. See Issue #14210 for more info.
13+
14+Therefore, this test is limited to the remaining protection criteria:
15+1.) Protect 4 nodes having sent us a block most recently

jnewbery commented at 11:49 pm on June 3, 2020:

Consider removing these criteria from the file-level doc string and just relying on the comments below. If the criteria ever change, it’s likely that this docstring will not get updated and will become outdated.

mzumsande commented at 8:20 pm on June 6, 2020:

done

jnewbery commented at 11:49 pm on June 3, 2020: contributor

Looks great. A bunch of minor suggestions inline.

Add functional test for P2P eviction logic of inbound peers 45eff751c6

mzumsande force-pushed on Jun 6, 2020

mzumsande commented at 8:33 pm on June 6, 2020: contributor

I just pushed an update that should address all outstanding suggestions from review.

jonatack commented at 10:06 am on June 7, 2020: contributor

Looking at it again, this test can be much simpler. One example: the current_peer counter and the protected_peers set building can all be removed in favor of just checking that the evicted peer is one of the slow-pinging peers. (After all this review, I may propose the simplifications as a follow-up.)

ACK 45eff751c6d07007dabc365dc4c0e6c63e3fe5cf

naumenkogs commented at 3:09 pm on June 9, 2020: member

Tested ACK 45eff75

fjahr commented at 1:09 pm on June 12, 2020: contributor

re-ACK 45eff751c6d07007dabc365dc4c0e6c63e3fe5cf

Changes since my last reviews were minor improvements addressing nit review comments.

in test/functional/p2p_eviction.py:2 in 45eff751c6

0@@ -0,0 +1,129 @@
1+#!/usr/bin/env python3
2+# Copyright (c) 2019 The Bitcoin Core developers

andrewtoth commented at 3:20 pm on June 12, 2020:

nit

0# Copyright (c) 2020 The Bitcoin Core developers

andrewtoth commented at 3:21 pm on June 12, 2020: contributor

re-ACK 45eff751c6d07007dabc365dc4c0e6c63e3fe5cf

maflcko merged this on Jun 12, 2020

maflcko closed this on Jun 12, 2020

sidhujag referenced this in commit 0fa1d20796 on Jun 13, 2020

Fabcien referenced this in commit 607527e0db on May 13, 2021

bitcoin locked this on Feb 15, 2022

mzumsande deleted the branch on Oct 13, 2023

test: Connection eviction logic tests #16756

Conflicts