test: modernize interface_http and cover more libevent behavior

pinheadmz commented at 11:07 pm on March 7, 2026: member

This is a follow-up to #32408 and a new prerequisite for #32061 in response to a few review comments there.

New test: `check_server_busy_idle_timeout()`

In #32061 (review) it is pointed out that the idle timeout set by -rpcservertimeout could disconnect a client unfairly if it was the server that was taking too long to process a response. That misbehavior was confirmed and #32061 was updated to match current libevent behavior. This PR asserts the current libevent behavior by adding another test using RPC waitforblock past the -rpcservertimeout value and then verifying that the HTTP connection is still open.

Expanded tests: `check_excessive_request_size()` and `check_chunked_transfer()`

#32061 (review) made me realize that I was not testing HTTPRequest body size at all, and that headers size was being tested one line at a time but not in total. The libevent server does both things so that behavior is asserted in these expanded tests, and the mechanism in #32061 was improved to match.

Clean up `interface_http.py`

Since I am extending this test module again I refactored the monolithic test to resemble the current functional test style in the repository, de-duplicating HTTP connection code with a helper class and separating individual test cases into functions.

DrahtBot added the label Tests on Mar 7, 2026

DrahtBot commented at 11:08 pm on March 7, 2026: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	fjahr
Stale ACK	Bortlesboat

If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#32061 (Replace libevent with our own HTTP and socket-handling implementation by pinheadmz)

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.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

single reuqest -> single request [Typo: “reuqest” makes the comment harder to understand.]

Possible places where comparison-specific test macros should replace generic comparisons:

[test/functional/interface_http.py] assert duration >= seconds - 1, f"Server disconnected too fast: {duration} < {seconds}" -> Replace with assert_greater_than_or_equal(duration, seconds - 1) to use the comparison-specific helper.

2026-03-26 11:27:12

pinheadmz force-pushed on Mar 7, 2026

DrahtBot added the label CI failed on Mar 7, 2026

DrahtBot removed the label CI failed on Mar 8, 2026

Bortlesboat commented at 6:59 pm on March 8, 2026: none

Tested ACK b2bfeabf4c. Checked out the branch, ran interface_http.py locally — all passes. The BitcoinHTTPConnection helper cleans up the test nicely and the new check_server_busy_idle_timeout case is a good addition for the rpcservertimeout behavior. Found one minor typo (inline).

in test/functional/interface_http.py:246 in b2bfeabf4c

261+
262+        # Sanity check -- complete requests don't timeout waiting for completion
263+        good_http_request = "GET /test2 HTTP/1.1\r\nHost: somehost\r\n\r\n"
264+        conn.reset_conn()
265+        conn.conn.sock.sendall(good_http_request.encode("utf-8"))
266+        respose2 = conn.recv_raw()

Bortlesboat commented at 6:59 pm on March 8, 2026:

Nit: typo respose2 -> response2

pinheadmz commented at 2:36 pm on March 18, 2026:

Thanks, will fix in next rebase

pinheadmz marked this as a draft on Mar 19, 2026

pinheadmz force-pushed on Mar 23, 2026

pinheadmz renamed this:
~~test: clean up interface_http and improve idle timeout coverage~~
test: modernize interface_http and cover more libevent behavior
on Mar 23, 2026

pinheadmz marked this as ready for review on Mar 23, 2026

DrahtBot added the label CI failed on Mar 23, 2026

pinheadmz force-pushed on Mar 23, 2026

DrahtBot removed the label CI failed on Mar 23, 2026

in test/functional/interface_http.py:44 in af80c9b710 outdated

39+        self.conn.close()
40+
41+    def set_timeout(self, seconds):
42+        self.conn.sock.settimeout(seconds)
43+
44+    def add_header(self, key, value):

fjahr commented at 9:57 pm on March 24, 2026:

It seems this is unused here

pinheadmz commented at 6:24 pm on March 25, 2026:

It is used in a following commit in the PR: test: ensure HTTP server enforces limits on headers and body size L168 in this file. I’ll remove here and add back in the commit where it is used.

in test/functional/interface_http.py:83 in af80c9b710 outdated

78+        # Server disconnected within an acceptable range of time:
79+        # not immediately, and not too far over the configured duration.
80+        # This allows for some jitter in the test between client and server.
81+        duration = stop - start
82+        assert duration <= seconds + 2, f"Server disconnected too slow: {duration} > {seconds}"
83+        assert duration >= seconds - 2, f"Server disconnected too fast: {duration} < {seconds}"

fjahr commented at 10:03 pm on March 24, 2026:

This assertion is as bit funny because with the current values the connection would need to time travel (duration negative) for this to fail :)

pinheadmz commented at 6:54 pm on March 25, 2026:

lol whoops and thanks. I had these boundaries hard-coded to 4 and 1 previously and for some reason I thought that was +/-2 🤷

Changed this to - 1 for the lower bound.

in test/functional/interface_http.py:270 in 7cf55430a5 outdated

265+        # Wait until after the timeout, then generate a block with a second HTTP connection
266+        time.sleep(3)
267+        generated_block = self.generate(self.nodes[2], 1, sync_fun=self.no_op)[0]
268+
269+        # The first connection gets the response it is patiently waiting for
270+        response1 = conn.recv_raw().decode()

fjahr commented at 10:16 pm on March 24, 2026:

This seems racy, shouldn’t this use a loop like the pipelining test? E.g. something like this:

0res = b""
1while generated_block.encode() not in res:
2    res += conn.recv_raw()

But this isn’t very patient, it’s nervously checking as many times as it can ;)

vasild commented at 5:43 am on March 25, 2026:

recv_raw() with blocking sockets will wait efficiently for something to arrive on the socket. Where is the race? The server may respond with something else to the waitforblockheight instead of the block generated by self.generate()?

fjahr commented at 11:50 am on March 25, 2026:

Good point on the blocking socket. Maybe raciness isn’t the right word then but indeed I was also not sure if we can be certain that the “something” that arrives is the block and also the full block. Not sure what else server might send and if we need to account for this. And I guess the block here is well below 1024 bytes because the test is not putting anything into it either. So feel free to ignore if this isn’t a real issue. In that case I would still be happy if you could add a small comment why this is ok to do here.

pinheadmz commented at 10:28 am on March 26, 2026:

Comment added!

in test/functional/interface_http.py:266 in 7cf55430a5 outdated

261+        tip_height = self.nodes[2].getblockcount()
262+        conn = BitcoinHTTPConnection(self.nodes[2])
263+        conn.post_raw('/', f'{{"method": "waitforblockheight", "params": [{tip_height + 1}]}}')
264+
265+        # Wait until after the timeout, then generate a block with a second HTTP connection
266+        time.sleep(3)

fjahr commented at 10:21 pm on March 24, 2026:

Maybe a bit tight with just one second margin given our many CI issues? Maybe it would also make sense to keep it oriented on the timeout value time.sleep(RPCSERVERTIMEOUT*2)?

vasild commented at 5:30 am on March 25, 2026:

Such sleep() calls are better avoided - 1. eventually they fail on “odd” or slow CI machines and turn out not to be enough and 2. most of the time they unnecessary slow down the test, when e.g. waiting 0.5s would have been enough but the code waited 2.5s more.

Should be possible to use mocktime here?

pinheadmz commented at 10:25 am on March 26, 2026:

I’m not sure mocktime would work here because the timer we are comparing against is inside libevent set with evhttp_set_timeout().

And yeah the concern about using sleep() on CI machines is real but we aren’t waiting for some computation to complete that we think should take 2 seconds on an average machine –sleep() should guarantee at least 3 seconds and libevent timer should guarantee at least 2 seconds.

I will change the value to RPCSERVERTIMEOUT + 1 for clarity, and maybe we can fine-tune this more once we’ve replaced libevent!

in test/functional/interface_http.py:36 in af80c9b710 outdated

31+            self.conn.request('GET', '/')
32+            self.conn.getresponse().read()
33+            return False
34+        #       macos/linux           windows
35+        except (ConnectionResetError, ConnectionAbortedError):
36+            return True

fjahr commented at 10:25 pm on March 24, 2026:

I think we could use select to be more efficient here:

 0import select
 1import socket
 2
 3    def sock_closed(self):
 4        if self.conn.sock is None:
 5            return True
 6        r, _, _ = select.select([self.conn.sock], [], [], 0)
 7        if r:
 8            data = self.conn.sock.recv(1, socket.MSG_PEEK)
 9            return len(data) == 0
10        return False

vasild commented at 5:18 am on March 25, 2026:

This will check whether the socket is ready to receive, i.e. if there is any data pending to be received on the socket. There is SocketIsClosed() in src/test/sock_tests.cpp which may be applicable here.

pinheadmz commented at 6:21 pm on March 25, 2026:

Interesting, I tried a python equivalent but it didn’t work:

0from socket import SOL_SOCKET, SO_TYPE, SO_ERROR
1return self.conn.sock.getsockopt(SOL_SOCKET, SO_TYPE) == SO_ERROR

I think the reason is because I can only test the client-side socket, but it’s the server-side that has closed due to an error. I think this is verified by a unit test like this:

 0BOOST_AUTO_TEST_CASE(close_socket)
 1{
 2    int s[2];
 3    CreateSocketPair(s);
 4
 5    Sock* sock0 = new Sock(s[0]);
 6    Sock* sock1 = new Sock(s[1]);
 7
 8    SendAndRecvMessage(*sock0, *sock1);
 9
10    // server closes
11    close(s[0]);
12
13    // server is closed but client is still open
14    BOOST_CHECK(SocketIsClosed(s[0]));
15    BOOST_CHECK(!SocketIsClosed(s[1]));
16
17    // ERROR - we have been hung up on
18    SendAndRecvMessage(*sock0, *sock1);
19}

Incidentally I did have a hard time testing “have we been hung up on” for this method and I got a suggestion from ChatGPT with the select mechanism but decided against it. I’d love something more clear for the test but what’s here has been the best so far for readability and consistency at all call sites testing on master and #32061.

in test/functional/interface_http.py:167 in 72992f5390

163+        response2 = conn.get(f'/{"x"*8200}')
164         assert_equal(response2.status, http.client.BAD_REQUEST)
165 
166+        # Many small header lines is ok
167+        conn = BitcoinHTTPConnection(self.nodes[2])
168+        for i in range(400):

fjahr commented at 10:38 pm on March 24, 2026:

These magic values above and below seem pretty arbitrary. 400 is ok but 600 is not, maybe either add comments for these and following numbers why they are chosen. It likely could be a much narrower bound and the comment might mention the threshold we are working with explicitly.

pinheadmz commented at 7:49 pm on March 25, 2026:

Ok I’ll add comments and a little math that determine the constants. There is some fuzziness here because we don’t know exactly what other headers http.client adds by default and, somewhat mysteriously, libevent doesn’t seem to reject a request until it’s about 1,000 bytes over the configured limit (!).

in test/functional/interface_http.py:181 in 72992f5390

177+        response3 = conn.get('/x')
178+        assert_equal(response3.status, http.client.BAD_REQUEST)
179+
180+        # Large request body size is ok
181+        conn = BitcoinHTTPConnection(self.nodes[0])
182+        response4 = conn.post('/', f'{{"jsonrpc": "2.0", "id": "0", "method": "submitblock", "params": ["{"0"*0x01ffff00}"]}}')

fjahr commented at 10:40 pm on March 24, 2026:

nit: At first sight I thought this formatting was broken. It works but maybe ' + "0"*0x01ffff00 + ' may be a bit more readable. Bonus for giving 0x01ffff00 a variable with a name that makes it easier to reason about (connected to my call to explain the magic numbers above).

pinheadmz commented at 8:03 pm on March 25, 2026:

Like above I’ll spell out the reasoning for the constants and make it easier to read.

fjahr commented at 10:43 pm on March 24, 2026: contributor

Concept ACK

in test/functional/interface_http.py:59 in 72992f5390

54+    def get(self, path, connection_header=None):
55+        headers = self.headers
56+        if connection_header is not None:
57+            headers["Connection"] = connection_header
58+        self.conn.request('GET', path, '', headers)
59+        return self.conn.getresponse()

vasild commented at 5:19 am on March 25, 2026:

Can do some minor deduplication - have one function that takes also the method name and then get() and post() call that one.

pinheadmz commented at 6:38 pm on March 25, 2026:

Sure, moved the common lines to internal _request() method

in test/functional/interface_http.py:187 in 72992f5390

268+        assert_equal(response4.status, http.client.OK)
269+
270+        conn = BitcoinHTTPConnection(self.nodes[1])
271+        try:
272+            # Excessive body size is invalid
273+            response5 = conn.post('/', f'{{"jsonrpc": "2.0", "id": "0", "method": "submitblock", "params": ["{"0"*0x02000000}"]}}')

vasild commented at 5:46 am on March 25, 2026:

There are a lot of “sizes” hardcoded around in this test. They somehow relate to constants in the C++ code. It is difficult to assess which limit is a given test exercising. Would be more clear like:

0MAX_WHATEVER = 1000 # a constant with the same name exists in the C++ code
1...
2test MAX_WHATEVER is ok
3test MAX_WHATEVER + 1 is error

pinheadmz commented at 8:04 pm on March 25, 2026:

Spelling this out, thanks.

test: clean up and modernize interface_http 846f959b46

test: ensure HTTP server timeout is not caused by a delayed response d7f9c43755

test: ensure HTTP server enforces limits on headers and body size e9653667f2

pinheadmz force-pushed on Mar 26, 2026

pinheadmz commented at 11:28 am on March 26, 2026: member

push to e9653667f22c982d873452281a45223fa4e34eb8

Address feedback from @vasild and @fjahr, adding comments and cleaning up the commits.

There is one or two “funny” parts of this test due to libevent which I expect to remove in #32061

test: modernize interface_http and cover more libevent behavior #34772

New test: check_server_busy_idle_timeout()