Page MenuHomePhabricator

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

Authored by deadalnix on Nov 30 2018, 06:34.


Group Reviewers
Restricted Project

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

Diff Detail

Event Timeline

schancel added inline comments.
295 ↗(On Diff #6194)

Move these to where they can get the seed.

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

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.

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

jasonbcox requested changes to this revision.Nov 30 2018, 18:24
jasonbcox added a subscriber: jasonbcox.
jasonbcox added inline comments.
364 ↗(On Diff #6194)

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

schancel retitled this revision from [qa] De-globalize port seeding to [Part 1 of 3] [qa] De-globalize port seeding.Nov 30 2018, 21:01

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.Nov 30 2018, 23:01
deadalnix requested changes to this revision.Dec 2 2018, 23:06
deadalnix added inline comments.

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.


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.Dec 2 2018, 23:06
schancel requested review of this revision.Dec 3 2018, 04:10
schancel marked an inline comment as done.
schancel added inline comments.

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.

deadalnix abandoned this revision.
deadalnix edited reviewers, added: schancel; removed: deadalnix.

This is superseded by D2247 , D2248 , D2250 and D2251