Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 6 commits into from
Mar 3, 2022

Conversation

vasild
Copy link
Contributor

@vasild vasild commented Oct 20, 2020

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

@fanquake fanquake added the P2P label Oct 20, 2020
@maflcko maflcko modified the milestone: 0.21.1 Oct 20, 2020
@vasild vasild force-pushed the fix_GetListenPort branch from 202d8df to e6c3de7 Compare October 20, 2020 10:52
@DrahtBot
Copy link
Contributor

DrahtBot commented Oct 20, 2020

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

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.

Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left some style test nits

Comment on lines 67 to 72
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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
test/functional/feature_bind_port_externalip.py Outdated Show resolved Hide resolved
src/init.cpp Outdated Show resolved Hide resolved
src/test/net_tests.cpp Outdated Show resolved Hide resolved
src/test/net_tests.cpp Outdated Show resolved Hide resolved
@vasild vasild force-pushed the fix_GetListenPort branch from e6c3de7 to 1ae0d40 Compare October 22, 2020 11:21
@vasild
Copy link
Contributor Author

vasild commented Oct 22, 2020

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

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

vasild commented Oct 22, 2020

Addressed review suggestions.

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

vasild commented Oct 22, 2020

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

@vasild
Copy link
Contributor Author

vasild commented Dec 17, 2020

Rebased and changed port numbers used in the functional tests.

src/test/net_tests.cpp Outdated Show resolved Hide resolved
@vasild vasild force-pushed the fix_GetListenPort branch from 3e55fbf to 5647ca3 Compare January 14, 2021 13:03
@laanwj laanwj added this to the 23.0 milestone Feb 10, 2022
@jonatack
Copy link
Member

Tagged for v23, needs rebase.

@vasild
Copy link
Contributor Author

vasild commented Feb 11, 2022

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

Invalidates ACK from @jonatack

Previously invalidated ACK from @sipa

Copy link
Member

@jonatack jonatack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re-ACK 38f49fc

src/init.cpp Outdated Show resolved Hide resolved
@vasild
Copy link
Contributor Author

vasild commented Feb 14, 2022

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

Invalidates ACK from @jonatack

Previously invalidated ACK from @sipa

@jonatack
Copy link
Member

re-ACK 35ec977

@jonatack
Copy link
Member

jonatack commented Mar 1, 2022

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!

vasild added 6 commits March 2, 2022 15:40
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 vasild force-pushed the fix_GetListenPort branch from 35ec977 to 7d64ea4 Compare March 2, 2022 14:53
@vasild
Copy link
Contributor Author

vasild commented Mar 2, 2022

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

Invalidates ACK from @jonatack

Previously invalidated ACK from @sipa

@sipa
Copy link
Member

sipa commented Mar 2, 2022

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

Copy link
Member

@jonatack jonatack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ghost
Copy link

ghost commented Mar 2, 2022

Concept ACK

@laanwj laanwj merged commit 30308cc into bitcoin:master Mar 3, 2022
@vasild 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
@bitcoin bitcoin locked and limited conversation to collaborators Mar 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong listening port could be assumed resulting in bogus self-advertise
9 participants