-
Notifications
You must be signed in to change notification settings - Fork 36.8k
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
Conversation
202d8df
to
e6c3de7
Compare
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, 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. |
There was a problem hiding this 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
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) |
There was a problem hiding this comment.
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
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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
e6c3de7
to
1ae0d40
Compare
Addressed review suggestions and added a test to cover case |
1ae0d40
to
15a6a59
Compare
Addressed review suggestions. |
15a6a59
to
053ba08
Compare
Fixed linter error |
053ba08
to
3e55fbf
Compare
Rebased and changed port numbers used in the functional tests. |
3e55fbf
to
5647ca3
Compare
Tagged for v23, needs rebase. |
cb86c1f
to
38f49fc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re-ACK 38f49fc
38f49fc
to
35ec977
Compare
re-ACK 35ec977 |
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.)
35ec977
to
7d64ea4
Compare
utACK 7d64ea4. I didn't review the tests in detail. |
There was a problem hiding this 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
Concept ACK |
GetListenPort()
uses a simple logic: "if-port=P
is given, then wemust be listening on
P
, otherwise we must be listening on8333
".This is however not true if
-bind=
has been provided with:port
partor if
-whitebind=
has been provided. Thus, extendGetListenPort()
toreturn the port from
-bind=
or-whitebind=
, if any.Also, if
-bind=
is provided then we would bind only to a particular addressand should not add all the other addresses of the machine to the list of
local addresses.
Fixes #20184