test: Check that RPCs do not time out, even under load #34927

In https://github.com/bitcoin/bitcoin/pull/34927/changes/fac378d434b7947ca79b068206b0d0fcc20885cd test: Map cli CalledProcessError on server error to JSONRPCException

Would it be worth passing http_status=503 when constructing this JSONRPCException, so callers retain the HTTP status in addition to the RPC error code and message?

JSONRPCException has this constructor def __init__(self, rpc_error, http_status=None): so i think this can be used as

index cfd841c92a..f2456589ff 100755
--- a/test/functional/test_framework/test_node.py
+++ b/test/functional/test_framework/test_node.py
@@ -952,7 +952,7 @@ class TestNodeCLI():
                 raise JSONRPCException(dict(
                     code=-342,
                     message=f"non-JSON HTTP response with '503 Service Unavailable' from server: {message}",
-                ))
+                ), http_status=503)
             # Ignore cli_stdout, raise with cli_stderr
             raise subprocess.CalledProcessError(returncode, p_args, output=cli_stderr)
         try:

thx, done

In https://github.com/bitcoin/bitcoin/pull/34927/changes/fa04bad48b2e825b28a0a9f0300ff235409cc684 test: Check that RPCs do not time out, even under load

I do not understand what this Sla means here, Could this be named RPCPayloadTest It is difficult to infer from the class name which behavior the test covers

SLA is a contract about the service level. Happy to pick any other name. Maybe RPCEchoPayloadTest?

Thanks, RPCEchoPayloadTest is clearer to me.

In https://github.com/bitcoin/bitcoin/pull/34927/changes/fa04bad48b2e825b28a0a9f0300ff235409cc684#r3727565203 test: Check that RPCs do not time out, even under load

My understanding is that random data is not necessarily the same as invalid transaction data. Most random payloads will not decode, but occasionally the bytes can have the structure of a transaction such as a version, input count, inputs, outputs, and locktime. In that case, decoding succeeds, and it moves on to validation

I checked this by temporarily submitting the following syntactically valid serialized transaction

valid_tx = "020000000101010101010101010101010101010101010101010101010101010101010101010000000000ffffffff010000000000000000016a00000000"

assert_raises_rpc_error( -26,  "tx-size-small", node.sendrawtransaction,  valid_tx,)

This returned tx-size-small rather than tx decode failed', indicating that the payload was successfully decoded and reached transaction validation. If the randomized test selects such a payload, its existing error message check will fail even though the RPC behaved correctly.

I am not sure how likely this is in practice, and it may be very unlikely. However, the test makes hundreds of random selections per run and may be executed many times across CI. Since the test depends on the payload being undecodable, it seems better to guarantee that condition rather than rely on random data being invalid.

Could we guarantee that the payload is undecodable by appending a non-hex character?

rpc.sendrawtransaction(payload + "x")

Since x is not hexadecimal, the payload cannot be decoded as a serialized transaction and must take the expected decode-error path.

thx, done, but for the first char.

Thanks, this does address the decodable payload, but looking at it, I wonder whether IsHex() would reject it immediately.

There is a line in the method

if (HexDigit(c) < 0) return false

This would imply that the leading z causes IsHex() to return false after inspecting only the first character, regardless of the payload size.

The existing test most likely would not catch this difference because it only checks the externally visible result, which is still the expected decode error. It does not check how much of the payload was processed.

I still think it may be better to append the non hex character:

That would still guarantee that the payload is undecodable, but IsHex() would have to inspect the entire large payload before reaching the invalid character.

I think we only care about the server here, not about any RPC-method internal logic.

So I think it is fine?

Okay, that's fine then. Thanks for clarifying this