Attempt to address some of the consistently flaky integration tests

newsfragments/3440.feature.rst

@@ -0,0 +1 @@
+Implement a ``RequestTimedOut`` exception, extending from ``Web3RPCError``, for when requests to the node time out.

newsfragments/3440.internal.rst

@@ -0,0 +1 @@
+Mitigate inconsistently failing tests for CI runs with appropriate ``flaky`` or ``pytest.mark.xfail()`` decorators.

tests/core/manager/test_response_formatters.py

@@ -16,6 +16,7 @@
     BlockNotFound,
     ContractLogicError,
     MethodUnavailable,
+    RequestTimedOut,
     TransactionNotFound,
     Web3RPCError,
 )
@@ -77,9 +78,15 @@
     VALID_ERROR_RESPONSE,
     {"error": {"code": -32601, "message": (METHOD_UNAVAILABLE_MSG)}},
 )
+ERROR_RESPONSE_REQUEST_TIMED_OUT = merge(
+    VALID_ERROR_RESPONSE,
+    {"error": {"code": -32002, "message": "Request timed out."}},
+)
 ERROR_RESPONSE_INVALID_ID = merge(VALID_ERROR_RESPONSE, {"id": b"invalid"})
 
-ERROR_RESPONSE_INVALID_CODE = merge(VALID_ERROR_RESPONSE, {"error": {"code": "-32601"}})
+ERROR_RESPONSE_INVALID_CODE = merge(
+    VALID_ERROR_RESPONSE, {"error": {"code": "-32601", "message": ""}}
+)
 ERROR_RESPONSE_INVALID_MISSING_CODE = merge(
     VALID_ERROR_RESPONSE, {"error": {"message": "msg"}}
 )
@@ -190,6 +197,11 @@ def test_formatted_response_invalid_response_object(w3, response, error, error_m
             MethodUnavailable,
             METHOD_UNAVAILABLE_MSG,
         ),
+        (
+            ERROR_RESPONSE_REQUEST_TIMED_OUT,
+            RequestTimedOut,
+            f'{ERROR_RESPONSE_REQUEST_TIMED_OUT["error"]}',
+        ),
     ),
 )
 def test_formatted_response_valid_error_object(response, w3, error, error_message):

web3/_utils/module_testing/eth_module.py

@@ -58,6 +58,7 @@
     assert_contains_log,
     async_mock_offchain_lookup_request_response,
     flaky_geth_dev_mining,
+    flaky_with_xfail_on_exception,
     mock_offchain_lookup_request_response,
 )
 from web3._utils.module_testing.utils import (
@@ -76,6 +77,7 @@
     MultipleFailedRequests,
     NameNotFound,
     OffchainLookup,
+    RequestTimedOut,
     TimeExhausted,
     TooManyRequests,
     TransactionNotFound,
@@ -498,6 +500,7 @@ async def test_eth_send_transaction(
             "maxFeePerGas": async_w3.to_wei(3, "gwei"),
             "maxPriorityFeePerGas": async_w3.to_wei(1, "gwei"),
         }
+
         txn_hash = await async_w3.eth.send_transaction(txn_params)
         txn = await async_w3.eth.get_transaction(txn_hash)
 
@@ -507,7 +510,7 @@ async def test_eth_send_transaction(
         assert txn["gas"] == 21000
         assert txn["maxFeePerGas"] == txn_params["maxFeePerGas"]
         assert txn["maxPriorityFeePerGas"] == txn_params["maxPriorityFeePerGas"]
-        assert txn["gasPrice"] == txn_params["maxFeePerGas"]
+        assert txn["gasPrice"] <= txn["maxFeePerGas"]  # effective gas price
 
     @pytest.mark.asyncio
     async def test_eth_send_transaction_default_fees(
@@ -521,6 +524,7 @@ async def test_eth_send_transaction_default_fees(
             "value": Wei(1),
             "gas": 21000,
         }
+
         txn_hash = await async_w3.eth.send_transaction(txn_params)
         txn = await async_w3.eth.get_transaction(txn_hash)
 
@@ -530,7 +534,7 @@ async def test_eth_send_transaction_default_fees(
         assert txn["gas"] == 21000
         assert is_integer(txn["maxPriorityFeePerGas"])
         assert is_integer(txn["maxFeePerGas"])
-        assert txn["gasPrice"] == txn["maxFeePerGas"]
+        assert txn["gasPrice"] <= txn["maxFeePerGas"]  # effective gas price
 
     @pytest.mark.asyncio
     async def test_eth_send_transaction_hex_fees(
@@ -807,7 +811,7 @@ def gas_price_strategy(w3: "Web3", txn: TxParams) -> Wei:
             else 2 * latest_block["baseFeePerGas"] + max_priority_fee
         )
         assert txn["maxPriorityFeePerGas"] == max_priority_fee
-        assert txn["gasPrice"] == txn["maxFeePerGas"]
+        assert txn["gasPrice"] <= txn["maxFeePerGas"]  # effective gas price
 
         async_w3.eth.set_gas_price_strategy(None)  # reset strategy
 
@@ -1692,6 +1696,7 @@ async def test_async_eth_get_transaction_receipt_mined(
         assert isinstance(effective_gas_price, int)
         assert effective_gas_price > 0
 
+    @flaky_geth_dev_mining
     @pytest.mark.asyncio
     async def test_async_eth_get_transaction_receipt_unmined(
         self,
@@ -1757,6 +1762,7 @@ async def test_async_eth_wait_for_transaction_receipt_mined(
         assert isinstance(effective_gas_price, int)
         assert effective_gas_price > 0
 
+    @flaky_geth_dev_mining
     @pytest.mark.asyncio
     async def test_async_eth_wait_for_transaction_receipt_unmined(
         self,
@@ -2356,7 +2362,10 @@ async def test_async_eth_replace_transaction_gas_price_too_low(
         with pytest.raises(Web3ValueError):
             await async_w3.eth.replace_transaction(txn_hash, txn_params)
 
-    @flaky_geth_dev_mining
+    @flaky_with_xfail_on_exception(
+        reason="Very flaky on CI runs, hard to reproduce locally.",
+        exception=RequestTimedOut,
+    )
     @pytest.mark.asyncio
     async def test_async_eth_replace_transaction_gas_price_defaulting_minimum(
         self, async_w3: "AsyncWeb3", async_keyfile_account_address: ChecksumAddress
@@ -2380,7 +2389,10 @@ async def test_async_eth_replace_transaction_gas_price_defaulting_minimum(
             gas_price * 1.125
         )  # minimum gas price
 
-    @flaky_geth_dev_mining
+    @flaky_with_xfail_on_exception(
+        reason="Very flaky on CI runs, hard to reproduce locally.",
+        exception=RequestTimedOut,
+    )
     @pytest.mark.asyncio
     async def test_async_eth_replace_transaction_gas_price_defaulting_strategy_higher(
         self, async_w3: "AsyncWeb3", async_keyfile_account_address: ChecksumAddress
@@ -2409,7 +2421,10 @@ def higher_gas_price_strategy(async_w3: "AsyncWeb3", txn: TxParams) -> Wei:
         )  # Strategy provides higher gas price
         async_w3.eth.set_gas_price_strategy(None)  # reset strategy
 
-    @flaky_geth_dev_mining
+    @flaky_with_xfail_on_exception(
+        reason="Very flaky on CI runs, hard to reproduce locally.",
+        exception=RequestTimedOut,
+    )
     @pytest.mark.asyncio
     async def test_async_eth_replace_transaction_gas_price_defaulting_strategy_lower(
         self, async_w3: "AsyncWeb3", async_keyfile_account_address: ChecksumAddress
@@ -3101,6 +3116,7 @@ def test_eth_send_transaction(
             "maxFeePerGas": w3.to_wei(3, "gwei"),
             "maxPriorityFeePerGas": w3.to_wei(1, "gwei"),
         }
+
         txn_hash = w3.eth.send_transaction(txn_params)
         txn = w3.eth.get_transaction(txn_hash)
 
@@ -3110,7 +3126,7 @@ def test_eth_send_transaction(
         assert txn["gas"] == 21000
         assert txn["maxFeePerGas"] == txn_params["maxFeePerGas"]
         assert txn["maxPriorityFeePerGas"] == txn_params["maxPriorityFeePerGas"]
-        assert txn["gasPrice"] == txn_params["maxFeePerGas"]
+        assert txn["gasPrice"] <= txn["maxFeePerGas"]  # effective gas price
 
     def test_eth_send_transaction_with_nonce(
         self, w3: "Web3", keyfile_account_address: ChecksumAddress
@@ -3161,7 +3177,7 @@ def test_eth_send_transaction_default_fees(
         assert txn["gas"] == 21000
         assert is_integer(txn["maxPriorityFeePerGas"])
         assert is_integer(txn["maxFeePerGas"])
-        assert txn["gasPrice"] == txn["maxFeePerGas"]
+        assert txn["gasPrice"] <= txn["maxFeePerGas"]  # effective gas price
 
     def test_eth_send_transaction_hex_fees(
         self, w3: "Web3", keyfile_account_address_dual_type: ChecksumAddress
@@ -3327,7 +3343,7 @@ def gas_price_strategy(_w3: "Web3", _txn: TxParams) -> Wei:
             else 2 * latest_block["baseFeePerGas"] + max_priority_fee
         )
         assert txn["maxPriorityFeePerGas"] == max_priority_fee
-        assert txn["gasPrice"] == txn["maxFeePerGas"]
+        assert txn["gasPrice"] <= txn["maxFeePerGas"]  # effective gas price
 
         w3.eth.set_gas_price_strategy(None)  # reset strategy
 
@@ -3596,7 +3612,6 @@ def test_eth_replace_transaction_gas_price_defaulting_strategy_lower(
         self, w3: "Web3", keyfile_account_address: ChecksumAddress
     ) -> None:
         gas_price = w3.to_wei(2, "gwei")
-
         txn_params: TxParams = {
             "from": keyfile_account_address,
             "to": keyfile_account_address,
@@ -4360,6 +4375,7 @@ def test_eth_get_transaction_receipt_mined(
         assert isinstance(effective_gas_price, int)
         assert effective_gas_price > 0
 
+    @flaky_geth_dev_mining
     def test_eth_get_transaction_receipt_unmined(
         self, w3: "Web3", keyfile_account_address_dual_type: ChecksumAddress
     ) -> None:
@@ -4417,6 +4433,7 @@ def test_eth_wait_for_transaction_receipt_mined(
         assert isinstance(effective_gas_price, int)
         assert effective_gas_price > 0
 
+    @flaky_geth_dev_mining
     def test_eth_wait_for_transaction_receipt_unmined(
         self, w3: "Web3", keyfile_account_address_dual_type: ChecksumAddress
     ) -> None:

web3/_utils/module_testing/module_testing_utils.py

@@ -1,12 +1,17 @@
 import asyncio
+import functools
+import pytest
 from typing import (
     TYPE_CHECKING,
     Any,
+    Callable,
     Collection,
     Dict,
     Generator,
     Literal,
     Sequence,
+    Tuple,
+    Type,
     Union,
 )
 
@@ -55,7 +60,44 @@
 for the duration of the test. This behavior can be flaky
 due to timing of the test running as a block is mined.
 """
-flaky_geth_dev_mining = flaky(max_runs=3)
+flaky_geth_dev_mining = flaky(max_runs=3, min_passes=1)
+
+
+def flaky_with_xfail_on_exception(
+    reason: str,
+    exception: Union[Type[Exception], Tuple[Type[Exception], ...]],
+    max_runs: int = 3,
+    min_passes: int = 1,
+) -> Callable[[Any], Any]:
+    """
+    Some tests inconsistently fail hard with a particular exception and retrying
+    these tests often times does not get them "unstuck". If we've exhausted all flaky
+    retries and this expected exception is raised, `xfail` the test with the given
+    reason.
+    """
+    runs = max_runs
+
+    def decorator(func: Any) -> Any:
+        @flaky(max_runs=max_runs, min_passes=min_passes)
+        @functools.wraps(func)
+        async def wrapper(self: Any, *args: Any, **kwargs: Any) -> Any:
+            nonlocal runs
+            try:
+                return await func(self, *args, **kwargs)
+            except exception:
+                # xfail the test only if the exception is raised and we have exhausted
+                # all flaky retries
+                if runs == 1:
+                    pytest.xfail(reason)
+                runs -= 1
+                pytest.fail(f"xfailed but {runs} run(s) remaining with flaky...")
+            except Exception as e:
+                # let flaky handle it
+                raise e
+
+        return wrapper
+
+    return decorator
 
 
 def assert_contains_log(

web3/exceptions.py

@@ -364,6 +364,12 @@ class MethodUnavailable(Web3RPCError):
     """
 
 
+class RequestTimedOut(Web3RPCError):
+    """
+    Raised when a request to the node times out.
+    """
+
+
 class TransactionNotFound(Web3RPCError):
     """
     Raised when a tx hash used to look up a tx in a jsonrpc call cannot be found.

web3/manager.py

@@ -37,6 +37,7 @@
     BadResponseFormat,
     MethodUnavailable,
     ProviderConnectionError,
+    RequestTimedOut,
     TaskNotRunning,
     Web3RPCError,
     Web3TypeError,
@@ -89,6 +90,13 @@
 
 
 NULL_RESPONSES = [None, HexBytes("0x"), "0x"]
+KNOWN_REQUEST_TIMEOUT_MESSAGING = {
+    # Note: It's important to be very explicit here and not too broad. We don't want
+    # to accidentally catch a message that is not for a request timeout. In the worst
+    # case, we raise something more generic like `Web3RPCError`. JSON-RPC unfortunately
+    # has not standardized error codes for request timeouts.
+    "request timed out",  # go-ethereum
+}
 METHOD_NOT_FOUND = -32601
 
 
@@ -185,6 +193,7 @@ def _validate_response(
             response, 'Response must include either "error" or "result".'
         )
     elif "error" in response:
+        web3_rpc_error: Optional[Web3RPCError] = None
         error = response["error"]
 
         # raise the error when the value is a string
@@ -195,14 +204,21 @@ def _validate_response(
                 "JSON-RPC 2.0 specification.",
             )
 
+        # errors must include a message
+        error_message = error.get("message")
+        if not isinstance(error_message, str):
+            _raise_bad_response_format(
+                response, 'error["message"] is required and must be a string value.'
+            )
+
         # errors must include an integer code
         code = error.get("code")
         if not isinstance(code, int):
             _raise_bad_response_format(
                 response, 'error["code"] is required and must be an integer value.'
             )
         elif code == METHOD_NOT_FOUND:
-            exception = MethodUnavailable(
+            web3_rpc_error = MethodUnavailable(
                 repr(error),
                 rpc_response=response,
                 user_message=(
@@ -211,20 +227,26 @@ def _validate_response(
                     "currently enabled."
                 ),
             )
-            logger.error(exception.user_message)
-            logger.debug(f"RPC error response: {response}")
-            raise exception
-
-        # errors must include a message
-        error_message = error.get("message")
-        if not isinstance(error_message, str):
-            _raise_bad_response_format(
-                response, 'error["message"] is required and must be a string value.'
+        elif any(
+            # parse specific timeout messages
+            timeout_str in error_message.lower()
+            for timeout_str in KNOWN_REQUEST_TIMEOUT_MESSAGING
+        ):
+            web3_rpc_error = RequestTimedOut(
+                repr(error),
+                rpc_response=response,
+                user_message=(
+                    "The request timed out. Check the connection to your node and "
+                    "try again."
+                ),
             )
 
-        apply_error_formatters(error_formatters, response)
+        if web3_rpc_error is None:
+            # if no condition was met above, raise a more generic `Web3RPCError`
+            web3_rpc_error = Web3RPCError(repr(error), rpc_response=response)
+
+        response = apply_error_formatters(error_formatters, response)
 
-        web3_rpc_error = Web3RPCError(repr(error), rpc_response=response)
         logger.error(web3_rpc_error.user_message)
         logger.debug(f"RPC error response: {response}")
         raise web3_rpc_error