Page MenuHomePhabricator

[Part 1 of 4] [qa] De-globalize port seeding
Needs ReviewPublic

Authored by schancel on Fri, Nov 30, 06:34.

Details

Reviewers
deadalnix
jasonbcox
Group Reviewers
Restricted Project
Summary

This commit removes the dependency on the global function p2p_port when connecting nodes to each other. This is a first
step in removing PortSeed.n. We accomplish this by attaching the relevant information to the TestNode object and passing that in directly in to connect_nodes and disconnect_nodes.

Test Plan
./test/functional/test_runner.py

Diff Detail

Event Timeline

schancel created this revision.Fri, Nov 30, 06:34
Herald added a reviewer: Restricted Project. · View Herald TranscriptFri, Nov 30, 06:34
schancel edited the summary of this revision. (Show Details)Fri, Nov 30, 06:37
schancel marked 8 inline comments as done.Fri, Nov 30, 06:45
schancel added inline comments.
test/functional/test_framework/test_framework.py
295 ↗(On Diff #6194)

Move these to where they can get the seed.

test/functional/test_framework/test_node.py
43 ↗(On Diff #6194)

Rename as it's not just for rpc

46 ↗(On Diff #6194)

Store state here in case other functions need it (they will).

test/functional/test_framework/util.py
273 ↗(On Diff #6194)

The point of this diff is to remove this guy.

299 ↗(On Diff #6194)

This was simplified so there isn't a need to construct a string to specify the port. The port now needs to be passed in because rpc_port is attached to BitcoinTestFramework

310 ↗(On Diff #6194)

Require the ports to be passed in as we don't have access to a BitcoinTestFramework instance, nor do I think it makes sense to pass one in.

364 ↗(On Diff #6194)

This change could probably be separated out. I made it because I wasn't sure if something had gotten messed up elsewhere with string formatting.

376 ↗(On Diff #6194)

Switch from node_num to node. We store the port there so we can disconnect/connect nodes. An Alternative would be to take a BitcoinTestFramework, or make this a method there. I think this is the best option, but requires a lot of tests to change.

schancel planned changes to this revision.Fri, Nov 30, 06:48

rpcbind_test is using static port. Need to test this on linux and fix it.

jasonbcox requested changes to this revision.Fri, Nov 30, 18:24
jasonbcox added a subscriber: jasonbcox.
jasonbcox added inline comments.
test/functional/test_framework/util.py
364 ↗(On Diff #6194)

Yes, please separate it out. This diff is big enough as it is.

schancel updated this revision to Diff 6207.Fri, Nov 30, 20:59

Split diff up part 1

schancel retitled this revision from [qa] De-globalize port seeding to [Part 1 of 3] [qa] De-globalize port seeding.Fri, Nov 30, 21:01
schancel edited the summary of this revision. (Show Details)Fri, Nov 30, 21:02
schancel updated this revision to Diff 6212.Fri, Nov 30, 22:07

Fix splitting

schancel updated this revision to Diff 6214.Fri, Nov 30, 22:16

Remove update to rpc_url

schancel updated this revision to Diff 6215.Fri, Nov 30, 22:58

Rebase and fix extraneous changes

schancel retitled this revision from [Part 1 of 3] [qa] De-globalize port seeding to [Part 1 of 4] [qa] De-globalize port seeding.Fri, Nov 30, 23:01
schancel updated this revision to Diff 6224.Sat, Dec 1, 21:55

Fix syntax error

deadalnix requested changes to this revision.Sun, Dec 2, 23:06
deadalnix added inline comments.
test/functional/test_framework/test_node.py
48

It's very unclear to me why there refactoring about the RPC port happens in the middle of a giant refactoring of connect_node, disconnect_node and connect_bi.

It seems like this whole thing should be 4 diffs.

test/functional/test_framework/util.py
406

It does look like this could be de-globalized easily without changing half the tests. The new API is arguably better, but changing that API does nothing for the current diff but make it bigger.

This revision now requires changes to proceed.Sun, Dec 2, 23:06
schancel requested review of this revision.Mon, Dec 3, 04:10
schancel marked an inline comment as done.
schancel added inline comments.
test/functional/test_framework/util.py
406

If you look carefully you will see that connect_nodes does not provide the nodes object, or the node object for the 2nd paramater. Therefor there's no place to fetch the p2p port from. I don't see how this can be fixed without changing the API.

If you don't agree, please give an example of what I could do, because I don't understand how.