net: remove more CConnman globals (theuni)

dongcarl commented at 12:41 pm on December 2, 2018: member

Rebased @theuni’s #11387

fanquake added the label P2P on Dec 2, 2018

MarcoFalke renamed this:
~~[rebase] net: remove more CConnman globals~~
net: remove more CConnman globals (theuni)
on Dec 2, 2018

DrahtBot commented at 5:04 pm on December 3, 2018: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#15976 (refactor: move methods under CChainState (pt. 1) by jamesob)
#15759 ([p2p] Add 2 outbound blocks-only connections by sdaftuar)
#15651 (torcontrol: Use the default/standard network port for Tor hidden services, even if the internal port is set differently by luke-jr)
#15558 (Don’t query all DNS seeds at once by sipa)
#15421 (torcontrol: Launch a private Tor instance when not already running by luke-jr)
#15112 (build: Optionally enable -Wzero-as-null-pointer-constant by Empact)

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.

in src/torcontrol.cpp:23 in b9fa99f650 outdated

19@@ -20,6 +20,7 @@
20 #include <boost/algorithm/string/split.hpp>
21 #include <boost/algorithm/string/classification.hpp>
22 #include <boost/algorithm/string/replace.hpp>
23+#include <boost/thread.hpp>

MarcoFalke commented at 6:51 pm on December 3, 2018:

Why is this needed?

dongcarl commented at 2:11 am on December 4, 2018:

Fixed. Thanks for spotting it!

dongcarl renamed this:
~~net: remove more CConnman globals (theuni)~~
[WIP] net: remove more CConnman globals (theuni)
on Dec 3, 2018

dongcarl force-pushed on Dec 4, 2018

in src/init.cpp:1369 in 389d7e36f4 outdated

1362@@ -1363,9 +1363,16 @@ bool AppInitMain(InitInterfaces& interfaces)
1363     fDiscover = gArgs.GetBoolArg("-discover", true);
1364     fRelayTxes = !gArgs.GetBoolArg("-blocksonly", DEFAULT_BLOCKSONLY);
1365 
1366+    int listen_port = chainparams.GetDefaultPort();
1367+    assert(listen_port <= 0xffff);
1368+    listen_port = gArgs.GetArg("-port", listen_port);
1369+    if (listen_port > 0xffff) {

Empact commented at 10:59 am on December 4, 2018:

nit: how about check against <1 values here? src/qt/optionsdialog.cpp does so: https://github.com/bitcoin/bitcoin/blob/42653570373dad1fb431de2889764297d59ebcc0/src/qt/optionsdialog.cpp#L59

theuni commented at 7:58 pm on December 6, 2018:

+1. Good catch.

dongcarl commented at 12:57 pm on January 3, 2019:

@Empact Any reason why port 0 isn’t valid?

Empact commented at 6:43 am on January 9, 2019:

Not that I know of, for proxyPort it was disallowed in #1571.

“It is defined as an invalid port number.” according to https://www.grc.com/port_0.htm

Port 0 carries special significance in network programming, particularly in the Unix OS when it comes to socket programming where the port is used for requesting system-allocated, dynamic ports. Port 0 is like a wildcard port that tells the system to find a suitable port number. https://www.lifewire.com/port-0-in-tcp-and-udp-818145

Empact commented at 11:00 am on December 4, 2018: member

Concept ACK

DrahtBot added the label Needs rebase on Dec 4, 2018

in src/init.cpp:1370 in 1f3a5b4282 outdated

1362@@ -1363,6 +1363,13 @@ bool AppInitMain(InitInterfaces& interfaces)
1363     fDiscover = gArgs.GetBoolArg("-discover", true);
1364     fRelayTxes = !gArgs.GetBoolArg("-blocksonly", DEFAULT_BLOCKSONLY);
1365 
1366+    int listen_port = chainparams.GetDefaultPort();
1367+    assert(listen_port <= 0xffff);
1368+    listen_port = gArgs.GetArg("-port", listen_port);
1369+    if (listen_port > 0xffff) {
1370+        return InitError(strprintf(_("Invalid port specified in -port: '%s'"), listen_port));

theuni commented at 8:02 pm on December 6, 2018:

Format string should be %i.

theuni commented at 8:14 pm on December 6, 2018: member

ACK after rebasing and fixing my bugs :p

dongcarl force-pushed on Jan 3, 2019

dongcarl commented at 12:57 pm on January 3, 2019: member

Rebased, fixed bugs, and added port check!

dongcarl commented at 4:40 pm on January 3, 2019: member

If anyone can help with the error here it would be much appreciated. I’m not sure what the proper way is to pass listen_port to the qt modules so that it can be specified here.

DrahtBot removed the label Needs rebase on Jan 3, 2019

ryanofsky commented at 5:26 pm on January 3, 2019: member

If anyone can help with the error here it would be much appreciated.

From https://travis-ci.org/bitcoin/bitcoin/jobs/474841712#L2792

0qt/optionsmodel.cpp: In member function ‘virtual bool OptionsModel::setData(const QModelIndex&, const QVariant&, int)’:
1qt/optionsmodel.cpp:334:42: error: no matching function for call to ‘interfaces::Node::mapPort(bool)’
2             m_node.mapPort(value.toBool());

Lots of ways to fix this error. I’d probably bring back the deleted GetListenPort() function (and nice cnode_listen_port test) and call it two places: 1) In AppInitMain to set listen_port local variable. 2) In NodeImpl::mapPort method to get rid of uint16_t port argument.

Another option would be to store listen_port permanently somewhere, like a global variable, or in some network class as a member. But you might not want to do that since this code is going in opposite direction. Also, it is pretty common in bitcoin code to parse command line arguments as needed instead of storing them.

dongcarl commented at 0:56 am on January 4, 2019: member

I’d probably bring back the deleted GetListenPort() function (and nice cnode_listen_port test) and call it two places: 1) In AppInitMain to set listen_port local variable. 2) In NodeImpl::mapPort method to get rid of uint16_t port argument.

That seems acceptable to me, @theuni any thoughts?

Empact commented at 8:28 am on January 9, 2019: member

I think I found an answer that maintains the separation here: you can pull the default listen port through m_node via g_connman. https://github.com/Empact/bitcoin/commit/3859f2aaa https://travis-ci.org/Empact/bitcoin/builds/477278546

I also looked at adding a field for the port to the form but ran into some issues there: https://github.com/Empact/bitcoin/commit/77cc6218b144b3de6d9bdcd4e438a659c87adf16

DrahtBot added the label Needs rebase on Jan 14, 2019

dongcarl commented at 2:45 pm on January 14, 2019: member

I think I found an answer that maintains the separation here: you can pull the default listen port through m_node via g_connman. Empact@3859f2a travis-ci.org/Empact/bitcoin/builds/477278546

I also looked at adding a field for the port to the form but ran into some issues there: Empact@77cc621

That sounds promising! I’ll try it out. Thanks for the help!

dongcarl force-pushed on Jan 14, 2019

DrahtBot removed the label Needs rebase on Jan 14, 2019

dongcarl commented at 11:45 pm on January 14, 2019: member

Many thanks to @Empact for his suggestion above, I’ve reorganized the commits to make things work.

As @theuni mentioned here: #14605 (review), a followup PR should pass CConnman and Banman into NodeImpl’s constructor so that we can stop using globals.

dongcarl renamed this:
~~[WIP] net: remove more CConnman globals (theuni)~~
net: remove more CConnman globals (theuni)
on Jan 14, 2019

DrahtBot added the label Needs rebase on Jan 21, 2019

dongcarl force-pushed on Jan 23, 2019

DrahtBot removed the label Needs rebase on Jan 23, 2019

fanquake added this to the "In progress" column in a project

dongcarl force-pushed on Feb 1, 2019

dongcarl commented at 3:59 pm on February 1, 2019: member

Rebased!

in src/net.cpp:1492 in 4d0cac7be1 outdated

1489+void StartMapPort(uint16_t port)
1490 {
1491     if (!g_upnp_thread.joinable()) {
1492         assert(!g_upnp_interrupt);
1493-        g_upnp_thread = std::thread((std::bind(&TraceThread<void (*)()>, "upnp", &ThreadMapPort)));
1494+        g_upnp_thread = std::thread(&TraceThread<std::function<void()> >, "upnp", std::bind(&ThreadMapPort, port));

MarcoFalke commented at 4:19 pm on February 1, 2019:

style-nit: in commit 4d0cac7be150145ec45119188458a9625f882b29

Any reason to not use a lambda? It might make this easier to read.

in src/torcontrol.cpp:756 in 8f9062a84d outdated

752@@ -752,7 +753,7 @@ void StartTorControl()
753         return;
754     }
755 
756-    torControlThread = std::thread(std::bind(&TraceThread<void (*)()>, "torcontrol", &TorControlThread));
757+    torControlThread = std::thread(&TraceThread<std::function<void()> >, "torcontrol", std::bind(&TorControlThread, target, default_port));

MarcoFalke commented at 4:20 pm on February 1, 2019:

style-nit: in commit 8f9062a84d01a709d9303d21f25e244a1dbd0044

same

in src/torcontrol.cpp:412 in 8f9062a84d outdated

408@@ -409,7 +409,7 @@ static bool WriteBinaryFile(const fs::path &filename, const std::string &data)
409 class TorController
410 {
411 public:
412-    TorController(struct event_base* base, const std::string& target);
413+    TorController(struct event_base* base, const std::string& target, uint16_t default_port);

ryanofsky commented at 8:12 pm on February 1, 2019:

In commit “tor: pass in Tor control port rather than using globals” (8f9062a84d01a709d9303d21f25e244a1dbd0044)

It seems like you should s/default_port/listen_port/ everywhere in this commit. If I understand correctly, the value starts off being called listen_port in AppInitMain, then is called default_port in StartTorControl, then is called m_listen_port again in TorController. default_port just seems like a seems like confusing and undescriptive name here.

in src/net.h:384 in dcf461abdb outdated

380@@ -378,6 +381,8 @@ class CConnman
381     unsigned int nSendBufferMaxSize{0};
382     unsigned int nReceiveFloodSize{0};
383 
384+    uint16_t m_default_listen_port;

ryanofsky commented at 8:20 pm on February 1, 2019:

In commit “net: pass listen port into CConnman” (dcf461abdb9ad6c769aab072b7522d1e5c189b20)

Throughout this commit I’m confused why default_listen_port and DefaultListenPort are used instead of just listen_port and ListenPort. It seems like these names are referring to the actual, active listening port, not the network default port.

in src/interfaces/node.cpp:86 in 4d0cac7be1 outdated

82@@ -83,8 +83,8 @@ class NodeImpl : public Node
83     bool shutdownRequested() override { return ShutdownRequested(); }
84     void mapPort(bool use_upnp) override
85     {
86-        if (use_upnp) {
87-            StartMapPort();
88+        if (use_upnp && g_connman) {

ryanofsky commented at 8:24 pm on February 1, 2019:

In commit “net: pass port into MapPort/Discover rather than using globals” (4d0cac7be150145ec45119188458a9625f882b29)

I think it instead of adding && g_connman here, it would make more sense to assert(g_connman), or just do nothing if g_connman is null. Falling back to the else condition and having a mapPort(use_upnp=true) trigger StopMapPort in this case doesn’t seem like it could be helpful.

dongcarl force-pushed on Feb 4, 2019

dongcarl commented at 2:26 pm on February 4, 2019: member

Addressed concerns, @ryanofsky I went with doing nothing for when g_connman is null.

in src/net.cpp:162 in f2ad517e13 outdated

158@@ -159,9 +159,9 @@ static std::vector<CAddress> convertSeed6(const std::vector<SeedSpec6> &vSeedsIn
159 // Otherwise, return the unroutable 0.0.0.0 but filled in with
160 // the normal parameters, since the IP may be changed to a useful
161 // one by discovery.
162-CAddress GetLocalAddress(const CNetAddr *paddrPeer, ServiceFlags nLocalServices)
163+CAddress GetLocalAddress(const CNetAddr *paddrPeer, ServiceFlags nLocalServices, uint16_t default_listen_port)

ryanofsky commented at 7:02 pm on February 4, 2019:

In commit “net: pass listen port into CConnman” (f2ad517e131142de1d28670d759c07bdf5466b3f)

Should be port or listen_port, not default_listen_port as far as I can tell. There are also some other uses of default_listen_port in this commit that look like they should be changed.

in src/init.cpp:1731 in f01ebc1a8f outdated

1727@@ -1728,6 +1728,7 @@ bool AppInitMain(InitInterfaces& interfaces)
1728     connOptions.nMaxOutboundLimit = nMaxOutboundLimit;
1729     connOptions.m_peer_connect_timeout = peer_connect_timeout;
1730     connOptions.m_listen_port = listen_port;
1731+    connOptions.m_default_outbound_port = chainparams.GetDefaultPort();

ryanofsky commented at 7:17 pm on February 4, 2019:

In commit “net: pass in default outgoing port” (f01ebc1a8f0a1e5845c90d6a156ccc05b2c366eb):

I’d maybe s/outbound_port/connect_port/ in this commit. I could be off, but “outbound” to me suggests the port connected from not the port connecting to.

ryanofsky approved

ryanofsky commented at 7:18 pm on February 4, 2019: member

utACK f01ebc1a8f0a1e5845c90d6a156ccc05b2c366eb. Looks good. Left some more rename suggestions, but they are not important.

dongcarl force-pushed on Feb 5, 2019

dongcarl commented at 5:56 pm on February 5, 2019: member

Addressed comments and rebased. Thank you @ryanofsky!

in src/net.cpp:1474 in bca901843e outdated

1471+void StartMapPort(uint16_t port)
1472 {
1473     if (!g_upnp_thread.joinable()) {
1474         assert(!g_upnp_interrupt);
1475-        g_upnp_thread = std::thread((std::bind(&TraceThread<void (*)()>, "upnp", &ThreadMapPort)));
1476+        g_upnp_thread = std::thread(&TraceThread<std::function<void()> >, "upnp", [port] { ThreadMapPort(port) });

MarcoFalke commented at 8:17 pm on February 5, 2019:

; :eyes:

dongcarl force-pushed on Feb 5, 2019

dongcarl commented at 3:19 pm on February 7, 2019: member

Fixed.

ryanofsky approved

ryanofsky commented at 4:59 pm on February 28, 2019: member

utACK e4b08081520023d5f5c30e209a5ac2f1b45c4d1c. Just suggested renames and suggested additional semicolon since last review.

ryanofsky commented at 4:35 pm on March 11, 2019: member

Let’s maybe add this to the high priority https://github.com/bitcoin/bitcoin/projects/8 list. It’s a pretty simple change which has been around for a while and (as I understand it) is blocking work on better connman testing.

in src/init.cpp:1379 in c1595190ee outdated

1372@@ -1373,6 +1373,13 @@ bool AppInitMain(InitInterfaces& interfaces)
1373     fDiscover = gArgs.GetBoolArg("-discover", true);
1374     fRelayTxes = !gArgs.GetBoolArg("-blocksonly", DEFAULT_BLOCKSONLY);
1375 
1376+    int listen_port = chainparams.GetDefaultPort();
1377+    assert((0 < listen_port) && (listen_port <= 0xffff));
1378+    listen_port = gArgs.GetArg("-port", listen_port);
1379+    if ((listen_port <= 0) || (0xffff < listen_port)) {

MarcoFalke commented at 4:51 pm on March 11, 2019:

in commit c1595190ee6daa8f85c202e3dbdda927fc5d6067

style-nit: Could extract into a helper IsValidPort or similar

in src/interfaces/node.cpp:90 in 8055e8770a outdated

82@@ -83,8 +83,11 @@ class NodeImpl : public Node
83     bool shutdownRequested() override { return ShutdownRequested(); }
84     void mapPort(bool use_upnp) override
85     {
86+        // TODO: pass in CConnman and Banman into NodeImpl's constructor
87+        if (!g_connman) return;
88+
89         if (use_upnp) {
90-            StartMapPort();
91+            StartMapPort(g_connman->GetListenPort());

MarcoFalke commented at 5:08 pm on March 11, 2019:

In commit 8055e8770a4f43a6381542042d5e54d1a48f54e

This seems fragile. What happens if this interface method is called before CConnman::Start?

ryanofsky commented at 6:19 pm on March 12, 2019:

AppInitMain

ryanofsky commented at 6:30 pm on March 12, 2019:

re: #14856 (review)

I agree with Marco, this is more fragile. It doesn’t seem like it should be necessary to rely on connman here just to retrieve a static setting, and there isn’t much in the current code organization that would guarantee connman is initialized with the right port number before this call. I think it’d be better to bring back the GetListenPort() function and test and call it here and AppInitMain. This should also make the diff a little smaller.

MarcoFalke commented at 5:12 pm on March 11, 2019: member

utACK e4b08081520023d5f5c30e209a5ac2f1b45c4d1c, but I am not sure about that /interfaces/ change.

in src/init.cpp:1376 in e4b0808152 outdated

1372@@ -1373,9 +1373,16 @@ bool AppInitMain(InitInterfaces& interfaces)
1373     fDiscover = gArgs.GetBoolArg("-discover", true);
1374     fRelayTxes = !gArgs.GetBoolArg("-blocksonly", DEFAULT_BLOCKSONLY);
1375 
1376+    int listen_port = chainparams.GetDefaultPort();

MarcoFalke commented at 5:14 pm on March 11, 2019:

style-nit: Should use int64_t, otherwise it might wrap with GetArg?

0    int64_t listen_port = chainparams.GetDefaultPort();

MarcoFalke added the label Refactoring on Mar 11, 2019

fanquake added this to the "Blockers" column in a project

dongcarl force-pushed on Mar 14, 2019

dongcarl commented at 7:05 pm on March 14, 2019: member

Brought back deleted GetListenPort() function, checks port before casting.

in src/net.cpp:101 in 7e314526e7 outdated

 97+    int default_port = Params().GetDefaultPort();
 98+    assert((0 < default_port) && (default_port <= UINT16_MAX));
 99+    // An int that fits in uint16 always fits in int64_t
100+    int64_t listen_port = gArgs.GetArg("-port", default_port);
101+    // We don't support the reserved port 0 for system-allocated dynamic ports yet.
102+    assert((0 < listen_port) && (listen_port <= UINT16_MAX));

ryanofsky commented at 8:10 pm on March 14, 2019:

In commit “tor: pass in Tor control port rather than using globals” (7e314526e7c4bb5bbf0e351f92cab940158eae24)

It’s not a good idea to validate user input with an assert, because it causes the program to crash suddenly and different environments to display the error in confusing ways (in stderr, system logs, or popup windows, without the full context).

Simplest thing to do here would be to drop this assert, change the function return value to int64_t, and restore the error checking code in init.cpp where this function is called:

0if ((listen_port <= 0) || (0xffff < listen_port)) {
1    return InitError(strprintf(_("Invalid port specified in -port: '%i'"), listen_port));
2}

Keeping the other assert for default_port should be ok, since that value is compiled into the program, and if it is out of range it indicates there’s probably a bug.

ryanofsky commented at 8:21 pm on March 14, 2019: member

utACK 98855dc4e81e8bee8c98a21531994bee6d9aae49, except I don’t think you can use an assert to validate user input (see comment below). Changes since last review were restoring the GetListenPort() and moving some validation code into it.

ryanofsky commented at 10:10 pm on March 15, 2019: member

[15:29:49] <cfields> dongcarl / ryanofsky: there’s a lot of history in 14856 about GetDefaultPort() that I can’t quite follow. Maybe you could briefly explain why it’s still needed? [15:30:48] <cfields> Sorry, GetListenPort()

Reason for adding back GetListenPort() was to make GUI initialization less fragile. Querying connman for the port is less reliable than just reading the config value directly, because connman gets initialized relatively late, and querying it at the wrong time would silently return the wrong value. It also seemed nice to decouple portmapping code from connman. If you think it is better to query connman, though, that is fine with me. I actually just suggested this in response to Marco’s question #14856 (review).

promag commented at 11:29 pm on March 17, 2019: member

Concept ACK.

ryanofsky commented at 4:17 pm on March 18, 2019: member

re: http://www.erisian.com.au/bitcoin-core-dev/log-2019-03-15.html#l-403

[15:38:27] <cfields> Sure. As-is, the change seems like a lateral move at best :(

I don’t think this is true. Previously GetListenPort was called 4 different places: twice in network code, twice in init code. Now it is only called twice in init code and passed into network code. I think this is what you’d want under the original rationale for #11387, which was supporting multiple instantiations of network code to “test instances against themselves.”

practicalswift commented at 5:02 pm on March 18, 2019: contributor

Concept ACK

dongcarl removed this from the "Blockers" column in a project

dongcarl commented at 7:31 pm on March 20, 2019: member

Removed from high-priority for review as I need some time to think about how to structure this.

tor: pass in Tor control port rather than using globals

- Return int64_t from GetListenPort to avoid wrapping
- Validate and emit appropriate error for GetListenPort in init as it is user
  input
- Rather than adding to the existing unused parameters (threadGroup, scheduler),
  remove them and their header dependencies.

143a8debb2

net: pass listen port into CConnman

Instead of using GetListenPort(), we can now use the member passed in to
CConnman.

e42e3dde0d

net: pass port into MapPort/Discover rather than using globals

This also allows the port-less AddLocal overload to be removed.

8a774bb474

net: pass in default outgoing port 4302c8ba37

dongcarl force-pushed on Mar 26, 2019

ryanofsky approved

ryanofsky commented at 4:13 pm on March 29, 2019: member

utACK 4302c8ba37172ee365ed5d3dd380e3d025c91b0d. Only changes since last review are rebase and handling invalid listen ports better in first and third commits (as suggested #14856 (review))

in src/torcontrol.cpp:756 in 143a8debb2 outdated

752@@ -752,7 +753,7 @@ void StartTorControl()
753         return;
754     }
755 
756-    torControlThread = std::thread(std::bind(&TraceThread<void (*)()>, "torcontrol", &TorControlThread));
757+    torControlThread = std::thread(&TraceThread<std::function<void()> >, "torcontrol", [target, listen_port] { TorControlThread(target, listen_port); });

Empact commented at 6:33 am on April 3, 2019:

~~Do you want to capture target and listen_port by reference?~~

ryanofsky commented at 5:04 pm on April 5, 2019:

re: #14856 (review)

Do you want to capture target and listen_port by reference?

Definitely not listen_port because it will go out of scope. Probably not target because it is a reference to an object this has no control over. In C++14, the better thing would be to pass target by value then capture-by-move. But the current code is reasonable and the simplest correct implementation for C++11.

ryanofsky approved

DrahtBot commented at 10:21 am on June 5, 2019: member

DrahtBot added the label Needs rebase on Jun 5, 2019

dongcarl commented at 7:01 pm on August 14, 2019: member

Closed, need to rethink.

dongcarl closed this on Aug 14, 2019

laanwj removed the label Needs rebase on Oct 24, 2019

DrahtBot locked this on Dec 16, 2021

net: remove more CConnman globals (theuni) #14856

Conflicts