util, cli: name the Sock wait timeout cap and use it for -rpcclienttimeout=0 #36453

kriss39 commented at 2:32 PM on October 7, 2026: contributor

Follow-up to #36340, addressing the review comments from pinheadmz and winterrdog there.

#36340 capped the Sock::Wait/WaitMany timeout at INT_MAX milliseconds (about 24.8 days) inside WaitMany. This moves that value to a named constant, Sock::MAX_WAIT_TIMEOUT, and mentions the cap in the doxygen comments of Wait() and WaitMany(), since it applies to every caller and not only to bitcoin-cli. The constant is a whole number of seconds (2147483 s), so bitcoin-cli, which works in seconds, uses exactly the same value as Sock.

bitcoin-cli still turned -rpcclienttimeout=0 into a 5 year timeout. The timeout is applied to each wait for the socket, and each wait is now capped, so in practice 0 already means "give up after about 24.8 days without data". The 5 year value is replaced with Sock::MAX_WAIT_TIMEOUT, and the help text now says that 0 is the longest supported timeout (about 24 days) instead of no timeout.

There is no behaviour change. sock_tests, interface_bitcoin_cli.py (including the -rpcclienttimeout=0 call added in #36340) and interface_rpc.py pass on macOS.

(Changes prepared with AI tools.)

DrahtBot commented at 2:32 PM on October 7, 2026: contributor

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

External sites

Code coverage & benchmarks: https://corecheck.dev/bitcoin/bitcoin/pulls/36453
Ralph review <img src=https://review.fish.foo/assets/bitcoin/bitcoin/36453.png alt="Ralph review status"> https://git.fish.foo/bitcoin/bitcoin/pulls/36453#author-ralph

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
ACK	winterrdog
Stale ACK	maflcko, pinheadmz

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

No conflicts as of last run.

in src/bitcoin-cli.cpp:1198 in b497503376

1193 | @@ -1194,8 +1194,8 @@ static UniValue CallRPC(BaseRequestHandler* rh, const std::string& strMethod, co
1194 |      if (timeout > 0) {
1195 |          timeout_duration = std::chrono::seconds(timeout);
1196 |      } else {
1197 | -        // Use 5 year timeout for "indefinite"
1198 | -        timeout_duration = std::chrono::years(5);
1199 | +        // There is no indefinite wait, so use the longest one that Sock supports.
1200 | +        timeout_duration = std::chrono::duration_cast<std::chrono::seconds>(Sock::MAX_WAIT_TIMEOUT);

maflcko commented at 2:46 PM on October 7, 2026:

not sure about the cast here. this will again lead to two (minimally) different values in the cli and in the sock impl. the goal of this pull should be to fix this. Either:

Cast MAX_WAIT_TIMEOUT to seconds.
Extend the precision here to have the exact same value.

Otherwise, there will be an obscure platform in the future where this is silently broken again.

kriss39 commented at 6:53 PM on October 7, 2026:

Done, went with the first option: MAX_WAIT_TIMEOUT is now a std::chrono::seconds (INT_MAX ms truncated to whole seconds, 2147483 s), so the cli uses it directly without a cast and both use exactly the same value. WaitMany converts it to milliseconds for the std::min.

kriss39 force-pushed on Oct 7, 2026

maflcko commented at 5:42 AM on October 8, 2026: member

lgtm ACK 5b4a438c95eb40e0e9cf7f2ce441147eccd1b48e

sedited requested review from pinheadmz on Oct 8, 2026

pinheadmz commented at 10:05 PM on October 8, 2026: member

concept ACK, thanks for taking care of this so quickly!

winterrdog commented at 2:09 PM on October 9, 2026: contributor

approach ACK

thanks for following up on this

in src/util/sock.h:169 in 5b4a438c95 outdated

 160 | @@ -161,9 +161,18 @@ class Sock
 161 |       */
 162 |      static constexpr Event ErrorEvent = 0b100;
 163 |  
 164 | +    /**
 165 | +     * Longest timeout that `Wait()` and `WaitMany()` use, about 24.8 days. Longer timeouts are
 166 | +     * capped to it, because poll() takes the timeout as an int and select() rejects large values
 167 | +     * on some systems (e.g. macOS). It is a whole number of seconds, so that callers that work in
 168 | +     * seconds can use the exact same value.
 169 | +     */

winterrdog commented at 2:43 PM on October 9, 2026:

nit: could we say "maximum timeout" rather than "longest timeout", since longer requested timeouts are capped to this value ?

this is what i had in mind (feel free to put a spin to it however you like):

    /**
     * Maximum timeout supported by `Wait()` and `WaitMany()` (about 24.8 days).
     * Longer timeouts are capped because `poll()` takes an `int` timeout in
     * milliseconds, and `select()` rejects large timeouts on some platforms
     * (e.g. macOS). Expressed in seconds so that callers can use the same value.
     */

"whole number" is missing because std::chrono::seconds is a signed integer of at least 35 bits i.e. std::chrono::duration</* int35 */> (see "Helper types" section in https://en.cppreference.com/cpp/chrono/duration)

kriss39 commented at 8:15 AM on October 12, 2026:

Thanks, taken as suggested in c5dd5ebb25.

kriss39 force-pushed on Oct 12, 2026

pinheadmz approved

pinheadmz commented at 2:46 AM on October 14, 2026: member

ACK c5dd5ebb25d4e92b2d799875812d6286e993c1e9

Built and tested on macos/arm64. Also built the doxygen HTML to make sure the new notes look OK and they do!

<details><summary>Show Signature</summary>

pinheadmz's public key is on openpgp.org

</details>

DrahtBot requested review from winterrdog on Oct 14, 2026

DrahtBot requested review from maflcko on Oct 14, 2026

in src/util/sock.h:170 in c5dd5ebb25

 165 | +     * Maximum timeout supported by `Wait()` and `WaitMany()` (about 24.8 days).
 166 | +     * Longer timeouts are capped because `poll()` takes an `int` timeout in
 167 | +     * milliseconds, and `select()` rejects large timeouts on some platforms
 168 | +     * (e.g. macOS). Expressed in seconds so that callers can use the same value.
 169 | +     */
 170 | +    static constexpr std::chrono::seconds MAX_WAIT_TIMEOUT{std::chrono::duration_cast<std::chrono::seconds>(std::chrono::milliseconds{std::numeric_limits<int>::max()})};

pinheadmz commented at 2:58 AM on October 14, 2026:

nit, could be more legible I think

    static constexpr std::chrono::seconds MAX_WAIT_TIMEOUT{
    std::numeric_limits<int>::max() / 1000
};

kriss39 commented at 8:34 AM on October 17, 2026:

Taken in d22d356512, thanks.

winterrdog commented at 7:07 PM on October 14, 2026: contributor

tACK c5dd5ebb25d4e92b2d799875812d6286e993c1e9

successfully built and tested (10x, just to be sure) with clang++-19 on FreeBSD 15.0, targeting x86_64. the generated Doxygen docs are clear to readers and the code changes LGTM

util, cli: name the Sock wait timeout cap and use it for -rpcclienttimeout=0

#36340 capped the timeout of Sock::Wait/WaitMany at INT_MAX milliseconds
(about 24.8 days) inside WaitMany. Move the value to a named constant,
Sock::MAX_WAIT_TIMEOUT, and mention the cap in the Wait() and WaitMany()
docs, since it applies to every caller. The constant is a whole number
of seconds (2147483 s), so callers that work in seconds use exactly the
same value.

bitcoin-cli turned -rpcclienttimeout=0 into a 5 year timeout, which is
now capped to the same value on every wait, so "5 years" no longer
describes what happens. Use Sock::MAX_WAIT_TIMEOUT directly, and say in
the help text that 0 means the longest supported timeout (about 24 days)
instead of no timeout.

No behaviour change.

d22d356512

kriss39 force-pushed on Oct 17, 2026

winterrdog commented at 9:46 AM on October 17, 2026: contributor

reACK d22d3565128828e158c6684a1b5bf1a7f2454c80

DrahtBot requested review from pinheadmz on Oct 17, 2026