test: p2p: check disconnect due to lack of desirable service flags #29279

pull theStack wants to merge 3 commits into bitcoin:master from theStack:202401-test-check_disconnect_lack_desirable_flags changing 4 files +92 −2
  1. theStack commented at 2:37 am on January 19, 2024: contributor

    This PR adds missing test coverage for disconnecting peers which don’t offer the desirable service flags in their VERSION message: https://github.com/bitcoin/bitcoin/blob/5f3a0574c45477288bc678b15f24940486084576/src/net_processing.cpp#L3384-L3389 This check is relevant for the connection types “outbound-full-relay”, “block-relay-only” and “addr-fetch” (see CNode::ExpectServicesFromConn(...)). Feeler connections always disconnect, which is also tested here.

    In lack of finding a proper file where this test would fit in, I created a new one. Happy to take suggestions there.

  2. DrahtBot commented at 2:37 am on January 19, 2024: contributor

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

    Code Coverage

    For detailed information about the code coverage, see the test coverage report.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK davidgumberg, fjahr, cbergqvist, stratospher, itornaza
    Concept ACK furszy, BrandonOdiwuor
    Stale ACK delta1, epiccurious, marcofleon, mzumsande

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    Conflicts

    No conflicts as of last run.

  3. DrahtBot added the label Tests on Jan 19, 2024
  4. theStack force-pushed on Jan 19, 2024
  5. DrahtBot added the label CI failed on Jan 19, 2024
  6. DrahtBot removed the label CI failed on Jan 19, 2024
  7. delta1 approved
  8. delta1 commented at 8:54 am on January 19, 2024: none

    Code Review ACK 358561e54ef84f64b216ca8a4271ad17d91b1d51

    Adds a new test, increases code coverage.

  9. furszy commented at 1:23 pm on January 19, 2024: member
    See #28170. Obvious concept ACK. We do have a post-IBD bug on this process.
  10. BrandonOdiwuor commented at 8:13 pm on January 19, 2024: contributor

    Concept ACK 358561e54ef84f64b216ca8a4271ad17d91b1d51

    Increasing code coverage

  11. in test/functional/p2p_handshake.py:29 in 358561e54e outdated 
    24+
25+    def run_test(self):
26+        node = self.nodes[0]
27+        self.log.info("Check that lacking desired service flags leads to disconnect")
28+        for i, conn_type in enumerate(["outbound-full-relay", "block-relay-only", "addr-fetch"]):
29+            services = random.choice([0, NODE_NETWORK, NODE_WITNESS])
    stratospher commented at 3:43 pm on January 25, 2024:
    358561e: nit: s/0/NODE_NONE
  12. stratospher commented at 3:45 pm on January 25, 2024: contributor
    Concept ACK! i think flaky tests are possible because disconnection could happen before p2p_conn.wait_for_connect() in add_outbound_connection()?
  13. theStack commented at 5:52 pm on January 25, 2024: contributor

    Concept ACK! i think flaky tests are possible because disconnection could happen before p2p_conn.wait_for_connect() in add_outbound_connection()?

    True, good catch. The flaky case can be reproduced by adding an artificial delay before the wait_for_connect() call, e.g.:

     0diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py
 1index 77f6e69e98..4aa54fa64a 100755
 2--- a/test/functional/test_framework/test_node.py
 3+++ b/test/functional/test_framework/test_node.py
 4@@ -725,6 +725,7 @@ class TestNode():
 5             p2p_conn.wait_until(lambda: p2p_conn.message_count["version"] == 1, check_connected=False)
 6             p2p_conn.wait_until(lambda: not p2p_conn.is_connected, check_connected=False)
 7         else:
 8+            time.sleep(1)
 9             p2p_conn.wait_for_connect()
10             self.p2ps.append(p2p_conn)

    I guess one solution could be to add a send_version parameter to add_outbound_p2p_connection (like add_p2p_connection already has) to prevent the immediate sending of the version message. We could set that to false in the test and send the version message triggering the disconnect manually. Will set the PR to draft state until I have something working.

  14. theStack marked this as a draft on Jan 25, 2024
  15. theStack force-pushed on Jan 26, 2024
  16. theStack commented at 2:53 am on January 26, 2024: contributor
    Solved the flaky test issue by introducing a wait_for_disconnect parameter to add_p2p_outbound_connection, which hits the same code path as for feeler connections if set (i.e. waiting for immediate disconnect).
  17. theStack marked this as ready for review on Jan 26, 2024
  18. in test/functional/p2p_handshake.py:45 in ff5f99400a outdated 
    40+            with node.assert_debug_log([expected_debug_log]):
41+                self.check_outbound_disconnect(node, i, conn_type, services)
42+
43+        self.log.info("Check that feeler connections get disconnected immediately")
44+        with node.assert_debug_log(["feeler connection completed"]):
45+            self.check_outbound_disconnect(node, i+1, "feeler", 0)
    stratospher commented at 7:56 am on January 29, 2024:
    ff5f994: nit: NODE_NONE here too?
  19. stratospher commented at 7:57 am on January 29, 2024: contributor
    tested ACK ff5f994.
  20. DrahtBot requested review from BrandonOdiwuor on Jan 29, 2024
  21. DrahtBot requested review from furszy on Jan 29, 2024
  22. DrahtBot requested review from delta1 on Jan 29, 2024
  23. DrahtBot removed review request from delta1 on Jan 29, 2024
  24. DrahtBot removed review request from BrandonOdiwuor on Jan 29, 2024
  25. DrahtBot requested review from delta1 on Jan 29, 2024
  26. DrahtBot requested review from BrandonOdiwuor on Jan 29, 2024
  27. DrahtBot added the label Needs rebase on Jan 29, 2024
  28. theStack force-pushed on Jan 29, 2024
  29. theStack commented at 10:59 pm on January 29, 2024: contributor
    Rebased on master (after the merge of #24748 :tada: ), and tackled #29279 (review). Note that I tried enabling v2 transport for this test, but it seems that it is non-trivial and needs some deeper changes in the test framework regarding the order of version messages sent (see slightly related comment https://mirror.b10c.me/bitcoin-bitcoin/24748/#discussion_r1465420112). Will tackle that in a follow-up.
  30. DrahtBot removed the label Needs rebase on Jan 30, 2024
  31. stratospher commented at 3:56 am on January 30, 2024: contributor
    ACK d82eafb173d6bfa98a59e86a845013cc8528b65d.
  32. DrahtBot removed review request from delta1 on Jan 30, 2024
  33. DrahtBot removed review request from BrandonOdiwuor on Jan 30, 2024
  34. DrahtBot requested review from BrandonOdiwuor on Jan 30, 2024
  35. DrahtBot requested review from delta1 on Jan 30, 2024
  36. in test/functional/p2p_handshake.py:35 in d82eafb173 outdated 
    30+
31+    def run_test(self):
32+        node = self.nodes[0]
33+        self.log.info("Check that lacking desired service flags leads to disconnect")
34+        for i, conn_type in enumerate(["outbound-full-relay", "block-relay-only", "addr-fetch"]):
35+            services = random.choice([NODE_NONE, NODE_NETWORK, NODE_WITNESS])
    Empact commented at 5:10 pm on February 2, 2024:
    ACK d82eafb173d6bfa98a59e86a845013cc8528b65d nit: Why not enumerate all combinations of services and connection types here?
    epiccurious commented at 2:23 am on February 11, 2024:
    ACK this nit. Any reason to not include all here?
    Empact commented at 11:24 pm on February 22, 2024:

    Benefits of enumerating: tests will be comprehensive and ensure that results can be consistently reproduced on every run, instead of being possibly flaky. Costs: 9 instead of 3 iterations of this case.

    Seems to weigh in favor of iteration - imo our tests should tend towards comprehensive and consistent.

    theStack commented at 12:51 pm on February 23, 2024:
    Sorry for the extra-late reply! I agree that enumerating through the combinations makes sense. Since there is already a good number of ACKs, I prefer to tackle that in a follow-up, together with other proposed changes / new test coverage ideas (e.g. #29279 (review) and #29279#pullrequestreview-1860288188).
    cbergqvist commented at 2:49 pm on February 23, 2024:
    (ACKing nit).
  37. DrahtBot removed review request from BrandonOdiwuor on Feb 2, 2024
  38. DrahtBot removed review request from delta1 on Feb 2, 2024
  39. DrahtBot requested review from BrandonOdiwuor on Feb 2, 2024
  40. DrahtBot requested review from delta1 on Feb 2, 2024
  41. furszy commented at 8:12 pm on February 2, 2024: member
    Now that #28170 has been merged, it would be good to include a test case for it. The node should only connect to NODE_NETWORK_LIMITED peers when the local chain is within 24h window from the tip.
  42. DrahtBot removed review request from BrandonOdiwuor on Feb 2, 2024
  43. DrahtBot removed review request from delta1 on Feb 2, 2024
  44. DrahtBot requested review from BrandonOdiwuor on Feb 2, 2024
  45. DrahtBot requested review from furszy on Feb 2, 2024
  46. DrahtBot requested review from delta1 on Feb 2, 2024
  47. glozow referenced this in commit 4572f48fd5 on Feb 6, 2024
  48. DrahtBot removed review request from BrandonOdiwuor on Feb 11, 2024
  49. DrahtBot removed review request from delta1 on Feb 11, 2024
  50. DrahtBot requested review from delta1 on Feb 11, 2024
  51. DrahtBot requested review from BrandonOdiwuor on Feb 11, 2024
  52. epiccurious commented at 2:27 am on February 11, 2024: none

    good to include a test case … only connect to NODE_NETWORK_LIMITED peers when the local chain is within 24h window from the tip

    ACK

  53. DrahtBot removed review request from delta1 on Feb 11, 2024
  54. DrahtBot removed review request from BrandonOdiwuor on Feb 11, 2024
  55. DrahtBot requested review from delta1 on Feb 11, 2024
  56. DrahtBot requested review from BrandonOdiwuor on Feb 11, 2024
  57. epiccurious commented at 2:28 am on February 11, 2024: none
    utACK d82eafb173d6bfa98a59e86a845013cc8528b65d.
  58. DrahtBot removed review request from delta1 on Feb 11, 2024
  59. DrahtBot removed review request from BrandonOdiwuor on Feb 11, 2024
  60. DrahtBot requested review from delta1 on Feb 11, 2024
  61. DrahtBot requested review from BrandonOdiwuor on Feb 11, 2024
  62. in test/functional/p2p_handshake.py:28 in d82eafb173 outdated 
    23+    def set_test_params(self):
24+        self.num_nodes = 1
25+
26+    def check_outbound_disconnect(self, node, p2p_idx, connection_type, services):
27+        node.add_outbound_p2p_connection(
28+            P2PInterface(), p2p_idx=p2p_idx, wait_for_disconnect=True,
    fjahr commented at 11:59 pm on February 18, 2024:
    nit: since we wait for the disconnect anyway it seems that passing a different p2p_idx each time isn’t necessary, so the test could be simplified a tiny bit
    theStack commented at 6:26 pm on February 28, 2024:
    Good catch, done. The p2p_idx is now passed with a fixed value of zero.
  63. fjahr commented at 0:04 am on February 19, 2024: contributor
    tACK d82eafb173d6bfa98a59e86a845013cc8528b65d
  64. DrahtBot removed review request from delta1 on Feb 19, 2024
  65. DrahtBot removed review request from BrandonOdiwuor on Feb 19, 2024
  66. DrahtBot requested review from BrandonOdiwuor on Feb 19, 2024
  67. DrahtBot requested review from delta1 on Feb 19, 2024
  68. DrahtBot removed review request from BrandonOdiwuor on Feb 19, 2024
  69. DrahtBot removed review request from delta1 on Feb 19, 2024
  70. DrahtBot requested review from BrandonOdiwuor on Feb 19, 2024
  71. DrahtBot requested review from delta1 on Feb 19, 2024
  72. DrahtBot removed review request from BrandonOdiwuor on Feb 22, 2024
  73. DrahtBot removed review request from delta1 on Feb 22, 2024
  74. DrahtBot requested review from BrandonOdiwuor on Feb 22, 2024
  75. DrahtBot requested review from delta1 on Feb 22, 2024
  76. DrahtBot removed review request from BrandonOdiwuor on Feb 23, 2024
  77. DrahtBot removed review request from delta1 on Feb 23, 2024
  78. DrahtBot requested review from delta1 on Feb 23, 2024
  79. DrahtBot requested review from BrandonOdiwuor on Feb 23, 2024
  80. cbergqvist commented at 2:52 pm on February 23, 2024: none

    ACK d82eafb.

    ++Verbosity

    Review

    Test code is fairly clear.

    Tested (23 Feb)

    Ran ./test/functional/p2p_handshake.py a bunch of times unmodified and observed the different service flags being chosen randomly and tests succeeding.

    Poked (27 Feb)

    Modified p2p_handshake.py to always make the outbound connection have DESIRABLE_SERVICE_FLAGS. Observed that the outbound node would no longer get disconnected.

  81. DrahtBot removed review request from delta1 on Feb 23, 2024
  82. DrahtBot removed review request from BrandonOdiwuor on Feb 23, 2024
  83. DrahtBot requested review from delta1 on Feb 23, 2024
  84. DrahtBot requested review from BrandonOdiwuor on Feb 23, 2024
  85. DrahtBot removed review request from delta1 on Feb 23, 2024
  86. DrahtBot removed review request from BrandonOdiwuor on Feb 23, 2024
  87. DrahtBot requested review from delta1 on Feb 23, 2024
  88. DrahtBot requested review from BrandonOdiwuor on Feb 23, 2024
  89. DrahtBot added the label Needs rebase on Feb 27, 2024
  90. DrahtBot removed review request from delta1 on Feb 27, 2024
  91. DrahtBot removed review request from BrandonOdiwuor on Feb 27, 2024
  92. DrahtBot requested review from BrandonOdiwuor on Feb 27, 2024
  93. DrahtBot requested review from delta1 on Feb 27, 2024
  94. theStack force-pushed on Feb 28, 2024
  95. theStack commented at 6:20 pm on February 28, 2024: contributor

    Thanks for the reviews! In the course of rebasing the PR, I’ve restructured it a bit to tackle all suggestions. A new method test_desirable_service_flags is now introduced, which exercises the combination of all relevant outbound connection types with a list of given service flags (see comment #29279 (review)). For testing the successful cases where no disconnect happens, a boolean parameter expect_disconnect is introduced. Lastly, another commit adds a test for the adaptive service flags logic introduced in #28170 (see comment #29279#pullrequestreview-1860288188). Support for --v2transport was also introduced.

    The test cases are really simple now and could be extended quite a bit, but I think it’s a good start.

  96. DrahtBot removed the label Needs rebase on Feb 28, 2024
  97. in test/functional/p2p_handshake.py:77 in 2f5b69eaeb outdated 
    61@@ -60,6 +62,13 @@ def run_test(self):
62         self.test_desirable_service_flags(node, [NODE_NONE, NODE_NETWORK, NODE_WITNESS], expect_disconnect=True)
63         self.test_desirable_service_flags(node, [NODE_NETWORK | NODE_WITNESS], expect_disconnect=False)
64 
65+        self.log.info("Check that limited peers are only desired if the local chain is close to the tip (<24h)")
66+        node.setmocktime(int(time.time()) + 25 * 3600)  # tip outside the 24h window, should fail
67+        self.test_desirable_service_flags(node, [NODE_NETWORK_LIMITED | NODE_WITNESS], expect_disconnect=True)
68+        self.generate(node, 1)  # catch up by mining a block
69+        node.setmocktime(int(time.time()) + 23 * 3600)  # tip inside the 24h window, should succeed
    fjahr commented at 8:13 pm on February 28, 2024:
    nit: This is a bit redundant. Either mining the block or changing the mock time alone will make the following test succeed.
    davidgumberg commented at 5:00 am on March 4, 2024:

    I think it’s a bug, since this causes ApproximateBestBlockDepth() to return a negative number, any positive value of NODE_NETWORK_LIMITED_ALLOW_CONN_BLOCKS will pass here.

    One suggestion:

    0        self.generate(node, 1)  # catch up by mining a block
1        node.bumpmocktime(23 * 3600)  # tip inside the 24h window, should succeed
    theStack commented at 4:44 pm on March 5, 2024:

    I think it’s a bug, since this causes ApproximateBestBlockDepth() to return a negative number, any positive value of NODE_NETWORK_LIMITED_ALLOW_CONN_BLOCKS will pass here.

    Good catch. What happened here was that the mock time was set to before the best block time (something that should never happen in practice), leading to a negative result. That was indeed not intended.

    I decided to simply drop the generate call to keep the change simple.

  98. fjahr commented at 8:15 pm on February 28, 2024: contributor
    re-ACK 2f5b69eaeb623a7b0306e7f41fd2d28741f131c0
  99. DrahtBot removed review request from BrandonOdiwuor on Feb 28, 2024
  100. DrahtBot removed review request from delta1 on Feb 28, 2024
  101. DrahtBot requested review from stratospher on Feb 28, 2024
  102. DrahtBot requested review from BrandonOdiwuor on Feb 28, 2024
  103. DrahtBot requested review from cbergqvist on Feb 28, 2024
  104. DrahtBot requested review from delta1 on Feb 28, 2024
  105. DrahtBot requested review from epiccurious on Feb 28, 2024
  106. DrahtBot removed review request from BrandonOdiwuor on Feb 28, 2024
  107. DrahtBot removed review request from cbergqvist on Feb 28, 2024
  108. DrahtBot removed review request from delta1 on Feb 28, 2024
  109. DrahtBot removed review request from epiccurious on Feb 28, 2024
  110. DrahtBot requested review from cbergqvist on Feb 28, 2024
  111. DrahtBot requested review from epiccurious on Feb 28, 2024
  112. DrahtBot requested review from BrandonOdiwuor on Feb 28, 2024
  113. DrahtBot requested review from delta1 on Feb 28, 2024
  114. cbergqvist approved
  115. cbergqvist commented at 10:59 pm on February 28, 2024: none

    ACK 2f5b69e!

    Tried to make sense of git range-diff d82eafb~2..d82eafb 2f5b69e~3..2f5b69e but the GitHub Files changed view did help.

    Ran

    0./test/functional/p2p_handshake.py

    and

    0./test/functional/p2p_handshake.py --v2transport

    a couple of times each.

  116. DrahtBot removed review request from epiccurious on Feb 28, 2024
  117. DrahtBot removed review request from BrandonOdiwuor on Feb 28, 2024
  118. DrahtBot removed review request from delta1 on Feb 28, 2024
  119. DrahtBot requested review from delta1 on Feb 28, 2024
  120. DrahtBot requested review from epiccurious on Feb 28, 2024
  121. DrahtBot requested review from BrandonOdiwuor on Feb 28, 2024
  122. marcofleon commented at 0:03 am on March 1, 2024: contributor

    Good work, ACK 2f5b69eaeb623a7b0306e7f41fd2d28741f131c0. Read over the changes, built the PR, ran all functional tests and then individually ran p2p_handshake.py and p2p_handshake.py --v2transport. I also inverted some of the logic of expect_disconnect throughout the test. Everything passed/failed as expected.

    The test is designed well and is easily extensible, so I’d say current test coverage is sufficient for merging.

  123. DrahtBot removed review request from delta1 on Mar 1, 2024
  124. DrahtBot removed review request from epiccurious on Mar 1, 2024
  125. DrahtBot removed review request from BrandonOdiwuor on Mar 1, 2024
  126. DrahtBot requested review from BrandonOdiwuor on Mar 1, 2024
  127. DrahtBot requested review from delta1 on Mar 1, 2024
  128. DrahtBot requested review from epiccurious on Mar 1, 2024
  129. DrahtBot requested review from marcofleon on Mar 1, 2024
  130. DrahtBot removed review request from BrandonOdiwuor on Mar 1, 2024
  131. DrahtBot removed review request from delta1 on Mar 1, 2024
  132. DrahtBot removed review request from epiccurious on Mar 1, 2024
  133. DrahtBot removed review request from marcofleon on Mar 1, 2024
  134. DrahtBot requested review from BrandonOdiwuor on Mar 1, 2024
  135. DrahtBot requested review from epiccurious on Mar 1, 2024
  136. DrahtBot requested review from delta1 on Mar 1, 2024
  137. in test/functional/test_framework/test_node.py:729 in a991f1b5d1 outdated 
    725@@ -726,7 +726,7 @@ def add_p2p_connection(self, p2p_conn, *, wait_for_verack=True, send_version=Tru
726 
727         return p2p_conn
728 
729-    def add_outbound_p2p_connection(self, p2p_conn, *, wait_for_verack=True, p2p_idx, connection_type="outbound-full-relay", supports_v2_p2p=None, advertise_v2_p2p=None, **kwargs):
730+    def add_outbound_p2p_connection(self, p2p_conn, *, wait_for_verack=True, wait_for_disconnect=False, p2p_idx, connection_type="outbound-full-relay", supports_v2_p2p=None, advertise_v2_p2p=None, **kwargs):
    mzumsande commented at 6:41 pm on March 1, 2024:
    nit: Maybe it would be more flexible to just have a expect_success parameter here, and let the calling test deal with the waiting for the disconnect. That way, in future tests it could also be used to test for disconnections that happen before sending the version message (e.g. during the v2 handshake, similar to what #29431 does).
    theStack commented at 4:44 pm on March 5, 2024:
    I looked a bit into that but decided to leave it as a follow-up, as it opens up some questions (e.g. should the treatment for feeler connections then also removed and the waiting happen on the caller side?).
  138. mzumsande commented at 6:45 pm on March 1, 2024: contributor
    Code Review ACK 2f5b69eaeb623a7b0306e7f41fd2d28741f131c0
  139. DrahtBot removed review request from BrandonOdiwuor on Mar 1, 2024
  140. DrahtBot removed review request from epiccurious on Mar 1, 2024
  141. DrahtBot removed review request from delta1 on Mar 1, 2024
  142. DrahtBot requested review from BrandonOdiwuor on Mar 1, 2024
  143. DrahtBot requested review from delta1 on Mar 1, 2024
  144. DrahtBot requested review from epiccurious on Mar 1, 2024
  145. DrahtBot removed review request from BrandonOdiwuor on Mar 1, 2024
  146. DrahtBot removed review request from delta1 on Mar 1, 2024
  147. DrahtBot removed review request from epiccurious on Mar 1, 2024
  148. DrahtBot requested review from BrandonOdiwuor on Mar 1, 2024
  149. DrahtBot requested review from epiccurious on Mar 1, 2024
  150. DrahtBot requested review from delta1 on Mar 1, 2024
  151. DrahtBot removed review request from BrandonOdiwuor on Mar 4, 2024
  152. DrahtBot removed review request from epiccurious on Mar 4, 2024
  153. DrahtBot removed review request from delta1 on Mar 4, 2024
  154. DrahtBot requested review from BrandonOdiwuor on Mar 4, 2024
  155. DrahtBot requested review from epiccurious on Mar 4, 2024
  156. DrahtBot requested review from delta1 on Mar 4, 2024
  157. theStack force-pushed on Mar 5, 2024
  158. fjahr commented at 9:37 pm on March 5, 2024: contributor
    re-ACK 49fbd57eebe60c3a36459df7b1545c2c477b2e68
  159. DrahtBot removed review request from BrandonOdiwuor on Mar 5, 2024
  160. DrahtBot removed review request from epiccurious on Mar 5, 2024
  161. DrahtBot removed review request from delta1 on Mar 5, 2024
  162. DrahtBot requested review from mzumsande on Mar 5, 2024
  163. DrahtBot requested review from BrandonOdiwuor on Mar 5, 2024
  164. DrahtBot requested review from marcofleon on Mar 5, 2024
  165. DrahtBot requested review from epiccurious on Mar 5, 2024
  166. DrahtBot requested review from delta1 on Mar 5, 2024
  167. DrahtBot requested review from cbergqvist on Mar 5, 2024
  168. davidgumberg commented at 9:41 pm on March 5, 2024: contributor

    Tested ACK https://github.com/bitcoin/bitcoin/pull/29279/commits/49fbd57eebe60c3a36459df7b1545c2c477b2e68

    Modified values and logic in HasAllDesirableServiceFlags and GetDesirableServiceFlags and verified that the tests added here complain.

    It might be nice to have a test to ensure that we won’t evict manual or inbound connections for having bad service flags, but as far as I can tell, that would require more refactoring of the test framework, maybe in a follow-up.

    One question that still lingers for me after reviewing this is why the limit for NETWORK_LIMITED being desirable (added in #28170) is: NODE_NETWORK_LIMITED_ALLOW_CONN_BLOCKS = 144; instead of 288 which is how many blocks BIP 159 requires NETWORK_LIMITED nodes to store.

  169. DrahtBot removed review request from BrandonOdiwuor on Mar 5, 2024
  170. DrahtBot removed review request from marcofleon on Mar 5, 2024
  171. DrahtBot removed review request from epiccurious on Mar 5, 2024
  172. DrahtBot removed review request from delta1 on Mar 5, 2024
  173. DrahtBot removed review request from cbergqvist on Mar 5, 2024
  174. DrahtBot requested review from cbergqvist on Mar 5, 2024
  175. DrahtBot requested review from epiccurious on Mar 5, 2024
  176. DrahtBot requested review from BrandonOdiwuor on Mar 5, 2024
  177. DrahtBot requested review from delta1 on Mar 5, 2024
  178. DrahtBot requested review from marcofleon on Mar 5, 2024
  179. DrahtBot removed review request from cbergqvist on Mar 5, 2024
  180. DrahtBot removed review request from epiccurious on Mar 5, 2024
  181. DrahtBot removed review request from BrandonOdiwuor on Mar 5, 2024
  182. DrahtBot removed review request from delta1 on Mar 5, 2024
  183. DrahtBot removed review request from marcofleon on Mar 5, 2024
  184. DrahtBot requested review from cbergqvist on Mar 5, 2024
  185. DrahtBot requested review from delta1 on Mar 5, 2024
  186. DrahtBot requested review from BrandonOdiwuor on Mar 5, 2024
  187. DrahtBot requested review from marcofleon on Mar 5, 2024
  188. DrahtBot requested review from epiccurious on Mar 5, 2024
  189. DrahtBot removed review request from cbergqvist on Mar 5, 2024
  190. DrahtBot removed review request from delta1 on Mar 5, 2024
  191. DrahtBot removed review request from BrandonOdiwuor on Mar 5, 2024
  192. DrahtBot removed review request from marcofleon on Mar 5, 2024
  193. DrahtBot removed review request from epiccurious on Mar 5, 2024
  194. DrahtBot requested review from epiccurious on Mar 5, 2024
  195. DrahtBot requested review from BrandonOdiwuor on Mar 5, 2024
  196. DrahtBot requested review from marcofleon on Mar 5, 2024
  197. DrahtBot requested review from delta1 on Mar 5, 2024
  198. DrahtBot requested review from cbergqvist on Mar 5, 2024
  199. cbergqvist approved
  200. cbergqvist commented at 10:07 pm on March 6, 2024: none

    ACK 49fbd57.

    Still have some things to learn about git diffing it seems. I revoke my review of 2f5b69e and re-raise with this one now that I have absorbed a rough idea of NODE_NETWORK_LIMITED and ApproximateBestBlockDepth(). Cheers @davidgumberg!

    https://github.com/bitcoin/bitcoin/blob/49fbd57eebe60c3a36459df7b1545c2c477b2e68/test/functional/p2p_handshake.py#L66-L69

    nit: Would feel more comfortable if the test moved forwards in time instead of backwards, setting the node’s mocked hours to 23 before 25. I re-tested p2p_handshake.py with and without --v2transport, both with backwards time travel as currently implemented, and switching around the lines for forwards time travel (also tried a version using bumpmocktime() 2 hours which also worked, but may be less clear).

  201. DrahtBot removed review request from epiccurious on Mar 6, 2024
  202. DrahtBot removed review request from BrandonOdiwuor on Mar 6, 2024
  203. DrahtBot removed review request from marcofleon on Mar 6, 2024
  204. DrahtBot removed review request from delta1 on Mar 6, 2024
  205. DrahtBot requested review from epiccurious on Mar 6, 2024
  206. DrahtBot requested review from marcofleon on Mar 6, 2024
  207. DrahtBot requested review from BrandonOdiwuor on Mar 6, 2024
  208. DrahtBot requested review from delta1 on Mar 6, 2024
  209. furszy commented at 10:19 pm on March 6, 2024: member

    One question that still lingers for me after reviewing this is why the limit for NETWORK_LIMITED being desirable (added in #28170) is: NODE_NETWORK_LIMITED_ALLOW_CONN_BLOCKS = 144; instead of 288 which is how many blocks BIP 159 requires NETWORK_LIMITED nodes to store. @davidgumberg start from this comment #28170#pullrequestreview-1825332930. We had the same conversation there.

  210. DrahtBot removed review request from epiccurious on Mar 6, 2024
  211. DrahtBot removed review request from marcofleon on Mar 6, 2024
  212. DrahtBot removed review request from BrandonOdiwuor on Mar 6, 2024
  213. DrahtBot removed review request from delta1 on Mar 6, 2024
  214. DrahtBot requested review from furszy on Mar 6, 2024
  215. DrahtBot requested review from epiccurious on Mar 6, 2024
  216. DrahtBot requested review from marcofleon on Mar 6, 2024
  217. DrahtBot requested review from BrandonOdiwuor on Mar 6, 2024
  218. DrahtBot requested review from delta1 on Mar 6, 2024
  219. in test/functional/p2p_handshake.py:21 in b6c7ae6bde outdated 
    16+)
17+from test_framework.p2p import P2PInterface
18+
19+
20+# usual desirable service flags for outbound non-pruned peers
21+DESIRABLE_SERVICE_FLAGS = NODE_NETWORK | NODE_WITNESS
    stratospher commented at 6:13 am on March 7, 2024:
    b6c7ae6: shouldn’t we use NODE_NETWORK_LIMITED | NODE_WITNESS for pruned peers debug log in 49fbd57e.
    itornaza commented at 7:42 pm on March 8, 2024:
    I think it is valid to assume that the limited peers that satisfy the 24 hour time frame are to be included the expected logic too. From a broader perspective, they act as normal peers but take too long due to the limitations they face.
    theStack commented at 2:40 pm on March 11, 2024:

    b6c7ae6: shouldn’t we use NODE_NETWORK_LIMITED | NODE_WITNESS for pruned peers debug log in 49fbd57.

    Indeed, good find. This worked because in the disconnect case, the desirable service flags (appearing in the log as “… does not offer the expected services …”) fall back to NODE_NETWORK | NODE_WITNESS, and in the successful case, the concrete desirable service flags are not tested anywhere. Reworked the last commit (see comment in main thread below).

  220. DrahtBot removed review request from epiccurious on Mar 7, 2024
  221. DrahtBot removed review request from marcofleon on Mar 7, 2024
  222. DrahtBot removed review request from BrandonOdiwuor on Mar 7, 2024
  223. DrahtBot removed review request from delta1 on Mar 7, 2024
  224. DrahtBot requested review from epiccurious on Mar 7, 2024
  225. DrahtBot requested review from delta1 on Mar 7, 2024
  226. DrahtBot requested review from stratospher on Mar 7, 2024
  227. DrahtBot requested review from BrandonOdiwuor on Mar 7, 2024
  228. DrahtBot requested review from marcofleon on Mar 7, 2024
  229. DrahtBot removed review request from epiccurious on Mar 7, 2024
  230. DrahtBot removed review request from delta1 on Mar 7, 2024
  231. DrahtBot removed review request from BrandonOdiwuor on Mar 7, 2024
  232. DrahtBot removed review request from marcofleon on Mar 7, 2024
  233. DrahtBot requested review from BrandonOdiwuor on Mar 7, 2024
  234. DrahtBot requested review from marcofleon on Mar 7, 2024
  235. DrahtBot requested review from epiccurious on Mar 7, 2024
  236. DrahtBot requested review from delta1 on Mar 7, 2024
  237. DrahtBot removed review request from BrandonOdiwuor on Mar 8, 2024
  238. DrahtBot removed review request from marcofleon on Mar 8, 2024
  239. DrahtBot removed review request from epiccurious on Mar 8, 2024
  240. DrahtBot removed review request from delta1 on Mar 8, 2024
  241. DrahtBot requested review from BrandonOdiwuor on Mar 8, 2024
  242. DrahtBot requested review from marcofleon on Mar 8, 2024
  243. DrahtBot requested review from delta1 on Mar 8, 2024
  244. DrahtBot requested review from epiccurious on Mar 8, 2024
  245. test: p2p: support disconnect waiting for `add_outbound_p2p_connection` 
    Adds a new boolean parameter `wait_for_disconnect` to the
`add_outbound_p2p_connection` method. If set, the node under
test is checked to disconnect immediately after receiving the
version message (same logic as for feeler connections).
    405ac819af
  246. test: p2p: check disconnect due to lack of desirable service flags c4a67d396d
  247. test: p2p: check limited peers desirability (depending on best block depth) 
    This adds coverage for the logic introduced in PR #28170
("p2p: adaptive connections services flags").
    2f23987849
  248. theStack force-pushed on Mar 11, 2024
  249. theStack commented at 2:42 pm on March 11, 2024: contributor

    Force-pushed with rebase on master and tackled comment #29279 (review).

    I reworked the test_desirable_service_flags method in the last commit by introducing a new desirable_service_flags parameter that has to be passed. Also, there are now two constants DESIRABLE_SERVICE_FLAGS_FULL and DESIRABLE_SERVICE_FLAGS_PRUNED with a comment that the latter is dynamic. Hope that this all is not considered too confusing. Re-review would be much appreciated!

  250. davidgumberg commented at 3:14 am on March 12, 2024: contributor

    reACK https://github.com/bitcoin/bitcoin/commit/2f23987849758537f76df7374d85a7e87b578b61

    Looks good, retested by modifying HasAllDesirableServiceFlags and ExpectServicesFromConn

  251. DrahtBot requested review from fjahr on Mar 12, 2024
  252. DrahtBot requested review from cbergqvist on Mar 12, 2024
  253. in test/functional/p2p_handshake.py:78 in 2f23987849 
    76+        self.log.info("Check that limited peers are only desired if the local chain is close to the tip (<24h)")
77+        node.setmocktime(int(time.time()) + 25 * 3600)  # tip outside the 24h window, should fail
78+        self.test_desirable_service_flags(node, [NODE_NETWORK_LIMITED | NODE_WITNESS],
79+                                          DESIRABLE_SERVICE_FLAGS_FULL, expect_disconnect=True)
80+        node.setmocktime(int(time.time()) + 23 * 3600)  # tip inside the 24h window, should succeed
81+        self.test_desirable_service_flags(node, [NODE_NETWORK_LIMITED | NODE_WITNESS],
    fjahr commented at 7:06 pm on March 12, 2024:
    nit: The DESIRABLE_SERVICE_FLAGS_* above are the same as the combinations passed as service_flag_tests so that variable could have been used in both places IMO (with a slight rename maybe). But this isn’t critical.
  254. fjahr commented at 7:06 pm on March 12, 2024: contributor

    re-utACK 2f23987849758537f76df7374d85a7e87b578b61

    (CI failure can be ignored)

  255. cbergqvist approved
  256. cbergqvist commented at 12:11 pm on March 14, 2024: none

    re ACK 2f23987849758537f76df7374d85a7e87b578b61

    Built & re-ran test/functional/test_runner.py --cachedir=/mnt/tmp/cache --tmpdir=/mnt/tmp.

  257. stratospher commented at 3:53 am on March 15, 2024: contributor
    tested ACK 2f23987. 🚀
  258. itornaza commented at 8:03 pm on March 18, 2024: none

    tested ACK 2f23987849758537f76df7374d85a7e87b578b61

    • Configured with --with-incompatible-bdb and --enable-suppress-external-warnings
    • Run unit tests with make check and all tests pass
    • Run all functional tests with no extra flags and all tests pass
    • Run all functional tests with --extended and all tests pass

    Notes:

    • First time I run the tests for a PR so I am verbose to avoid any pitfalls
    • Tests run on macOS 14.3.1 (23D60)
  259. glozow merged this on Mar 19, 2024
  260. glozow closed this on Mar 19, 2024
  261. theStack deleted the branch on Mar 19, 2024


theStack DrahtBot delta1 furszy BrandonOdiwuor stratospher Empact epiccurious cbergqvist fjahr davidgumberg marcofleon mzumsande itornaza


delta1 furszy BrandonOdiwuor mzumsande marcofleon epiccurious

Labels
Tests

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-24 03:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me