cli: Replace libevent usage with simple http client #34342

pull fjahr wants to merge 5 commits into bitcoin:master from fjahr:2026-01-cli-noev changing 7 files +482 −143
  1. fjahr commented at 1:27 PM on January 19, 2026: contributor

    Part of the effort to remove the libevent dependency altogether, see #31194

    This takes the Read and FindFirst functions from the HTTPHeaders class from #32061 and puts them into bitcoin-cli as free functions in a separate namespace with a TODO comment to revisit potentially sharing this code somehow. I played with picking the necessary commits and moving the HTTPHeaders class to common or something similar but I am not loving it so I am going with this route for now since it also decouples the two PRs.

    Otherwise the change itself replaces the libevent-based HTTP client with a simple synchronous implementation which uses the Sock class directly.

  2. DrahtBot added the label Scripts and tools on Jan 19, 2026
  3. DrahtBot commented at 1:28 PM on January 19, 2026: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--006a51241073e994b41acfe9ec718e94-->

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34342.

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    Concept ACK bradleystachurski, w0xlt, hodlinator, theStack
    Stale ACK pinheadmz

    If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #34411 ([POC] Full Libevent removal by fanquake)

    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.

    <!--5faf32d7da4f0f540f40219e4f7537a3-->

  4. DrahtBot added the label CI failed on Jan 19, 2026
  5. DrahtBot commented at 3:30 PM on January 19, 2026: contributor

    <!--85328a0da195eb286784d51f73fa0af9-->

    🚧 At least one of the CI tasks failed. <sub>Task tidy: https://github.com/bitcoin/bitcoin/actions/runs/21139253857/job/60789761512</sub> <sub>LLM reason (✨ experimental): Clang-Tidy failure: modernize-use-starts-ends-with flags substr usage in bitcoin-cli.cpp, treated as error and causing CI to fail.</sub>

    <details><summary>Hints</summary>

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly 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.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

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

    </details>

  6. fjahr force-pushed on Jan 19, 2026
  7. fjahr force-pushed on Jan 19, 2026
  8. fjahr force-pushed on Jan 20, 2026
  9. fjahr force-pushed on Jan 20, 2026
  10. DrahtBot removed the label CI failed on Jan 20, 2026
  11. DrahtBot added the label Needs rebase on Jan 23, 2026
  12. fjahr force-pushed on Jan 23, 2026
  13. DrahtBot removed the label Needs rebase on Jan 23, 2026
  14. fjahr force-pushed on Jan 23, 2026
  15. fjahr marked this as ready for review on Jan 23, 2026
  16. pinheadmz commented at 12:13 AM on January 24, 2026: member

    concept ACK Nice moves, yo! 🔥

  17. bradleystachurski commented at 8:23 PM on February 6, 2026: none

    Concept ACK (and supportive of the broader libevent removal effort)

    Reviewed efd74a822d9577c53259b3d27526957b0674f7d8

    Found a minor behavioral regression: connection failures (e.g. unresolvable host, non-listening port) lose the help text shown on master.

    Master:

    error: timeout on transient error: Could not connect to the server 127.0.0.1:19999
Make sure the bitcoind server is running and that you are connecting to the correct RPC port.
Use "bitcoin-cli -help" for more info.

    Branch:

    error: timeout on transient error: Could not connect to the server 127.0.0.1:19999

    Catching instead of re-throwing preserves the help text:

    diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp
index 8414ad0317..3cd6e4b29c 100644
--- a/src/bitcoin-cli.cpp
+++ b/src/bitcoin-cli.cpp
@@ -1258,7 +1258,7 @@ static UniValue CallRPC(BaseRequestHandler* rh, const std::string& strMethod, co
     try {
         response = client.Post(endpoint, headers, strRequest);
     } catch (const CConnectionFailed&) {
-        throw;
+        response.status = 0;
     }

     if (response.status == 0) {

    Verified this preserves the help text and passes interface_bitcoin_cli.py.

  18. fjahr force-pushed on Feb 8, 2026
  19. fjahr commented at 8:10 PM on February 8, 2026: contributor

    @bradleystachurski

    Found a minor behavioral regression: connection failures (e.g. unresolvable host, non-listening port) lose the help text shown on master.

    You were right, good catch! Your suggested fix looks good to me as well, I have just applied that change.

  20. in src/common/http.h:31 in 58d7fe0777 
      26 | +     */
  27 | +    bool Read(util::LineReader& reader);
  28 | +    std::string Stringify() const;
  29 | +
  30 | +private:
  31 | +    std::unordered_map<std::string, std::string, util::AsciiCaseInsensitiveHash, util::AsciiCaseInsensitiveKeyEqual> m_map;
    fanquake commented at 3:03 PM on February 25, 2026:

    Note this is making use of code being removed in #32061 (https://github.com/bitcoin/bitcoin/pull/32061/changes/c24f5d3c3c879f7a43667866a92fa5c05b955dde).

    pinheadmz commented at 3:24 PM on February 25, 2026:

    I think there's a few things we learned in #34158 as well that can be applied. This PR (bitcoin-cli) can maybe be draft until torcontol is ironed out.

    fjahr commented at 10:07 PM on March 6, 2026:

    It is already draft but thanks for the hints, I will address these once we have made some more progress on torcontol and the server.

  21. fanquake marked this as a draft on Feb 25, 2026
  22. w0xlt commented at 5:32 PM on March 10, 2026: contributor

    Concept ACK

  23. DrahtBot added the label Needs rebase on Mar 31, 2026
  24. fjahr force-pushed on Apr 5, 2026
  25. fjahr commented at 8:07 PM on April 5, 2026: contributor

    Still very much draft mode but I rebased and resolved some of the conflicts with the latest changes in bitcoin-cli and the http server PR. I can drop the cherry-picked commits once #34905 is merged.

  26. DrahtBot removed the label Needs rebase on Apr 5, 2026
  27. DrahtBot added the label Needs rebase on Apr 9, 2026
  28. fjahr force-pushed on Apr 9, 2026
  29. fjahr commented at 1:38 PM on April 9, 2026: contributor

    Rebased since most of the prerequisits have been merged with #34905. I also went through the code again and made several improvements, reducing the LoC count quite a bit. I was also never really sure whether I liked the idea of ripping the HTTPHeaders class out of the rest of the http code and moving it into common, when I really only need less than half of the functionality here. Instead have now taken the Read and FindFirst function that I needed and moved them into bitcoin-cli.cpp in a separate namespace with minor edits to be able to make them free functions. There is a big, fat TODO comment at the top to revisit sharing this code when #32061 is merged but for now I am at least sure that I don't dislike the idea as much as the move to common, at least looking at it in isolation for now. The nice side effect is that this makes this PR now reviewable and possible to merge without having to wait for #32061 necessarily. Thus taking it out of draft status.

  30. fjahr marked this as ready for review on Apr 9, 2026
  31. DrahtBot removed the label Needs rebase on Apr 9, 2026
  32. in src/bitcoin-cli.cpp:1259 in ca1ac2aeb7 
    1277 | +    std::chrono::seconds timeout_duration;
1278 | +    if (timeout > 0) {
1279 | +        timeout_duration = std::chrono::seconds(timeout);
1280 | +    } else {
1281 | +        // Use 5 year timeout for "indefinite"
1282 | +        timeout_duration = std::chrono::seconds(5 * 365 * 24 * 60 * 60);
    maflcko commented at 1:59 PM on April 9, 2026:

    nit: No need to do those manually. You can use std::chrono::years (since C++20), which is slightly larger: 365.2425 days (31556952s)

    fjahr commented at 7:27 PM on April 9, 2026:

    Nice, done, thanks!

  33. DrahtBot added the label CI failed on Apr 9, 2026
  34. DrahtBot commented at 2:20 PM on April 9, 2026: contributor

    <!--85328a0da195eb286784d51f73fa0af9-->

    🚧 At least one of the CI tasks failed. <sub>Task test ancestor commits: https://github.com/bitcoin/bitcoin/actions/runs/24192925766/job/70614859784</sub> <sub>LLM reason (✨ experimental): CI failed due to a build compilation error while compiling bitcoin-cli.cpp (target bitcoin-cli, Error 1 from gmake).</sub>

    <details><summary>Hints</summary>

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly 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.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

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

    </details>

  35. maflcko commented at 2:28 PM on April 9, 2026: member

    If you want to fix the CI failure, you may adjust the CI script to exclude that warning as well:

    .github/ci-test-each-commit-exec.py:44:        "-DCMAKE_CXX_FLAGS=-Wno-error=unused-member-function",

    Or you can adjust your code, up to you.

  36. fjahr force-pushed on Apr 9, 2026
  37. fjahr commented at 7:30 PM on April 9, 2026: contributor

    If you want to fix the CI failure, you may adjust the CI script to exclude that warning as well:

    Yeah, I saw the error and thought that check doesn't make sense in this context but I didn't have time to find the right place and address it yet. I added the exception for free functions.

  38. DrahtBot removed the label CI failed on Apr 9, 2026
  39. in src/bitcoin-cli.cpp:83 in 6dbb51040f 
      78 | +static constexpr size_t MAX_BODY_SIZE = 32 * 1024 * 1024;
  79 | +
  80 | +// TODO: These helpers may be replaced with the corresponding methods in HTTPHeaders
  81 | +// from https://github.com/bitcoin/bitcoin/pull/32061 if that is moved to a shared
  82 | +// location.
  83 | +namespace http_bitcoin {
    hodlinator commented at 8:26 PM on April 9, 2026:

    nit: Seems like we should be going from local (bitcoin) to general (http):

    namespace bitcoin_http {
    fjahr commented at 8:35 PM on April 11, 2026:

    To be honest, I just copied the name of the temp namespace in #32061 since am not sure this one will stay around as well. But who knows, maybe it will stick around, so I renamed it.

  40. in src/bitcoin-cli.cpp:841 in 6dbb51040f 
     834 | @@ -829,6 +835,376 @@ static std::optional<UniValue> CallIPC(BaseRequestHandler* rh, const std::string
 835 |      return rh->ProcessReply(reply);
 836 |  }
 837 |  
 838 | +/**
 839 | + * Simple synchronous HTTP client using Sock class.
 840 | + */
 841 | +class HttpClient
    hodlinator commented at 8:35 PM on April 9, 2026:

    I find it confusing to have two classes with the same names between here and #32061 (okay, they might be in different contexts and the one in the other PR is cased as HTTPClient).

    • Most of the code style I've seen uses UPPERCASE for acronyms so I suggest aligning with #32061.
    • #32061's client is the server's perspective of the client, while this is the client's own, "actor". It seems to me like the CLI has a stronger claim and #32061 should rename to HTTPRemoteClient to resolve the conflict. cc @pinheadmz
    fjahr commented at 8:35 PM on April 11, 2026:

    Sure, renamed to HTTPClient. Happy to discuss another name change if you disagree with the suggestion above @pinheadmz

  41. in src/common/url.cpp:41 in 6dbb51040f 
      36 | @@ -37,3 +37,25 @@ std::string UrlDecode(std::string_view url_encoded)
  37 |  
  38 |      return res;
  39 |  }
  40 | +
  41 | +std::string UrlEncode(const std::string& str)
    hodlinator commented at 9:01 PM on April 9, 2026:

    Should avoid requiring a string to be constructed for the input param (also mirrors the decode function):

    std::string UrlEncode(std::string_view str)
    hodlinator commented at 9:02 PM on April 9, 2026:

    Possible test:

    --- a/src/test/common_url_tests.cpp
+++ b/src/test/common_url_tests.cpp
@@ -5,6 +5,7 @@
 #include <common/url.h>
 
 #include <string>
+#include <string_view>
 
 #include <boost/test/unit_test.hpp>
 
@@ -72,4 +73,41 @@ BOOST_AUTO_TEST_CASE(decode_internal_nulls_test) {
     BOOST_CHECK_EQUAL(UrlDecode("abc%00%00"), result2);
 }
 
+BOOST_AUTO_TEST_CASE(url_encode) {
+    BOOST_CHECK_EQUAL(UrlEncode(std::string_view{
+        "\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f"
+        "\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f"
+        "\x20\x21\x22\x23\x24\x25\x26\x27\x28\x29\x2a\x2b\x2c\x2d\x2e\x2f"
+        "\x30\x31\x32\x33\x34\x35\x36\x37\x38\x39\x3a\x3b\x3c\x3d\x3e\x3f"
+        "\x40\x41\x42\x43\x44\x45\x46\x47\x48\x49\x4a\x4b\x4c\x4d\x4e\x4f"
+        "\x50\x51\x52\x53\x54\x55\x56\x57\x58\x59\x5a\x5b\x5c\x5d\x5e\x5f"
+        "\x60\x61\x62\x63\x64\x65\x66\x67\x68\x69\x6a\x6b\x6c\x6d\x6e\x6f"
+        "\x70\x71\x72\x73\x74\x75\x76\x77\x78\x79\x7a\x7b\x7c\x7d\x7e\x7f"
+        "\x80\x81\x82\x83\x84\x85\x86\x87\x88\x89\x8a\x8b\x8c\x8d\x8e\x8f"
+        "\x90\x91\x92\x93\x94\x95\x96\x97\x98\x99\x9a\x9b\x9c\x9d\x9e\x9f"
+        "\xa0\xa1\xa2\xa3\xa4\xa5\xa6\xa7\xa8\xa9\xaa\xab\xac\xad\xae\xaf"
+        "\xb0\xb1\xb2\xb3\xb4\xb5\xb6\xb7\xb8\xb9\xba\xbb\xbc\xbd\xbe\xbf"
+        "\xc0\xc1\xc2\xc3\xc4\xc5\xc6\xc7\xc8\xc9\xca\xcb\xcc\xcd\xce\xcf"
+        "\xd0\xd1\xd2\xd3\xd4\xd5\xd6\xd7\xd8\xd9\xda\xdb\xdc\xdd\xde\xdf"
+        "\xe0\xe1\xe2\xe3\xe4\xe5\xe6\xe7\xe8\xe9\xea\xeb\xec\xed\xee\xef"
+        "\xf0\xf1\xf2\xf3\xf4\xf5\xf6\xf7\xf8\xf9\xfa\xfb\xfc\xfd\xfe\xff",
+        256}),
+        "%00%01%02%03%04%05%06%07%08%09%0A%0B%0C%0D%0E%0F"
+        "%10%11%12%13%14%15%16%17%18%19%1A%1B%1C%1D%1E%1F"
+        "%20%21%22%23%24%25%26%27%28%29%2A%2B%2C-.%2F"
+        "0123456789%3A%3B%3C%3D%3E%3F"
+        "%40ABCDEFGHIJKLMNO"
+        "PQRSTUVWXYZ%5B%5C%5D%5E_"
+        "%60abcdefghijklmno"
+        "pqrstuvwxyz%7B%7C%7D~%7F"
+        "%80%81%82%83%84%85%86%87%88%89%8A%8B%8C%8D%8E%8F"
+        "%90%91%92%93%94%95%96%97%98%99%9A%9B%9C%9D%9E%9F"
+        "%A0%A1%A2%A3%A4%A5%A6%A7%A8%A9%AA%AB%AC%AD%AE%AF"
+        "%B0%B1%B2%B3%B4%B5%B6%B7%B8%B9%BA%BB%BC%BD%BE%BF"
+        "%C0%C1%C2%C3%C4%C5%C6%C7%C8%C9%CA%CB%CC%CD%CE%CF"
+        "%D0%D1%D2%D3%D4%D5%D6%D7%D8%D9%DA%DB%DC%DD%DE%DF"
+        "%E0%E1%E2%E3%E4%E5%E6%E7%E8%E9%EA%EB%EC%ED%EE%EF"
+        "%F0%F1%F2%F3%F4%F5%F6%F7%F8%F9%FA%FB%FC%FD%FE%FF");
+}
+
 BOOST_AUTO_TEST_SUITE_END()
    fjahr commented at 8:35 PM on April 11, 2026:

    Oh, nice, thanks. Weird, I feel like I had written a small unit test at some point but I guess it got lost and it certainly wasn't that exhaustive anyway.

    fjahr commented at 8:35 PM on April 11, 2026:

    Sure, changed.

  42. fjahr force-pushed on Apr 11, 2026
  43. fjahr commented at 8:36 PM on April 11, 2026: contributor

    Addressed feedback from @hodlinator , thanks a lot for the review!

  44. in src/bitcoin-cli.cpp:78 in fdf907d8cd 
      73 | @@ -74,6 +74,66 @@ static const std::string DEFAULT_NBLOCKS = "1";
  74 |  /** Default -color setting. */
  75 |  static const std::string DEFAULT_COLOR_SETTING{"auto"};
  76 |  
  77 | +/** Maximum size of http response body (32 MB) */
  78 | +static constexpr size_t MAX_BODY_SIZE = 32 * 1024 * 1024;
    hodlinator commented at 12:43 PM on April 16, 2026:

    Could instead reuse MAX_SIZE from /src/serialize.h or move this one into the bitcoin_http namespace and use 32_MiB?

    fjahr commented at 8:17 AM on April 21, 2026:

    I am using 32_MiB now but I left this a separate value since I am not sure if we want to tie this to MAX_SIZE or to the http namespace. I would say this is a cli-specific policy that should be in the file.

  45. in src/bitcoin-cli.cpp:257 in fdf907d8cd 
     248 | @@ -198,68 +249,23 @@ static int AppInitRPC(int argc, char* argv[])
 249 |      return CONTINUE_EXECUTION;
 250 |  }
 251 |  
 252 | +enum HTTPError {
 253 | +    HTTP_NO_ERROR = -1,
 254 | +    HTTP_TIMEOUT = 0,
 255 | +    HTTP_READ_ERROR = 1,
 256 | +    HTTP_EOF = 2,
 257 | +};
    hodlinator commented at 1:09 PM on April 16, 2026:

    nit: Cleaner way to avoid collisions with enum HTTPStatusCode from /src/rpc/protocol.h which has things like HTTP_OK, and also to avoid the EOF #define. Also use more specific enum name:

    enum HTTPReplyError {
    Ok,
    Timeout,
    ReadError,
    Eof,
};

    https://github.com/hodlinator/bitcoin/commit/58dddfd908c4531c4f5155246e52b252d314cf10

    fjahr commented at 8:18 AM on April 21, 2026:

    Done and also switched to enum class along with the change.

  46. in src/bitcoin-cli.cpp:92 in fdf907d8cd 
      87 | +//! And libevent http.c evhttp_parse_headers_()
  88 | +constexpr size_t MAX_HEADERS_SIZE{8192};
  89 | +
  90 | +using Headers = std::vector<std::pair<std::string, std::string>>;
  91 | +
  92 | +static Headers Read(util::LineReader& reader)
    hodlinator commented at 1:37 PM on April 16, 2026:

    nit: Somewhat less surprising name for a free function?

    static Headers ReadHeaders(util::LineReader& reader)
    fjahr commented at 8:18 AM on April 21, 2026:

    Sure, I deliberately tried to keep this close to the functions from #32061 but as I am tending more towards not sharing this code with the server implementation it's fine for the functions to diverge in name as well. I added a comment about this though.

  47. in src/bitcoin-cli.cpp:1172 in fdf907d8cd 
    1167 | +
1168 | +    reply.error = HTTP_NO_ERROR;
1169 | +    return reply;
1170 | +}
1171 | +
1172 | +HTTPReply HTTPClient::Post(const std::string& endpoint,
    hodlinator commented at 2:21 PM on April 16, 2026:

    nit: Would expect the Post() implementation to be defined before ReadResponse() (both to conform with order of operations and class definition). ReadResponse() and WaitForReadable() also have mismatching declaration/definition orders.

    fjahr commented at 8:18 AM on April 21, 2026:

    Switched the ordering.

  48. in src/bitcoin-cli.cpp:1296 in fdf907d8cd 
    1308 | +                responseErrorMessage = " (timeout)";
1309 | +            } else if (response.error == HTTP_READ_ERROR) {
1310 | +                responseErrorMessage = " (read error)";
1311 | +            } else if (response.error == HTTP_EOF) {
1312 | +                responseErrorMessage = " (EOF)";
1313 | +            }
    hodlinator commented at 2:27 PM on April 16, 2026:

    nit: More concise layout + compiler warning for missing case if enum value is added:

            switch (response.error) {
        case Ok:                                                break;
        case Timeout:   responseErrorMessage = " (timeout)";    break;
        case ReadError: responseErrorMessage = " (read error)"; break;
        case Eof:       responseErrorMessage = " (EOF)";        break;
        }
    fjahr commented at 8:18 AM on April 21, 2026:

    Taken

  49. in src/bitcoin-cli.cpp:881 in fdf907d8cd 
     876 | +
 877 | +bool HTTPClient::SendRequest(Sock& sock, const std::string& request)
 878 | +{
 879 | +    size_t total_sent = 0;
 880 | +    const char* data = request.data();
 881 | +    size_t remaining = request.size();
    hodlinator commented at 8:47 PM on April 16, 2026:

    Could simplify using string_view, see https://github.com/hodlinator/bitcoin/commit/786d81c73faa4f6224bb786341815457d30655ae

    bool HTTPClient::SendRequest(Sock& sock, std::string_view request)
{
    fjahr commented at 8:18 AM on April 21, 2026:

    Yeah, that looks nice, taken.

  50. in src/bitcoin-cli.cpp:927 in fdf907d8cd 
     922 | +}
 923 | +
 924 | +HTTPReply HTTPClient::ReadResponse(Sock& sock)
 925 | +{
 926 | +    HTTPReply reply;
 927 | +    std::vector<std::byte> buffer;
    hodlinator commented at 8:50 PM on April 16, 2026:

    Never thought I'd say this but avoiding std::byte makes the code simpler - https://github.com/hodlinator/bitcoin/commit/4eb9384dce73922ddb2ce83b6807ad1006be6c7d:

        std::string buffer;
    fjahr commented at 8:18 AM on April 21, 2026:

    Looks good, taken as well.

  51. in src/bitcoin-cli.cpp:950 in fdf907d8cd 
     945 | +        }
 946 | +
 947 | +        if (!WaitForReadable(sock, time_left)) {
 948 | +            reply.error = HTTP_TIMEOUT;
 949 | +            return reply;
 950 | +        }
    hodlinator commented at 8:52 PM on April 16, 2026:

    There are a bunch of seemingly redundant timeout checks:

            auto time_left = std::chrono::duration_cast<std::chrono::milliseconds>(
            deadline - std::chrono::steady_clock::now());
        if (time_left.count() <= 0 || !WaitForReadable(sock, time_left)) {
            reply.error = HTTP_TIMEOUT;
            return reply;
        }

    See: https://github.com/hodlinator/bitcoin/commit/8cb27d08d3b35b92f46c2f86a635eca76a231a7b

    fjahr commented at 8:18 AM on April 21, 2026:

    Simplified the timeout checks

  52. in src/bitcoin-cli.cpp:1185 in fdf907d8cd 
    1180 | +
1181 | +        // Build HTTP request
1182 | +        std::string request = strprintf("POST %s HTTP/1.1\r\n", endpoint);
1183 | +        request += strprintf("Host: %s\r\n", m_host);
1184 | +        request += "Connection: close\r\n";
1185 | +        request += "Content-Length: " + ToString(body.size()) + "\r\n";
    hodlinator commented at 8:54 PM on April 16, 2026:

    nanonit: Could merge strprintf() - see https://github.com/hodlinator/bitcoin/commit/d108d7e0ad08e8b2768d664b0e61a8094b0d864b:

            std::string request = strprintf("POST %s HTTP/1.1\r\n"
                                        "Host: %s\r\n"
                                        "Connection: close\r\n"
                                        "Content-Length: %d\r\n",
                                        endpoint, m_host, body.size());
    fjahr commented at 8:18 AM on April 21, 2026:

    Sure, done.

  53. hodlinator commented at 8:59 PM on April 16, 2026: contributor

    Concept ACK fdf907d8cd6b2940c1c74bdf4a24670e8314b320

    Edit: My suggestions branch: https://github.com/bitcoin/bitcoin/compare/master...hodlinator:bitcoin:pr/34342_suggestions

  54. fjahr force-pushed on Apr 21, 2026
  55. fjahr commented at 8:19 AM on April 21, 2026: contributor

    Addressed the latest comments from @hodlinator , thanks again for these thoughtful suggestions!

  56. fanquake commented at 9:10 AM on April 21, 2026: member

    You could also apply this diff:

    diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
index b37f3c8ca5..13cd749727 100644
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -357,8 +357,6 @@ if(BUILD_CLI)
     bitcoin_common
     bitcoin_ipc
     bitcoin_util
-    libevent::core
-    libevent::extra
   )
   install_binary_component(bitcoin-cli HAS_MANPAGE)
 endif()
  57. fjahr commented at 9:31 AM on April 21, 2026: contributor

    You could also apply this diff:

    Done, thanks!

  58. in src/bitcoin-cli.cpp:1096 in 8959ac8a1e 
    1091 | +            // Need more data
1092 | +            auto time_left = std::chrono::duration_cast<std::chrono::milliseconds>(
1093 | +                deadline - std::chrono::steady_clock::now());
1094 | +            if (time_left.count() <= 0 || !WaitForReadable(sock, time_left)) {
1095 | +                reply.error = HTTPReplyError::Timeout;
1096 | +                return reply;
    hodlinator commented at 1:46 PM on April 21, 2026:

    Potential bug - If we return here with a timeout, reply.status will have been set to non-zero above, which means if (response.status == 0) in the outer scope will fail, is that what we want?

    Seems almost like we have a case for util::Expected:

    struct HTTPReply
{
    HTTPReply() = default;

    util::Expected<int, HTTPReplyError> status{0};
    std::string body;
};

    allowing us to remove HTTPReplyError::Ok.

    fjahr commented at 11:37 AM on April 25, 2026:

    Yeah, great catch on the bug, I addressed this along with the larger refactor that removed the custom error types. Please let me know if you are also happy with this simplification.

    hodlinator commented at 9:12 AM on April 28, 2026:

    Yes, while I dislike exceptions I do think this should work. At least it's all happening within one file, and it allows deferring the rabbit hole I went down in order to make util::Expected usable.

  59. in src/bitcoin-cli.cpp:1247 in 8959ac8a1e 
    1255 | -    evbuffer_add(output_buffer, strRequest.data(), strRequest.size());
1256 |  
1257 | -    int r = evhttp_make_request(evcon.get(), req.release(), EVHTTP_REQ_POST, endpoint.c_str());
1258 | -    if (r != 0) {
1259 | -        throw CConnectionFailed("send http request failed");
1260 | +    HTTPReply response;
    hodlinator commented at 2:04 PM on April 21, 2026:

    nanonit: Consider renaming the type to match the variable and common HTTP "request"/"response" nouns. Would also go well with being returned from ReadResponse().

        HTTPResponse response;
    fjahr commented at 11:37 AM on April 25, 2026:

    Yeah, the inconsistency in the naming bothers me more than the correctness actually, done.

  60. in src/bitcoin-cli.cpp:869 in 8959ac8a1e 
     864 | +
 865 | +HTTPReply HTTPClient::Post(const std::string& endpoint,
 866 | +                           const std::vector<std::pair<std::string, std::string>>& headers,
 867 | +                           const std::string& body)
 868 | +{
 869 | +    HTTPReply reply;
    hodlinator commented at 8:51 AM on April 22, 2026:

    nit: Could remove this named instance:

    Then switch the ReadResponse() call to return directly:

    -        reply ReadResponse(*sock);
- 
+        return ReadResponse(*sock);

    ...and remove return reply at the end since we would only get there if an exception not inheriting from std::exception would be thrown.

    fjahr commented at 11:37 AM on April 25, 2026:

    Done

  61. in src/bitcoin-cli.cpp:950 in 8959ac8a1e 
     945 | +
 946 | +HTTPReply HTTPClient::ReadResponse(Sock& sock)
 947 | +{
 948 | +    HTTPReply reply;
 949 | +    std::string buffer;
 950 | +    auto deadline = std::chrono::steady_clock::now() + m_timeout;
    hodlinator commented at 8:55 AM on April 22, 2026:

    nit: Could add const since this has a large scope:

        const auto deadline{std::chrono::steady_clock::now() + m_timeout};

    Probably same in SendRequest() as well.

    fjahr commented at 11:37 AM on April 25, 2026:

    Done in both places

  62. in src/bitcoin-cli.cpp:1162 in 8959ac8a1e 
    1157 | +    } else {
1158 | +        // No body or read until connection close
1159 | +        reply.body = std::move(buffer);
1160 | +    }
1161 | +
1162 | +    reply.error = HTTPReplyError::Ok;
    hodlinator commented at 8:57 AM on April 22, 2026:

    It would be good to detect whether we're accidentally overwriting an error, could change to checking the default value instead:

        assert(reply.error == HTTPReplyError::Ok);
    fjahr commented at 11:37 AM on April 25, 2026:

    Taken but using Assume() because it seems to be a better fit for what should be a developer error.

  63. in .github/ci-test-each-commit-exec.py:43 in 567fb60f32 outdated 
      39 | @@ -40,8 +40,8 @@ def main():
  40 |          "-DCMAKE_BUILD_TYPE=Debug",
  41 |          "-DCMAKE_COMPILE_WARNING_AS_ERROR=ON",
  42 |          "--preset=dev-mode",
  43 | -        # Tolerate unused member functions in intermediate commits in a pull request
  44 | -        "-DCMAKE_CXX_FLAGS=-Wno-error=unused-member-function",
  45 | +        # Tolerate unused (member) functions in intermediate commits in a pull request
    pinheadmz commented at 1:31 PM on April 22, 2026:

    567fb60f329402bd855f7b3eaaabf7da4356f402

    nit in commit message (x2): s/intermittend/intermediate

    fjahr commented at 11:37 AM on April 25, 2026:

    Fixed

  64. in src/bitcoin-cli.cpp:82 in 29ef3d51bc 
      75 | @@ -74,6 +76,64 @@ static const std::string DEFAULT_NBLOCKS = "1";
  76 |  /** Default -color setting. */
  77 |  static const std::string DEFAULT_COLOR_SETTING{"auto"};
  78 |  
  79 | +// TODO: These helpers may be replaced with the corresponding methods in HTTPHeaders
  80 | +// from https://github.com/bitcoin/bitcoin/pull/32061 if that is moved to a shared
  81 | +// location.
  82 | +namespace bitcoin_http {
    pinheadmz commented at 1:48 PM on April 22, 2026:

    29ef3d51bcd43ea9dc148a2a7ee64b40acd4ae8c

    Just noting we use http_bitcoin (word order reversed in #32061). Feels ok for now.

    hodlinator commented at 7:38 PM on April 22, 2026:

    @pinheadmz it's the result of #34342 (review), I think #32061 should be adjusted as well. This is bitcoin( core)'s HTTP subdomain, not HTTP's bitcoin subdomain. WDYT?

    fjahr commented at 11:38 AM on April 25, 2026:

    This led me to revisit the headers code again. Initially I tried to keep everything as close to the original server code because I expected server would move forward quicker and should be merged by the time cli was even ready for review. I then expected this code be replaced with the http server header code that would be moved to a shared location instead in this commit.

    However, the situation has changed a bit: This PR has the chance to get in before the server one and I am not so sure anymore sharing this little piece of code (two functions) justifies moving the headers code out of the server. It felt awkward from the beginning and I couldn't find a way yet to make that go away. Also, based on some feedback the code already drifted a bit apart from the http server implementation.

    So my conclusion is that this code should rather be written in a way that it could potentially stick around much longer and only try to mirror the interface from the server more losely. It shouldn't be much of a problem to replace the code here with the shared headers code in future still but implementing it with this different mindset makes me feel much better about moving forward with this indepently from the server and the deduplication is something that can be done at any time after libevent is already a part of history.

    Let me know what you think @pinheadmz @hodlinator

    hodlinator commented at 8:55 AM on April 27, 2026:

    Would make sense to share headers logic eventually, but no strong opinion on when. Indeed it's a bit of a chicken/egg problem.

    Seems fine to not introduce the namespace for now.

    It might be nice to move HTTPResponseHeaders and HTTPClient into a header-file in order to add unit test coverage. But maybe functional test coverage of bitcoin-cli is sufficient.

    Seems like libevent-versions of bitcoind use unchunked evhttp_send_reply() while evhttp_send_reply_start() would have been used for chunking. It seems we never return chunked responses in HTTPRequest::WriteReply from #32061 either. My LLM tells me HTTP proxies can introduce chunking though, so probably best to keep it around.

  65. hodlinator commented at 1:52 PM on April 22, 2026: contributor

    Reviewed 8959ac8a1ef42aecb08b9f0cf9d7d2324edc5c90

    Thanks for incorporating my earlier feedback! Nice that you could remove chunk_buffer and use .append().

    I feel like it would be good to only use one error handling mechanism for the new code in HTTPClient. The current version mixes HTTPReplyError and exceptions. My preference would be to use util::Expected which while verbose, clearly documents outcomes of calling a function (unlike exceptions). IIRC there was some issue recently found in one of the libevent removal PRs where exceptions where not caught, leading to crashing of the node if it had not been found during review.

    My suggestion adds 2-3 preparatory commits to also alter LineReader to no longer throw exceptions.

    New suggestion branch up at: https://github.com/bitcoin/bitcoin/compare/master...hodlinator:bitcoin:pr/34342_suggestions2 Or alternatively with commits layered on top of 8959ac8a1ef42aecb08b9f0cf9d7d2324edc5c90: https://github.com/bitcoin/bitcoin/compare/master...hodlinator:bitcoin:pr/34342_suggestions2_ontop

    Totally understand if you'd rather I propose the changes to LineReader as a separate PR to avoid hijacking this one.

  66. in src/bitcoin-cli.cpp:80 in e0a9dabd69 outdated 
      75 | @@ -76,6 +76,9 @@ static const std::string DEFAULT_NBLOCKS = "1";
  76 |  /** Default -color setting. */
  77 |  static const std::string DEFAULT_COLOR_SETTING{"auto"};
  78 |  
  79 | +/** Maximum size of http response body */
  80 | +static constexpr size_t MAX_BODY_SIZE{32_MiB};
    pinheadmz commented at 1:52 PM on April 22, 2026:

    e0a9dabd69269c38c2be6f05407c346485813fc2

    Could be imported from here as well (thats what I have in 32061 currently)

    https://github.com/bitcoin/bitcoin/blob/3f09a4703f441b183dd1d44a3e1ad4a47a03a79d/src/serialize.h#L30-L34

    hodlinator commented at 7:38 PM on April 22, 2026:

    Same topic: #34342 (review)

    fjahr commented at 11:38 AM on April 25, 2026:

    Yeah, like I said in my response to @hodlinator as well, it seems strange to me to tie this cli (or server) higher level policy to this very low level one. So a local value makes more sense to me, even if it's set to the same size. But I won't die on this hill.

  67. in src/bitcoin-cli.cpp:1254 in e0a9dabd69 
    1264 | +        response.status = 0;
1265 |      }
1266 |  
1267 | -    event_base_dispatch(base.get());
1268 | -
1269 |      if (response.status == 0) {
    pinheadmz commented at 2:19 PM on April 22, 2026:

    e0a9dabd69269c38c2be6f05407c346485813fc2

    Could this be moved into the previous catch block instead of using "magic number" status == 0 ? Then start next block with if instead of else if

    fjahr commented at 11:38 AM on April 25, 2026:

    Refactored and simplified this a lot, dropping the custom error types as well. See my other comment.

  68. in src/bitcoin-cli.cpp:268 in e0a9dabd69 
     265 |  {
 266 |      HTTPReply() = default;
 267 |  
 268 |      int status{0};
 269 | -    int error{-1};
 270 | +    HTTPReplyError error{HTTPReplyError::Ok};
    pinheadmz commented at 2:34 PM on April 22, 2026:

    e0a9dabd69269c38c2be6f05407c346485813fc2

    Just seems a little weird for there to be any default here at all, especially "Ok", but I guess this is simpler than allowing null or making it an optional ?

    When reply.error is read in CallRPC() after catching CConnectionFailed it shouldn't ever be "Ok", right?

    fjahr commented at 11:38 AM on April 25, 2026:

    This and the other comment on the catch blocks inspired me to rework this and I think I was able to simplify it a lot without losing clarity. There is now no more custom error type and I am instead using CConnectionFailed directly. Please let me know what you think :)

  69. in src/bitcoin-cli.cpp:1070 in e0a9dabd69 outdated 
    1065 | +
1066 | +                size_t chunk_size{0};
1067 | +                auto [p, ec] = std::from_chars(size_str.data(), size_str.data() + size_str.size(), chunk_size, /*base=*/16);
1068 | +                if (ec != std::errc{} || p != size_str.data() + size_str.size()) {
1069 | +                    throw std::runtime_error("Invalid chunk size");
1070 | +                }
    pinheadmz commented at 2:41 PM on April 22, 2026:

    e0a9dabd69269c38c2be6f05407c346485813fc2

    I don't expect bitcoind to ever violate this (or reply with chunked encoding at all?) But you could check body.size() + chunk_size > MAX_BODY_SIZE here before even reading from the socket.

    fjahr commented at 11:38 AM on April 25, 2026:

    Added

  70. in src/bitcoin-cli.cpp:917 in e0a9dabd69 
     912 | +    }
 913 | +
 914 | +    throw CConnectionFailed(strprintf("Could not connect to the server %s:%d", m_host, m_port));
 915 | +}
 916 | +
 917 | +bool HTTPClient::SendRequest(Sock& sock, std::string_view request)
    pinheadmz commented at 3:04 PM on April 22, 2026:

    e0a9dabd69269c38c2be6f05407c346485813fc2

    Sonarcloud thinks this should be const? (And WaitForReadable() as well

    fjahr commented at 11:38 AM on April 25, 2026:

    Done, made both const

    hodlinator commented at 6:03 PM on April 28, 2026:

    nanonit: To me it's weird that Sock::Send is const, feels more honest to drop const from SendRequest at least.

  71. pinheadmz approved
  72. pinheadmz commented at 3:06 PM on April 22, 2026: member

    ACK 8959ac8a1ef42aecb08b9f0cf9d7d2324edc5c90

    Built and tested on macos/arm64. Reviewed each commit with claude and my own brain. Ran cli commands from both master and branch through wireshark to compare packet structure.

    I left some non-blocking nits below. Also not blocking is a broader concern of missing code coverage around chunked-encoding and most of the errors.

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

    -----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

ACK 8959ac8a1ef42aecb08b9f0cf9d7d2324edc5c90
-----BEGIN PGP SIGNATURE-----

iQJPBAEBCAA5FiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmno44gbFIAAAAAABAAO
bWFudTIsMi41KzEuMTIsMCwzAAoJEOfimEtiick67VkP/izTvVkSntzO7/CxQuIO
jId+AONIVeVgEC6zKhd7ZfKyG6uaMNK8RfED+Uir3xujdM0bu3HxwGGFAwIxwzn+
bvPc5Bld3vU9gdu00HGj5KM51x+u6ci0lVd2EeYYXLU0NJz5toFV9vSB7hmfk9LQ
opK3fCbhqlGkPcxrkuVq1W85yS68SUCVsgN/2LXJTyCLtgTsowRin0edhnTdth1X
9iPZRj0yUTQ3nP9MhjeZ6ILQq5J5ayycQ1q4389IRFZ9c1DWLbDI60QrL8bBkmO+
TJoVdYMwq2ADjsaXUpsnpNJOvhkF0ILMEYzzrfcu3s7aFTTKEfFF+wm+iXkIW0E/
5JzRPIpLXcdFxDLe7GULzvS8wHKtAbqOr1ajHO4wf3Uu4FTLuLFcna5nM6nMzCWr
PtIb/2I9kKkVimUal5EX3j5IeSEGFnCCuKapvfEJwr343/qcG9mrrVmmuFoA5UXY
EpaA3AOick48Y9DHeKMDc1bdD6T0H+h/Z9WSx3PsGAprZGFQ/2IZbYidhCWADtAq
WCgggkLWnksFmqGt3D0i2Bzowlam9eg4AoHhDzieo8JATXzRjUC96T36kQUKNsJL
1felydBH7BERL+hsuvGzwMMakKTYpjr1VPdI9fyw2LRKZRosUCyYEQAblg3eyh6N
usjDuUCESwtHMeURzK+UYFYB
=qZUo
-----END PGP SIGNATURE-----

    pinheadmz's public key is on openpgp.org

    </details>

  73. DrahtBot requested review from bradleystachurski on Apr 22, 2026
  74. DrahtBot requested review from hodlinator on Apr 22, 2026
  75. theStack commented at 7:12 PM on April 22, 2026: contributor

    Concept ACK

    Looks like the raii_evhttp_{request,connection} types and obtain functions could also be removed after e0a9dabd69269c38c2be6f05407c346485813fc2:

    diff --git a/src/support/events.h b/src/support/events.h
index f89daf6d38..a118ba32b5 100644
--- a/src/support/events.h
+++ b/src/support/events.h
@@ -24,8 +24,6 @@ typedef std::unique_ptr<struct type, type##_deleter> raii_##type
 MAKE_RAII(event_base);
 MAKE_RAII(event);
 MAKE_RAII(evhttp);
-MAKE_RAII(evhttp_request);
-MAKE_RAII(evhttp_connection);

 inline raii_event_base obtain_event_base() {
     auto result = raii_event_base(event_base_new());
@@ -42,15 +40,4 @@ inline raii_evhttp obtain_evhttp(struct event_base* base) {
     return raii_evhttp(evhttp_new(base));
 }

-inline raii_evhttp_request obtain_evhttp_request(void(*cb)(struct evhttp_request *, void *), void *arg) {
-    return raii_evhttp_request(evhttp_request_new(cb, arg));
-}
-
-inline raii_evhttp_connection obtain_evhttp_connection_base(struct event_base* base, std::string host, uint16_t port) {
-    auto result = raii_evhttp_connection(evhttp_connection_base_new(base, nullptr, host.c_str(), port));
-    if (!result.get())
-        throw std::runtime_error("create connection failed");
-    return result;
-}
-
 #endif // BITCOIN_SUPPORT_EVENTS_H
  76. ci: Tolerate unused free functions in intermediate commits
    When bigger changes are split across multiple commits, intermediate
commits may introduce unused functions. Do not check for this error
in intermediate commits.
    ba36bc3937
  77. common: Add unused UrlEncode function
    Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>
    e99dc3460c
  78. cli: Add HTTPResponseHeaders class for parsing response headers
    Adds a small class to parse and query the field lines of an HTTP
response. Used by the upcoming libevent-free HTTPClient implementation.
    24f4f44558
  79. cli: Remove libevent usage
    This also removes the now-unused raii_evhttp_{request,connection}
helpers from support/events.h.
    0d17cbbc62
  80. build: Drop libevent from bitcoin-cli link libraries
    Co-authored-by: fanquake <fanquake@gmail.com>
    82ea87ff81
  81. in .github/ci-test-each-commit-exec.py:44 in 567fb60f32 outdated 
      39 | @@ -40,8 +40,8 @@ def main():
  40 |          "-DCMAKE_BUILD_TYPE=Debug",
  41 |          "-DCMAKE_COMPILE_WARNING_AS_ERROR=ON",
  42 |          "--preset=dev-mode",
  43 | -        # Tolerate unused member functions in intermediate commits in a pull request
  44 | -        "-DCMAKE_CXX_FLAGS=-Wno-error=unused-member-function",
  45 | +        # Tolerate unused (member) functions in intermediate commits in a pull request
  46 | +        "-DCMAKE_CXX_FLAGS=-Wno-error=unused-member-function -Wno-error=unused-function",
    theStack commented at 7:40 PM on April 22, 2026:

    meta nit: I guess now that 2754c9217686dc31c15c20e22b6ebaf1560b48a0 includes a unit test and thus UrlEncode is used in the same commit, adding this compile flag wouldn't be strictly needed anymore? (Seems fine to keep the commit anyway though, as it's probably only a question of time until another PR would run into the same issue).

    fjahr commented at 11:39 AM on April 25, 2026:

    I think the header helper functions would have triggered it as well but then also I have turned these functions into methods now, suspecting that reviewers will agree with this change you are still right :) But I would leave it in unless anyone sees a downside to it and maybe want to enforce/encourage test coverage this way.

  82. fjahr force-pushed on Apr 25, 2026
  83. fjahr commented at 11:39 AM on April 25, 2026: contributor

    Looks like the raii_evhttp_{request,connection} types and obtain functions could also be removed after https://github.com/bitcoin/bitcoin/commit/e0a9dabd69269c38c2be6f05407c346485813fc2

    Done, makes sense to not leave dead code lying around.

  84. fjahr commented at 11:41 AM on April 25, 2026: contributor

    Addressed review comments from @hodlinator @pinheadmz @theStack , thank you all for the great reviews!

    This diff is now a bit bigger that I thought at first. I hope you still all agree that the changes are worth it. At the very least this latest push gets rid of some more lines of code :)

    $ git diff 8959ac8a1ef42aecb08b9f0cf9d7d2324edc5c90 --shortstat
 2 files changed, 68 insertions(+), 106 deletions(-)
  85. in src/bitcoin-cli.cpp:846 in 82ea87ff81 
     841 | +    HTTPClient(const std::string& host, uint16_t port, std::chrono::seconds timeout)
 842 | +        : m_host(host), m_port(port), m_timeout(timeout) {}
 843 | +
 844 | +    HTTPResponse Post(const std::string& endpoint,
 845 | +                   const std::vector<std::pair<std::string, std::string>>& headers,
 846 | +                   const std::string& body);
    hodlinator commented at 12:16 PM on April 28, 2026:

    nanonit: Misaligned after Reply -> Response rename here and in implementation:

                          const std::vector<std::pair<std::string, std::string>>& headers,
                      const std::string& body);
  86. in src/bitcoin-cli.cpp:1054 in 82ea87ff81 
    1049 | +                    size_str = size_str.substr(0, semi);
1050 | +                }
1051 | +
1052 | +                size_t chunk_size{0};
1053 | +                auto [p, ec] = std::from_chars(size_str.data(), size_str.data() + size_str.size(), chunk_size, /*base=*/16);
1054 | +                if (ec != std::errc{} || p != size_str.data() + size_str.size()) {
    hodlinator commented at 12:30 PM on April 28, 2026:

    nit: Seems better to do something closer to new httpserver.cpp.

    • Avoid platform-dependent size_t
    • Trim whitespace
    • Use our own wrapper function with less args:
                    const auto chunk_size{ToIntegral<uint64_t>(util::TrimStringView(size_str), /*base=*/16)};
                if (!chunk_size) {
                    throw std::runtime_error("Invalid chunk size");
                }
  87. in src/bitcoin-cli.cpp:1127 in 82ea87ff81 
    1122 | +            ssize_t nrecv = sock.Recv(recv_buf, sizeof(recv_buf), /*flags=*/0);
1123 | +
1124 | +            if (nrecv < 0) {
1125 | +                int err = WSAGetLastError();
1126 | +                if (err == WSAEWOULDBLOCK || err == WSAEINTR) {
1127 | +                    continue;
    hodlinator commented at 2:58 PM on April 28, 2026:

    nit: Could make this loop slightly less hot in case we have a choppy connection / slow bitcoind:

                        std::this_thread::yield();
                    continue;

    See https://github.com/hodlinator/bitcoin/commit/c6300f760ee7b81b1962c8802e2bee0af58330e2

  88. in src/bitcoin-cli.cpp:925 in 0d17cbbc62 
     920 | +        }
 921 | +
 922 | +        ssize_t sent = sock.Send(request.data(), request.size(), MSG_NOSIGNAL);
 923 | +        if (sent < 0) {
 924 | +            int err = WSAGetLastError();
 925 | +            if (err == WSAEWOULDBLOCK || err == WSAEINTR) {
    theStack commented at 4:19 PM on April 28, 2026:

    in 0d17cbbc62cf8c095880cfb43b78f07e5bb4b97e: looking at similar places in the existing code base where socket errors are handled with a simple "try again", I found that in TorControlConnection::ReceiveAndProcess we also check for WSAEINPROGRESS. Is this also needed here, or generally only relevant for receiving? (According to the manpages, EINPROGRESS can only occur in connect on UNIX-like systems, but not sure if that's also the case on Windows systems).

  89. in src/test/common_url_tests.cpp:77 in e99dc3460c 
      72 | @@ -72,4 +73,41 @@ BOOST_AUTO_TEST_CASE(decode_internal_nulls_test) {
  73 |      BOOST_CHECK_EQUAL(UrlDecode("abc%00%00"), result2);
  74 |  }
  75 |  
  76 | +BOOST_AUTO_TEST_CASE(url_encode) {
  77 | +    BOOST_CHECK_EQUAL(UrlEncode(std::string_view{
    theStack commented at 4:24 PM on April 28, 2026:

    in e99dc3460c22016ca16f656f63594102b7cf655a: nit: could go further and do a Url{Encode,Decode} round-trip test, something like e.g.

    diff --git a/src/test/common_url_tests.cpp b/src/test/common_url_tests.cpp
index 97d7598c0c..33e80c82dd 100644
--- a/src/test/common_url_tests.cpp
+++ b/src/test/common_url_tests.cpp
@@ -74,7 +74,7 @@ BOOST_AUTO_TEST_CASE(decode_internal_nulls_test) {
 }

 BOOST_AUTO_TEST_CASE(url_encode) {
-    BOOST_CHECK_EQUAL(UrlEncode(std::string_view{
+    auto to_encode = std::string_view{
         "\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f"
         "\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f"
         "\x20\x21\x22\x23\x24\x25\x26\x27\x28\x29\x2a\x2b\x2c\x2d\x2e\x2f"
@@ -91,7 +91,8 @@ BOOST_AUTO_TEST_CASE(url_encode) {
         "\xd0\xd1\xd2\xd3\xd4\xd5\xd6\xd7\xd8\xd9\xda\xdb\xdc\xdd\xde\xdf"
         "\xe0\xe1\xe2\xe3\xe4\xe5\xe6\xe7\xe8\xe9\xea\xeb\xec\xed\xee\xef"
         "\xf0\xf1\xf2\xf3\xf4\xf5\xf6\xf7\xf8\xf9\xfa\xfb\xfc\xfd\xfe\xff",
-        256}),
+        256};
+    auto expected_encoded =
         "%00%01%02%03%04%05%06%07%08%09%0A%0B%0C%0D%0E%0F"
         "%10%11%12%13%14%15%16%17%18%19%1A%1B%1C%1D%1E%1F"
         "%20%21%22%23%24%25%26%27%28%29%2A%2B%2C-.%2F"
@@ -107,7 +108,9 @@ BOOST_AUTO_TEST_CASE(url_encode) {
         "%C0%C1%C2%C3%C4%C5%C6%C7%C8%C9%CA%CB%CC%CD%CE%CF"
         "%D0%D1%D2%D3%D4%D5%D6%D7%D8%D9%DA%DB%DC%DD%DE%DF"
         "%E0%E1%E2%E3%E4%E5%E6%E7%E8%E9%EA%EB%EC%ED%EE%EF"
-        "%F0%F1%F2%F3%F4%F5%F6%F7%F8%F9%FA%FB%FC%FD%FE%FF");
+        "%F0%F1%F2%F3%F4%F5%F6%F7%F8%F9%FA%FB%FC%FD%FE%FF";
+    BOOST_CHECK_EQUAL(UrlEncode(to_encode), expected_encoded);
+    BOOST_CHECK_EQUAL(UrlDecode(expected_encoded), to_encode);
 }

 BOOST_AUTO_TEST_SUITE_END()

    (But something more generic applying to all existing tests could be done in a separate PR).

  90. in src/bitcoin-cli.cpp:1094 in 82ea87ff81 
    1089 | +            ssize_t nrecv = sock.Recv(recv_buf, sizeof(recv_buf), /*flags=*/0);
1090 | +
1091 | +            if (nrecv < 0) {
1092 | +                int err = WSAGetLastError();
1093 | +                if (err == WSAEWOULDBLOCK || err == WSAEINTR) {
1094 | +                    continue;
    hodlinator commented at 5:29 PM on April 28, 2026:

    Feels a bit wasteful to perform the upper half of the loop around the chunk data when we have not appended to the buffer. Maybe this receive logic could be put inside an inner while loop? Suggestion: https://github.com/hodlinator/bitcoin/commit/b915e626b1f4b4934e57262a29357ddac4a07549

  91. in src/bitcoin-cli.cpp:854 in 82ea87ff81 
     849 | +    std::string m_host;
 850 | +    uint16_t m_port;
 851 | +    std::chrono::seconds m_timeout;
 852 | +
 853 | +    std::unique_ptr<Sock> Connect();
 854 | +    bool SendRequest(Sock& sock, std::string_view request) const;
    hodlinator commented at 5:35 PM on April 28, 2026:

    nanonit: To me it's weird that Sock::Send is const, feels more honest to drop const:

        bool SendRequest(Sock& sock, std::string_view request);
  92. in src/bitcoin-cli.cpp:1235 in 82ea87ff81 
    1252 | +    HTTPResponse response;
1253 | +    try {
1254 | +        response = client.Post(endpoint, headers, strRequest);
1255 | +    } catch (const CConnectionFailed& e) {
1256 | +        const std::string formatted_error{*e.what() ? strprintf(" (%s)", e.what()) : ""};
1257 |          throw CConnectionFailed(strprintf("Could not connect to the server %s:%d%s\n\n"
    hodlinator commented at 9:11 PM on April 28, 2026:

    nit: Might as well make the error message more accurate since we can fail after successfully establishing a connection:

            throw CConnectionFailed(strprintf("Error while attempting to communicate with server %s:%d%s\n\n"

    This allows the last exception in Connect() to be restored to

        throw CConnectionFailed{"Could not connect to the server"};

    See: https://github.com/hodlinator/bitcoin/commit/f619714d57a0b16a3887b533f6966a1ed78dc015

  93. in src/bitcoin-cli.cpp:1122 in 82ea87ff81 
    1117 | +            if (time_left.count() <= 0 || !WaitForReadable(sock, time_left)) {
1118 | +                throw CConnectionFailed{"timeout"};
1119 | +            }
1120 | +
1121 | +            char recv_buf[4096];
1122 | +            ssize_t nrecv = sock.Recv(recv_buf, sizeof(recv_buf), /*flags=*/0);
    hodlinator commented at 9:41 PM on April 28, 2026:

    Realized this timeout+Recv-logic is duplicated 3 times and could be extracted, see: https://github.com/hodlinator/bitcoin/commit/52bea0db58f5b18d49450bcca4f111402320e9aa

  94. in src/bitcoin-cli.cpp:856 in 82ea87ff81 
     851 | +    std::chrono::seconds m_timeout;
 852 | +
 853 | +    std::unique_ptr<Sock> Connect();
 854 | +    bool SendRequest(Sock& sock, std::string_view request) const;
 855 | +    HTTPResponse ReadResponse(Sock& sock);
 856 | +    bool WaitForReadable(Sock& sock, std::chrono::milliseconds timeout) const;
    hodlinator commented at 9:43 PM on April 28, 2026:

    nit: Seems like the socket belongs as a member rather than being sent into these 3 methods: https://github.com/hodlinator/bitcoin/commit/093a8a60b1f252c3ba78a6908cc65347d97217fd

  95. in src/bitcoin-cli.cpp:887 in 82ea87ff81 
     882 | +
 883 | +        return ReadResponse(*sock);
 884 | +    } catch (const CConnectionFailed&) {
 885 | +        throw;
 886 | +    } catch (const std::exception& e) {
 887 | +        throw CConnectionFailed(strprintf("HTTP error: %s", e.what()));
    hodlinator commented at 9:45 PM on April 28, 2026:

    nit: More correct to introduce our own exception type instead of throwing naked runtime_errors?

        } catch (const HTTPError& e) {
        throw CConnectionFailed(strprintf("HTTP error: %s", e.what()));

    See: https://github.com/hodlinator/bitcoin/commit/c5809f101e6354e5ce47c2b357af816f077827d2

  96. in src/bitcoin-cli.cpp:885 in 82ea87ff81 
     880 | +            throw CConnectionFailed("Failed to send HTTP request");
 881 | +        }
 882 | +
 883 | +        return ReadResponse(*sock);
 884 | +    } catch (const CConnectionFailed&) {
 885 | +        throw;
    hodlinator commented at 9:45 PM on April 28, 2026:

    nit: Could remove redundant rethrow (unless you want to keep it as documentation?):

  97. in src/bitcoin-cli.cpp:1320 in 82ea87ff81
    hodlinator commented at 9:48 PM on April 28, 2026:

    nit: You're not (yet) touching these lines in this PR, but while testing I couldn't help but notice this prefix seemed uncalled for. Would change to:

                } else if (fWait) {
                throw CConnectionFailed(strprintf("timeout on transient error: %s", e.what()));
            } else {
                throw;
            }

    Edit: Example output without the above fix:

    â‚¿ ./build/bin/bitcoin-cli -rpcport=123 help
error: timeout on transient error: Error while attempting to communicate with server 127.0.0.1:123:  (Could not connect to the server 127.0.0.1:123)

Make sure the bitcoind server is running and that you are connecting to the correct RPC port.
Use "bitcoin-cli -help" for more info.
  98. hodlinator commented at 9:56 PM on April 28, 2026: contributor

    Reviewed 82ea87ff81eac21fa37c96e21762241c589e71ec

    Built with ENABLE_IPC off. Tested incorrect args such as invalid ports:

    ./build/bin/bitcoin-cli help
./build/bin/bitcoin rpc help
./build/bin/bitcoin-cli -rpcport=123 help
./build/bin/bitcoin rpc -rpcport=123 help
Contributors
fjahrDrahtBotpinheadmzbradleystachurskifanquakew0xltmaflckohodlinatortheStack


bradleystachurski

Labels
Scripts and tools
Linked (view graph)
#31194 Remove libevent as a dependency (HTTP / cli / torcontrol)#32061 Replace libevent with our own HTTP and socket-handling implementation#34411 [POC] Full Libevent removal
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: 2026-05-02 12:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me