test: Remove connect_nodes_bi #16898

pull MarcoFalke wants to merge 4 commits into bitcoin:master from MarcoFalke:1908-testConnectNodes changing 20 files +108 −69
  1. MarcoFalke commented at 5:04 PM on September 17, 2019: member

    By default all test nodes are connected in a chain. However, instead of just a single connection between each pair of nodes, we end up with up to four connections for a "middle" node (two outbound, two inbound, from each side).

    This is generally redundant (tx and block relay should succeed with just a single connection) and confusing. For example, test timeouts after a call to sync_ may be racy and hard to reproduce. On top of that, the test debug.logs are hard to read because txs and block invs may be relayed on the same connection multiple times.

    Fix this by inlining connect_nodes_bi in the two tests that need it, and then replace it with a single connect_nodes in all other tests.

    Historic background:

    connect_nodes_bi has been introduced as a (temporary?) workaround for bug #5113 and #5138, which has long been fixed in #5157 and #5662.

  2. test: Reformat python imports to aid scripted diff 1111bb91f5
  3. MarcoFalke added the label Tests on Sep 17, 2019
  4. test: Use connect_nodes when connecting nodes in the test_framework faaee1e39a
  5. scripted-diff: test: Replace connect_nodes_bi with connect_nodes
    -BEGIN VERIFY SCRIPT-
sed -i --regexp-extended -e 's/connect_nodes_bi\(self.nodes,\s*(.),\s*/connect_nodes(self.nodes[\1], /g' $(git grep -l connect_nodes_bi)
sed -i --regexp-extended -e 's/connect_nodes_bi(,| )/connect_nodes\1/g'                                  $(git grep -l connect_nodes_bi)
-END VERIFY SCRIPT-
    fa3b9ee8b2
  6. test: Remove unused connect_nodes_bi fadfd844de
  7. MarcoFalke force-pushed on Sep 17, 2019
  8. mzumsande commented at 5:32 PM on September 17, 2019: member

    Concept ACK on removing this wherever possible - this double-connecting seems like a strange hack and it has confused me before. Though I wonder if there will be adjustments to timeouts needed, especially with Travis runs.

  9. MarcoFalke commented at 5:53 PM on September 17, 2019: member

    Are you referring to the poisson delay in tx relay? Good point, but it passed locally and on travis, so I think it is fine.

  10. in test/functional/rpc_net.py:58 in faaee1e39a outdated 
      53 | @@ -54,6 +54,10 @@ def set_test_params(self):
  54 |          self.extra_args = [["-minrelaytxfee=0.00001000"],["-minrelaytxfee=0.00000500"]]
  55 |  
  56 |      def run_test(self):
  57 | +        self.log.info('Connect nodes both way')
  58 | +        connect_nodes(self.nodes[0], 1)
    mzumsande commented at 7:15 PM on September 17, 2019:

    Does the double connection have a functional role here? Otherwise, rpc_net.py should also work with a simple connection if some asserts wrt getconnectioncount are adjusted and the factor 2 in the part where we wait for the ping is dropped.

    MarcoFalke commented at 7:26 PM on September 17, 2019:

    Yes, it is needed to check that the sum of the bytes used on multiple connections (two) is the same as the total of bytes used over all connections.

  11. DrahtBot commented at 1:40 AM on September 18, 2019: member

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #16528 ([WIP] Native Descriptor Wallets (take 2) by achow101)

    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.

  12. laanwj commented at 11:51 AM on September 18, 2019: member

    Looks good to me, in most cases it's redundant (and unrealistic), and the cases where connect_nodes_bi is used are rare enough that it doesn't warrant keeping at around as function.

    The commits are self-explanatory and self-contained.

    ACK fadfd844de8c53034a97dfa6f771ffe9f523fba2

  13. jonasschnelli commented at 12:57 PM on September 18, 2019: contributor

    Thanks for digging out the history of connect_nodes_bi (#5113 and #5138). utACK fadfd844de8c53034a97dfa6f771ffe9f523fba2 - more of less a cleanup PR.

  14. promag commented at 2:30 PM on September 18, 2019: member

    Tested ACK fadfd844de8c53034a97dfa6f771ffe9f523fba2, ran extended tests.

  15. MarcoFalke referenced this in commit 59c138d2f1 on Sep 18, 2019
  16. MarcoFalke merged this on Sep 18, 2019
  17. MarcoFalke closed this on Sep 18, 2019
  18. MarcoFalke deleted the branch on Sep 18, 2019
  19. random-zebra referenced this in commit 46585f7b62 on Mar 31, 2020
  20. deadalnix referenced this in commit 109fb473d2 on Jun 5, 2020
  21. deadalnix referenced this in commit 44f14ad447 on Jun 8, 2020
  22. deadalnix referenced this in commit 2804ca503a on Jun 9, 2020
  23. deadalnix referenced this in commit 68a7440760 on Jun 9, 2020
  24. deadalnix referenced this in commit c7721f4a83 on Jun 9, 2020
  25. ftrader referenced this in commit e7d4fee950 on Aug 17, 2020
  26. ftrader referenced this in commit 243cb91ba3 on Aug 17, 2020
  27. vijaydasmp referenced this in commit 7b55c87291 on Dec 7, 2021
  28. Munkybooty referenced this in commit ecf36ae433 on Dec 7, 2021
  29. Munkybooty referenced this in commit 594a335474 on Dec 7, 2021
  30. Munkybooty referenced this in commit 9cbffbe8df on Dec 7, 2021
  31. Munkybooty referenced this in commit 7b869bf612 on Dec 7, 2021
  32. vijaydasmp referenced this in commit b93b520cea on Dec 10, 2021
  33. vijaydasmp referenced this in commit c0a0c174d3 on Dec 10, 2021
  34. vijaydasmp referenced this in commit a11e7b9f71 on Dec 10, 2021
  35. vijaydasmp referenced this in commit 41cb360564 on Dec 10, 2021
  36. vijaydasmp referenced this in commit 36598d8ec6 on Dec 13, 2021
  37. vijaydasmp referenced this in commit de0618b419 on Dec 13, 2021
  38. vijaydasmp referenced this in commit d03d75fba8 on Dec 15, 2021
  39. Munkybooty referenced this in commit 6e29e43bfa on Dec 15, 2021
  40. Munkybooty referenced this in commit 5eb8cfc359 on Dec 15, 2021
  41. DrahtBot locked this on Dec 16, 2021
Contributors
MarcoFalkemzumsandeDrahtBotlaanwjjonasschnellipromag
Labels
Tests
Linked (view graph)
#5113 Syncing of chains with different height depends on "connection direction"#5138 Headers-first sync never gets going#5157 Introduce preferred download peers#5662 Change download logic to allow calling getdata on inbound peers
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: 2026-04-17 06:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me