<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | maflcko, hodlinator |
| Stale ACK | fjahr |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
330 | @@ -325,41 +331,45 @@ def check_idle_timeout(self): 331 | # the server. Negating this setting will force the AuthServiceProxy 332 | # for this node to create a fresh new HTTP connection for every command 333 | # called for the remainder of this test. 334 | - self.nodes[2].reuse_http_connections = False 335 | + self.nodes[0].reuse_http_connections = False
nit: How about doing this only once at the top of run_test() with a variation of the comment above to ensure less state is carried between all test functions? https://github.com/hodlinator/bitcoin/commit/27134f49a833fa77a74dbc2c4ecfe12964af0305
(I guess it doesn't really make a difference until the last 2 tests which restart the node).
sure will hoist up the setting
67 | @@ -68,7 +68,7 @@ def post_raw(self, path, data): 68 | req += f'Authorization: Basic {str_to_b64str(self.authpair)}\r\n' 69 | req += f'Content-Length: {len(data)}\r\n\r\n' 70 | req += data 71 | - self.conn.sock.sendall(req.encode("utf-8")) 72 | + self.conn.sock.sendall(req.encode("ascii"))
nit: I prefer keeping it to UTF-8 - something closer to suggestion commit: https://github.com/hodlinator/bitcoin/commit/758d5da7377ff5a43e8ab98b4c52637279ee4b59
def post_raw(self, path, data):
if isinstance(data, str):
data = data.encode("utf-8")
assert isinstance(data, bytes), f"Expected data as bytes but got: {type(data)}"
req = (f"POST {path} HTTP/1.1\r\n" +
f"Authorization: Basic {str_to_b64str(self.authpair)}\r\n" +
f"Content-Length: {len(data)}\r\n\r\n").encode("utf-8")
req += data
self.conn.sock.sendall(req)
With ASCII it's easy to trigger errors like:
2026-04-16T07:48:52.476912Z TestFramework (INFO): Checking excessive request size
2026-04-16T07:48:52.881198Z TestFramework (ERROR): Unexpected exception:
Traceback (most recent call last):
File "/home/hodlinator/bc/2/test/functional/test_framework/test_framework.py", line 143, in main
self.run_test()
~~~~~~~~~~~~~^^
File "/home/hodlinator/bc/2/./build/test/functional/interface_http.py", line 107, in run_test
self.check_excessive_request_size()
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^
File "/home/hodlinator/bc/2/./build/test/functional/interface_http.py", line 213, in check_excessive_request_size
conn.post_raw('/', f'{{"jsonrpc": "2.0", "id": "0", "method": "submitblock", "params": ["{"₿" * bytes_above_limit}"]}}')
~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/hodlinator/bc/2/./build/test/functional/interface_http.py", line 71, in post_raw
self.conn.sock.sendall(req.encode("ascii"))
~~~~~~~~~~^^^^^^^^^
UnicodeEncodeError: 'ascii' codec can't encode characters in position 235-33554597: ordinal not in range(128)
re @maflcko at #34772 (review):
Sure, looks good. Just seems odd to use uf8 when it is not needed in the test. For the data it should be fine, maybe a test can echo an emoji? For the headers I am not sure, I think they must be ascii?
Hm... from the specs I could find it seems like everything before the body itself should be visible-ish ASCII. So I guess using/requiring ASCII-encoding for all but the body is a safeguard on the test-side unless we want to test how the server reacts to this form of standards-breaking.
In light of this I think only data should be encoded as UTF-8.
Ok will encode the body data as utf8 and keep everything old-school, like HTTP ;-)
211 | + conn = BitcoinHTTPConnection(self.nodes[0]) 212 | try: 213 | # Excessive body size is invalid 214 | - response5 = conn.post('/', f'{{"jsonrpc": "2.0", "id": "0", "method": "submitblock", "params": ["{"0" * bytes_above_limit}"]}}') 215 | + conn.post_raw('/', f'{{"jsonrpc": "2.0", "id": "0", "method": "submitblock", "params": ["{"0" * bytes_above_limit}"]}}') 216 | + self.log.debug("Client finished sending request before connection was terminated")
nit: I think it might be good to use info to log the racy behavior.
self.log.info("Client finished sending request before connection was terminated")
Full suggestion: https://github.com/hodlinator/bitcoin/commit/c592b6df07d39b6b77b9ca9190f3618b1d84f287
Cool, promoting these messages to info level
93 | @@ -94,7 +94,7 @@ def expect_timeout(self, seconds): 94 | 95 | class HTTPBasicsTest (BitcoinTestFramework): 96 | def set_test_params(self): 97 | - self.num_nodes = 3 98 | + self.num_nodes = 1
nanonit: Could introduce self.node = self.nodes[0] to decrease tokens: https://github.com/hodlinator/bitcoin/commit/758d5da7377ff5a43e8ab98b4c52637279ee4b59
sure, using self.node everywhere
ACK f0e63edcb186efed0cd88b369360ba332e0e7ca7
I'd really prefer keeping it to UTF-8, feel less strongly about the other nits.
Full suggestion branch: https://github.com/bitcoin/bitcoin/compare/master...hodlinator:bitcoin:pr/35086_suggestions
re-ACK 7d81713d76dd5cffb74636f506f28d063fa4253f
See #35086#pullrequestreview-4118857237
Code review ACK 7d81713d76dd5cffb74636f506f28d063fa4253f
Happy to re-review if you decide to address @hodlinator 's nits.
- Only one node needed for test
- Use ascii encoding instead of utf-8
- Make tests independent of each other
- Expect HTTP error code 413 for too-large request
- Clarify python client race condition in comment
push to f49a2afd94
lgtm ACK f49a2afd94c3b8a5c06d7b77155d0596300bdc2e
re-ACK f49a2afd94c3b8a5c06d7b77155d0596300bdc2e
Thanks for taking my suggestions!
post-merge ACK f49a2afd94c3b8a5c06d7b77155d0596300bdc2e
Milestone
32.0