cli: Detect port errors in rpcconnect and rpcport #29521

tdb3 commented at 10:11 PM on February 29, 2024: contributor

Adds invalid port detection to bitcoin-cli for -rpcconnect and -rpcport.

In addition to detecting malformed/invalid ports (e.g. those outside of the 16-bit port range, not numbers, etc.), bitcoin-cli also now considers usage of port 0 to be invalid. bitcoin-cli previously considered port 0 to be valid and attempted to use it to reach bitcoind.

Functional tests were added for invalid port detection as well as port prioritization. Additionally, a warning is provided when a port is specified in both -rpcconnect and -rpcport.

This PR is an alternate approach to PR #27820 (e.g. SplitHostPort is unmodified).

Considered an alternative to 127.0.0.1 being specified in functional tests, but at first glance, this might need an update to test_framework/util.py (e.g. rpc_url), which might be left to a future PR.

DrahtBot commented at 10:11 PM on February 29, 2024: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	cbergqvist, S3RK, achow101
Stale ACK	rkrux, davidgumberg, marcofleon

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

DrahtBot added the label Scripts and tools on Feb 29, 2024

in src/bitcoin-cli.cpp:765 in 6052f49150 outdated

 762 | +        }
 763 | +
 764 | +        std::optional<std::string> rpcport_arg = gArgs.GetArg("-rpcport");
 765 | +        if (rpcport_arg.has_value()) {
 766 | +            uint16_t n{0};
 767 | +            if (!ParseUInt16(rpcport_arg.value(), &n) || n == 0) {

maflcko commented at 9:30 AM on March 1, 2024:

Is there any downside to use ToIntegral for new code?

tdb3 commented at 5:45 PM on March 1, 2024:

Thanks @maflcko. Rebased and updated to the new style of using ToIntegral to better align with doc/developer-notes.

tdb3 force-pushed on Mar 1, 2024

in test/functional/interface_bitcoin_cli.py:117 in e73e76affb outdated

 107 | @@ -107,6 +108,23 @@ def run_test(self):
 108 |          self.log.info("Test connecting to a non-existing server")
 109 |          assert_raises_process_error(1, "Could not connect to the server", self.nodes[0].cli('-rpcport=1').echo)
 110 |  
 111 | +        self.log.info("Test rpcconnect handling of invalid ports")
 112 | +        assert_raises_process_error(1, "Invalid port provided in -rpcconnect: 127.0.0.1:notaport", self.nodes[0].cli("-rpcconnect=127.0.0.1:notaport").echo)
 113 | +        assert_raises_process_error(1, "Invalid port provided in -rpcconnect: 127.0.0.1:-1", self.nodes[0].cli("-rpcconnect=127.0.0.1:-1").echo)
 114 | +        assert_raises_process_error(1, "Invalid port provided in -rpcconnect: 127.0.0.1:0", self.nodes[0].cli("-rpcconnect=127.0.0.1:0").echo)
 115 | +

davidgumberg commented at 6:37 PM on March 11, 2024:

Nice to have: a check for oversized ports

+ assert_raises_process_error(1, "Invalid port provided in -rpcconnect: 127.0.0.1:65536", self.nodes[0].cli("-rpcconnect=127.0.0.1:65536").echo)
+

Below as well

tdb3 commented at 9:47 PM on March 12, 2024:

Thanks. Added. Testing upperbound makes sense.

in test/functional/interface_bitcoin_cli.py:158 in 46c91a25d9 outdated

 122 | +        node_rpc_port = rpc_port(self.nodes[0].index)
 123 | +        assert_raises_process_error(1, "Could not connect to the server", self.nodes[0].cli(f"-rpcconnect=127.0.0.1:{node_rpc_port}", "-rpcport=1").echo)
 124 | +        assert_equal(BLOCKS, self.nodes[0].cli("-rpcconnect=127.0.0.1:18999", f'-rpcport={node_rpc_port}').getblockcount())
 125 | +        assert_equal(BLOCKS, self.nodes[0].cli(f"-rpcconnect=127.0.0.1:{node_rpc_port}").getblockcount())
 126 | +        assert_equal(BLOCKS, self.nodes[0].cli(f'-rpcport={node_rpc_port}').getblockcount())
 127 | +

davidgumberg commented at 7:07 PM on March 11, 2024:

Also nice to have would be a test when no port is provided that we fall back to BaseParams().RPCPort(), it isn't obvious to me how that could be done here though.

tdb3 commented at 9:48 PM on March 12, 2024:

Added test for default port in regtest.

Thank you for drawing attention to this! Looks like explicitly defined -rpcport during the creation of the node's bitcoin.conf was causing bitcoin-cli to use the conf-provided rpcport when using rpcconnect without rpcport in command line arguments. One of the tests (involving f"-rpcconnect=127.0.0.1:{node_rpc_port}") would always pass because of this (i.e. the test wouldn't fail even if a valid but unused port is specified in rpcconnect). This was verified by creating a temporary test f"-rpcconnect=127.0.0.1:1234" that should have failed but didn't.

This was remedied by using replace_in_config().

davidgumberg commented at 7:11 PM on March 11, 2024: contributor

crACK https://github.com/bitcoin/bitcoin/pull/29521/commits/46c91a25d93595e3934418b1001fabe906326061

tdb3 force-pushed on Mar 12, 2024

tdb3 commented at 9:46 PM on March 12, 2024: contributor

Thanks for the review @davidgumberg! Added the suggestions (force pushed). Responses inline.

in src/bitcoin-cli.cpp:770 in 6e779d8323 outdated

 767 | +            if (!rpcport_port.has_value() || rpcport_port.value() == 0) {
 768 | +                throw std::runtime_error(strprintf("Invalid port provided in -rpcport: %s", rpcport_arg.value()));
 769 | +            }
 770 | +
 771 | +            // Use the valid port provided
 772 | +            port = rpcport_port.value();

cbergqvist commented at 9:28 AM on March 29, 2024:

Style: narrow scope of rpcport_arg since it is only used inside the if-block. (In this case the outer scope ends it anyway, this is just a general rule of thumb).

Naming: rpcport_port -> rpcport_int since the former is a bit repetitive and the latter at least describes its purpose somewhat, despite being a bit Hungarian.

        if (std::optional<std::string> rpcport_arg = gArgs.GetArg("-rpcport")) {
            std::optional<uint16_t> rpcport_int{ToIntegral<uint16_t>(rpcport_arg.value())};
            if (!rpcport_int.has_value() || rpcport_int.value() == 0) {
                throw std::runtime_error(strprintf("Invalid port provided in -rpcport: %s", rpcport_arg.value()));
            }

            // Use the valid port provided
            port = rpcport_int.value();

tdb3 commented at 1:34 AM on March 30, 2024:

Good suggestions. I've implemented these changes and added a small comment to increase readability (since operator bool of std::optional is being used).

cbergqvist approved

cbergqvist commented at 12:55 PM on March 29, 2024: contributor

ACK 6e779d83232025ba83e2bd79bd07d2299410193d

Testing

Ran ./test/functional/test_runner.py --extended and got two failures feature_index_prune.py and wallet_importdescriptors.py --descriptors. Reran on the base of the PR, 1105aa46dd1008c556b8c435f1efcb9be09a1644 and got both failures, so shouldn't be related. Will investigate further.

Code

I like how you limited the scope of variables introduced in this PR, I wish fewer lines were needed for the added error detection, but I didn't figure out a way to shrink it either.

Some would suggest breaking out to a function, but since it's only used here I agree it's better to inline it.

DrahtBot requested review from davidgumberg on Mar 29, 2024

tdb3 force-pushed on Mar 30, 2024

tdb3 commented at 1:34 AM on March 30, 2024: contributor

Thank you for the review @cbergqvist! Implemented your suggestions and rebased.

in src/bitcoin-cli.cpp:746 in cb4f9fc584 outdated

 746 | @@ -747,8 +747,35 @@ static UniValue CallRPC(BaseRequestHandler* rh, const std::string& strMethod, co
 747 |      //     2. port in -rpcconnect (ie following : in ipv4 or ]: in ipv6)
 748 |      //     3. default port for chain
 749 |      uint16_t port{BaseParams().RPCPort()};
 750 | -    SplitHostPort(gArgs.GetArg("-rpcconnect", DEFAULT_RPCCONNECT), port, host);
 751 | -    port = static_cast<uint16_t>(gArgs.GetIntArg("-rpcport", port));
 752 | +    {

rkrux commented at 7:59 AM on March 30, 2024:

Curious as to why all this is inside a block - so that the variables declared inside are not exposed for the rest of the function?

tdb3 commented at 12:18 PM on March 30, 2024:

yes, it enforces scope. Not strictly necessary, but will leave it for now.

in src/bitcoin-cli.cpp:755 in cb4f9fc584 outdated

 753 | +        uint16_t rpcconnect_port{0};
 754 | +        const std::string rpcconnect_str = gArgs.GetArg("-rpcconnect", DEFAULT_RPCCONNECT);
 755 | +        if (!SplitHostPort(rpcconnect_str, rpcconnect_port, host)) {
 756 | +            throw std::runtime_error(strprintf("Invalid port provided in -rpcconnect: %s", rpcconnect_str));
 757 | +        } else {
 758 | +            if (rpcconnect_port != 0) {

rkrux commented at 8:03 AM on March 30, 2024:

Interesting to find out that why the use of port number 0 is discouraged: https://www.lifewire.com/port-0-in-tcp-and-udp-818145

cbergqvist commented at 11:38 AM on March 30, 2024:

Interesting article. Wonder what the behavior is before this PR when specifying 0..

tdb3 commented at 11:54 AM on March 30, 2024:

$ src/bitcoin-cli -rpcport=0 getblockchaininfo
error: timeout on transient error: Could not connect to the server 127.0.0.1:0

Make sure the bitcoind server is running and that you are connecting to the correct RPC port.

$ src/bitcoin-cli -rpcconnect=127.0.0.1:0 getblockchaininfo
error: timeout on transient error: Could not connect to the server 127.0.0.1:0

Make sure the bitcoind server is running and that you are connecting to the correct RPC port.

The client tries to reach port 0, which wouldn't be successful.

cbergqvist commented at 9:33 PM on March 30, 2024:

Ah yeah, makes sense for the client. 👍

rkrux approved

rkrux commented at 8:05 AM on March 30, 2024: contributor

ACK cb4f9fc

Testing Methodolgy:

Ran make, which was successful.
Ran functional tests, all passed.
Tried the following cli commands in terminal while the daemon was running:

➜  bitcoin git:(20240229_rpcconnectinvalidportdetection) ✗ bcli -rpcconnect=127.0.0.1:notaport help
error: Invalid port provided in -rpcconnect: 127.0.0.1:notaport
➜  bitcoin git:(20240229_rpcconnectinvalidportdetection) ✗ bcli -rpcconnect=127.0.0.1:-1 help
error: Invalid port provided in -rpcconnect: 127.0.0.1:-1
➜  bitcoin git:(20240229_rpcconnectinvalidportdetection) ✗ bcli -rpcconnect=127.0.0.1:65536 help
error: Invalid port provided in -rpcconnect: 127.0.0.1:65536
➜  bitcoin git:(20240229_rpcconnectinvalidportdetection) ✗ bcli -rpcport=notaport help
error: Invalid port provided in -rpcport: notaport
➜  bitcoin git:(20240229_rpcconnectinvalidportdetection) ✗ bcli -rpcport=-1 help
error: Invalid port provided in -rpcport: -1

➜  bitcoin git:(20240229_rpcconnectinvalidportdetection) ✗ bcli -rpcconnect=127.0.0.1:9000 -rpcport=8332 getblockcount
Warning: Port specified in both -rpcconnect and -rpcport. Using -rpcport 8332
448483

DrahtBot requested review from cbergqvist on Mar 30, 2024

tdb3 commented at 11:50 AM on March 30, 2024: contributor

ACK cb4f9fc

Thank you for the review and testing @rkrux! Will add a note to the description about usage of port 0 now considered invalid.

cbergqvist approved

cbergqvist commented at 9:32 PM on March 30, 2024: contributor

ACK cb4f9fc5847a7e53fe45d54b1fddf504dee5af82

Verified coverage by changing the new C++ code-block to always throw and ran test/functional/interface_bitcoin_cli.py, confirming the exception message, reset to only PR changes and re-ran test/functional/interface_bitcoin_cli.py successfully.

Ran ./test/functional/test_runner.py --extended --jobs=16 --timeout-factor=1 and saw the same unrelated tests error out as on the previous version of this PR. Still investigating.

marcofleon commented at 11:07 PM on April 2, 2024: contributor

Tested ACK. The new error handling code looks good to me upon review. Ran the daemon and gave invalid values for rpcconnect and rpcport and the proper error messages came up. The behavior of port 0 being invalid is consistent with src/init.cpp. Ran interface_bitcoin_cli.py too and there were no issues with the added tests.

tdb3 commented at 11:09 PM on April 2, 2024: contributor

Tested ACK. The new error handling code looks good to me upon review. Ran the daemon and gave invalid values for rpcconnect and rpcport and the proper error messages came up. The behavior of port 0 being invalid is consistent with src/init.cpp. Ran interface_bitcoin_cli.py too and there were no issues with the added tests.

Thank you for reviewing, @marcofleon!

davidgumberg commented at 5:16 PM on April 3, 2024: contributor

ACK https://github.com/bitcoin/bitcoin/pull/29521/commits/cb4f9fc5847a7e53fe45d54b1fddf504dee5af82

The changes in bitcoin-cli.cpp since the last ACK look good to me, and the test coverage is more extensive.

Tested with make check and running the functional test suite.

I also verified that passing bad ports to rpcconnect and rpcport trigger the warning.

DrahtBot requested review from marcofleon on Apr 3, 2024

in test/functional/interface_bitcoin_cli.py:136 in caa5242f84 outdated

 131 | +        # prefer rpcconnect port over default
 132 | +        assert_equal(BLOCKS, self.nodes[0].cli(f"-rpcconnect=127.0.0.1:{node_rpc_port}").getblockcount())
 133 | +        # prefer rpcport over default
 134 | +        assert_equal(BLOCKS, self.nodes[0].cli(f'-rpcport={node_rpc_port}').getblockcount())
 135 | +        # use default if nothing else available
 136 | +        assert_raises_process_error(1, "Could not connect to the server 127.0.0.1:18443", self.nodes[0].cli("-rpcconnect=127.0.0.1").echo)

achow101 commented at 10:29 PM on April 22, 2024:

In caa5242f84a6e1ef90b8a44c1206c7b5b942a00c "cli: Sanitize ports in rpcconnect and rpcport"

This will fail if you just have a regtest node running independent of the test:

2024-04-22T22:30:22.751000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
  File "/home/ava/bitcoin/bitcoin/29521/test/functional/test_framework/util.py", line 122, in assert_raises_process_error
    fun(*args, **kwds)
  File "/home/ava/bitcoin/bitcoin/29521/test/functional/test_framework/test_node.py", line 821, in __call__
    return self.cli.send_cli(self.command, *args, **kwargs)
  File "/home/ava/bitcoin/bitcoin/29521/test/functional/test_framework/test_node.py", line 886, in send_cli
    raise subprocess.CalledProcessError(returncode, self.binary, output=cli_stderr)
subprocess.CalledProcessError: Command '/home/ava/bitcoin/bitcoin/29521/src/bitcoin-cli' returned non-zero exit status 1.

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/ava/bitcoin/bitcoin/29521/test/functional/test_framework/test_framework.py", line 132, in main
    self.run_test()
  File "/home/ava/bitcoin/bitcoin/29521/test/functional/interface_bitcoin_cli.py", line 136, in run_test
    assert_raises_process_error(1, "Could not connect to the server 127.0.0.1:18443", self.nodes[0].cli("-rpcconnect=127.0.0.1").echo)
  File "/home/ava/bitcoin/bitcoin/29521/test/functional/test_framework/util.py", line 127, in assert_raises_process_error
    raise AssertionError("Expected substring not found:" + e.output)
AssertionError: Expected substring not found:error: Authorization failed: Incorrect rpcuser or rpcpassword

tdb3 commented at 1:11 AM on April 23, 2024:

Thanks for taking a look and trying it out @achow101. Much appreciated.

Good point. Yes, the test would fail if an independent node is running on 18443.

Although not ideal, do you think this would be reasonable since a warning is provided when bitcoind is running independently? (i.e. WARNING! There is already a bitcoind process running on this system. Tests may fail unexpectedly due to resource contention!)

Alternatively, the assert statement could be removed, but that seems a bit lacking, since it seems good to check for default port usage (even if the current check is limited to the default regtest port).

Could also look into refactoring a bit, but that might lead to touching a bit more code than desirable.

Thoughts?

maflcko commented at 4:51 AM on April 23, 2024:

I'd say it should be avoided to use test outside of the allowed range assigned to this test case. Each test is assigned 12, or so, p2p and rpc ports. I think this test can be dropped, because if the default behavior changed, pretty much every developer would notice quickly anyway?

tdb3 commented at 1:15 AM on April 24, 2024:

Yes, that sounds reasonable. Overall, port error checking and associated functional test cases introduce an improvement. Removed the "18443" test and pushed.

tdb3 force-pushed on Apr 24, 2024

in test/functional/interface_bitcoin_cli.py:116 in 6c17bdeb70 outdated

 107 | @@ -107,6 +108,33 @@ def run_test(self):
 108 |          self.log.info("Test connecting to a non-existing server")
 109 |          assert_raises_process_error(1, "Could not connect to the server", self.nodes[0].cli('-rpcport=1').echo)
 110 |  
 111 | +        self.log.info("Test handling of invalid ports in rpcconnect")
 112 | +        assert_raises_process_error(1, "Invalid port provided in -rpcconnect: 127.0.0.1:notaport", self.nodes[0].cli("-rpcconnect=127.0.0.1:notaport").echo)
 113 | +        assert_raises_process_error(1, "Invalid port provided in -rpcconnect: 127.0.0.1:-1", self.nodes[0].cli("-rpcconnect=127.0.0.1:-1").echo)
 114 | +        assert_raises_process_error(1, "Invalid port provided in -rpcconnect: 127.0.0.1:0", self.nodes[0].cli("-rpcconnect=127.0.0.1:0").echo)
 115 | +        assert_raises_process_error(1, "Invalid port provided in -rpcconnect: 127.0.0.1:65536", self.nodes[0].cli("-rpcconnect=127.0.0.1:65536").echo)

kevkevinpal commented at 2:49 PM on April 26, 2024:

I added this line and it seemed to pass

assert_raises_process_error(1, 'timeout on transient error: Could not connect to the server 127.0.0.1:::', self.nodes[0].cli("-rpcconnect=127.0.0.1::").echo)

looks like SplitHostPort does not throw an exception if there are two colons and no port provided

I 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

tdb3 commented at 10:18 PM on April 27, 2024:

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).

https://github.com/bitcoin/bitcoin/blob/7fee0ca014b1513e836d2550d1b7512739d5a79a/src/util/strencodings.h#L97-L106

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 why Could not connect to the server 127.0.0.1:::18443 is the error provided for src/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 in CNetAddr. In general, I would think bitcoin-cli shouldn't be overly restrictive in what it lets the user attempt to use as a host.

https://github.com/bitcoin/bitcoin/blob/7fee0ca014b1513e836d2550d1b7512739d5a79a/src/netaddress.h#L157-L181

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.

tdb3 force-pushed on Apr 27, 2024

in test/functional/interface_bitcoin_cli.py:119 in 867a673bcf outdated

 114 | +        assert_raises_process_error(1, "Invalid port provided in -rpcconnect: 127.0.0.1:-1", self.nodes[0].cli("-rpcconnect=127.0.0.1:-1").echo)
 115 | +        assert_raises_process_error(1, "Invalid port provided in -rpcconnect: 127.0.0.1:0", self.nodes[0].cli("-rpcconnect=127.0.0.1:0").echo)
 116 | +        assert_raises_process_error(1, "Invalid port provided in -rpcconnect: 127.0.0.1:65536", self.nodes[0].cli("-rpcconnect=127.0.0.1:65536").echo)
 117 | +
 118 | +        self.log.info("Check for ipv6")
 119 | +        have_ipv6 = test_ipv6_local()

cbergqvist commented at 9:34 PM on May 8, 2024:

(IMO it's slightly cleaner to split the test into --ipv4 and --ipv6 variants similar to rpc_bind.py, that way it becomes explicit whether that part of the test coverage is being skipped or not. This being more of a parsing-test, I guess it is not on the critical end of the spectrum. Also it's just a minor part of interface_bitcoin_cli.py so I completely understand the resistance to breaking it out).

tdb3 commented at 1:31 AM on May 13, 2024:

Thanks @cbergqvist. Good observation.

Looks like rpc_bind.py uses --ipv4 and --ipv6 arguments to run the test exclusively with ipv4 or ipv6 (not both), which at first glance doesn't seem to be desirable for interface_bitcoin_cli.py (dependence on network type is a somewhat small part of the test). I could see there being value in refactoring interface_bitcoin_cli.py to include wider coverage of bitcoin-cli with ipv4 and ipv6, but I'm thinking this sort of refactor would be a good follow-up PR rather than included in the scope of this PR.

In the interim, I did push a minor update to add a log message stating when ipv6 testing is being skipped (and also opportunistically rebased).

cbergqvist approved

cbergqvist commented at 10:33 PM on May 8, 2024: contributor

reACK 867a673bcfd3ca254c306d8cf0c68d4ceb163f97

Inspected git range-diff cb4f9fc~2..cb4f9fc 867a673~2..867a673.

Passed --extended functional tests (-feature_dbcrash, excluded, some others skipped due to configuration). Passed make check.

DrahtBot requested review from rkrux on May 8, 2024

tdb3 force-pushed on May 13, 2024

tdb3 commented at 2:40 AM on May 13, 2024: contributor

Seeing a CI error for arm-linux-gnueabihf-g++ (on a line unchanged in this latest push). Will look into this.

In function ‘UniValue CallRPC(BaseRequestHandler*, const std::string&, const std::vector<std::__cxx11::basic_string<char> >&, const std::optional<std::__cxx11::basic_string<char> >&)’,
    inlined from ‘UniValue ConnectAndCallRPC(BaseRequestHandler*, const std::string&, const std::vector<std::__cxx11::basic_string<char> >&, const std::optional<std::__cxx11::basic_string<char> >&)’ at bitcoin-cli.cpp:908:31:
bitcoin-cli.cpp:764:42: error: ‘result’ may be used uninitialized [-Werror=maybe-uninitialized]
  764 |             if (!rpcport_int.has_value() || rpcport_int.value() == 0) {
      |                 ~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from bitcoin-cli.cpp:23:
./util/strencodings.h: In function ‘UniValue ConnectAndCallRPC(BaseRequestHandler*, const std::string&, const std::vector<std::__cxx11::basic_string<char> >&, const std::optional<std::__cxx11::basic_string<char> >&)’:
./util/strencodings.h:184:7: note: ‘result’ was declared here
  184 |     T result;
      |       ^~~~~~
cc1plus: all warnings being treated as errors

DrahtBot added the label CI failed on May 13, 2024

DrahtBot commented at 2:53 AM on May 13, 2024: contributor

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

<sub>Debug: https://github.com/bitcoin/bitcoin/runs/24877615319</sub>

cli: Sanitize ports in rpcconnect and rpcport

Adds error handling of invalid ports to rpcconnect and rpcport,
with associated functional tests.

e208fb5d3b

cli: Add warning for duplicate port definition

Adds a warning when both -rpcconnect and -rpcport define
a port to be used.

24bc46c83b

in src/bitcoin-cli.cpp:764 in b331a607e4 outdated

 761 | +        }
 762 | +
 763 | +        if (std::optional<std::string> rpcport_arg = gArgs.GetArg("-rpcport")) {
 764 | +            // -rpcport was specified
 765 | +            std::optional<uint16_t> rpcport_int{ToIntegral<uint16_t>(rpcport_arg.value())};
 766 | +            if (!rpcport_int.has_value() || rpcport_int.value() == 0) {

maflcko commented at 7:03 AM on May 13, 2024:

            port=ToIntegral<uint16_t>(rpcport_arg).value_or(0);
            if (port == 0) {

if you treat 0 as invalid, there is no need to also have nullopt be invalid as well, no?

This should also fix the false-positive compiler warning?

An alternative would be to suppress the false-positive warning in the CI task config.

cbergqvist commented at 12:52 PM on May 13, 2024:

ToIntegral itself returns a std::optional unfortunately so the suggested change won't compile as is Super-weird that this error popped up now.

maflcko commented at 12:59 PM on May 13, 2024:

I placed the ) in the wrong place, I think. However, value_or exists and should be compile-able. See https://en.cppreference.com/w/cpp/utility/optional/value_or

cbergqvist commented at 1:23 PM on May 13, 2024:

Yeah, that new version should work. Another possible variant.. but no way to know if the compiler becomes happier:

            const uint16_t rpcport_int{ToIntegral<uint16_t>(rpcport_arg.value()).value_or(0)};
            if (rpcport_int == 0) {

maflcko commented at 1:30 PM on May 13, 2024:

no way to know if the compiler becomes happier:

If it compiles locally, the code should be fine. If the CI warning persists, it should be disabled in the CI task config. (you can test by pushing, or running locally)

tdb3 commented at 4:21 PM on May 13, 2024:

Thanks all for the suggestions. Pushed an update. Since locally I have x86_64 (and not arm), I tried configuring/compiling with --enable-werror added to ./configure (to minimize excess CI churn). Interestingly enough, this didn't result in the same warning as seen on arm CI.

tdb3 commented at 3:12 AM on May 14, 2024:

No luck with the arm CI again. Will try playing with the specific stage locally more (https://github.com/bitcoin/bitcoin/tree/master/ci).

maflcko commented at 6:40 AM on May 14, 2024:

It is a false positive, so it needs to be disabled, like in the other CI tasks.

maflcko commented at 6:12 AM on May 21, 2024:

Fixed in https://github.com/bitcoin/bitcoin/pull/30144

tdb3 force-pushed on May 13, 2024

marcofleon commented at 3:36 PM on May 15, 2024: contributor

re-ACK e208fb5d3bea4c1fb750cb0028819635ecdeb415. I successfully compiled and built (make check passed) on my Arm64 machine but the CI error is related to 32-bit machines if I'm not mistaken so might not be relevant.

Either way, the port error detection looks good to me. Clear comments and more readable since my last review. I also successfully ran the functional test. I modified the test using valid ports and mixed -rpcconnect and -rpcport configurations to ensure robustness. The test handled everything correctly.

cbergqvist approved

cbergqvist commented at 10:32 AM on May 20, 2024: contributor

re ACK 24bc46c83b39149f4845a575a82337eb46d91bdb

Inspected git range-diff 867a673~2..867a673 24bc46c~2..24bc46c.

Ran same tests as last time (https://github.com/bitcoin/bitcoin/pull/29521/#pullrequestreview-2046768298) with same results.

Tried to repro a boiled down version of the mysterious compiler warning but was unable to reproduce it. Here is what I ended up with: https://godbolt.org/z/fh5KcbbeW (Not sure about the legalese around uploading Bitcoin Core source to Godbolt but just pasting ToIntegral() shouldn't be that bad).

DrahtBot requested review from marcofleon on May 20, 2024

fanquake referenced this in commit fa8cb0516d on May 22, 2024

DrahtBot removed the label CI failed on May 22, 2024

S3RK commented at 7:03 AM on June 10, 2024: contributor

light code review ACK 24bc46c83b39149f4845a575a82337eb46d91bdb

achow101 commented at 7:36 PM on June 11, 2024: member

ACK 24bc46c83b39149f4845a575a82337eb46d91bdb

achow101 merged this on Jun 11, 2024

achow101 closed this on Jun 11, 2024

luke-jr referenced this in commit 3db6f44847 on Aug 1, 2024

luke-jr referenced this in commit 5064b0de97 on Aug 1, 2024

in src/bitcoin-cli.cpp:777 in 24bc46c83b

 774 | +            port = rpcport_int;
 775 | +
 776 | +            // If there was a valid port provided in rpcconnect,
 777 | +            // rpcconnect_port is non-zero.
 778 | +            if (rpcconnect_port != 0) {
 779 | +                tfm::format(std::cerr, "Warning: Port specified in both -rpcconnect and -rpcport. Using -rpcport %u\n", port);

pinheadmz commented at 7:28 PM on September 19, 2024:

I'm seeing something a bit unexpected here which is that the IP address passed to rpcconnect is still respected even though the port is ignored / overridden by rpcport. This came to my attention in the interface_bitcoin_cli test while working on another branch where I'd borked ipv6. This line in the test unexpectedly tried to connect to bitcoin on ::1 even though the port passed to rpcconnect was junk:

assert_equal(BLOCKS, self.nodes[0].cli("-rpcconnect=[::1]:18999", f'-rpcport={node_rpc_port}').getblockcount())

pinheadmz commented at 7:43 PM on September 19, 2024:

on second thought, this behavior does make sense if a user is crazy enough to pass IP:port and then a second port

bitcoin locked this on Sep 19, 2025