net: fix GetListenPort() to derive the proper port by vasild · Pull Request #20196 · bitcoin/bitcoin

vasild · 2020-10-20T10:01:49Z

GetListenPort() uses a simple logic: "if -port=P is given, then we
must be listening on P, otherwise we must be listening on 8333".
This is however not true if -bind= has been provided with :port part
or if -whitebind= has been provided. Thus, extend GetListenPort() to
return the port from -bind= or -whitebind=, if any.

Also, if -bind= is provided then we would bind only to a particular address
and should not add all the other addresses of the machine to the list of
local addresses.

Fixes #20184

DrahtBot · 2020-10-20T14:37:33Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#24455 (refactor: Split ArgsManager out of util/system by Empact)
#24273 (p2p: Split network logging into two categories p2p: Split network logging into two categories #24247 by anshu-khare-design)
#23561 (BIP324: Handshake prerequisites by dhruv)
#23233 (BIP324: Add encrypted p2p transport {de}serializer by dhruv)

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.

maflcko

left some style test nits

maflcko · 2020-10-21T14:40:21Z

test/functional/feature_bind_port_externalip.py

I believe this can be written shorter

Suggested change

found = False

for local in self.nodes[i].getnetworkinfo()['localaddresses']:

if local['address'] == '2.2.2.2':

assert_equal(local['port'], expected_port)

found = True

break

assert(found)

local = next(l for l in self.nodes[i].getnetworkinfo()['localaddresses'] if l['address'] == '2.2.2.2')

assert_equal(local['port'], expected_port)

To me that is less readable. Maybe my python level is poor, but then maybe other readers of this are also poor pythoners. I prefer to keep it simple stupid:

for ... if ... break

that is nearly the same in any programming language.

A few micro-optimisations

+LEN_EXPECTED = len(EXPECTED) class BindPortExternalIPTest(BitcoinTestFramework): def set_test_params(self): # Avoid any -bind= on the command line. Force the framework to avoid adding -bind=127.0.0.1. self.setup_clean_chain = True self.bind_to_localhost_only = False - self.num_nodes = len(EXPECTED) + self.num_nodes = LEN_EXPECTED self.extra_args = list(map(lambda e: e[0], EXPECTED)) def add_options(self, parser): @@ -58,11 +59,13 @@ class BindPortExternalIPTest(BitcoinTestFramework): "to one of the interfaces on this machine and rerun with --ihave1111".format(ADDR)) def run_test(self): - for i in range(len(EXPECTED)): - if EXPECTED[i][1] == 'default_p2p_port': + self.log.info("Test the proper port is used for -externalip=") + for i in range(LEN_EXPECTED): + port = EXPECTED[i][1] + if port == 'default_p2p_port': expected_port = p2p_port(i) else: - expected_port = EXPECTED[i][1] + expected_port = port

test/functional/feature_bind_port_externalip.py

src/init.cpp

src/test/net_tests.cpp

vasild · 2020-10-22T11:25:21Z

Addressed review suggestions and added a test to cover case 4. from #20184.

test/functional/feature_bind_port_discover.py

vasild · 2020-10-22T12:34:37Z

Addressed review suggestions.

vasild · 2020-10-22T16:32:54Z

Fixed linter error feature_bind_port_discover.py:25:18: E703 statement ends with a semicolon 🤦

test/functional/feature_bind_port_externalip.py

vasild · 2020-12-17T17:34:13Z

Rebased and changed port numbers used in the functional tests.

src/test/net_tests.cpp

jonatack · 2022-02-11T14:42:46Z

Tagged for v23, needs rebase.

vasild · 2022-02-11T15:34:26Z

cb86c1fd20...38f49fc11d: rebase due to conflicts (thanks for the poke!)

Invalidates ACK from @jonatack

Previously invalidated ACK from @sipa

jonatack

re-ACK 38f49fc11d1d184a6c804d46340e4f91f47901df

src/init.cpp

vasild · 2022-02-14T08:15:02Z

38f49fc11d...35ec977b06: rebase and use /*fAllowLookup=*/

Invalidates ACK from @jonatack

Previously invalidated ACK from @sipa

jonatack · 2022-02-14T09:56:22Z

re-ACK 35ec977b067d05c318b68b4eafbf708870ba9d3f

jonatack · 2022-03-01T14:58:06Z

This has been open for a year and half, has seen a fair amount of review, has ACKs by myself and @sipa, and is tagged for 23.0, if anyone else would like to have a look!

Add a new function `TestOnlyResetTimeData()` which would reset the internal state used by `GetTimeOffset()`, `GetAdjustedTime()` and `AddTimeData()`. This is needed so that unit tests that call `AddTimeData()` can restore the state in order not to confuse other tests that rely on it. Currently `timedata_tests/addtimedata` is the only test that modifies the state (via `AddTimeData()`) and also the only test that relies on that state.

Rename the local variables in `src/timedata.cpp`: `setKnown` -> `g_sources` `vTimeOffsets` -> `g_time_offsets` `fDone` -> `g_warning_emitted`

Rename `CaptureMessage()` to `CaptureMessageToFile()` and introduce a `std::function` variable called `CaptureMessage` whose value can be changed by unit tests, should they need to inspect message contents.

Span is lightweight and need not be passed by const reference.

`GetListenPort()` uses a simple logic: "if `-port=P` is given, then we must be listening on `P`, otherwise we must be listening on `8333`". This is however not true if `-bind=` has been provided with `:port` part or if `-whitebind=` has been provided. Thus, extend `GetListenPort()` to return the port from `-bind=` or `-whitebind=`, if any. Fixes bitcoin#20184 (cases 1. 2. 3. 5.)

If `-bind=` is provided then we would bind only to a particular address and should not add all the other addresses of the machine to the list of local addresses. Fixes bitcoin#20184 (case 4.)

vasild · 2022-03-02T14:54:23Z

35ec977b06...7d64ea4a01: rebase due to conflicts

Invalidates ACK from @jonatack

Previously invalidated ACK from @sipa

sipa · 2022-03-02T15:06:47Z

utACK 7d64ea4. I didn't review the tests in detail.

jonatack

re-ACK 7d64ea4 per git range-diff 08bcfa27 35ec977 7d64ea4, changes are rebase-only, light re-review, re-ran the new tests locally

ghost · 2022-03-02T16:46:30Z

Concept ACK

fanquake added the P2P label Oct 20, 2020

maflcko modified the milestone: 0.21.1 Oct 20, 2020

vasild force-pushed the fix_GetListenPort branch from 202d8df to e6c3de7 Compare October 20, 2020 10:52

This was referenced Oct 20, 2020

Refactoring and minor improvement for self-advertisements #19843

Closed

net: Add NAT-PMP port forwarding support #18077

Merged

maflcko reviewed Oct 21, 2020

View reviewed changes

vasild force-pushed the fix_GetListenPort branch from e6c3de7 to 1ae0d40 Compare October 22, 2020 11:21

maflcko reviewed Oct 22, 2020

View reviewed changes

test/functional/feature_bind_port_discover.py Outdated Show resolved Hide resolved

vasild force-pushed the fix_GetListenPort branch from 1ae0d40 to 15a6a59 Compare October 22, 2020 12:32

vasild force-pushed the fix_GetListenPort branch from 15a6a59 to 053ba08 Compare October 22, 2020 16:31

vasild commented Oct 23, 2020

View reviewed changes

test/functional/feature_bind_port_externalip.py Outdated Show resolved Hide resolved

This was referenced Oct 25, 2020

net: don't bind on 0.0.0.0 if binds are restricted to Tor #20234

Merged

[tests] Allow outbound & block-relay-only connections in functional tests. #19315

Merged

net: Add blockfilters white{bind,list} permission flag #18972

Closed

This was referenced Nov 3, 2020

Add BIP324 encrypted p2p transport de-/serializer (only used in tests) #18242

Closed

Disable and fix tests for when BDB is not compiled #20267

Merged

DrahtBot mentioned this pull request Nov 24, 2020

net: Add unit testing of node eviction logic #20477

Merged

DrahtBot added the Needs rebase label Dec 16, 2020

vasild force-pushed the fix_GetListenPort branch from 053ba08 to 3e55fbf Compare December 17, 2020 17:31

DrahtBot removed the Needs rebase label Dec 17, 2020

DrahtBot mentioned this pull request Jan 6, 2021

net: Move SocketSendData lock annotation to header #20864

Merged

DrahtBot added the Needs rebase label Jan 7, 2021

maflcko reviewed Jan 13, 2021

View reviewed changes

src/test/net_tests.cpp Outdated Show resolved Hide resolved

vasild force-pushed the fix_GetListenPort branch from 3e55fbf to 5647ca3 Compare January 14, 2021 13:03

laanwj added this to the 23.0 milestone Feb 10, 2022

vasild force-pushed the fix_GetListenPort branch from cb86c1f to 38f49fc Compare February 11, 2022 15:33

DrahtBot removed the Needs rebase label Feb 11, 2022

jonatack reviewed Feb 12, 2022

View reviewed changes

src/init.cpp Outdated Show resolved Hide resolved

This was referenced Feb 13, 2022

p2p: Split network logging into two categories #24247 #24273

Closed

net: open p2p connections to nodes that listen on non-default ports #23542

Merged

vasild force-pushed the fix_GetListenPort branch from 38f49fc to 35ec977 Compare February 14, 2022 08:14

DrahtBot mentioned this pull request Mar 2, 2022

refactor: Split ArgsManager out of util/system #24455

Closed

DrahtBot added the Needs rebase label Mar 2, 2022

vasild added 6 commits March 2, 2022 15:40

timedata: rename variables to match the coding style

43868ba

Rename the local variables in `src/timedata.cpp`: `setKnown` -> `g_sources` `vTimeOffsets` -> `g_time_offsets` `fDone` -> `g_warning_emitted`

net: make CaptureMessage() mockable

3cb9d9c

Rename `CaptureMessage()` to `CaptureMessageToFile()` and introduce a `std::function` variable called `CaptureMessage` whose value can be changed by unit tests, should they need to inspect message contents.

net: pass Span by value to CaptureMessage()

f98cdcb

Span is lightweight and need not be passed by const reference.

net: only assume all local addresses if listening on any

7d64ea4

If `-bind=` is provided then we would bind only to a particular address and should not add all the other addresses of the machine to the list of local addresses. Fixes bitcoin#20184 (case 4.)

vasild force-pushed the fix_GetListenPort branch from 35ec977 to 7d64ea4 Compare March 2, 2022 14:53

DrahtBot removed the Needs rebase label Mar 2, 2022

jonatack reviewed Mar 2, 2022

View reviewed changes

laanwj merged commit 30308cc into bitcoin:master Mar 3, 2022

vasild deleted the fix_GetListenPort branch March 3, 2022 12:50

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Mar 3, 2022

Merge bitcoin#20196: net: fix GetListenPort() to derive the proper port

93f6b84

bitcoin locked and limited conversation to collaborators Mar 3, 2023

-            found = False
-            for local in self.nodes[i].getnetworkinfo()['localaddresses']:
-                if local['address'] == '2.2.2.2':
-                    assert_equal(local['port'], expected_port)
-                    found = True
-                    break
-            assert(found)
+            local = next(l for l in self.nodes[i].getnetworkinfo()['localaddresses'] if l['address'] == '2.2.2.2')
+            assert_equal(local['port'], expected_port)

Conversation

vasild commented Oct 20, 2020

Uh oh!

DrahtBot commented Oct 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

maflcko left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko Oct 21, 2020

Choose a reason for hiding this comment

Uh oh!

vasild Oct 22, 2020

Choose a reason for hiding this comment

Uh oh!

jonatack Mar 17, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vasild commented Oct 22, 2020

Uh oh!

Uh oh!

vasild commented Oct 22, 2020

Uh oh!

vasild commented Oct 22, 2020

Uh oh!

Uh oh!

vasild commented Dec 17, 2020

Uh oh!

Uh oh!

jonatack commented Feb 11, 2022

Uh oh!

vasild commented Feb 11, 2022

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vasild commented Feb 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonatack commented Feb 14, 2022

Uh oh!

jonatack commented Mar 1, 2022

Uh oh!

vasild commented Mar 2, 2022

Uh oh!

sipa commented Mar 2, 2022

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Mar 2, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

DrahtBot commented Oct 20, 2020 •

edited

Loading

maflcko left a comment •

edited

Loading

vasild commented Feb 14, 2022 •

edited

Loading