cli: Detect port errors in rpcconnect and rpcport #29521

pull tdb3 wants to merge 2 commits into bitcoin:master from tdb3:20240229_rpcconnectinvalidportdetection changing 2 files +84 −2
  1. 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.

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

  3. DrahtBot added the label Scripts and tools on Feb 29, 2024
  4. 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.
  5. tdb3 force-pushed on Mar 1, 2024
  6. 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

    0+ 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)
1+

    Below as well

    tdb3 commented at 9:47 pm on March 12, 2024:
    Thanks. Added. Testing upperbound makes sense.
  7. 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().

  9. tdb3 force-pushed on Mar 12, 2024
  10. tdb3 commented at 9:46 pm on March 12, 2024: contributor
    Thanks for the review @davidgumberg! Added the suggestions (force pushed). Responses inline.
  11. 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.

    0        if (std::optional<std::string> rpcport_arg = gArgs.GetArg("-rpcport")) {
1            std::optional<uint16_t> rpcport_int{ToIntegral<uint16_t>(rpcport_arg.value())};
2            if (!rpcport_int.has_value() || rpcport_int.value() == 0) {
3                throw std::runtime_error(strprintf("Invalid port provided in -rpcport: %s", rpcport_arg.value()));
4            }
5
6            // Use the valid port provided
7            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).
  12. cbergqvist approved
  13. 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.

  14. DrahtBot requested review from davidgumberg on Mar 29, 2024
  15. tdb3 force-pushed on Mar 30, 2024
  16. tdb3 commented at 1:34 am on March 30, 2024: contributor
    Thank you for the review @cbergqvist! Implemented your suggestions and rebased.
  17. 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.
  18. 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: 
    0$ src/bitcoin-cli -rpcport=0 getblockchaininfo
1error: timeout on transient error: Could not connect to the server 127.0.0.1:0
2
3Make sure the bitcoind server is running and that you are connecting to the correct RPC port.
4
5$ src/bitcoin-cli -rpcconnect=127.0.0.1:0 getblockchaininfo
6error: timeout on transient error: Could not connect to the server 127.0.0.1:0
7
8Make 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. 👍
  19. rkrux approved
  20. rkrux commented at 8:05 am on March 30, 2024: none

    ACK cb4f9fc

    Testing Methodolgy:

    1. Ran make, which was successful.
    2. Ran functional tests, all passed.
    3. Tried the following cli commands in terminal while the daemon was running:
     0➜  bitcoin git:(20240229_rpcconnectinvalidportdetection) ✗ bcli -rpcconnect=127.0.0.1:notaport help
 1error: Invalid port provided in -rpcconnect: 127.0.0.1:notaport
 2➜  bitcoin git:(20240229_rpcconnectinvalidportdetection) ✗ bcli -rpcconnect=127.0.0.1:-1 help
 3error: Invalid port provided in -rpcconnect: 127.0.0.1:-1
 4➜  bitcoin git:(20240229_rpcconnectinvalidportdetection) ✗ bcli -rpcconnect=127.0.0.1:65536 help
 5error: Invalid port provided in -rpcconnect: 127.0.0.1:65536
 6➜  bitcoin git:(20240229_rpcconnectinvalidportdetection) ✗ bcli -rpcport=notaport help
 7error: Invalid port provided in -rpcport: notaport
 8➜  bitcoin git:(20240229_rpcconnectinvalidportdetection) ✗ bcli -rpcport=-1 help
 9error: Invalid port provided in -rpcport: -1
10
11➜  bitcoin git:(20240229_rpcconnectinvalidportdetection) ✗ bcli -rpcconnect=127.0.0.1:9000 -rpcport=8332 getblockcount
12Warning: Port specified in both -rpcconnect and -rpcport. Using -rpcport 8332
13448483
  21. DrahtBot requested review from cbergqvist on Mar 30, 2024
  22. 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.

  23. cbergqvist approved
  24. 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.

  25. 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.
  26. 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!

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

  28. DrahtBot requested review from marcofleon on Apr 3, 2024
  29. 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:

     02024-04-22T22:30:22.751000Z TestFramework (ERROR): Assertion failed
 1Traceback (most recent call last):
 2  File "/home/ava/bitcoin/bitcoin/29521/test/functional/test_framework/util.py", line 122, in assert_raises_process_error
 3    fun(*args, **kwds)
 4  File "/home/ava/bitcoin/bitcoin/29521/test/functional/test_framework/test_node.py", line 821, in __call__
 5    return self.cli.send_cli(self.command, *args, **kwargs)
 6  File "/home/ava/bitcoin/bitcoin/29521/test/functional/test_framework/test_node.py", line 886, in send_cli
 7    raise subprocess.CalledProcessError(returncode, self.binary, output=cli_stderr)
 8subprocess.CalledProcessError: Command '/home/ava/bitcoin/bitcoin/29521/src/bitcoin-cli' returned non-zero exit status 1.
 9
10During handling of the above exception, another exception occurred:
11
12Traceback (most recent call last):
13  File "/home/ava/bitcoin/bitcoin/29521/test/functional/test_framework/test_framework.py", line 132, in main
14    self.run_test()
15  File "/home/ava/bitcoin/bitcoin/29521/test/functional/interface_bitcoin_cli.py", line 136, in run_test
16    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)
17  File "/home/ava/bitcoin/bitcoin/29521/test/functional/test_framework/util.py", line 127, in assert_raises_process_error
18    raise AssertionError("Expected substring not found:" + e.output)
19AssertionError: 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.
  30. tdb3 force-pushed on Apr 24, 2024
  31. 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

    0assert_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.

  32. tdb3 force-pushed on Apr 27, 2024
  33. 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).

  34. cbergqvist approved
  35. 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.

  36. DrahtBot requested review from rkrux on May 8, 2024
  37. tdb3 force-pushed on May 13, 2024
  38. 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.

     0In function ‘UniValue CallRPC(BaseRequestHandler*, const std::string&, const std::vector<std::__cxx11::basic_string<char> >&, const std::optional<std::__cxx11::basic_string<char> >&)’,
 1    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:
 2bitcoin-cli.cpp:764:42: error: ‘result’ may be used uninitialized [-Werror=maybe-uninitialized]
 3  764 |             if (!rpcport_int.has_value() || rpcport_int.value() == 0) {
 4      |                 ~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~
 5In file included from bitcoin-cli.cpp:23:
 6./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> >&)’:
 7./util/strencodings.h:184:7: note: ‘result’ was declared here
 8  184 |     T result;
 9      |       ^~~~~~
10cc1plus: all warnings being treated as errors
  39. DrahtBot added the label CI failed on May 13, 2024
  40. 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.

    Debug: https://github.com/bitcoin/bitcoin/runs/24877615319

  41. cli: Sanitize ports in rpcconnect and rpcport 
    Adds error handling of invalid ports to rpcconnect and rpcport,
with associated functional tests.
    e208fb5d3b
  42. cli: Add warning for duplicate port definition 
    Adds a warning when both -rpcconnect and -rpcport define
a port to be used.
    24bc46c83b
  43. 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: 
    0            port=ToIntegral<uint16_t>(rpcport_arg).value_or(0);
1            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:

    0            const uint16_t rpcport_int{ToIntegral<uint16_t>(rpcport_arg.value()).value_or(0)};
1            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
  44. tdb3 force-pushed on May 13, 2024
  45. 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.

  46. cbergqvist approved
  47. 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).

  48. DrahtBot requested review from marcofleon on May 20, 2024
  49. fanquake referenced this in commit fa8cb0516d on May 22, 2024
  50. DrahtBot removed the label CI failed on May 22, 2024
  51. S3RK commented at 7:03 am on June 10, 2024: contributor
    light code review ACK 24bc46c83b39149f4845a575a82337eb46d91bdb
  52. achow101 commented at 7:36 pm on June 11, 2024: member
    ACK 24bc46c83b39149f4845a575a82337eb46d91bdb
  53. achow101 merged this on Jun 11, 2024
  54. achow101 closed this on Jun 11, 2024
  55. luke-jr referenced this in commit 3db6f44847 on Aug 1, 2024
  56. luke-jr referenced this in commit 5064b0de97 on Aug 1, 2024


tdb3 DrahtBot maflcko davidgumberg cbergqvist rkrux marcofleon achow101 kevkevinpal S3RK pinheadmz


davidgumberg rkrux marcofleon

Labels
Scripts and tools

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-26 09:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me