test: Remove connect_nodes_bi #16898

maflcko · 2019-09-17T17:04:04Z

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.

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

mzumsande · 2019-09-17T17:32:06Z

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.

maflcko · 2019-09-17T17:53:11Z

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.

mzumsande · 2019-09-17T19:15:10Z

test/functional/rpc_net.py

@@ -54,6 +54,10 @@ def set_test_params(self):
        self.extra_args = [["-minrelaytxfee=0.00001000"],["-minrelaytxfee=0.00000500"]]

    def run_test(self):
+        self.log.info('Connect nodes both way')
+        connect_nodes(self.nodes[0], 1)


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.

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.

DrahtBot · 2019-09-18T01:40:01Z

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

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.

laanwj · 2019-09-18T11:51:46Z

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 fadfd84

jonasschnelli · 2019-09-18T12:57:45Z

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

promag · 2019-09-18T14:30:53Z

Tested ACK fadfd84, ran extended tests.

fadfd84 test: Remove unused connect_nodes_bi (MarcoFalke) fa3b9ee scripted-diff: test: Replace connect_nodes_bi with connect_nodes (MarcoFalke) faaee1e test: Use connect_nodes when connecting nodes in the test_framework (MarcoFalke) 1111bb9 test: Reformat python imports to aid scripted diff (MarcoFalke) Pull request description: 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.log`s 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. ACKs for top commit: laanwj: ACK fadfd84 jonasschnelli: utACK fadfd84 - more of less a cleanup PR. promag: Tested ACK fadfd84, ran extended tests. Tree-SHA512: 2d027a8fd150749c071b64438a0a78ec922178628a7dbb89fd1212b0fa34febd451798c940101155d3617c0426c2c4865174147709894f1f1bb6cfa336aa7e24

3e9081c [Tests] Fix tests duration and sorting in test_runner.py (random-zebra) 57dd5e7 [Tests][Refactor] Move connect_nodes_bi to p2p_time_offset (random-zebra) 9b36468 test: Replace connect_nodes_bi with connect_nodes (random-zebra) c2a701e test: Use connect_nodes when connecting nodes in the test_framework (random-zebra) 5d3f2a1 test: Reformat python imports to aid scripted diff (random-zebra) Pull request description: Mostly from upstream bitcoin#16898 > 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 dashpay#5113 and dashpay#5138, which has long been fixed in dashpay#5157 and dashpay#5662. ACKs for top commit: furszy: Tested ACK 3e9081c . Tree-SHA512: 4af3223c7e5a4b130855d9364b7ec3df5e141151fd1e108c55ef80dd523522780b6130e69cacd467906215c15da9a50496f12298246eb67876fcaa181463cae9

Summary: bitcoin/bitcoin@1111bb9 --- Partial backport of Core [[bitcoin/bitcoin#16898 | PR16898]] Test Plan: ninja ./test/functional/test_runner.py abc-parkedchain.py abc_p2p_compactblocks.py feature_notifications.py Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D6386

Summary: Core [[bitcoin/bitcoin#16898 | PR16898]] changes the test_framework to use connect_nodes instead of connect_nodes_bi, this changes the network topology of the tests which led some of our abc-* tests to fail. according to the logs nodes were disconnecting the other, these changes add a `-whitelist=noban@127.0.0.1` to prevent them from doing so allowing the tests to complete Test Plan: ninja test_runner.py abc-finalize-block abc-parkedchain Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D6456

… test_framework Summary: bitcoin/bitcoin@faaee1e --- Partial backport of Core [[bitcoin/bitcoin#16898 | PR16898]] Test Plan: ninja check-all Reviewers: #bitcoin_abc, jasonbcox Reviewed By: #bitcoin_abc, jasonbcox Subscribers: jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D6490

…onnect_nodes Summary: -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) ~~ sed -i -- 's/connect_nodes_bi/connect_nodes/g' $(git grep -l connect_nodes_bi) git restore test/functional/test_framework/util.py -END VERIFY SCRIPT- (script doesn't match our connect_nodes's API) bitcoin/bitcoin@fa3b9ee --- Depends on D6490 Partial backport of Core [[bitcoin/bitcoin#16898 | PR16898]] Test Plan: ninja check-all Reviewers: #bitcoin_abc, jasonbcox Reviewed By: #bitcoin_abc, jasonbcox Subscribers: jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D6492

Summary: bitcoin/bitcoin@fadfd84 --- Depends on D6492 Concludes backport of Core [[bitcoin/bitcoin#16898 | PR16898]] Test Plan: ninja check-all Reviewers: #bitcoin_abc, jasonbcox Reviewed By: #bitcoin_abc, jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D6493

Summary: Core [[bitcoin/bitcoin#16898 | PR16898]] changes the test_framework to use connect_nodes instead of connect_nodes_bi, this changes the network topology of the tests which led some of our abc-* tests to fail. according to the logs nodes were disconnecting the other, these changes add a `-whitelist=noban@127.0.0.1` to prevent them from doing so allowing the tests to complete Test Plan: ninja test_runner.py abc-finalize-block abc-parkedchain Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D6456

Summary: bitcoin/bitcoin@1111bb9 --- Partial backport of Core [[bitcoin/bitcoin#16898 | PR16898]] Test Plan: ninja ./test/functional/test_runner.py abc-parkedchain.py abc_p2p_compactblocks.py feature_notifications.py Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D6386

fadfd84 test: Remove unused connect_nodes_bi (MarcoFalke) fa3b9ee scripted-diff: test: Replace connect_nodes_bi with connect_nodes (MarcoFalke) faaee1e test: Use connect_nodes when connecting nodes in the test_framework (MarcoFalke) 1111bb9 test: Reformat python imports to aid scripted diff (MarcoFalke) Pull request description: 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.log`s 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 dashpay#5113 and dashpay#5138, which has long been fixed in dashpay#5157 and dashpay#5662. ACKs for top commit: laanwj: ACK fadfd84 jonasschnelli: utACK fadfd84 - more of less a cleanup PR. promag: Tested ACK fadfd84, ran extended tests. Tree-SHA512: 2d027a8fd150749c071b64438a0a78ec922178628a7dbb89fd1212b0fa34febd451798c940101155d3617c0426c2c4865174147709894f1f1bb6cfa336aa7e24

test: Reformat python imports to aid scripted diff

1111bb9

maflcko added the Tests label Sep 17, 2019

MarcoFalke added 3 commits September 17, 2019 13:08

test: Use connect_nodes when connecting nodes in the test_framework

faaee1e

test: Remove unused connect_nodes_bi

fadfd84

maflcko force-pushed the 1908-testConnectNodes branch from fa4c702 to fadfd84 Compare September 17, 2019 17:10

mzumsande reviewed Sep 17, 2019

View reviewed changes

maflcko merged commit fadfd84 into bitcoin:master Sep 18, 2019

maflcko deleted the 1908-testConnectNodes branch September 18, 2019 18:48

random-zebra mentioned this pull request Mar 30, 2020

[Tests][Refactor] Remove connect_nodes_bi PIVX-Project/PIVX#1472

Merged

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

test: Remove connect_nodes_bi #16898

test: Remove connect_nodes_bi #16898

Uh oh!

maflcko commented Sep 17, 2019 •

edited

Loading

Uh oh!

mzumsande commented Sep 17, 2019

Uh oh!

maflcko commented Sep 17, 2019

Uh oh!

mzumsande Sep 17, 2019

Uh oh!

maflcko Sep 17, 2019

Uh oh!

DrahtBot commented Sep 18, 2019

Uh oh!

laanwj commented Sep 18, 2019

Uh oh!

jonasschnelli commented Sep 18, 2019

Uh oh!

promag commented Sep 18, 2019

Uh oh!

Uh oh!

test: Remove connect_nodes_bi #16898

test: Remove connect_nodes_bi #16898

Uh oh!

Conversation

maflcko commented Sep 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mzumsande commented Sep 17, 2019

Uh oh!

maflcko commented Sep 17, 2019

Uh oh!

mzumsande Sep 17, 2019

Choose a reason for hiding this comment

Uh oh!

maflcko Sep 17, 2019

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Sep 18, 2019

Conflicts

Uh oh!

laanwj commented Sep 18, 2019

Uh oh!

jonasschnelli commented Sep 18, 2019

Uh oh!

promag commented Sep 18, 2019

Uh oh!

Uh oh!

maflcko commented Sep 17, 2019 •

edited

Loading