This should help to debug/fix https://github.com/hebasto/bitcoin-core-nightly/issues/368, but seems independently useful to avoid unbounded-time test runs.
Also rework the 413-code tests where a pre-existing issue was made more likely by the first commit.
Currently, the socket timeout in BitcoinHTTPConnection is not set, and
sometimes reset (redundantly) to None.
Fix both issues by using the RPC timeout of the node, which should be
large enough to cover all use-cases.
Specific tests can still call set_timeout() if they need to.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline and AI policy for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | pinheadmz |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
The Windows CI fails, as usual. It fails after 1s: interface_http.py failed (exit code 1), Duration: 1 s, so it seems unrelated to timeouts...
The tests had a few small problems:
* The comment claimed that the send operation was blocking until
timeout. However, a send should terminate once the server closes the
socket.
* The test possibly force-closed the connection, instead of testing the
outcome that the server closes it.
* The test tried to read a "413 Content too large", but such an outcome
is not guaranteed when the server is closing the connection.
Fix all issues, by adjusting the comments and asserting the server
closes the connection. The error code is only asserted in the debug
log.
Hmm, I guess with a timeout the socket can go through select() and then manifest this race. Still, the test seems like it should be fixed and only assert the essential outcome: 413 logged and socket closed. I'll push a commit for that ...
Heh, looks like this accidentally fixes https://github.com/hebasto/bitcoin-core-nightly/issues/368.
At least according to @pinheadmz's Fable, IIUC: https://github.com/hebasto/bitcoin-core-nightly/issues/368#issuecomment-6059544560
ACK fa5551cc6de1629ef800d8b1f7697af923a953bb
Built and tested on macos/arm64 as well as NetBSD VM in qemu, I know its not OpenBSD but its what i had lying around. Also ran the intermittent test on a loop in an OpenBSD ci: https://github.com/pinheadmz/bitcoin/actions/runs/37847855161
Reviewed the changes, came to the same conclusion that the connection needed a timeout set before the first connect(). I'm a little nervous that without a catch / retry we'll end up with a flaky test again but then we might have more data to diagnose with.
Change to the excessive size test makes sense as well - using the server log to assert something happened is more reliable than waiting for an event on the client side to tell us what happened.
<details><summary>Show Signature</summary>
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
ACK fa5551cc6de1629ef800d8b1f7697af923a953bb
-----BEGIN PGP SIGNATURE-----
iQJPBAEBCAA5FiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmrIEjsbFIAAAAAABAAO
bWFudTIsMi41KzEuMTIsMCwzAAoJEOfimEtiick6wrkP/RtTnr58mBRhy5w+4lru
0ODnc+0pvOg7v6TDBD9ZfuBCg8JNnoOQuCeZAQrn3V5zUsj+JeULwZgherL28fbB
Rq0wGLRWssVio5Ly+DYs5Sz0roX+G4TYsVr52ixmDq4KgzGBTghgMFYd7ijlQ1PF
LV/D5jPBqRtdkT9uT2HeZLh5tcZNGHH/LGh+wEeR672cb+bDw2Py2Q1I0r0cRkXD
cPV23o2l8AtttmG1dqTG9ChJJCd+8jwSMjCeCRUBnvyvK/Z/jQFzrphfPlNnDk3S
XKiEAtFIBNsFY3jAlTjpugWrLRZvz3dYBASCuC8K0NPuSCEZZirWA88t0xyLQNE1
MQ8M867SDX24R9sfKQwvLQMd7vjj5InI0MRe5UbssZQ+ICX0WF6AygirLEG7tKoS
vL9mhQayUcv0N+Ew9deu83NNEFaO3rM65/Lt3gjDv4kUIFm8jTIg6YfTmN+o3oQj
wgiSRc1Zg7cCkd1FO9biogN/gfnMAKv9S7BQnB/7qn9FqaRhvpyDQI61WAXb1AKo
nJk2I+cPqB3lBO0F37DBEJcZm+wC78eMG1l7Jd/hQsLhCWdal+qfiXGDaeChCxnr
54nJkekK+xoBHuZYO6ipJlc72zJSz1kL8zq0ah4KIPIhTs/4oXGBM0HXcqdlpMOE
7El6Zj7M2VZQ0sLbcSip0dmn
=TyJB
-----END PGP SIGNATURE-----
pinheadmz's public key is on openpgp.org
</details>