Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
cli: Detect port errors in rpcconnect and rpcport #29521
base: master
Are you sure you want to change the base?
cli: Detect port errors in rpcconnect and rpcport #29521
Changes from all commits
e208fb5
24bc46c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 added this line and it seemed to pass
looks like
SplitHostPort
does not throw an exception if there are two colons and no port providedI am unsure if that is needed logic in
SplitHostPort
I'm seeing the same error when I do
self.nodes[0].cli("-rpcconnect=127.0.0.256")
probably because 255 is the max value
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.
Thanks @kevkevinpal. Good example of why review is valued. Your findings nudged me to add some assert test statements (which otherwise wouldn't be present) to help cover cases involving IPv6 hosts.
We're experiencing the result of the scope of
SplitHostPort()
.SplitHostPort()
extracts the host and port. Although the port is validated, host is not (and the function declaration comment header mentions validating the port, but doesn't mention validating the host).bitcoin/src/util/strencodings.h
Lines 97 to 106 in 7fee0ca
SplitHostPort()
is treating "127.0.0.1::" as an IPv6 host since "::" is used for IPv6 address shortening.SplitHostPort()
detects no port as part of the input string, which would be whyCould not connect to the server 127.0.0.1:::18443
is the error provided forsrc/bitcoin-cli -rpcconnect=127.0.0.1:: getblockcount
(the ":18443" is tacked on the end because it's the default port for regtest).An enhancement would be to perform a validation of the host received from
SplitHostPort()
before bitcoin-cli attempts to contact the host. I'm thinking we would need to first define what is "valid" and what is "invalid" for use with bitcoin-cli. The gist of this kind of checking seems to be present inCNetAddr
. In general, I would think bitcoin-cli shouldn't be overly restrictive in what it lets the user attempt to use as a host.bitcoin/src/netaddress.h
Lines 157 to 181 in 7fee0ca
For now, it might make the most sense to leave the enhancement of host validation to a new PR since this PR is focused on port error detection.