From 73e3e0ebfce37791b1d3b108e61a80283f8b46a9 Mon Sep 17 00:00:00 2001 From: derekpierre Date: Fri, 1 Mar 2024 14:55:32 -0500 Subject: [PATCH 01/70] AsyncTxStrategy should do one of the following during execute: raise TransactionFaulted for unrecoverable/don't bother retrying errors, return updated params for an updated retry, or None if the strategy has no recommendation for updated params and the params should remain unchanged in which case wait for the existing tx a little while longer. There is no need to raise a Wait exception, since other strategies may feel differently. --- atxm/strategies.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/atxm/strategies.py b/atxm/strategies.py index b44fa96..28ec59e 100644 --- a/atxm/strategies.py +++ b/atxm/strategies.py @@ -30,7 +30,7 @@ def name(self) -> str: """Used to identify the strategy in logs.""" return self._NAME - def execute(self, pending: PendingTx) -> TxParams: + def execute(self, pending: PendingTx) -> Optional[TxParams]: """ Execute the strategy. @@ -40,9 +40,10 @@ def execute(self, pending: PendingTx) -> TxParams: (like tx.txhash, tx.params, tx.created, etc). This method must do one of the following: - - Raise `Wait` to pause retries and wait around for a bit. - - Raise `Fault`to signal the transaction cannot be retried. - - Returns a TxParams dictionary to use in the next attempt. + - Raise `TransactionFaulted`to signal the transaction cannot be retried. + - Returns an updated TxParams dictionary to use in the next attempt. + - Returns None if the strategy makes no changes to the existing TxParams and + signal that the machine should just wait for the existing tx NOTE: Do not mutate the input `tx` object. Return a new TxParams dictionary with the updated transaction parameters. The input From 013279cc525f468ca4f86317dba05b6fe4124b6c Mon Sep 17 00:00:00 2001 From: derekpierre Date: Fri, 1 Mar 2024 15:03:03 -0500 Subject: [PATCH 02/70] Adjust strategies to accomodate latest paradigm for execute i.e. return None if the machine should do nothing and wait based on this strategy. Remove Wait exception and its uses. --- atxm/exceptions.py | 7 ------- atxm/machine.py | 15 ++++++++++----- atxm/strategies.py | 45 +++++++++++++++++++++++++++++---------------- 3 files changed, 39 insertions(+), 28 deletions(-) diff --git a/atxm/exceptions.py b/atxm/exceptions.py index d09142f..9786d59 100644 --- a/atxm/exceptions.py +++ b/atxm/exceptions.py @@ -30,13 +30,6 @@ class InsufficientFunds(RPCError): """raised when a transaction exceeds the spending cap""" -class Wait(Exception): - """ - Raised when a strategy exceeds a limitation. - Used to mark a pending transaction as "wait, don't retry". - """ - - class TransactionFaulted(Exception): """Raised when a transaction has been faulted.""" diff --git a/atxm/machine.py b/atxm/machine.py index bb0b160..a7fb6c2 100644 --- a/atxm/machine.py +++ b/atxm/machine.py @@ -9,7 +9,7 @@ from web3 import Web3 from web3.types import TxParams -from atxm.exceptions import TransactionFaulted, TransactionReverted, Wait +from atxm.exceptions import TransactionFaulted, TransactionReverted from atxm.strategies import ( AsyncTxStrategy, FixedRateSpeedUp, @@ -249,7 +249,7 @@ def __handle_active_transaction(self) -> bool: 1. paused 2. reverted (fault) 3. finalized - 4. strategize: retry, wait, or fault + 4. strategize: retry, do nothing and wait, or fault Returns True if the next queued transaction can be broadcasted right now. """ @@ -323,12 +323,10 @@ def __strategize(self) -> Optional[PendingTx]: raise RuntimeError("No active transaction to strategize") _active_copy = deepcopy(self._tx_tracker.pending) + params_updated = False for strategy in self._strategies: try: params = strategy.execute(pending=_active_copy) - except Wait as e: - log.info(f"[wait] strategy {strategy.__class__} signalled wait: {e}") - return except TransactionFaulted as e: self._tx_tracker.fault(fault_error=e) return @@ -336,6 +334,13 @@ def __strategize(self) -> Optional[PendingTx]: # in case the strategy accidentally returns None # keep the parameters as they are. _active_copy.params.update(params) + params_updated = True + + if not params_updated: + log.info( + f"[wait] strategies made no suggested updates to pending tx #{_active_copy.id} - skipping retry round" + ) + return # (!) retry the transaction with the new parameters retry_params = TxParams(_active_copy.params) diff --git a/atxm/strategies.py b/atxm/strategies.py index 28ec59e..376c7a7 100644 --- a/atxm/strategies.py +++ b/atxm/strategies.py @@ -7,7 +7,6 @@ from atxm.exceptions import ( Fault, - Wait, TransactionFaulted, ) from atxm.logging import log @@ -69,7 +68,7 @@ class InsufficientFundsPause(AsyncTxStrategy): _NAME = "insufficient-funds" - def execute(self, pending: PendingTx) -> TxParams: + def execute(self, pending: PendingTx) -> Optional[TxParams]: balance = self.w3.eth.get_balance(pending._from) if balance == 0: self.log.warn( @@ -81,8 +80,7 @@ def execute(self, pending: PendingTx) -> TxParams: message="Insufficient funds", ) # log.warn(f"Insufficient funds for transaction #{pending.params['nonce']}") - # raise Wait("Insufficient funds") - return pending.params + return None class TimeoutStrategy(AsyncTxStrategy): @@ -125,7 +123,7 @@ def __active_timed_out(self, pending: PendingTx) -> bool: ) return False - def execute(self, pending: PendingTx) -> TxParams: + def execute(self, pending: PendingTx) -> Optional[TxParams]: if not pending: # should never get here raise RuntimeError("pending tx should not be None") @@ -137,43 +135,58 @@ def execute(self, pending: PendingTx) -> TxParams: fault=Fault.TIMEOUT, message="Transaction has timed out", ) - return pending.params + return None class FixedRateSpeedUp(AsyncTxStrategy): """Speedup strategy for pending transactions.""" - SPEEDUP_FACTOR = 1.125 # 12.5% increase - MAX_TIP = Gwei(1) # gwei maxPriorityFeePerGas per transaction + _SPEEDUP_INCREASE = 0.125 # 12.5% + _MAX_TIP = Gwei(1) # gwei maxPriorityFeePerGas per transaction - _NAME = f"speedup-{SPEEDUP_FACTOR}%" + _NAME = "speedup" + + def __init__( + self, + w3: Web3, + speedup_percentage: Optional[float] = None, + max_tip: Optional[Gwei] = None, + ): + super().__init__(w3) + + if speedup_percentage and speedup_percentage >= 1: + raise ValueError( + f"Invalid speedup increase percentage - {speedup_percentage}" + ) + + self.speedup_factor = 1 + (speedup_percentage or self._SPEEDUP_INCREASE) + self.max_tip = max_tip or self._MAX_TIP def _calculate_speedup_fee(self, pending: TxParams) -> Tuple[Wei, Wei]: base_fee = self.w3.eth.get_block("latest")["baseFeePerGas"] suggested_tip = self.w3.eth.max_priority_fee _log_gas_weather(base_fee, suggested_tip) max_priority_fee = round( - max(pending["maxPriorityFeePerGas"], suggested_tip) * self.SPEEDUP_FACTOR + max(pending["maxPriorityFeePerGas"], suggested_tip) * self.speedup_factor ) max_fee_per_gas = round( max( - pending["maxFeePerGas"] * self.SPEEDUP_FACTOR, + pending["maxFeePerGas"] * self.speedup_factor, (base_fee * 2) + max_priority_fee, ) ) return max_priority_fee, max_fee_per_gas - def execute(self, pending: PendingTx) -> TxParams: + def execute(self, pending: PendingTx) -> Optional[TxParams]: params = pending.params old_tip, old_max_fee = params["maxPriorityFeePerGas"], params["maxFeePerGas"] new_tip, new_max_fee = self._calculate_speedup_fee(params) tip_increase = round(Web3.from_wei(new_tip - old_tip, "gwei"), 4) fee_increase = round(Web3.from_wei(new_max_fee - old_max_fee, "gwei"), 4) - if new_tip > self.MAX_TIP: - raise Wait( - f"Pending transaction maxPriorityFeePerGas exceeds spending cap {self.MAX_TIP}" - ) + if new_tip > self.max_tip: + # nothing the strategy can do here - don't change the params + return None latest_nonce = self.w3.eth.get_transaction_count(params["from"], "latest") pending_nonce = self.w3.eth.get_transaction_count(params["from"], "pending") From 9c2ec6e9af4e35a3315a47e2b550dca5c54791e0 Mon Sep 17 00:00:00 2001 From: derekpierre Date: Fri, 1 Mar 2024 15:04:53 -0500 Subject: [PATCH 03/70] Adjust existing strategy tests since None can now be returned from execute(). --- tests/test_strategy.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/test_strategy.py b/tests/test_strategy.py index 1966314..0d5a949 100644 --- a/tests/test_strategy.py +++ b/tests/test_strategy.py @@ -39,7 +39,7 @@ def test_timeout_strategy(w3, mocker): # 1) tx just created a does not time out for i in range(3): - assert timeout_strategy.execute(pending_tx) == params + assert timeout_strategy.execute(pending_tx) is None # no change to params # 2) remaining time is < warn factor; still doesn't time out but we warn about it pending_tx.created = ( @@ -53,7 +53,7 @@ def warning_trapper(event): warnings.append(event) globalLogPublisher.addObserver(warning_trapper) - assert timeout_strategy.execute(pending_tx) == params + assert timeout_strategy.execute(pending_tx) is None # no change to params globalLogPublisher.removeObserver(warning_trapper) assert len(warnings) == 1 @@ -65,7 +65,7 @@ def warning_trapper(event): # 3) close to timeout but not quite (5s short) pending_tx.created = (now - timedelta(seconds=(TIMEOUT - 5))).timestamp() - assert timeout_strategy.execute(pending_tx) == params + assert timeout_strategy.execute(pending_tx) is None # no change to params # 4) timeout pending_tx.created = (now - timedelta(seconds=(TIMEOUT + 1))).timestamp() From 39b4b365c036cb2285879780da51285bdf4ce238 Mon Sep 17 00:00:00 2001 From: derekpierre Date: Sat, 2 Mar 2024 20:51:01 -0500 Subject: [PATCH 04/70] Update timeout strategy logging. --- atxm/strategies.py | 4 ++-- tests/test_strategy.py | 5 +---- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/atxm/strategies.py b/atxm/strategies.py index 376c7a7..a7e7ce9 100644 --- a/atxm/strategies.py +++ b/atxm/strategies.py @@ -111,12 +111,12 @@ def __active_timed_out(self, pending: PendingTx) -> bool: human_end_time = end_time.strftime("%Y-%m-%d %H:%M:%S") if time_remaining.seconds < (self.timeout * self._WARN_FACTOR): self.log.warn( - f"[pending_timeout] Transaction {pending.txhash.hex()} will timeout in " + f"[timeout] Transaction {pending.txhash.hex()} will timeout in " f"{time_remaining} at {human_end_time}" ) else: self.log.info( - f"[pending] {pending.txhash.hex()} " + f"[timeout] {pending.txhash.hex()} " f"{elapsed_time.seconds}s Elapsed | " f"{time_remaining} Remaining | " f"Timeout at {human_end_time}" diff --git a/tests/test_strategy.py b/tests/test_strategy.py index 0d5a949..4385780 100644 --- a/tests/test_strategy.py +++ b/tests/test_strategy.py @@ -58,10 +58,7 @@ def warning_trapper(event): assert len(warnings) == 1 warning = warnings[0]["log_format"] - assert ( - f"[pending_timeout] Transaction {pending_tx.txhash.hex()} will timeout in" - in warning - ) + assert f"[timeout] Transaction {pending_tx.txhash.hex()} will timeout in" in warning # 3) close to timeout but not quite (5s short) pending_tx.created = (now - timedelta(seconds=(TIMEOUT - 5))).timestamp() From cf8881040b61543977e38f31e58ebe46eae85218 Mon Sep 17 00:00:00 2001 From: derekpierre Date: Sat, 2 Mar 2024 20:51:59 -0500 Subject: [PATCH 05/70] Update speedup strategy calcs to use math.ceil instead of round Ensure that even if some txs don't include both maxPriorityFeePerGas and maxFeePerGas the strategy still works. Handle legacy transaction speed up. Parameterize speed up factor and max tip. Questions remain about the default for max tip, but since parameterized, less of an issue - but something to revisit. --- atxm/strategies.py | 101 ++++++++++++++++++++++++++++------------- atxm/utils.py | 8 ++-- tests/test_strategy.py | 18 +++++++- 3 files changed, 91 insertions(+), 36 deletions(-) diff --git a/atxm/strategies.py b/atxm/strategies.py index a7e7ce9..bb2fd6c 100644 --- a/atxm/strategies.py +++ b/atxm/strategies.py @@ -1,9 +1,10 @@ +import math from abc import ABC from datetime import datetime, timedelta -from typing import Tuple, Optional +from typing import Optional, Tuple from web3 import Web3 -from web3.types import Gwei, TxParams, Wei, PendingTx +from web3.types import Gwei, PendingTx, TxParams from atxm.exceptions import ( Fault, @@ -142,65 +143,103 @@ class FixedRateSpeedUp(AsyncTxStrategy): """Speedup strategy for pending transactions.""" _SPEEDUP_INCREASE = 0.125 # 12.5% + + # TODO: is this chain dependent? _MAX_TIP = Gwei(1) # gwei maxPriorityFeePerGas per transaction _NAME = "speedup" + _GAS_PRICE_FIELD = "gasPrice" + _MAX_FEE_PER_GAS_FIELD = "maxFeePerGas" + _MAX_PRIORITY_FEE_PER_GAS_FIELD = "maxPriorityFeePerGas" + def __init__( self, w3: Web3, - speedup_percentage: Optional[float] = None, - max_tip: Optional[Gwei] = None, + speedup_percentage: float = _SPEEDUP_INCREASE, + max_tip: Gwei = _MAX_TIP, ): super().__init__(w3) - if speedup_percentage and speedup_percentage >= 1: + if speedup_percentage >= 1 or speedup_percentage <= 0.10: raise ValueError( f"Invalid speedup increase percentage - {speedup_percentage}" ) + if max_tip <= 0: + raise ValueError(f"Invalid max tip - {max_tip}") - self.speedup_factor = 1 + (speedup_percentage or self._SPEEDUP_INCREASE) - self.max_tip = max_tip or self._MAX_TIP + self.speedup_factor = 1 + speedup_percentage + self.max_tip = max_tip - def _calculate_speedup_fee(self, pending: TxParams) -> Tuple[Wei, Wei]: - base_fee = self.w3.eth.get_block("latest")["baseFeePerGas"] + def _calculate_eip1559_speedup_fee(self, params: TxParams) -> Tuple[int, int]: + current_base_fee = self.w3.eth.get_block("latest")["baseFeePerGas"] suggested_tip = self.w3.eth.max_priority_fee - _log_gas_weather(base_fee, suggested_tip) - max_priority_fee = round( - max(pending["maxPriorityFeePerGas"], suggested_tip) * self.speedup_factor + _log_gas_weather(current_base_fee, suggested_tip) + + # default to 1 if not specified in tx + prior_max_priority_fee = params.get("maxPriorityFeePerGas", 1) + updated_max_priority_fee = math.ceil( + max(prior_max_priority_fee, suggested_tip) * self.speedup_factor ) - max_fee_per_gas = round( + updated_max_fee_per_gas = math.ceil( max( - pending["maxFeePerGas"] * self.speedup_factor, - (base_fee * 2) + max_priority_fee, + params.get("maxFeePerGas", 0) * self.speedup_factor, + (current_base_fee * 2) + updated_max_priority_fee, ) ) - return max_priority_fee, max_fee_per_gas + return updated_max_priority_fee, updated_max_fee_per_gas + + def _calculate_legacy_speedup_fee(self, params: TxParams) -> int: + generated_gas_price = self.w3.eth.generate_gas_price(params) + # increase prior value by speedup factor + minimum_gas_price = int( + math.ceil(params[self._GAS_PRICE_FIELD] * self.speedup_factor) + ) + if generated_gas_price and generated_gas_price > minimum_gas_price: + return generated_gas_price + + return minimum_gas_price def execute(self, pending: PendingTx) -> Optional[TxParams]: params = pending.params - old_tip, old_max_fee = params["maxPriorityFeePerGas"], params["maxFeePerGas"] - new_tip, new_max_fee = self._calculate_speedup_fee(params) - tip_increase = round(Web3.from_wei(new_tip - old_tip, "gwei"), 4) - fee_increase = round(Web3.from_wei(new_max_fee - old_max_fee, "gwei"), 4) - if new_tip > self.max_tip: - # nothing the strategy can do here - don't change the params - return None + if self._GAS_PRICE_FIELD in pending.params: + old_gas_price = params[self._GAS_PRICE_FIELD] + new_gas_price = self._calculate_legacy_speedup_fee(pending.params) + log.info( + f"[speedup] Speeding up legacy transaction #{params['nonce']} \n" + f"gasPrice {old_gas_price} -> {new_gas_price}" + ) + params[self._GAS_PRICE_FIELD] = new_gas_price + else: + old_tip, old_max_fee = ( + params[self._MAX_PRIORITY_FEE_PER_GAS_FIELD], + params[self._MAX_FEE_PER_GAS_FIELD], + ) + new_tip, new_max_fee = self._calculate_eip1559_speedup_fee(params) + if new_tip > self.max_tip: + # nothing the strategy can do here - don't change the params + log.warn( + f"[speedup] Pending transaction's maxPriorityFeePerGas exceeds spending cap {self.max_tip}; can't speed up any faster" + ) + return None + + tip_increase = round(Web3.from_wei(new_tip - old_tip, "gwei"), 4) + fee_increase = round(Web3.from_wei(new_max_fee - old_max_fee, "gwei"), 4) + log.info( + f"[speedup] Speeding up transaction #{params['nonce']} \n" + f"{self._MAX_PRIORITY_FEE_PER_GAS_FIELD} (~+{tip_increase} gwei) {old_tip} -> {new_tip} \n" + f"{self._MAX_FEE_PER_GAS_FIELD} (~+{fee_increase} gwei) {old_max_fee} -> {new_max_fee}" + ) + params = dict(params) + params[self._MAX_PRIORITY_FEE_PER_GAS_FIELD] = new_tip + params[self._MAX_FEE_PER_GAS_FIELD] = new_max_fee latest_nonce = self.w3.eth.get_transaction_count(params["from"], "latest") pending_nonce = self.w3.eth.get_transaction_count(params["from"], "pending") if pending_nonce - latest_nonce > 0: log.warn("Overriding pending transaction!") - log.info( - f"Speeding up transaction #{params['nonce']} \n" - f"maxPriorityFeePerGas (~+{tip_increase} gwei) {old_tip} -> {new_tip} \n" - f"maxFeePerGas (~+{fee_increase} gwei) {old_max_fee} -> {new_max_fee}" - ) - params = dict(params) - params["maxPriorityFeePerGas"] = new_tip - params["maxFeePerGas"] = new_max_fee params["nonce"] = latest_nonce params = TxParams(params) return params diff --git a/atxm/utils.py b/atxm/utils.py index b444068..3a0db04 100644 --- a/atxm/utils.py +++ b/atxm/utils.py @@ -31,10 +31,12 @@ def _get_average_blocktime(w3: Web3, sample_size: int) -> float: return average_block_time -def _log_gas_weather(base_fee: Wei, tip: Wei) -> None: +def _log_gas_weather(base_fee: Wei, suggested_tip: Wei) -> None: base_fee_gwei = Web3.from_wei(base_fee, "gwei") - tip_gwei = Web3.from_wei(tip, "gwei") - log.info(f"Gas conditions: base {base_fee_gwei} gwei | tip {tip_gwei} gwei") + tip_gwei = Web3.from_wei(suggested_tip, "gwei") + log.info( + f"[gas] Gas conditions: base {base_fee_gwei} gwei | max tip {tip_gwei} gwei" + ) def _get_receipt_from_txhash(w3: Web3, txhash: TxHash) -> Optional[TxReceipt]: diff --git a/tests/test_strategy.py b/tests/test_strategy.py index 4385780..517ab00 100644 --- a/tests/test_strategy.py +++ b/tests/test_strategy.py @@ -3,10 +3,10 @@ import pytest from hexbytes import HexBytes from twisted.logger import LogLevel, globalLogPublisher -from web3.types import TxParams +from web3.types import Gwei, TxParams from atxm.exceptions import Fault, TransactionFaulted -from atxm.strategies import TimeoutStrategy +from atxm.strategies import FixedRateSpeedUp, TimeoutStrategy from atxm.tx import PendingTx @@ -72,3 +72,17 @@ def warning_trapper(event): e.tx = pending_tx e.fault = Fault.TIMEOUT e.message = "Transaction has timed out" + + +def test_speedup_strategy(w3, eip1559_transaction, mocker): + # invalid increase percentage + for speedup_perc in [-1, -0.24, 0, 1, 1.01, 1.1]: + with pytest.raises(ValueError): + _ = FixedRateSpeedUp(w3=w3, speedup_percentage=speedup_perc) + + # invalid max tip + with pytest.raises(ValueError): + _ = FixedRateSpeedUp(w3=w3, max_tip=Gwei(0)) + + speedup_strategy = FixedRateSpeedUp(w3) + assert speedup_strategy.name == "speedup" From 1c0198054a7ac38498815e72033108ae6d80706a Mon Sep 17 00:00:00 2001 From: derekpierre Date: Mon, 4 Mar 2024 09:44:16 -0500 Subject: [PATCH 06/70] Move from a fixed max tip to a max tip factor based on the current suggested tip as a way to limit the speedup strategy getting out of control. Update maxFeePerGas depending on whether previously set or not - web3 py uses a default that doubles the base fee as part of maxFeePerGas so we probably don't want to further double that again. Clean up constructor params validation and update tests. --- atxm/strategies.py | 64 +++++++++++++++++++++++++++--------------- tests/test_strategy.py | 11 ++++---- 2 files changed, 47 insertions(+), 28 deletions(-) diff --git a/atxm/strategies.py b/atxm/strategies.py index bb2fd6c..06c19e1 100644 --- a/atxm/strategies.py +++ b/atxm/strategies.py @@ -4,7 +4,7 @@ from typing import Optional, Tuple from web3 import Web3 -from web3.types import Gwei, PendingTx, TxParams +from web3.types import PendingTx, TxParams from atxm.exceptions import ( Fault, @@ -142,10 +142,9 @@ def execute(self, pending: PendingTx) -> Optional[TxParams]: class FixedRateSpeedUp(AsyncTxStrategy): """Speedup strategy for pending transactions.""" - _SPEEDUP_INCREASE = 0.125 # 12.5% + _SPEEDUP_INCREASE_PERCENTAGE = 0.125 # 12.5% - # TODO: is this chain dependent? - _MAX_TIP = Gwei(1) # gwei maxPriorityFeePerGas per transaction + _MAX_TIP_FACTOR = 3 # max 3x over suggested tip _NAME = "speedup" @@ -156,38 +155,51 @@ class FixedRateSpeedUp(AsyncTxStrategy): def __init__( self, w3: Web3, - speedup_percentage: float = _SPEEDUP_INCREASE, - max_tip: Gwei = _MAX_TIP, + speedup_increase_percentage: float = _SPEEDUP_INCREASE_PERCENTAGE, + max_tip_factor: int = _MAX_TIP_FACTOR, ): super().__init__(w3) - if speedup_percentage >= 1 or speedup_percentage <= 0.10: + if speedup_increase_percentage > 1 or speedup_increase_percentage < 0.10: raise ValueError( - f"Invalid speedup increase percentage - {speedup_percentage}" + f"Invalid speedup increase percentage {speedup_increase_percentage}; " + f"must be in range [0.10, 1]" ) - if max_tip <= 0: - raise ValueError(f"Invalid max tip - {max_tip}") + if max_tip_factor <= 1: + raise ValueError(f"Invalid max tip factor {max_tip_factor}; must be > 1") - self.speedup_factor = 1 + speedup_percentage - self.max_tip = max_tip + self.speedup_factor = 1 + speedup_increase_percentage + self.max_tip_factor = max_tip_factor - def _calculate_eip1559_speedup_fee(self, params: TxParams) -> Tuple[int, int]: + def _calculate_eip1559_speedup_fee(self, params: TxParams) -> Tuple[int, int, int]: current_base_fee = self.w3.eth.get_block("latest")["baseFeePerGas"] suggested_tip = self.w3.eth.max_priority_fee _log_gas_weather(current_base_fee, suggested_tip) # default to 1 if not specified in tx - prior_max_priority_fee = params.get("maxPriorityFeePerGas", 1) + prior_max_priority_fee = params.get("maxPriorityFeePerGas", 0) updated_max_priority_fee = math.ceil( max(prior_max_priority_fee, suggested_tip) * self.speedup_factor ) - updated_max_fee_per_gas = math.ceil( - max( - params.get("maxFeePerGas", 0) * self.speedup_factor, - (current_base_fee * 2) + updated_max_priority_fee, + + current_max_fee_per_gas = params.get("maxFeePerGas") + if current_max_fee_per_gas: + # already previously set, just increase by factor but ensure base fee hasn't + # also increased. The defaults used by web3py for this value is already pretty + # high so don't overdo the multiplication factor. + updated_max_fee_per_gas = math.ceil( + max( + current_max_fee_per_gas * self.speedup_factor, + (current_base_fee * self.speedup_factor) + updated_max_priority_fee, + ) ) - ) - return updated_max_priority_fee, updated_max_fee_per_gas + else: + # not previously set, set to same default as web3py (transactions.py) + updated_max_fee_per_gas = math.ceil( + updated_max_priority_fee + (current_base_fee * 2) + ) + + return suggested_tip, updated_max_priority_fee, updated_max_fee_per_gas def _calculate_legacy_speedup_fee(self, params: TxParams) -> int: generated_gas_price = self.w3.eth.generate_gas_price(params) @@ -216,11 +228,17 @@ def execute(self, pending: PendingTx) -> Optional[TxParams]: params[self._MAX_PRIORITY_FEE_PER_GAS_FIELD], params[self._MAX_FEE_PER_GAS_FIELD], ) - new_tip, new_max_fee = self._calculate_eip1559_speedup_fee(params) - if new_tip > self.max_tip: + suggested_tip, new_tip, new_max_fee = self._calculate_eip1559_speedup_fee( + params + ) + if new_tip > (suggested_tip * self.max_tip_factor): # nothing the strategy can do here - don't change the params log.warn( - f"[speedup] Pending transaction's maxPriorityFeePerGas exceeds spending cap {self.max_tip}; can't speed up any faster" + f"[speedup] Increasing pending transaction's maxPriorityFeePerGas " + f"({round(Web3.from_wei(old_tip, 'gwei'), 4)} gwei) will exceed " + f"spending cap factor {self.max_tip_factor} over suggested tip " + f"({round(Web3.from_wei(suggested_tip, 'gwei'), 4)} gwei); " + f"don't speed up" ) return None diff --git a/tests/test_strategy.py b/tests/test_strategy.py index 517ab00..ca050c6 100644 --- a/tests/test_strategy.py +++ b/tests/test_strategy.py @@ -3,7 +3,7 @@ import pytest from hexbytes import HexBytes from twisted.logger import LogLevel, globalLogPublisher -from web3.types import Gwei, TxParams +from web3.types import TxParams from atxm.exceptions import Fault, TransactionFaulted from atxm.strategies import FixedRateSpeedUp, TimeoutStrategy @@ -76,13 +76,14 @@ def warning_trapper(event): def test_speedup_strategy(w3, eip1559_transaction, mocker): # invalid increase percentage - for speedup_perc in [-1, -0.24, 0, 1, 1.01, 1.1]: + for speedup_perc in [-1, -0.24, 0, 1.01, 1.1]: with pytest.raises(ValueError): - _ = FixedRateSpeedUp(w3=w3, speedup_percentage=speedup_perc) + _ = FixedRateSpeedUp(w3=w3, speedup_increase_percentage=speedup_perc) # invalid max tip - with pytest.raises(ValueError): - _ = FixedRateSpeedUp(w3=w3, max_tip=Gwei(0)) + for max_tip in [-1, 0, 0.5, 0.9, 1]: + with pytest.raises(ValueError): + _ = FixedRateSpeedUp(w3=w3, max_tip_factor=max_tip) speedup_strategy = FixedRateSpeedUp(w3) assert speedup_strategy.name == "speedup" From 352f74ae15f8512f121aa15d35aca4d81ee67300 Mon Sep 17 00:00:00 2001 From: derekpierre Date: Mon, 4 Mar 2024 11:04:44 -0500 Subject: [PATCH 07/70] Simplify legacy transaction speedup logic. Use tx.id in addition to nonce for speed up logs. --- atxm/strategies.py | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/atxm/strategies.py b/atxm/strategies.py index 06c19e1..846808c 100644 --- a/atxm/strategies.py +++ b/atxm/strategies.py @@ -202,15 +202,18 @@ def _calculate_eip1559_speedup_fee(self, params: TxParams) -> Tuple[int, int, in return suggested_tip, updated_max_priority_fee, updated_max_fee_per_gas def _calculate_legacy_speedup_fee(self, params: TxParams) -> int: - generated_gas_price = self.w3.eth.generate_gas_price(params) - # increase prior value by speedup factor - minimum_gas_price = int( - math.ceil(params[self._GAS_PRICE_FIELD] * self.speedup_factor) - ) - if generated_gas_price and generated_gas_price > minimum_gas_price: - return generated_gas_price + generated_gas_price = self.w3.eth.generate_gas_price(params) or 0 + old_gas_price = params[self._GAS_PRICE_FIELD] + + base_price_to_increase = old_gas_price + if generated_gas_price > old_gas_price: + log.info( + f"[speedup] increase gas price based on updated generated " + f"gas price value {generated_gas_price} vs prior value {old_gas_price}" + ) + base_price_to_increase = generated_gas_price - return minimum_gas_price + return math.ceil(base_price_to_increase * self.speedup_factor) def execute(self, pending: PendingTx) -> Optional[TxParams]: params = pending.params @@ -219,7 +222,7 @@ def execute(self, pending: PendingTx) -> Optional[TxParams]: old_gas_price = params[self._GAS_PRICE_FIELD] new_gas_price = self._calculate_legacy_speedup_fee(pending.params) log.info( - f"[speedup] Speeding up legacy transaction #{params['nonce']} \n" + f"[speedup] Speeding up legacy transaction #atx-{pending.id} (nonce=#{params['nonce']}) \n" f"gasPrice {old_gas_price} -> {new_gas_price}" ) params[self._GAS_PRICE_FIELD] = new_gas_price @@ -245,7 +248,7 @@ def execute(self, pending: PendingTx) -> Optional[TxParams]: tip_increase = round(Web3.from_wei(new_tip - old_tip, "gwei"), 4) fee_increase = round(Web3.from_wei(new_max_fee - old_max_fee, "gwei"), 4) log.info( - f"[speedup] Speeding up transaction #{params['nonce']} \n" + f"[speedup] Speeding up transaction #atx-{pending.id} (nonce=#{params['nonce']}) \n" f"{self._MAX_PRIORITY_FEE_PER_GAS_FIELD} (~+{tip_increase} gwei) {old_tip} -> {new_tip} \n" f"{self._MAX_FEE_PER_GAS_FIELD} (~+{fee_increase} gwei) {old_max_fee} -> {new_max_fee}" ) From 0be4f528156f4213bbc215eac2ebace023efd7ee Mon Sep 17 00:00:00 2001 From: derekpierre Date: Mon, 4 Mar 2024 11:06:13 -0500 Subject: [PATCH 08/70] Add tests for constructor parameters and legacy tx speed up functionality. --- tests/conftest.py | 2 + tests/test_strategy.py | 83 +++++++++++++++++++++++++++++++++++++++++- 2 files changed, 83 insertions(+), 2 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index 2b8c43d..5d59a09 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -28,6 +28,7 @@ def legacy_transaction(account, w3): params = { "chainId": 1337, "nonce": 0, + "from": account.address, "to": account.address, "value": 0, "gas": 21000, @@ -44,6 +45,7 @@ def eip1559_transaction(account, w3): params = { "chainId": 1337, "nonce": 0, + "from": account.address, "to": account.address, "value": 0, "gas": 21000, diff --git a/tests/test_strategy.py b/tests/test_strategy.py index ca050c6..a60cae2 100644 --- a/tests/test_strategy.py +++ b/tests/test_strategy.py @@ -1,3 +1,4 @@ +import math from datetime import datetime, timedelta import pytest @@ -74,7 +75,7 @@ def warning_trapper(event): e.message = "Transaction has timed out" -def test_speedup_strategy(w3, eip1559_transaction, mocker): +def test_speedup_strategy_constructor(w3): # invalid increase percentage for speedup_perc in [-1, -0.24, 0, 1.01, 1.1]: with pytest.raises(ValueError): @@ -85,5 +86,83 @@ def test_speedup_strategy(w3, eip1559_transaction, mocker): with pytest.raises(ValueError): _ = FixedRateSpeedUp(w3=w3, max_tip_factor=max_tip) - speedup_strategy = FixedRateSpeedUp(w3) + # defaults + speedup_strategy = FixedRateSpeedUp(w3=w3) + assert speedup_strategy.speedup_factor == ( + 1 + FixedRateSpeedUp._SPEEDUP_INCREASE_PERCENTAGE + ) + assert speedup_strategy.max_tip_factor == FixedRateSpeedUp._MAX_TIP_FACTOR + + # other values + speedup_increase = 0.223 + max_tip_factor = 4 + speedup_strategy = FixedRateSpeedUp( + w3=w3, + speedup_increase_percentage=speedup_increase, + max_tip_factor=max_tip_factor, + ) + assert speedup_strategy.speedup_factor == (1 + speedup_increase) + assert speedup_strategy.max_tip_factor == max_tip_factor + + +def test_speedup_strategy_legacy_tx(w3, legacy_transaction, mocker): + speedup_percentage = 0.112 # 11.2% + speedup_strategy = FixedRateSpeedUp( + w3, speedup_increase_percentage=speedup_percentage + ) assert speedup_strategy.name == "speedup" + + transaction_count = legacy_transaction["nonce"] + mocker.patch.object(w3.eth, "get_transaction_count", return_value=transaction_count) + pending_tx = mocker.Mock(spec=PendingTx) + pending_tx.id = 1 + + # legacy transaction - keep speeding up + + # generated gas price < existing tx gas price + generated_gas_price = legacy_transaction["gasPrice"] - 1 # < what is in tx + mocker.patch.object(w3.eth, "generate_gas_price", return_value=generated_gas_price) + + assert legacy_transaction["gasPrice"] + tx_params = dict(legacy_transaction) + for i in range(3): + pending_tx.params = tx_params + old_gas_price = tx_params["gasPrice"] + old_nonce = tx_params["nonce"] + tx_params = speedup_strategy.execute(pending_tx) + + current_gas_price = tx_params["gasPrice"] + assert current_gas_price != old_gas_price + assert current_gas_price == math.ceil(old_gas_price * (1 + speedup_percentage)) + assert tx_params["nonce"] == old_nonce + + # generated gas price is None - same results as before + mocker.patch.object(w3.eth, "generate_gas_price", return_value=None) # set to None + for i in range(3): + tx_params = dict(legacy_transaction) + pending_tx.params = tx_params + old_gas_price = tx_params["gasPrice"] + old_nonce = tx_params["nonce"] + tx_params = speedup_strategy.execute(pending_tx) + + current_gas_price = tx_params["gasPrice"] + assert current_gas_price != old_gas_price + assert current_gas_price == math.ceil(old_gas_price * (1 + speedup_percentage)) + assert tx_params["nonce"] == old_nonce + + # increase generated gas price more than existing gas price in legacy tx + tx_params = dict(legacy_transaction) + pending_tx.params = tx_params + generated_gas_price = tx_params["gasPrice"] * 2 # > what is in tx + mocker.patch.object(w3.eth, "generate_gas_price", return_value=generated_gas_price) + + old_gas_price = tx_params["gasPrice"] + old_nonce = tx_params["nonce"] + updated_tx_params = speedup_strategy.execute(pending_tx) + + current_gas_price = updated_tx_params["gasPrice"] + assert current_gas_price != old_gas_price + assert current_gas_price == math.ceil( + generated_gas_price * (1 + speedup_percentage) + ) + assert updated_tx_params["nonce"] == old_nonce From c023dea9da24bceca594930f8adc4701378c6d97 Mon Sep 17 00:00:00 2001 From: derekpierre Date: Mon, 4 Mar 2024 13:49:03 -0500 Subject: [PATCH 09/70] Update logging of gas conditions. --- atxm/utils.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/atxm/utils.py b/atxm/utils.py index 3a0db04..875b3a6 100644 --- a/atxm/utils.py +++ b/atxm/utils.py @@ -35,7 +35,7 @@ def _log_gas_weather(base_fee: Wei, suggested_tip: Wei) -> None: base_fee_gwei = Web3.from_wei(base_fee, "gwei") tip_gwei = Web3.from_wei(suggested_tip, "gwei") log.info( - f"[gas] Gas conditions: base {base_fee_gwei} gwei | max tip {tip_gwei} gwei" + f"[gas] Gas conditions: base {base_fee_gwei} gwei | suggested tip {tip_gwei} gwei" ) From 979f3f91a2fa4fbea14e960b254a042ec892dccc Mon Sep 17 00:00:00 2001 From: derekpierre Date: Mon, 4 Mar 2024 13:49:57 -0500 Subject: [PATCH 10/70] Better annotations about what the strategy is doing. --- atxm/strategies.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/atxm/strategies.py b/atxm/strategies.py index 846808c..afde8eb 100644 --- a/atxm/strategies.py +++ b/atxm/strategies.py @@ -189,7 +189,9 @@ def _calculate_eip1559_speedup_fee(self, params: TxParams) -> Tuple[int, int, in # high so don't overdo the multiplication factor. updated_max_fee_per_gas = math.ceil( max( + # last attempt param current_max_fee_per_gas * self.speedup_factor, + # OR take current conditions and speedup (current_base_fee * self.speedup_factor) + updated_max_priority_fee, ) ) @@ -202,7 +204,9 @@ def _calculate_eip1559_speedup_fee(self, params: TxParams) -> Tuple[int, int, in return suggested_tip, updated_max_priority_fee, updated_max_fee_per_gas def _calculate_legacy_speedup_fee(self, params: TxParams) -> int: - generated_gas_price = self.w3.eth.generate_gas_price(params) or 0 + generated_gas_price = ( + self.w3.eth.generate_gas_price(params) or 0 + ) # 0 means no gas strategy old_gas_price = params[self._GAS_PRICE_FIELD] base_price_to_increase = old_gas_price From 9589151a4cdf0af13de754d451f38157056949cf Mon Sep 17 00:00:00 2001 From: derekpierre Date: Mon, 4 Mar 2024 13:52:40 -0500 Subject: [PATCH 11/70] Don't modify tx nonce as part of strategy. --- atxm/strategies.py | 6 ------ tests/test_strategy.py | 2 -- 2 files changed, 8 deletions(-) diff --git a/atxm/strategies.py b/atxm/strategies.py index afde8eb..f31544a 100644 --- a/atxm/strategies.py +++ b/atxm/strategies.py @@ -260,11 +260,5 @@ def execute(self, pending: PendingTx) -> Optional[TxParams]: params[self._MAX_PRIORITY_FEE_PER_GAS_FIELD] = new_tip params[self._MAX_FEE_PER_GAS_FIELD] = new_max_fee - latest_nonce = self.w3.eth.get_transaction_count(params["from"], "latest") - pending_nonce = self.w3.eth.get_transaction_count(params["from"], "pending") - if pending_nonce - latest_nonce > 0: - log.warn("Overriding pending transaction!") - - params["nonce"] = latest_nonce params = TxParams(params) return params diff --git a/tests/test_strategy.py b/tests/test_strategy.py index a60cae2..39e7a8f 100644 --- a/tests/test_strategy.py +++ b/tests/test_strategy.py @@ -112,8 +112,6 @@ def test_speedup_strategy_legacy_tx(w3, legacy_transaction, mocker): ) assert speedup_strategy.name == "speedup" - transaction_count = legacy_transaction["nonce"] - mocker.patch.object(w3.eth, "get_transaction_count", return_value=transaction_count) pending_tx = mocker.Mock(spec=PendingTx) pending_tx.id = 1 From b66f5afa11726382f51b4edbe5848f9875894c78 Mon Sep 17 00:00:00 2001 From: derekpierre Date: Mon, 4 Mar 2024 14:03:52 -0500 Subject: [PATCH 12/70] Fix outdated comment about value of warn factor. --- atxm/strategies.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/atxm/strategies.py b/atxm/strategies.py index f31544a..e0db95b 100644 --- a/atxm/strategies.py +++ b/atxm/strategies.py @@ -91,7 +91,7 @@ class TimeoutStrategy(AsyncTxStrategy): _TIMEOUT = 60 * 60 # 1 hour in seconds - _WARN_FACTOR = 0.05 # 10% of timeout remaining + _WARN_FACTOR = 0.05 # 5% of timeout remaining def __init__(self, w3: Web3, timeout: Optional[int] = None): super().__init__(w3) From bf131942bead9549898537fdb64a8f8a850329d4 Mon Sep 17 00:00:00 2001 From: derekpierre Date: Mon, 4 Mar 2024 15:22:37 -0500 Subject: [PATCH 13/70] Clean up logging logic since old_tip, old_max_fee may or may not be present in transaction parameters (eip1559 allows one or both). --- atxm/strategies.py | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/atxm/strategies.py b/atxm/strategies.py index e0db95b..1763059 100644 --- a/atxm/strategies.py +++ b/atxm/strategies.py @@ -232,8 +232,8 @@ def execute(self, pending: PendingTx) -> Optional[TxParams]: params[self._GAS_PRICE_FIELD] = new_gas_price else: old_tip, old_max_fee = ( - params[self._MAX_PRIORITY_FEE_PER_GAS_FIELD], - params[self._MAX_FEE_PER_GAS_FIELD], + params.get(self._MAX_PRIORITY_FEE_PER_GAS_FIELD), + params.get(self._MAX_FEE_PER_GAS_FIELD), ) suggested_tip, new_tip, new_max_fee = self._calculate_eip1559_speedup_fee( params @@ -242,19 +242,27 @@ def execute(self, pending: PendingTx) -> Optional[TxParams]: # nothing the strategy can do here - don't change the params log.warn( f"[speedup] Increasing pending transaction's maxPriorityFeePerGas " - f"({round(Web3.from_wei(old_tip, 'gwei'), 4)} gwei) will exceed " - f"spending cap factor {self.max_tip_factor} over suggested tip " + f"to {round(Web3.from_wei(new_tip, 'gwei'), 4)} gwei will exceed " + f"spending cap factor {self.max_tip_factor}x over suggested tip " f"({round(Web3.from_wei(suggested_tip, 'gwei'), 4)} gwei); " f"don't speed up" ) return None - tip_increase = round(Web3.from_wei(new_tip - old_tip, "gwei"), 4) - fee_increase = round(Web3.from_wei(new_max_fee - old_max_fee, "gwei"), 4) + tip_increase_message = ( + f"(~+{round(Web3.from_wei(new_tip - old_tip, 'gwei'), 4)} gwei) {old_tip}" + if old_tip + else "undefined" + ) + fee_increase_message = ( + f"(~+{round(Web3.from_wei(new_max_fee - old_max_fee, 'gwei'), 4)} gwei) {old_max_fee}" + if old_max_fee + else "undefined" + ) log.info( f"[speedup] Speeding up transaction #atx-{pending.id} (nonce=#{params['nonce']}) \n" - f"{self._MAX_PRIORITY_FEE_PER_GAS_FIELD} (~+{tip_increase} gwei) {old_tip} -> {new_tip} \n" - f"{self._MAX_FEE_PER_GAS_FIELD} (~+{fee_increase} gwei) {old_max_fee} -> {new_max_fee}" + f"{self._MAX_PRIORITY_FEE_PER_GAS_FIELD} {tip_increase_message} -> {new_tip} \n" + f"{self._MAX_FEE_PER_GAS_FIELD} {fee_increase_message} -> {new_max_fee}" ) params = dict(params) params[self._MAX_PRIORITY_FEE_PER_GAS_FIELD] = new_tip From f7bd316194e411b5c2fd612fc5172bb3d5136226 Mon Sep 17 00:00:00 2001 From: derekpierre Date: Mon, 4 Mar 2024 15:23:44 -0500 Subject: [PATCH 14/70] Add tests for eip1559 transaction speedup. --- tests/test_strategy.py | 254 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 251 insertions(+), 3 deletions(-) diff --git a/tests/test_strategy.py b/tests/test_strategy.py index 39e7a8f..d223cd7 100644 --- a/tests/test_strategy.py +++ b/tests/test_strategy.py @@ -1,5 +1,7 @@ import math +import random from datetime import datetime, timedelta +from unittest.mock import PropertyMock import pytest from hexbytes import HexBytes @@ -104,19 +106,18 @@ def test_speedup_strategy_constructor(w3): assert speedup_strategy.speedup_factor == (1 + speedup_increase) assert speedup_strategy.max_tip_factor == max_tip_factor + assert speedup_strategy.name == "speedup" + def test_speedup_strategy_legacy_tx(w3, legacy_transaction, mocker): speedup_percentage = 0.112 # 11.2% speedup_strategy = FixedRateSpeedUp( w3, speedup_increase_percentage=speedup_percentage ) - assert speedup_strategy.name == "speedup" pending_tx = mocker.Mock(spec=PendingTx) pending_tx.id = 1 - # legacy transaction - keep speeding up - # generated gas price < existing tx gas price generated_gas_price = legacy_transaction["gasPrice"] - 1 # < what is in tx mocker.patch.object(w3.eth, "generate_gas_price", return_value=generated_gas_price) @@ -164,3 +165,250 @@ def test_speedup_strategy_legacy_tx(w3, legacy_transaction, mocker): generated_gas_price * (1 + speedup_percentage) ) assert updated_tx_params["nonce"] == old_nonce + + +def test_speedup_strategy_eip1559_tx_no_blockchain_change( + w3, eip1559_transaction, mocker +): + # blockchain conditions have not changed since + ( + current_base_fee, + max_tip_factor, + pending_tx, + speedup_percentage, + speedup_strategy, + suggested_tip, + ) = eip1559_setup(mocker, w3) + + old_max_priority_fee_per_gas = eip1559_transaction["maxPriorityFeePerGas"] + old_max_fee_per_gas = eip1559_transaction["maxFeePerGas"] + + pending_tx.params = dict(eip1559_transaction) + tx_params = speedup_strategy.execute(pending_tx) + + updated_max_priority_fee_per_gas = tx_params["maxPriorityFeePerGas"] + assert updated_max_priority_fee_per_gas > old_max_priority_fee_per_gas + assert updated_max_priority_fee_per_gas == math.ceil( + old_max_priority_fee_per_gas * (1 + speedup_percentage) + ) + + updated_max_fee_per_gas = tx_params["maxFeePerGas"] + assert updated_max_fee_per_gas > old_max_fee_per_gas + assert updated_max_fee_per_gas == math.ceil( + old_max_fee_per_gas * (1 + speedup_percentage) + ) + assert updated_max_fee_per_gas >= ( + current_base_fee + updated_max_priority_fee_per_gas + ) + + +def test_speedup_strategy_eip1559_tx_base_fee_decreased( + w3, eip1559_transaction, mocker +): + # 2) suppose current base fee decreased; calc remains simple + ( + current_base_fee, + max_tip_factor, + pending_tx, + speedup_percentage, + speedup_strategy, + suggested_tip, + ) = eip1559_setup(mocker, w3) + + old_max_priority_fee_per_gas = eip1559_transaction["maxPriorityFeePerGas"] + old_max_fee_per_gas = eip1559_transaction["maxFeePerGas"] + + new_current_base_fee = math.ceil(current_base_fee * 0.95) + mocker.patch.object( + w3.eth, "get_block", return_value={"baseFeePerGas": new_current_base_fee} + ) + + pending_tx.params = dict(eip1559_transaction) + tx_params = speedup_strategy.execute(pending_tx) + + updated_max_priority_fee_per_gas = tx_params["maxPriorityFeePerGas"] + assert updated_max_priority_fee_per_gas > old_max_priority_fee_per_gas + assert updated_max_priority_fee_per_gas == math.ceil( + old_max_priority_fee_per_gas * (1 + speedup_percentage) + ) + + updated_max_fee_per_gas = tx_params["maxFeePerGas"] + assert updated_max_fee_per_gas > old_max_fee_per_gas + assert updated_max_fee_per_gas == math.ceil( + old_max_fee_per_gas * (1 + speedup_percentage) + ) + assert updated_max_fee_per_gas >= ( + new_current_base_fee + updated_max_priority_fee_per_gas + ) + + +def test_speedup_strategy_eip1559_tx_base_fee_increased( + w3, eip1559_transaction, mocker +): + # suppose current base fee increase; it gets used instead of prior value + ( + current_base_fee, + max_tip_factor, + pending_tx, + speedup_percentage, + speedup_strategy, + suggested_tip, + ) = eip1559_setup(mocker, w3) + + old_max_priority_fee_per_gas = eip1559_transaction["maxPriorityFeePerGas"] + old_max_fee_per_gas = eip1559_transaction["maxFeePerGas"] + + new_current_base_fee = math.ceil(current_base_fee * 1.1) + mocker.patch.object( + w3.eth, "get_block", return_value={"baseFeePerGas": new_current_base_fee} + ) + + pending_tx.params = dict(eip1559_transaction) + tx_params = speedup_strategy.execute(pending_tx) + + updated_max_priority_fee_per_gas = tx_params["maxPriorityFeePerGas"] + assert updated_max_priority_fee_per_gas > old_max_priority_fee_per_gas + assert updated_max_priority_fee_per_gas == math.ceil( + old_max_priority_fee_per_gas * (1 + speedup_percentage) + ) + + updated_max_fee_per_gas = tx_params["maxFeePerGas"] + assert updated_max_fee_per_gas > old_max_fee_per_gas + assert updated_max_fee_per_gas > math.ceil( + old_max_fee_per_gas * (1 + speedup_percentage) + ) + assert updated_max_fee_per_gas == math.ceil( + new_current_base_fee * (1 + speedup_percentage) + + updated_max_priority_fee_per_gas + ) + + +def test_speedup_strategy_eip1559_tx_no_max_fee(w3, eip1559_transaction, mocker): + # suppose no maxFeePerGas specified - use default calc (same as web3py) + ( + current_base_fee, + max_tip_factor, + pending_tx, + speedup_percentage, + speedup_strategy, + suggested_tip, + ) = eip1559_setup(mocker, w3) + + old_max_priority_fee_per_gas = eip1559_transaction["maxPriorityFeePerGas"] + + mocker.patch.object( + w3.eth, "get_block", return_value={"baseFeePerGas": current_base_fee} + ) + tx_params = dict(eip1559_transaction) + del tx_params["maxFeePerGas"] + + pending_tx.params = tx_params + tx_params = speedup_strategy.execute(pending_tx) + + updated_max_priority_fee_per_gas = tx_params["maxPriorityFeePerGas"] + assert updated_max_priority_fee_per_gas == math.ceil( + old_max_priority_fee_per_gas * (1 + speedup_percentage) + ) + + updated_max_fee_per_gas = tx_params["maxFeePerGas"] + assert updated_max_fee_per_gas == math.ceil( + (current_base_fee * 2) + updated_max_priority_fee_per_gas + ) + + +def test_speedup_strategy_eip1559_tx_no_max_priority_fee( + w3, eip1559_transaction, mocker +): + # suppose no maxPriorityFeePerGas specified - use suggested tip + ( + current_base_fee, + max_tip_factor, + pending_tx, + speedup_percentage, + speedup_strategy, + suggested_tip, + ) = eip1559_setup(mocker, w3) + + mocker.patch.object( + w3.eth, "get_block", return_value={"baseFeePerGas": current_base_fee} + ) + tx_params = dict(eip1559_transaction) + del tx_params["maxPriorityFeePerGas"] + + pending_tx.params = tx_params + tx_params = speedup_strategy.execute(pending_tx) + + updated_max_priority_fee_per_gas = tx_params["maxPriorityFeePerGas"] + assert updated_max_priority_fee_per_gas == math.ceil( + suggested_tip * (1 + speedup_percentage) + ) + + updated_max_fee_per_gas = tx_params["maxFeePerGas"] + assert updated_max_fee_per_gas == math.ceil( + current_base_fee * (1 + speedup_percentage) + updated_max_priority_fee_per_gas + ) + + +def test_speedup_strategy_eip1559_tx_hit_max_tip(w3, eip1559_transaction, mocker): + ( + current_base_fee, + max_tip_factor, + pending_tx, + speedup_percentage, + speedup_strategy, + suggested_tip, + ) = eip1559_setup(mocker, w3) + + pending_tx.params = dict(eip1559_transaction) + expected_num_iterations_before_hitting_max_tip = math.ceil( + math.log(max_tip_factor) / math.log(1 + speedup_percentage) + ) + + # do one less iteration than expected + for i in range(expected_num_iterations_before_hitting_max_tip - 1): + tx_params = speedup_strategy.execute(pending_tx) + assert tx_params is not None + assert tx_params["maxPriorityFeePerGas"] <= (suggested_tip * max_tip_factor) + # update params + pending_tx.params = tx_params + + # next attempt should cause tip to exceed max tip factor + assert (pending_tx.params["maxPriorityFeePerGas"] * (1 + speedup_percentage)) > ( + suggested_tip * max_tip_factor + ) + tx_params = speedup_strategy.execute(pending_tx) + # hit max factor so no more changes - return None + assert ( + tx_params is None + ), f"no updates after {expected_num_iterations_before_hitting_max_tip} iterations" + + +def eip1559_setup(mocker, w3): + speedup_percentage = round(random.randint(110, 230) / 1000, 3) # [11%, 23%] + max_tip_factor = random.randint(2, 5) + speedup_strategy = FixedRateSpeedUp( + w3, + speedup_increase_percentage=speedup_percentage, + max_tip_factor=max_tip_factor, + ) + pending_tx = mocker.Mock(spec=PendingTx) + pending_tx.id = 1 + pending_tx.txhash = HexBytes("0xdeadbeef") + + # use consistent mocked values + current_base_fee = w3.eth.get_block("latest")["baseFeePerGas"] + suggested_tip = w3.eth.max_priority_fee + mocker.patch.object( + w3.eth, "get_block", return_value={"baseFeePerGas": current_base_fee} + ) + mocker.patch( + "web3.eth.eth.Eth.max_priority_fee", PropertyMock(return_value=suggested_tip) + ) + return ( + current_base_fee, + max_tip_factor, + pending_tx, + speedup_percentage, + speedup_strategy, + suggested_tip, + ) From 699d38fd46d7c03f631b13eeac9fdda27ebb5d52 Mon Sep 17 00:00:00 2001 From: derekpierre Date: Mon, 4 Mar 2024 15:28:29 -0500 Subject: [PATCH 15/70] Rename speed up strategy; while the rate value is fixed the updates are expontential because they build on previous updates. --- atxm/machine.py | 4 ++-- atxm/strategies.py | 7 +++++-- tests/test_strategy.py | 20 +++++++++++--------- 3 files changed, 18 insertions(+), 13 deletions(-) diff --git a/atxm/machine.py b/atxm/machine.py index a7fb6c2..7655616 100644 --- a/atxm/machine.py +++ b/atxm/machine.py @@ -12,7 +12,7 @@ from atxm.exceptions import TransactionFaulted, TransactionReverted from atxm.strategies import ( AsyncTxStrategy, - FixedRateSpeedUp, + ExponentialSpeedupStrategy, InsufficientFundsPause, TimeoutStrategy, ) @@ -92,7 +92,7 @@ class _Machine(StateMachine): STRATEGIES: List[Type[AsyncTxStrategy]] = [ InsufficientFundsPause, TimeoutStrategy, - FixedRateSpeedUp, + ExponentialSpeedupStrategy, ] class LogObserver: diff --git a/atxm/strategies.py b/atxm/strategies.py index 1763059..94b8e38 100644 --- a/atxm/strategies.py +++ b/atxm/strategies.py @@ -139,8 +139,11 @@ def execute(self, pending: PendingTx) -> Optional[TxParams]: return None -class FixedRateSpeedUp(AsyncTxStrategy): - """Speedup strategy for pending transactions.""" +class ExponentialSpeedupStrategy(AsyncTxStrategy): + """ + Speedup strategy for pending transactions that increases fees by a + percentage over the prior value every time it is used. + """ _SPEEDUP_INCREASE_PERCENTAGE = 0.125 # 12.5% diff --git a/tests/test_strategy.py b/tests/test_strategy.py index d223cd7..a6818b6 100644 --- a/tests/test_strategy.py +++ b/tests/test_strategy.py @@ -9,7 +9,7 @@ from web3.types import TxParams from atxm.exceptions import Fault, TransactionFaulted -from atxm.strategies import FixedRateSpeedUp, TimeoutStrategy +from atxm.strategies import ExponentialSpeedupStrategy, TimeoutStrategy from atxm.tx import PendingTx @@ -81,24 +81,26 @@ def test_speedup_strategy_constructor(w3): # invalid increase percentage for speedup_perc in [-1, -0.24, 0, 1.01, 1.1]: with pytest.raises(ValueError): - _ = FixedRateSpeedUp(w3=w3, speedup_increase_percentage=speedup_perc) + _ = ExponentialSpeedupStrategy( + w3=w3, speedup_increase_percentage=speedup_perc + ) # invalid max tip for max_tip in [-1, 0, 0.5, 0.9, 1]: with pytest.raises(ValueError): - _ = FixedRateSpeedUp(w3=w3, max_tip_factor=max_tip) + _ = ExponentialSpeedupStrategy(w3=w3, max_tip_factor=max_tip) # defaults - speedup_strategy = FixedRateSpeedUp(w3=w3) + speedup_strategy = ExponentialSpeedupStrategy(w3=w3) assert speedup_strategy.speedup_factor == ( - 1 + FixedRateSpeedUp._SPEEDUP_INCREASE_PERCENTAGE + 1 + ExponentialSpeedupStrategy._SPEEDUP_INCREASE_PERCENTAGE ) - assert speedup_strategy.max_tip_factor == FixedRateSpeedUp._MAX_TIP_FACTOR + assert speedup_strategy.max_tip_factor == ExponentialSpeedupStrategy._MAX_TIP_FACTOR # other values speedup_increase = 0.223 max_tip_factor = 4 - speedup_strategy = FixedRateSpeedUp( + speedup_strategy = ExponentialSpeedupStrategy( w3=w3, speedup_increase_percentage=speedup_increase, max_tip_factor=max_tip_factor, @@ -111,7 +113,7 @@ def test_speedup_strategy_constructor(w3): def test_speedup_strategy_legacy_tx(w3, legacy_transaction, mocker): speedup_percentage = 0.112 # 11.2% - speedup_strategy = FixedRateSpeedUp( + speedup_strategy = ExponentialSpeedupStrategy( w3, speedup_increase_percentage=speedup_percentage ) @@ -386,7 +388,7 @@ def test_speedup_strategy_eip1559_tx_hit_max_tip(w3, eip1559_transaction, mocker def eip1559_setup(mocker, w3): speedup_percentage = round(random.randint(110, 230) / 1000, 3) # [11%, 23%] max_tip_factor = random.randint(2, 5) - speedup_strategy = FixedRateSpeedUp( + speedup_strategy = ExponentialSpeedupStrategy( w3, speedup_increase_percentage=speedup_percentage, max_tip_factor=max_tip_factor, From 3755667407ef20a8c545f647a9c6a73e7dbde6a8 Mon Sep 17 00:00:00 2001 From: derekpierre Date: Mon, 4 Mar 2024 15:30:14 -0500 Subject: [PATCH 16/70] Use constant instead of "maxPriorityFeePerGas". --- atxm/strategies.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/atxm/strategies.py b/atxm/strategies.py index 94b8e38..ffc91fa 100644 --- a/atxm/strategies.py +++ b/atxm/strategies.py @@ -180,12 +180,12 @@ def _calculate_eip1559_speedup_fee(self, params: TxParams) -> Tuple[int, int, in _log_gas_weather(current_base_fee, suggested_tip) # default to 1 if not specified in tx - prior_max_priority_fee = params.get("maxPriorityFeePerGas", 0) + prior_max_priority_fee = params.get(self._MAX_PRIORITY_FEE_PER_GAS_FIELD, 0) updated_max_priority_fee = math.ceil( max(prior_max_priority_fee, suggested_tip) * self.speedup_factor ) - current_max_fee_per_gas = params.get("maxFeePerGas") + current_max_fee_per_gas = params.get(self._MAX_FEE_PER_GAS_FIELD) if current_max_fee_per_gas: # already previously set, just increase by factor but ensure base fee hasn't # also increased. The defaults used by web3py for this value is already pretty @@ -244,7 +244,7 @@ def execute(self, pending: PendingTx) -> Optional[TxParams]: if new_tip > (suggested_tip * self.max_tip_factor): # nothing the strategy can do here - don't change the params log.warn( - f"[speedup] Increasing pending transaction's maxPriorityFeePerGas " + f"[speedup] Increasing pending transaction's {self._MAX_PRIORITY_FEE_PER_GAS_FIELD} " f"to {round(Web3.from_wei(new_tip, 'gwei'), 4)} gwei will exceed " f"spending cap factor {self.max_tip_factor}x over suggested tip " f"({round(Web3.from_wei(suggested_tip, 'gwei'), 4)} gwei); " From 16c2ed3b78702933078c8600fbadf403d66a90cd Mon Sep 17 00:00:00 2001 From: derekpierre Date: Mon, 4 Mar 2024 15:37:41 -0500 Subject: [PATCH 17/70] Use constant for minimum required speedup increase percentage. --- atxm/strategies.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/atxm/strategies.py b/atxm/strategies.py index ffc91fa..cb5b9ac 100644 --- a/atxm/strategies.py +++ b/atxm/strategies.py @@ -147,6 +147,8 @@ class ExponentialSpeedupStrategy(AsyncTxStrategy): _SPEEDUP_INCREASE_PERCENTAGE = 0.125 # 12.5% + _MIN_SPEEDUP_INCREASE = 0.10 # mandated by eth standard + _MAX_TIP_FACTOR = 3 # max 3x over suggested tip _NAME = "speedup" @@ -163,7 +165,10 @@ def __init__( ): super().__init__(w3) - if speedup_increase_percentage > 1 or speedup_increase_percentage < 0.10: + if ( + speedup_increase_percentage < self._MIN_SPEEDUP_INCREASE + or speedup_increase_percentage > 1 + ): raise ValueError( f"Invalid speedup increase percentage {speedup_increase_percentage}; " f"must be in range [0.10, 1]" From 1c670559bddf8de3bf226e5a57e65ea9b1774acd Mon Sep 17 00:00:00 2001 From: derekpierre Date: Mon, 4 Mar 2024 16:14:38 -0500 Subject: [PATCH 18/70] Use a max between a low default value and warn factor calc in case timeout is too short and warn threshold becomes too low. Loosen warn factor from 5% to 15%. --- atxm/strategies.py | 6 ++++-- tests/test_strategy.py | 15 ++++++++++++++- 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/atxm/strategies.py b/atxm/strategies.py index cb5b9ac..eeb781d 100644 --- a/atxm/strategies.py +++ b/atxm/strategies.py @@ -91,11 +91,13 @@ class TimeoutStrategy(AsyncTxStrategy): _TIMEOUT = 60 * 60 # 1 hour in seconds - _WARN_FACTOR = 0.05 # 5% of timeout remaining + _WARN_FACTOR = 0.15 # 15% of timeout remaining def __init__(self, w3: Web3, timeout: Optional[int] = None): super().__init__(w3) self.timeout = timeout or self._TIMEOUT + # use 30s as default in case timeout is too small for warn factor + self._warn_threshold = max(30, self.timeout * self._WARN_FACTOR) def __active_timed_out(self, pending: PendingTx) -> bool: """Returns True if the active transaction has timed out.""" @@ -110,7 +112,7 @@ def __active_timed_out(self, pending: PendingTx) -> bool: end_time = creation_time + timedelta(seconds=self.timeout) time_remaining = end_time - now human_end_time = end_time.strftime("%Y-%m-%d %H:%M:%S") - if time_remaining.seconds < (self.timeout * self._WARN_FACTOR): + if time_remaining.seconds < self._warn_threshold: self.log.warn( f"[timeout] Transaction {pending.txhash.hex()} will timeout in " f"{time_remaining} at {human_end_time}" diff --git a/tests/test_strategy.py b/tests/test_strategy.py index a6818b6..53c1c2a 100644 --- a/tests/test_strategy.py +++ b/tests/test_strategy.py @@ -14,15 +14,28 @@ def test_timeout_strategy(w3, mocker): - TIMEOUT = 600 # 10 mins + TIMEOUT = 900 # 15 mins # default timeout timeout_strategy = TimeoutStrategy(w3) assert timeout_strategy.timeout == TimeoutStrategy._TIMEOUT + assert ( + timeout_strategy._warn_threshold + == TimeoutStrategy._TIMEOUT * TimeoutStrategy._WARN_FACTOR + ) + + # specific timeout - low timeout + low_timeout = 60 # 60s + timeout_strategy = TimeoutStrategy(w3, timeout=low_timeout) + assert timeout_strategy.timeout == low_timeout + assert ( + timeout_strategy._warn_threshold == 30 + ) # timeout so low that max warn threshold hit # specific timeout timeout_strategy = TimeoutStrategy(w3, timeout=TIMEOUT) assert timeout_strategy.timeout == TIMEOUT + assert timeout_strategy._warn_threshold == TIMEOUT * TimeoutStrategy._WARN_FACTOR assert timeout_strategy.name == timeout_strategy._NAME From f8cf36b086a6d5a15a89d12308dffa7a858a5d1d Mon Sep 17 00:00:00 2001 From: derekpierre Date: Mon, 4 Mar 2024 16:27:12 -0500 Subject: [PATCH 19/70] Add todo about best way of setting a cap on the speedup strategy. --- atxm/strategies.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/atxm/strategies.py b/atxm/strategies.py index eeb781d..681b8e3 100644 --- a/atxm/strategies.py +++ b/atxm/strategies.py @@ -248,6 +248,8 @@ def execute(self, pending: PendingTx) -> Optional[TxParams]: suggested_tip, new_tip, new_max_fee = self._calculate_eip1559_speedup_fee( params ) + + # TODO: is this the best way of setting a cap? if new_tip > (suggested_tip * self.max_tip_factor): # nothing the strategy can do here - don't change the params log.warn( From 1f1fb288d86345de40890301b11a78be0a1a210d Mon Sep 17 00:00:00 2001 From: derekpierre Date: Mon, 4 Mar 2024 16:29:56 -0500 Subject: [PATCH 20/70] Use reactor to determine when hook is called instead of repeatedly calling _cycle() on the machine - the latter could hide bugs so don't do it. --- tests/test_faults.py | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/tests/test_faults.py b/tests/test_faults.py index 0ffd3ef..61cdb12 100644 --- a/tests/test_faults.py +++ b/tests/test_faults.py @@ -1,3 +1,6 @@ +import pytest_twisted +from twisted.internet import reactor +from twisted.internet.task import deferLater from web3.exceptions import TransactionNotFound from web3.types import TxReceipt @@ -26,10 +29,14 @@ def _broadcast_tx(machine, eip1559_transaction, account, mocker): return atx, fault_hook +@pytest_twisted.inlineCallbacks def _verify_tx_faulted(machine, atx, fault_hook, expected_fault: Fault): - while fault_hook.call_count == 0: - # ensure tx processed - machine._cycle() + machine._cycle() + + # ensure hook is called + yield deferLater(reactor, 0.2, lambda: None) + assert fault_hook.call_count == 1 + fault_hook.assert_called_with(atx) assert atx.final is False assert isinstance(atx, FaultedTx) @@ -42,9 +49,6 @@ def _verify_tx_faulted(machine, atx, fault_hook, expected_fault: Fault): assert machine.pending is None assert atx.final is False - assert fault_hook.call_count == 1 - fault_hook.assert_called_with(atx) - def test_revert( chain, From abb85558c9b9c31bb0c34762cbc6c0468c62a698 Mon Sep 17 00:00:00 2001 From: derekpierre Date: Mon, 4 Mar 2024 16:31:23 -0500 Subject: [PATCH 21/70] Add TODO to determine whether strategies can be overriden. --- tests/test_faults.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/test_faults.py b/tests/test_faults.py index 61cdb12..31fd52f 100644 --- a/tests/test_faults.py +++ b/tests/test_faults.py @@ -83,6 +83,8 @@ def test_strategy_fault( w3, machine, clock, eip1559_transaction, account, interval, mock_wake_sleep, mocker ): faulty_strategy = mocker.Mock(spec=AsyncTxStrategy) + + # TODO: consider whether strategies should just be overriden through the constructor machine._strategies.insert(0, faulty_strategy) # add first atx, fault_hook = _broadcast_tx(machine, eip1559_transaction, account, mocker) From 5491a6e27c35fe5aab3cfe2270a1b2e04618c5b0 Mon Sep 17 00:00:00 2001 From: derekpierre Date: Tue, 5 Mar 2024 11:27:03 -0500 Subject: [PATCH 22/70] Update logging category for monitor when no longer tracking tx. --- atxm/machine.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/atxm/machine.py b/atxm/machine.py index 7655616..2aede92 100644 --- a/atxm/machine.py +++ b/atxm/machine.py @@ -384,7 +384,7 @@ def __monitor_finalized(self) -> None: if tx in self._tx_tracker.finalized: self._tx_tracker.finalized.remove(tx) self.log.info( - f"[clear] stopped tracking {tx.txhash.hex()} after {confirmations} confirmations" + f"[monitor] stopped tracking {tx.txhash.hex()} after {confirmations} confirmations" ) continue self.log.info( From 2a9c0b5308607b9697cb54357e3761fd8d5455b8 Mon Sep 17 00:00:00 2001 From: derekpierre Date: Tue, 5 Mar 2024 12:18:12 -0500 Subject: [PATCH 23/70] Use enable/disabling of auto mining for better testing. --- tests/conftest.py | 15 +++++++++++++-- tests/test_faults.py | 18 +++++++----------- tests/test_machine.py | 26 ++++++++++++++++++++------ 3 files changed, 40 insertions(+), 19 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index 5d59a09..ec51efc 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -3,6 +3,7 @@ import pytest from eth_account import Account +from eth_tester import EthereumTester from statemachine import State from twisted.internet.task import Clock from twisted.logger import globalLogPublisher, textFileLogObserver @@ -28,7 +29,6 @@ def legacy_transaction(account, w3): params = { "chainId": 1337, "nonce": 0, - "from": account.address, "to": account.address, "value": 0, "gas": 21000, @@ -45,7 +45,6 @@ def eip1559_transaction(account, w3): params = { "chainId": 1337, "nonce": 0, - "from": account.address, "to": account.address, "value": 0, "gas": 21000, @@ -88,6 +87,18 @@ def mock_wake_sleep(machine, mocker): return wake, sleep +@pytest.fixture +def disable_auto_mining(ethereum_tester): + ethereum_tester.disable_auto_mine_transactions() + yield + ethereum_tester.enable_auto_mine_transactions() + + +@pytest.fixture +def ethereum_tester(w3) -> EthereumTester: + return w3.provider.ethereum_tester + + class StateObserver: def __init__(self): self.transitions: List[Tuple[State, State]] = [] diff --git a/tests/test_faults.py b/tests/test_faults.py index 31fd52f..3fe125b 100644 --- a/tests/test_faults.py +++ b/tests/test_faults.py @@ -1,7 +1,7 @@ +import pytest import pytest_twisted from twisted.internet import reactor from twisted.internet.task import deferLater -from web3.exceptions import TransactionNotFound from web3.types import TxReceipt from atxm.exceptions import Fault, TransactionFaulted @@ -51,7 +51,6 @@ def _verify_tx_faulted(machine, atx, fault_hook, expected_fault: Fault): def test_revert( - chain, w3, machine, clock, @@ -65,9 +64,7 @@ def test_revert( assert machine.pending - chain.mine(1) - - # force receipt to symbolize a revert of the tx + # tx auto-mined; force receipt to symbolize a revert of the tx receipt = _get_receipt_from_txhash(w3, atx.txhash) revert_receipt = dict(receipt) revert_receipt["status"] = 0 @@ -79,12 +76,13 @@ def test_revert( _verify_tx_faulted(machine, atx, fault_hook, expected_fault=Fault.REVERT) +@pytest.mark.usefixtures("disable_auto_mining") def test_strategy_fault( w3, machine, clock, eip1559_transaction, account, interval, mock_wake_sleep, mocker ): faulty_strategy = mocker.Mock(spec=AsyncTxStrategy) - # TODO: consider whether strategies should just be overriden through the constructor + # TODO: consider whether strategies should just be overridden through the constructor machine._strategies.insert(0, faulty_strategy) # add first atx, fault_hook = _broadcast_tx(machine, eip1559_transaction, account, mocker) @@ -94,20 +92,18 @@ def test_strategy_fault( tx=atx, fault=Fault.ERROR, message=faulty_message ) - mocker.patch.object( - w3.eth, "get_transaction_receipt", side_effect=TransactionNotFound - ) - + # tx not mined _verify_tx_faulted(machine, atx, fault_hook, expected_fault=Fault.ERROR) assert atx.error == faulty_message +@pytest.mark.usefixtures("disable_auto_mining") def test_timeout_strategy_fault( w3, machine, clock, eip1559_transaction, account, interval, mock_wake_sleep, mocker ): atx, fault_hook = _broadcast_tx(machine, eip1559_transaction, account, mocker) atx.created -= 9999999999 - mocker.patch.object(w3.eth, "get_transaction", side_effect=TransactionNotFound) + # tx not mined _verify_tx_faulted(machine, atx, fault_hook, expected_fault=Fault.TIMEOUT) diff --git a/tests/test_machine.py b/tests/test_machine.py index d7f369e..29b2f96 100644 --- a/tests/test_machine.py +++ b/tests/test_machine.py @@ -197,6 +197,7 @@ def test_wake_no_call_after_queuing_when_already_paused( @pytest_twisted.inlineCallbacks +@pytest.mark.usefixtures("disable_auto_mining") def test_broadcast( clock, machine, @@ -255,8 +256,9 @@ def test_broadcast( @pytest_twisted.inlineCallbacks +@pytest.mark.usefixtures("disable_auto_mining") def test_finalize( - chain, + ethereum_tester, clock, machine, state_observer, @@ -290,7 +292,7 @@ def test_finalize( # advance to finalize the transaction while machine.pending: - yield chain.mine(1) + yield ethereum_tester.mine_block() yield clock.advance(1) # The transaction is no longer pending @@ -321,8 +323,15 @@ def test_finalize( @pytest_twisted.inlineCallbacks +@pytest.mark.usefixtures("disable_auto_mining") def test_follow( - chain, machine, state_observer, clock, eip1559_transaction, account, mock_wake_sleep + ethereum_tester, + machine, + state_observer, + clock, + eip1559_transaction, + account, + mock_wake_sleep, ): machine.start() assert machine.current_state == machine._IDLE @@ -339,12 +348,13 @@ def test_follow( assert machine.current_state == machine._BUSY while not machine.finalized: + yield ethereum_tester.mine_block() yield clock.advance(1) assert atx.final is True while len(machine.finalized) > 0: - yield chain.mine(1) + yield ethereum_tester.mine_block() yield clock.advance(1) assert len(machine.finalized) == 0 @@ -431,8 +441,9 @@ def test_pause_when_busy(clock, machine, eip1559_transaction, account, mocker): machine.stop() +@pytest.mark.usefixtures("disable_auto_mining") def test_simple_state_transitions( - chain, machine, eip1559_transaction, account, mock_wake_sleep + ethereum_tester, machine, eip1559_transaction, account, mock_wake_sleep ): assert machine.current_state == machine._IDLE @@ -468,6 +479,8 @@ def test_simple_state_transitions( assert machine.current_state == machine._BUSY assert machine.busy + ethereum_tester.mine_block() + # busy -> pause machine.pause() assert machine.current_state == machine._PAUSED @@ -481,9 +494,10 @@ def test_simple_state_transitions( # finalize tx while machine.busy: - chain.mine(1) + ethereum_tester.mine_block() machine._cycle() + ethereum_tester.mine_block() assert atx.final is True # transition to idle From 436ce64549de7747db1f5b6bad7d443bdb76fd95 Mon Sep 17 00:00:00 2001 From: derekpierre Date: Tue, 5 Mar 2024 15:04:39 -0500 Subject: [PATCH 24/70] Fix nonce logging message for strategies. --- atxm/strategies.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/atxm/strategies.py b/atxm/strategies.py index 681b8e3..e04cad5 100644 --- a/atxm/strategies.py +++ b/atxm/strategies.py @@ -236,7 +236,7 @@ def execute(self, pending: PendingTx) -> Optional[TxParams]: old_gas_price = params[self._GAS_PRICE_FIELD] new_gas_price = self._calculate_legacy_speedup_fee(pending.params) log.info( - f"[speedup] Speeding up legacy transaction #atx-{pending.id} (nonce=#{params['nonce']}) \n" + f"[speedup] Speeding up legacy transaction #atx-{pending.id} (nonce={params['nonce']}) \n" f"gasPrice {old_gas_price} -> {new_gas_price}" ) params[self._GAS_PRICE_FIELD] = new_gas_price @@ -272,7 +272,7 @@ def execute(self, pending: PendingTx) -> Optional[TxParams]: else "undefined" ) log.info( - f"[speedup] Speeding up transaction #atx-{pending.id} (nonce=#{params['nonce']}) \n" + f"[speedup] Speeding up transaction #atx-{pending.id} (nonce={params['nonce']}) \n" f"{self._MAX_PRIORITY_FEE_PER_GAS_FIELD} {tip_increase_message} -> {new_tip} \n" f"{self._MAX_FEE_PER_GAS_FIELD} {fee_increase_message} -> {new_max_fee}" ) From c7851ae1e51df9784357d8668732239c6c4b9c60 Mon Sep 17 00:00:00 2001 From: derekpierre Date: Tue, 5 Mar 2024 15:17:43 -0500 Subject: [PATCH 25/70] Add the ability to update the tracker's active pending tx after a retry. --- atxm/tracker.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/atxm/tracker.py b/atxm/tracker.py index f5632be..57560d6 100644 --- a/atxm/tracker.py +++ b/atxm/tracker.py @@ -101,6 +101,17 @@ def __set_active(self, tx: PendingTx) -> None: return log.debug(f"[tracker] tracked active transaction {tx.txhash.hex()}") + def update_after_retry(self, tx: PendingTx) -> PendingTx: + if tx.id != self.__active.id: + raise RuntimeError( + f"Trying to update unexpected active tx: from {self.__active.id} to {tx.id}" + ) + + self.__active.txhash = tx.txhash + self.__active.params = tx.params + + return self.pending + def morph(self, tx: FutureTx, txhash: TxHash) -> PendingTx: """ Morphs a future transaction into a pending transaction. From 7e647c092fb15b3db1457e397e3ca7939383608a Mon Sep 17 00:00:00 2001 From: derekpierre Date: Tue, 5 Mar 2024 15:07:11 -0500 Subject: [PATCH 26/70] Ensure that "from" value in tx params matches/equals signer. Don't have FutureTx store "from" separately from the the value in the params; it is redundant and complicates usage. --- atxm/machine.py | 16 ++++++++++++---- atxm/tx.py | 9 +++++---- 2 files changed, 17 insertions(+), 8 deletions(-) diff --git a/atxm/machine.py b/atxm/machine.py index 2aede92..8020280 100644 --- a/atxm/machine.py +++ b/atxm/machine.py @@ -1,4 +1,4 @@ -from copy import deepcopy +from copy import copy, deepcopy from typing import List, Optional, Type from eth_account.signers.local import LocalAccount @@ -423,9 +423,17 @@ def queue_transaction( if signer.address not in self.signers: self.signers[signer.address] = signer - tx = self._tx_tracker._queue( - _from=signer.address, params=params, *args, **kwargs - ) + params_copy = copy(params) + + from_param = params_copy.get("from") + if from_param is None: + params_copy["from"] = signer.address + if from_param and from_param != signer.address: + raise ValueError( + f"Mismatched 'from' value ({from_param}) and 'signer' account ({signer.address})" + ) + + tx = self._tx_tracker._queue(params=params_copy, *args, **kwargs) if not previously_busy_or_paused: self._wake() diff --git a/atxm/tx.py b/atxm/tx.py index 3320aa7..573d2f5 100644 --- a/atxm/tx.py +++ b/atxm/tx.py @@ -15,6 +15,7 @@ @dataclass class AsyncTx(ABC): id: int + params: TxParams final: bool = field(default=None, init=False) fault: Optional[Fault] = field(default=None, init=False) on_broadcast: Optional[Callable[[PendingTx], None]] = field( @@ -49,17 +50,18 @@ def from_dict(cls, data: Dict): @dataclass class FutureTx(AsyncTx): final: bool = field(default=False, init=False) - params: TxParams - _from: ChecksumAddress info: Optional[Dict] = None def __hash__(self): return hash(self.id) + @property + def _from(self) -> ChecksumAddress: + return self.params["from"] + def to_dict(self) -> Dict: return { "id": self.id, - "from": self._from, "params": _serialize_tx_params(self.params), "info": self.info, } @@ -68,7 +70,6 @@ def to_dict(self) -> Dict: def from_dict(cls, data: Dict): return cls( id=int(data["id"]), - _from=data["from"], params=TxParams(data["params"]), info=dict(data["info"]), ) From 283ba70726a19ee516133ecb5bed46f47b1d0259 Mon Sep 17 00:00:00 2001 From: derekpierre Date: Tue, 5 Mar 2024 15:20:22 -0500 Subject: [PATCH 27/70] Make __fire reusable by both broadcast and strategize - it is now limited to send raw transaction and returning the tx hash. Update machine and strategies use "from" value in the tx params. Make some functions void since unnecessary to return anything. --- atxm/machine.py | 66 ++++++++++++++++++++++++++-------------------- atxm/strategies.py | 2 +- atxm/tracker.py | 3 --- atxm/utils.py | 5 ++-- 4 files changed, 41 insertions(+), 35 deletions(-) diff --git a/atxm/machine.py b/atxm/machine.py index 8020280..f82575a 100644 --- a/atxm/machine.py +++ b/atxm/machine.py @@ -18,8 +18,8 @@ ) from atxm.tracker import _TxTracker from atxm.tx import ( + AsyncTx, FutureTx, - PendingTx, TxHash, ) from atxm.utils import ( @@ -240,7 +240,7 @@ def _sleep(self) -> None: # Lifecycle # - def __handle_active_transaction(self) -> bool: + def __handle_active_transaction(self) -> None: """ Handles the currently tracked pending transaction. @@ -262,7 +262,7 @@ def __handle_active_transaction(self) -> bool: # Outcome 2: the pending transaction was reverted (final error) except TransactionReverted as e: self._tx_tracker.fault(fault_error=e) - return True + return # Outcome 3: pending transaction is finalized (final success) if receipt: @@ -273,14 +273,13 @@ def __handle_active_transaction(self) -> bool: f"with {confirmations} confirmation(s) txhash: {final_txhash.hex()}" ) self._tx_tracker.finalize_active_tx(receipt=receipt) - return True + return # Outcome 4: re-strategize the pending transaction - pending_tx = self.__strategize() - return pending_tx is not None + self.__strategize() # - # Broadcast + # Broadcast tx # def __get_signer(self, address: str) -> LocalAccount: @@ -290,7 +289,7 @@ def __get_signer(self, address: str) -> LocalAccount: raise ValueError(f"Signer {address} not found") return signer - def __fire(self, tx: FutureTx, msg: str) -> Optional[PendingTx]: + def __fire(self, tx: AsyncTx, msg: str) -> Optional[TxHash]: """ Signs and broadcasts a transaction, handling RPC errors and internal state changes. @@ -301,23 +300,16 @@ def __fire(self, tx: FutureTx, msg: str) -> Optional[PendingTx]: Morphs a `FutureTx` into a `PendingTx` and advances it into the active transaction slot if broadcast is successful. """ - signer: LocalAccount = self.__get_signer(tx._from) - try: - txhash = self.w3.eth.send_raw_transaction( - signer.sign_transaction(tx.params).rawTransaction - ) - except ValueError as e: - _handle_rpc_error(e, tx=tx) - return + signer: LocalAccount = self.__get_signer(tx.params["from"]) + txhash = self.w3.eth.send_raw_transaction( + signer.sign_transaction(tx.params).rawTransaction + ) self.log.info( f"[{msg}] fired transaction #atx-{tx.id}|{tx.params['nonce']}|{txhash.hex()}" ) - pending_tx = self._tx_tracker.morph(tx=tx, txhash=txhash) - if tx.on_broadcast: - fire_hook(hook=tx.on_broadcast, tx=pending_tx) - return pending_tx + return txhash - def __strategize(self) -> Optional[PendingTx]: + def __strategize(self) -> None: """Retry the currently tracked pending transaction with the configured strategy.""" if not self._tx_tracker.pending: raise RuntimeError("No active transaction to strategize") @@ -343,20 +335,28 @@ def __strategize(self) -> Optional[PendingTx]: return # (!) retry the transaction with the new parameters - retry_params = TxParams(_active_copy.params) _names = " -> ".join(s.name for s in self._strategies) - pending_tx = self.__fire(tx=retry_params, msg=_names) - self.log.info(f"[retry] transaction #{pending_tx.id} has been re-broadcasted") - return pending_tx + # TODO try-except needed here (similar to broadcast) #14, #18, #20 + txhash = self.__fire(tx=_active_copy, msg=_names) + + _active_copy.txhash = txhash + self._tx_tracker.update_after_retry(_active_copy) - def __broadcast(self) -> Optional[TxHash]: + pending_tx = self._tx_tracker.pending + self.log.info(f"[retry] transaction #{pending_tx.id} has been re-broadcasted") + if pending_tx.on_broadcast: + fire_hook(hook=pending_tx.on_broadcast, tx=pending_tx) + + def __broadcast(self): """ Attempts to broadcast the next `FutureTx` in the queue. If the broadcast is not successful, it is re-queued. """ future_tx = self._tx_tracker._pop() # popleft future_tx.params = _make_tx_params(future_tx.params) + + # update nonce as necessary signer = self.__get_signer(future_tx._from) nonce = self.w3.eth.get_transaction_count(signer.address, "latest") if nonce > future_tx.params["nonce"]: @@ -365,11 +365,19 @@ def __broadcast(self) -> Optional[TxHash]: f"by another transaction. Updating queued tx nonce {future_tx.params['nonce']} -> {nonce}" ) future_tx.params["nonce"] = nonce - pending_tx = self.__fire(tx=future_tx, msg="broadcast") - if not pending_tx: + + try: + txhash = self.__fire(tx=future_tx, msg="broadcast") + except ValueError as e: + _handle_rpc_error(e, tx=future_tx) + # TODO don't requeue forever #12, #20 self._tx_tracker._requeue(future_tx) return - return pending_tx.txhash + + self._tx_tracker.morph(tx=future_tx, txhash=txhash) + pending_tx = self._tx_tracker.pending + if pending_tx.on_broadcast: + fire_hook(hook=pending_tx.on_broadcast, tx=pending_tx) # # Monitoring diff --git a/atxm/strategies.py b/atxm/strategies.py index e04cad5..1bf7089 100644 --- a/atxm/strategies.py +++ b/atxm/strategies.py @@ -70,7 +70,7 @@ class InsufficientFundsPause(AsyncTxStrategy): _NAME = "insufficient-funds" def execute(self, pending: PendingTx) -> Optional[TxParams]: - balance = self.w3.eth.get_balance(pending._from) + balance = self.w3.eth.get_balance(pending.params["from"]) if balance == 0: self.log.warn( f"Insufficient funds for transaction #{pending.params['nonce']}" diff --git a/atxm/tracker.py b/atxm/tracker.py index 57560d6..93496d6 100644 --- a/atxm/tracker.py +++ b/atxm/tracker.py @@ -6,7 +6,6 @@ from pathlib import Path from typing import Callable, Deque, Dict, Optional, Set, Tuple -from eth_typing import ChecksumAddress from web3.types import TxParams, TxReceipt from atxm.exceptions import TransactionFaulted @@ -207,7 +206,6 @@ def _requeue(self, tx: FutureTx) -> None: def _queue( self, params: TxParams, - _from: ChecksumAddress, info: Dict[str, str] = None, on_broadcast: Optional[Callable[[PendingTx], None]] = None, on_finalized: Optional[Callable[[FinalizedTx], None]] = None, @@ -215,7 +213,6 @@ def _queue( ) -> FutureTx: """Queue a new transaction for broadcast and subsequent tracking.""" tx = FutureTx( - _from=_from, id=self.__COUNTER, params=params, info=info, diff --git a/atxm/utils.py b/atxm/utils.py index 875b3a6..825e459 100644 --- a/atxm/utils.py +++ b/atxm/utils.py @@ -13,7 +13,7 @@ TransactionReverted, ) from atxm.logging import log -from atxm.tx import AsyncTx, FinalizedTx, FutureTx, PendingTx, TxHash +from atxm.tx import AsyncTx, FinalizedTx, PendingTx, TxHash @memoize @@ -116,7 +116,7 @@ def _hook() -> None: log.info(f"[hook] fired hook {hook} for transaction #atx-{tx.id}") -def _handle_rpc_error(e: Exception, tx: FutureTx) -> None: +def _handle_rpc_error(e: Exception, tx: AsyncTx) -> None: try: error = RPCError(**e.args[0]) except TypeError: @@ -149,6 +149,7 @@ def _make_tx_params(data: TxData) -> TxParams: "nonce": data["nonce"], "chainId": data["chainId"], "gas": data["gas"], + "from": data["from"], "to": data["to"], "value": data["value"], "data": data.get("data", b""), From bd27cbfecf01bab53890493c93e2e62b1781e30c Mon Sep 17 00:00:00 2001 From: derekpierre Date: Tue, 5 Mar 2024 15:23:31 -0500 Subject: [PATCH 28/70] Add tests to ensure that strategies are employed when a tx hasn't already been mined and needs replacing. --- tests/test_machine.py | 126 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 126 insertions(+) diff --git a/tests/test_machine.py b/tests/test_machine.py index 29b2f96..95eb8d7 100644 --- a/tests/test_machine.py +++ b/tests/test_machine.py @@ -1,9 +1,12 @@ +import math + import pytest import pytest_twisted from twisted.internet import reactor from twisted.internet.task import deferLater +from atxm.strategies import TimeoutStrategy from atxm.tx import FutureTx, PendingTx @@ -372,6 +375,129 @@ def test_follow( machine.stop() +@pytest_twisted.inlineCallbacks +@pytest.mark.usefixtures("disable_auto_mining") +def test_use_strategies_speedup_used( + ethereum_tester, + machine, + state_observer, + clock, + eip1559_transaction, + account, + mock_wake_sleep, +): + machine.start() + assert machine.current_state == machine._IDLE + + old_max_fee = eip1559_transaction["maxFeePerGas"] + old_priority_fee = eip1559_transaction["maxPriorityFeePerGas"] + + atx = machine.queue_transaction( + params=eip1559_transaction, + signer=account, + ) + + # advance to broadcast the transaction + while machine.pending is None: + yield clock.advance(1) + + assert machine.current_state == machine._BUSY + + for i in range(2): + # advance 2 cycles + yield clock.advance(1) + + while not machine.finalized: + yield ethereum_tester.mine_block() + yield clock.advance(1) + + assert atx.final is True + + # speed up strategy kicked in + assert atx.params["maxFeePerGas"] > old_max_fee + assert atx.params["maxPriorityFeePerGas"] > old_priority_fee + + while len(machine.finalized) > 0: + yield ethereum_tester.mine_block() + yield clock.advance(1) + + assert len(machine.finalized) == 0 + assert len(machine.queued) == 0 + assert machine.pending is None + + assert not machine.busy + + assert machine.current_state == machine._IDLE + + assert len(state_observer.transitions) == 2 + assert state_observer.transitions[0] == (machine._IDLE, machine._BUSY) + assert state_observer.transitions[1] == (machine._BUSY, machine._IDLE) + + machine.stop() + + +@pytest_twisted.inlineCallbacks +@pytest.mark.usefixtures("disable_auto_mining") +def test_use_strategies_timeout_used( + ethereum_tester, + w3, + machine, + state_observer, + clock, + eip1559_transaction, + account, + mocker, + mock_wake_sleep, +): + # TODO consider whether this should just be provided to constructor - #23 + fault_hook = mocker.Mock() + + machine.start() + assert machine.current_state == machine._IDLE + + atx = machine.queue_transaction( + params=eip1559_transaction, signer=account, on_fault=fault_hook + ) + + # advance to broadcast the transaction + while machine.pending is None: + yield clock.advance(1) + + assert machine.current_state == machine._BUSY + + # don't mine transaction at all; wait for hook to be called when timeout occurs + + # need some cycles for strategies to kick in + num_cycles = 4 + for i in range(num_cycles): + # reduce creation time by timeout to force timeout + atx.created -= math.ceil(TimeoutStrategy._TIMEOUT / (num_cycles - 1)) + yield clock.advance(1) + + # ensure switch back to IDLE + yield clock.advance(1) + + # ensure that hook is called + yield deferLater(reactor, 0.2, lambda: None) + + assert fault_hook.call_count == 1 + fault_hook.assert_called_with(atx) + + assert len(machine.queued) == 0 + assert machine.pending is None + + assert not machine.busy + assert not atx.final + + assert machine.current_state == machine._IDLE + + assert len(state_observer.transitions) == 2 + assert state_observer.transitions[0] == (machine._IDLE, machine._BUSY) + assert state_observer.transitions[1] == (machine._BUSY, machine._IDLE) + + machine.stop() + + @pytest_twisted.inlineCallbacks def test_pause_when_idle(clock, machine, mocker): machine.start() From 25ad0b57a5973a854f2486c75b68fea9eee744b7 Mon Sep 17 00:00:00 2001 From: derekpierre Date: Wed, 6 Mar 2024 08:23:26 -0500 Subject: [PATCH 29/70] Add testing for "from" tx parameter handling when a tx is being queued. --- tests/test_machine.py | 44 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/tests/test_machine.py b/tests/test_machine.py index 95eb8d7..d8cb326 100644 --- a/tests/test_machine.py +++ b/tests/test_machine.py @@ -3,6 +3,7 @@ import pytest import pytest_twisted +from eth_account import Account from twisted.internet import reactor from twisted.internet.task import deferLater @@ -39,6 +40,49 @@ def test_no_rpc_calls_when_idle(clock, machine, state_observer, rpc_spy): machine.stop() +def test_queue_from_parameter_handling( + machine, + account, + eip1559_transaction, + mock_wake_sleep, +): + # 1. "from" parameter does not match account + with pytest.raises(ValueError): + tx_params = dict(eip1559_transaction) + + # use random checksum address + random_account = Account.create( + "YOUTH IS WASTED ON THE YOUNG" + ) # - George Bernard Shaw + assert random_account.address != account.address, "addresses don't match" + + tx_params["from"] = random_account.address + _ = machine.queue_transaction( + params=tx_params, + signer=account, + ) + + # 2. no "from" parameter + tx_params = dict(eip1559_transaction) + if "from" in eip1559_transaction: + del tx_params["from"] + + atx = machine.queue_transaction( + params=tx_params, + signer=account, + ) + assert atx.params["from"] == account.address, "same as signer account" + + # 3. matching "from" parameter + tx_params = dict(eip1559_transaction) + tx_params["from"] = account.address + atx = machine.queue_transaction( + params=tx_params, + signer=account, + ) + assert atx.params["from"] == account.address + + def test_queue( machine, state_observer, From adac77d8858fea60e90cba70a16730946aac2f0f Mon Sep 17 00:00:00 2001 From: derekpierre Date: Wed, 6 Mar 2024 09:01:54 -0500 Subject: [PATCH 30/70] Add test for strategies which do not make updates to parameters. --- tests/test_machine.py | 86 ++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 85 insertions(+), 1 deletion(-) diff --git a/tests/test_machine.py b/tests/test_machine.py index d8cb326..e5ab03d 100644 --- a/tests/test_machine.py +++ b/tests/test_machine.py @@ -7,7 +7,7 @@ from twisted.internet import reactor from twisted.internet.task import deferLater -from atxm.strategies import TimeoutStrategy +from atxm.strategies import AsyncTxStrategy, TimeoutStrategy from atxm.tx import FutureTx, PendingTx @@ -542,6 +542,90 @@ def test_use_strategies_timeout_used( machine.stop() +@pytest_twisted.inlineCallbacks +@pytest.mark.usefixtures("disable_auto_mining") +def test_use_strategies_that_dont_make_updates( + ethereum_tester, + w3, + machine, + state_observer, + clock, + eip1559_transaction, + account, + mocker, + mock_wake_sleep, +): + # TODO consider whether this should just be provided to constructor - #23 + machine._strategies.clear() + + # strategies that don't make updates + strategy_1 = mocker.Mock(spec=AsyncTxStrategy) + strategy_1.execute.return_value = None + strategy_2 = mocker.Mock(spec=AsyncTxStrategy) + strategy_2.execute.return_value = None + + machine._strategies = [strategy_1, strategy_2] + + update_spy = mocker.spy(machine._tx_tracker, "update_after_retry") + + machine.start() + assert machine.current_state == machine._IDLE + + broadcast_hook = mocker.Mock() + atx = machine.queue_transaction( + params=eip1559_transaction, signer=account, on_broadcast=broadcast_hook + ) + + # advance to broadcast the transaction + while machine.pending is None: + yield clock.advance(1) + + # ensure that hook is called + yield deferLater(reactor, 0.2, lambda: None) + assert broadcast_hook.call_count == 1 + broadcast_hook.assert_called_with(atx), "called with correct parameter" + + original_params = dict(atx.params) + + assert machine.current_state == machine._BUSY + + # need some cycles while tx unmined for strategies to kick in + num_cycles = 4 + for i in range(num_cycles): + yield clock.advance(1) + # params remained unchanged since strategies don't make updates + assert atx.params == original_params, "params remain unchanged" + + s1_call_count = strategy_1.execute.call_count + assert s1_call_count > 0, "strategy #1 was called" + s2_call_count = strategy_2.execute.call_count + assert s2_call_count > 0, "strategy #2 was called" + + assert atx.params == original_params, "params remain unchanged" + assert update_spy.call_count == 0, "update never called because no retry" + + # mine tx + ethereum_tester.mine_block() + yield clock.advance(1) + + # ensure switch back to IDLE + yield clock.advance(1) + + assert len(machine.queued) == 0 + assert machine.pending is None + + assert not machine.busy + assert atx.final + + assert machine.current_state == machine._IDLE + + assert len(state_observer.transitions) == 2 + assert state_observer.transitions[0] == (machine._IDLE, machine._BUSY) + assert state_observer.transitions[1] == (machine._BUSY, machine._IDLE) + + machine.stop() + + @pytest_twisted.inlineCallbacks def test_pause_when_idle(clock, machine, mocker): machine.start() From dc6cda30e1f83a3fed5cee9d12ab7ac070e474be Mon Sep 17 00:00:00 2001 From: derekpierre Date: Wed, 6 Mar 2024 09:02:26 -0500 Subject: [PATCH 31/70] Add testing statements to ensure that the broadcast hook is also called for tx retries. General testing cleanup and reinforcement of tests for scenario checking. --- tests/test_machine.py | 27 ++++++++++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/tests/test_machine.py b/tests/test_machine.py index e5ab03d..1254107 100644 --- a/tests/test_machine.py +++ b/tests/test_machine.py @@ -428,6 +428,7 @@ def test_use_strategies_speedup_used( clock, eip1559_transaction, account, + mocker, mock_wake_sleep, ): machine.start() @@ -436,21 +437,42 @@ def test_use_strategies_speedup_used( old_max_fee = eip1559_transaction["maxFeePerGas"] old_priority_fee = eip1559_transaction["maxPriorityFeePerGas"] + broadcast_hook = mocker.Mock() atx = machine.queue_transaction( params=eip1559_transaction, signer=account, + on_broadcast=broadcast_hook, ) + update_spy = mocker.spy(machine._tx_tracker, "update_after_retry") + # advance to broadcast the transaction while machine.pending is None: yield clock.advance(1) + # ensure that hook is called + yield deferLater(reactor, 0.2, lambda: None) + assert broadcast_hook.call_count == 1 + broadcast_hook.assert_called_with(atx), "called with correct parameter" + assert machine.current_state == machine._BUSY + original_params = dict(atx.params) + + # need some cycles while tx unmined for strategies to kick in for i in range(2): - # advance 2 cycles yield clock.advance(1) + # ensure that hook is called + yield deferLater(reactor, 0.2, lambda: None) + assert ( + broadcast_hook.call_count > 1 + ), "additional calls to broadcast since tx retried/replaced" + broadcast_hook.assert_called_with(atx), "called with correct parameter" + + assert atx.params != original_params, "params changed" + assert update_spy.call_count > 0, "params updated" + while not machine.finalized: yield ethereum_tester.mine_block() yield clock.advance(1) @@ -493,7 +515,6 @@ def test_use_strategies_timeout_used( mocker, mock_wake_sleep, ): - # TODO consider whether this should just be provided to constructor - #23 fault_hook = mocker.Mock() machine.start() @@ -511,7 +532,7 @@ def test_use_strategies_timeout_used( # don't mine transaction at all; wait for hook to be called when timeout occurs - # need some cycles for strategies to kick in + # need some cycles while tx unmined for strategies to kick in num_cycles = 4 for i in range(num_cycles): # reduce creation time by timeout to force timeout From 28f67f40f2368f5d5be96324e0ec96520e274ced Mon Sep 17 00:00:00 2001 From: derekpierre Date: Wed, 6 Mar 2024 10:39:23 -0500 Subject: [PATCH 32/70] txhash is not optionally returned from __fire - either it is or an exception was raised. --- atxm/machine.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/atxm/machine.py b/atxm/machine.py index f82575a..63191ae 100644 --- a/atxm/machine.py +++ b/atxm/machine.py @@ -289,7 +289,7 @@ def __get_signer(self, address: str) -> LocalAccount: raise ValueError(f"Signer {address} not found") return signer - def __fire(self, tx: AsyncTx, msg: str) -> Optional[TxHash]: + def __fire(self, tx: AsyncTx, msg: str) -> TxHash: """ Signs and broadcasts a transaction, handling RPC errors and internal state changes. From a7eff1889cb3cb203b32e75f487533f19f9b7e82 Mon Sep 17 00:00:00 2001 From: derekpierre Date: Wed, 6 Mar 2024 13:49:22 -0500 Subject: [PATCH 33/70] Potential specialized handling of error cases when firing a tx; this can happen during broadcast or retry. Broadcast failures are handled differently than retry failures so added a new callback. InsufficientFunds is raised separately - but not currently handed; punting this decision to a different issue/pr. --- atxm/machine.py | 72 +++++++++++++++++++++++++++++++++++++++++++------ atxm/tracker.py | 2 ++ atxm/tx.py | 7 +++-- atxm/utils.py | 22 +-------------- 4 files changed, 72 insertions(+), 31 deletions(-) diff --git a/atxm/machine.py b/atxm/machine.py index 63191ae..19f4fff 100644 --- a/atxm/machine.py +++ b/atxm/machine.py @@ -2,14 +2,26 @@ from typing import List, Optional, Type from eth_account.signers.local import LocalAccount +from eth_utils import ValidationError from statemachine import State, StateMachine from twisted.internet import reactor from twisted.internet.defer import Deferred from twisted.internet.task import LoopingCall from web3 import Web3 +from web3.exceptions import ( + ProviderConnectionError, + TimeExhausted, + TooManyRequests, + Web3Exception, +) from web3.types import TxParams -from atxm.exceptions import TransactionFaulted, TransactionReverted +from atxm.exceptions import ( + Fault, + InsufficientFunds, + TransactionFaulted, + TransactionReverted, +) from atxm.strategies import ( AsyncTxStrategy, ExponentialSpeedupStrategy, @@ -26,7 +38,6 @@ _get_average_blocktime, _get_confirmations, _get_receipt, - _handle_rpc_error, _make_tx_params, fire_hook, ) @@ -301,9 +312,18 @@ def __fire(self, tx: AsyncTx, msg: str) -> TxHash: into the active transaction slot if broadcast is successful. """ signer: LocalAccount = self.__get_signer(tx.params["from"]) - txhash = self.w3.eth.send_raw_transaction( - signer.sign_transaction(tx.params).rawTransaction - ) + try: + txhash = self.w3.eth.send_raw_transaction( + signer.sign_transaction(tx.params).rawTransaction + ) + except ValidationError as e: + # special case for insufficient funds + if "Sender does not have enough" in str(e): + # TODO raised exception should be handled in some way #13. + raise InsufficientFunds + + raise e + self.log.info( f"[{msg}] fired transaction #atx-{tx.id}|{tx.params['nonce']}|{txhash.hex()}" ) @@ -338,7 +358,29 @@ def __strategize(self) -> None: _names = " -> ".join(s.name for s in self._strategies) # TODO try-except needed here (similar to broadcast) #14, #18, #20 - txhash = self.__fire(tx=_active_copy, msg=_names) + try: + txhash = self.__fire(tx=_active_copy, msg=_names) + except (TooManyRequests, ProviderConnectionError, TimeExhausted) as e: + # recoverable + # TODO don't retry forever #12, #20 + log.warn( + f"[retry] transaction #atx-{_active_copy.id}|{_active_copy.params['nonce']} " + f"failed with updated params - {str(e)}; retry next round" + ) + return + except (ValidationError, Web3Exception, ValueError) as e: + # non-recoverable + log.error( + f"[retry] transaction #atx-{_active_copy.id}|{_active_copy.params['nonce']} " + f"faulted with critical error - {str(e)}; tx will not be retried" + ) + fault_error = TransactionFaulted( + tx=_active_copy, + fault=Fault.ERROR, + message=str(e), + ) + self._tx_tracker.fault(fault_error=fault_error) + return _active_copy.txhash = txhash self._tx_tracker.update_after_retry(_active_copy) @@ -368,11 +410,25 @@ def __broadcast(self): try: txhash = self.__fire(tx=future_tx, msg="broadcast") - except ValueError as e: - _handle_rpc_error(e, tx=future_tx) + except (TooManyRequests, ProviderConnectionError, TimeExhausted) as e: + # recoverable - try again another time # TODO don't requeue forever #12, #20 + log.warn( + f"[broadcast] transaction #atx-{future_tx.id}|{future_tx.params['nonce']} " + f"failed - {str(e)}; requeueing tx" + ) self._tx_tracker._requeue(future_tx) return + except (ValidationError, Web3Exception, ValueError) as e: + # non-recoverable + log.error( + f"[broadcast] transaction #atx-{future_tx.id}|{future_tx.params['nonce']} " + f"has non-recoverable failure - {str(e)}; tx will not be requeued" + ) + hook = future_tx.on_broadcast_failure + if hook: + fire_hook(hook=hook, tx=future_tx, error=e) + return self._tx_tracker.morph(tx=future_tx, txhash=txhash) pending_tx = self._tx_tracker.pending diff --git a/atxm/tracker.py b/atxm/tracker.py index 93496d6..9a989ac 100644 --- a/atxm/tracker.py +++ b/atxm/tracker.py @@ -208,6 +208,7 @@ def _queue( params: TxParams, info: Dict[str, str] = None, on_broadcast: Optional[Callable[[PendingTx], None]] = None, + on_broadcast_failure: Optional[Callable[[FutureTx, Exception], None]] = None, on_finalized: Optional[Callable[[FinalizedTx], None]] = None, on_fault: Optional[Callable[[FaultedTx], None]] = None, ) -> FutureTx: @@ -220,6 +221,7 @@ def _queue( # configure hooks tx.on_broadcast = on_broadcast + tx.on_broadcast_failure = on_broadcast_failure tx.on_finalized = on_finalized tx.on_fault = on_fault diff --git a/atxm/tx.py b/atxm/tx.py index 573d2f5..0aa1779 100644 --- a/atxm/tx.py +++ b/atxm/tx.py @@ -5,7 +5,7 @@ from eth_typing import ChecksumAddress from eth_utils import encode_hex from hexbytes import HexBytes -from web3.types import PendingTx, TxData, TxParams, TxReceipt +from web3.types import TxData, TxParams, TxReceipt from atxm.exceptions import Fault @@ -18,7 +18,10 @@ class AsyncTx(ABC): params: TxParams final: bool = field(default=None, init=False) fault: Optional[Fault] = field(default=None, init=False) - on_broadcast: Optional[Callable[[PendingTx], None]] = field( + on_broadcast: Optional[Callable[["PendingTx"], None]] = field( + default=None, init=False + ) + on_broadcast_failure: Optional[Callable[["FutureTx", Exception], None]] = field( default=None, init=False ) on_finalized: Optional[Callable[["FinalizedTx"], None]] = field( diff --git a/atxm/utils.py b/atxm/utils.py index 825e459..8ba1540 100644 --- a/atxm/utils.py +++ b/atxm/utils.py @@ -6,10 +6,9 @@ from web3 import Web3 from web3.exceptions import TransactionNotFound from web3.types import TxData, TxParams -from web3.types import RPCError, TxReceipt, Wei +from web3.types import TxReceipt, Wei from atxm.exceptions import ( - InsufficientFunds, TransactionReverted, ) from atxm.logging import log @@ -116,25 +115,6 @@ def _hook() -> None: log.info(f"[hook] fired hook {hook} for transaction #atx-{tx.id}") -def _handle_rpc_error(e: Exception, tx: AsyncTx) -> None: - try: - error = RPCError(**e.args[0]) - except TypeError: - log.critical( - f"[error] transaction #atx-{tx.id}|{tx.params['nonce']} failed with {e}" - ) - else: - log.critical( - f"[error] transaction #atx-{tx.id}|{tx.params['nonce']} failed with {error['code']} | {error['message']}" - ) - if error["code"] == -32000: - if "insufficient funds" in error["message"]: - raise InsufficientFunds - hook = tx.on_fault - if hook: - fire_hook(hook=hook, tx=tx, error=e) - - def _make_tx_params(data: TxData) -> TxParams: """ TxData -> TxParams: Creates a transaction parameters From 9ea695c65a7f0b4f70e3dfa1e710d344448efc86 Mon Sep 17 00:00:00 2001 From: derekpierre Date: Wed, 6 Mar 2024 16:03:37 -0500 Subject: [PATCH 34/70] Commonize check for recoverable type of error after calling sendRawTransaction. Insufficient funds is a special case. --- atxm/machine.py | 92 ++++++++++++++++++++++++++----------------------- atxm/utils.py | 11 +++++- 2 files changed, 58 insertions(+), 45 deletions(-) diff --git a/atxm/machine.py b/atxm/machine.py index 19f4fff..64e2a09 100644 --- a/atxm/machine.py +++ b/atxm/machine.py @@ -8,12 +8,7 @@ from twisted.internet.defer import Deferred from twisted.internet.task import LoopingCall from web3 import Web3 -from web3.exceptions import ( - ProviderConnectionError, - TimeExhausted, - TooManyRequests, - Web3Exception, -) +from web3.exceptions import Web3Exception from web3.types import TxParams from atxm.exceptions import ( @@ -38,6 +33,7 @@ _get_average_blocktime, _get_confirmations, _get_receipt, + _is_recoverable_send_tx_error, _make_tx_params, fire_hook, ) @@ -350,7 +346,8 @@ def __strategize(self) -> None: if not params_updated: log.info( - f"[wait] strategies made no suggested updates to pending tx #{_active_copy.id} - skipping retry round" + f"[wait] strategies made no suggested updates to " + f"pending tx #{_active_copy.id} - skipping retry round" ) return @@ -360,26 +357,29 @@ def __strategize(self) -> None: # TODO try-except needed here (similar to broadcast) #14, #18, #20 try: txhash = self.__fire(tx=_active_copy, msg=_names) - except (TooManyRequests, ProviderConnectionError, TimeExhausted) as e: - # recoverable - # TODO don't retry forever #12, #20 - log.warn( - f"[retry] transaction #atx-{_active_copy.id}|{_active_copy.params['nonce']} " - f"failed with updated params - {str(e)}; retry next round" - ) - return + except InsufficientFunds: + # special case re-raise insufficient funds (for now) + # TODO #13 + raise except (ValidationError, Web3Exception, ValueError) as e: - # non-recoverable - log.error( - f"[retry] transaction #atx-{_active_copy.id}|{_active_copy.params['nonce']} " - f"faulted with critical error - {str(e)}; tx will not be retried" - ) - fault_error = TransactionFaulted( - tx=_active_copy, - fault=Fault.ERROR, - message=str(e), - ) - self._tx_tracker.fault(fault_error=fault_error) + if _is_recoverable_send_tx_error(e): + # TODO don't retry forever #12, #20 + log.warn( + f"[retry] transaction #atx-{_active_copy.id}|{_active_copy.params['nonce']} " + f"failed with updated params - {str(e)}; retry next round" + ) + else: + # non-recoverable + log.error( + f"[retry] transaction #atx-{_active_copy.id}|{_active_copy.params['nonce']} " + f"faulted non-recoverable failure - {str(e)}; tx will not be retried and removed" + ) + fault_error = TransactionFaulted( + tx=_active_copy, + fault=Fault.ERROR, + message=str(e), + ) + self._tx_tracker.fault(fault_error=fault_error) return _active_copy.txhash = txhash @@ -404,30 +404,34 @@ def __broadcast(self): if nonce > future_tx.params["nonce"]: self.log.warn( f"[broadcast] nonce {future_tx.params['nonce']} has been front-run " - f"by another transaction. Updating queued tx nonce {future_tx.params['nonce']} -> {nonce}" + f"by another transaction. Updating queued tx " + f"nonce {future_tx.params['nonce']} -> {nonce}" ) future_tx.params["nonce"] = nonce try: txhash = self.__fire(tx=future_tx, msg="broadcast") - except (TooManyRequests, ProviderConnectionError, TimeExhausted) as e: - # recoverable - try again another time - # TODO don't requeue forever #12, #20 - log.warn( - f"[broadcast] transaction #atx-{future_tx.id}|{future_tx.params['nonce']} " - f"failed - {str(e)}; requeueing tx" - ) - self._tx_tracker._requeue(future_tx) - return + except InsufficientFunds: + # special case re-raise insufficient funds (for now) + # TODO #13 + raise except (ValidationError, Web3Exception, ValueError) as e: - # non-recoverable - log.error( - f"[broadcast] transaction #atx-{future_tx.id}|{future_tx.params['nonce']} " - f"has non-recoverable failure - {str(e)}; tx will not be requeued" - ) - hook = future_tx.on_broadcast_failure - if hook: - fire_hook(hook=hook, tx=future_tx, error=e) + if _is_recoverable_send_tx_error(e): + # TODO don't requeue forever #12, #20 + log.warn( + f"[broadcast] transaction #atx-{future_tx.id}|{future_tx.params['nonce']} " + f"failed - {str(e)}; requeueing tx" + ) + self._tx_tracker._requeue(future_tx) + else: + # non-recoverable + log.error( + f"[broadcast] transaction #atx-{future_tx.id}|{future_tx.params['nonce']} " + f"has non-recoverable failure - {str(e)}; tx will not be requeued and removed" + ) + hook = future_tx.on_broadcast_failure + if hook: + fire_hook(hook=hook, tx=future_tx, error=e) return self._tx_tracker.morph(tx=future_tx, txhash=txhash) diff --git a/atxm/utils.py b/atxm/utils.py index 8ba1540..82907a8 100644 --- a/atxm/utils.py +++ b/atxm/utils.py @@ -4,7 +4,12 @@ from cytoolz import memoize from twisted.internet import reactor from web3 import Web3 -from web3.exceptions import TransactionNotFound +from web3.exceptions import ( + ProviderConnectionError, + TimeExhausted, + TooManyRequests, + TransactionNotFound, +) from web3.types import TxData, TxParams from web3.types import TxReceipt, Wei @@ -148,3 +153,7 @@ def _make_tx_params(data: TxData) -> TxParams: raise ValueError(f"unrecognized tx data: {data}") return params + + +def _is_recoverable_send_tx_error(e: Exception) -> bool: + return isinstance(e, (TooManyRequests, ProviderConnectionError, TimeExhausted)) From f42264d8efa76680ee7f1d6e85eed15cd47b9339 Mon Sep 17 00:00:00 2001 From: derekpierre Date: Wed, 6 Mar 2024 16:07:54 -0500 Subject: [PATCH 35/70] Insufficient funds is really a special case and its own exception, not a subclass. It is also not needed as a extra strategy (at least for now), since it is encountered during firing to txs. It may/may not be considered a fault so left the Fault.INSUFFICIENT_FUNDS enum for now. More to be explored in a future issue/PR. --- atxm/exceptions.py | 7 ++----- atxm/machine.py | 2 -- atxm/strategies.py | 20 -------------------- 3 files changed, 2 insertions(+), 27 deletions(-) diff --git a/atxm/exceptions.py b/atxm/exceptions.py index 9786d59..05dfa17 100644 --- a/atxm/exceptions.py +++ b/atxm/exceptions.py @@ -1,6 +1,6 @@ from enum import Enum -from web3.types import PendingTx, RPCError, TxReceipt +from web3.types import PendingTx, TxReceipt class Fault(Enum): @@ -13,9 +13,6 @@ class Fault(Enum): # Strategy has been running for too long TIMEOUT = "timeout" - # Transaction has been capped and subsequently timed out - PAUSE = "pause" - # Transaction reverted REVERT = "revert" @@ -26,7 +23,7 @@ class Fault(Enum): INSUFFICIENT_FUNDS = "insufficient_funds" -class InsufficientFunds(RPCError): +class InsufficientFunds(Exception): """raised when a transaction exceeds the spending cap""" diff --git a/atxm/machine.py b/atxm/machine.py index 64e2a09..820f44f 100644 --- a/atxm/machine.py +++ b/atxm/machine.py @@ -20,7 +20,6 @@ from atxm.strategies import ( AsyncTxStrategy, ExponentialSpeedupStrategy, - InsufficientFundsPause, TimeoutStrategy, ) from atxm.tracker import _TxTracker @@ -97,7 +96,6 @@ class _Machine(StateMachine): _BLOCK_SAMPLE_SIZE = 10_000 # blocks STRATEGIES: List[Type[AsyncTxStrategy]] = [ - InsufficientFundsPause, TimeoutStrategy, ExponentialSpeedupStrategy, ] diff --git a/atxm/strategies.py b/atxm/strategies.py index 1bf7089..9ac5b33 100644 --- a/atxm/strategies.py +++ b/atxm/strategies.py @@ -64,26 +64,6 @@ def execute(self, pending: PendingTx) -> Optional[TxParams]: raise NotImplementedError -class InsufficientFundsPause(AsyncTxStrategy): - """Pause strategy for pending transactions.""" - - _NAME = "insufficient-funds" - - def execute(self, pending: PendingTx) -> Optional[TxParams]: - balance = self.w3.eth.get_balance(pending.params["from"]) - if balance == 0: - self.log.warn( - f"Insufficient funds for transaction #{pending.params['nonce']}" - ) - raise TransactionFaulted( - tx=pending, - fault=Fault.INSUFFICIENT_FUNDS, - message="Insufficient funds", - ) - # log.warn(f"Insufficient funds for transaction #{pending.params['nonce']}") - return None - - class TimeoutStrategy(AsyncTxStrategy): """Timeout strategy for pending transactions.""" From 9013c0dc7d915fd9cab910e47c275c182f5e1874 Mon Sep 17 00:00:00 2001 From: derekpierre Date: Wed, 6 Mar 2024 16:33:42 -0500 Subject: [PATCH 36/70] Fix firing of hook with only ordered args. --- atxm/machine.py | 2 +- atxm/utils.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/atxm/machine.py b/atxm/machine.py index 820f44f..a2292cf 100644 --- a/atxm/machine.py +++ b/atxm/machine.py @@ -429,7 +429,7 @@ def __broadcast(self): ) hook = future_tx.on_broadcast_failure if hook: - fire_hook(hook=hook, tx=future_tx, error=e) + fire_hook(hook, future_tx, e) return self._tx_tracker.morph(tx=future_tx, txhash=txhash) diff --git a/atxm/utils.py b/atxm/utils.py index 82907a8..1159d12 100644 --- a/atxm/utils.py +++ b/atxm/utils.py @@ -102,7 +102,7 @@ def _get_confirmations(w3: Web3, tx: Union[PendingTx, FinalizedTx]) -> int: return confirmations -def fire_hook(hook: Callable, tx: AsyncTx, *args, **kwargs) -> None: +def fire_hook(hook: Callable, tx: AsyncTx, *args) -> None: """ Fire a callable in a separate thread. Try exceptionally hard not to crash the async tasks during dispatch. @@ -112,7 +112,7 @@ def fire_hook(hook: Callable, tx: AsyncTx, *args, **kwargs) -> None: def _hook() -> None: """I'm inside a thread!""" try: - hook(tx, *args, **kwargs) + hook(tx, *args) except Exception as e: log.warn(f"[hook] raised {e}") From f5ecc5a8340a2ce1ad6054b36dcc52aefdf012e3 Mon Sep 17 00:00:00 2001 From: derekpierre Date: Wed, 6 Mar 2024 16:34:32 -0500 Subject: [PATCH 37/70] Add test for unrecoverable errors encoutered when trying to broadcast a tx. --- tests/test_machine.py | 68 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 68 insertions(+) diff --git a/tests/test_machine.py b/tests/test_machine.py index 1254107..6c62e3a 100644 --- a/tests/test_machine.py +++ b/tests/test_machine.py @@ -4,11 +4,14 @@ import pytest_twisted from eth_account import Account +from eth_utils import ValidationError from twisted.internet import reactor from twisted.internet.task import deferLater +from web3.exceptions import Web3Exception from atxm.strategies import AsyncTxStrategy, TimeoutStrategy from atxm.tx import FutureTx, PendingTx +from atxm.utils import _is_recoverable_send_tx_error @pytest.fixture() @@ -302,6 +305,71 @@ def test_broadcast( machine.stop() +@pytest_twisted.inlineCallbacks +@pytest.mark.usefixtures("disable_auto_mining") +@pytest.mark.parametrize( + "non_recoverable_error", [ValidationError, ValueError, Web3Exception] +) +def test_broadcast_non_recoverable_error( + non_recoverable_error, + clock, + w3, + machine, + state_observer, + eip1559_transaction, + account, + mocker, + mock_wake_sleep, +): + wake, _ = mock_wake_sleep + + assert machine.current_state == machine._IDLE + assert not machine.busy + + # Queue a transaction + broadcast_failure_hook = mocker.Mock() + atx = machine.queue_transaction( + params=eip1559_transaction, + signer=account, + on_broadcast_failure=broadcast_failure_hook, + info={"message": "something wonderful is happening..."}, + ) + + assert wake.call_count == 1 + + # There is one queued transaction + assert len(machine.queued) == 1 + + # make firing of transaction fail + error = non_recoverable_error("non-recoverable error") + assert not _is_recoverable_send_tx_error(error) + mocker.patch.object(w3.eth, "send_raw_transaction", side_effect=error) + + machine.start(now=True) + yield clock.advance(1) + + # wait for the hook to be called + yield deferLater(reactor, 0.2, lambda: None) + assert broadcast_failure_hook.call_count == 1 + broadcast_failure_hook.assert_called_with(atx, error) + + # The transaction failed and is not requeued + assert len(machine.queued) == 0 + + # run a few cycles + for i in range(2): + yield clock.advance(1) + + # tx failed and not requeued + assert machine.current_state == machine._IDLE + + assert len(state_observer.transitions) == 2 + assert state_observer.transitions[0] == (machine._IDLE, machine._BUSY) + assert state_observer.transitions[1] == (machine._BUSY, machine._IDLE) + + machine.stop() + + @pytest_twisted.inlineCallbacks @pytest.mark.usefixtures("disable_auto_mining") def test_finalize( From 4905e077f366df9d8e1ccbbc24fdecee2c0a219b Mon Sep 17 00:00:00 2001 From: derekpierre Date: Wed, 6 Mar 2024 19:44:46 -0500 Subject: [PATCH 38/70] Add test for recoverable errors encoutered when trying to broadcast a tx. --- tests/test_machine.py | 91 ++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 90 insertions(+), 1 deletion(-) diff --git a/tests/test_machine.py b/tests/test_machine.py index 6c62e3a..99f8b6c 100644 --- a/tests/test_machine.py +++ b/tests/test_machine.py @@ -7,7 +7,12 @@ from eth_utils import ValidationError from twisted.internet import reactor from twisted.internet.task import deferLater -from web3.exceptions import Web3Exception +from web3.exceptions import ( + ProviderConnectionError, + TimeExhausted, + TooManyRequests, + Web3Exception, +) from atxm.strategies import AsyncTxStrategy, TimeoutStrategy from atxm.tx import FutureTx, PendingTx @@ -328,9 +333,11 @@ def test_broadcast_non_recoverable_error( # Queue a transaction broadcast_failure_hook = mocker.Mock() + broadcast_hook = mocker.Mock() atx = machine.queue_transaction( params=eip1559_transaction, signer=account, + on_broadcast=broadcast_hook, on_broadcast_failure=broadcast_failure_hook, info={"message": "something wonderful is happening..."}, ) @@ -360,6 +367,8 @@ def test_broadcast_non_recoverable_error( for i in range(2): yield clock.advance(1) + assert broadcast_hook.call_count == 0 + # tx failed and not requeued assert machine.current_state == machine._IDLE @@ -370,6 +379,86 @@ def test_broadcast_non_recoverable_error( machine.stop() +@pytest_twisted.inlineCallbacks +@pytest.mark.usefixtures("disable_auto_mining") +@pytest.mark.parametrize( + "recoverable_error", [TooManyRequests, ProviderConnectionError, TimeExhausted] +) +def test_broadcast_recoverable_error( + recoverable_error, + clock, + w3, + machine, + state_observer, + eip1559_transaction, + account, + mocker, + mock_wake_sleep, +): + wake, _ = mock_wake_sleep + + assert machine.current_state == machine._IDLE + assert not machine.busy + + # Queue a transaction + broadcast_failure_hook = mocker.Mock() + broadcast_hook = mocker.Mock() + atx = machine.queue_transaction( + params=eip1559_transaction, + signer=account, + on_broadcast=broadcast_hook, + on_broadcast_failure=broadcast_failure_hook, + info={"message": "something wonderful is happening..."}, + ) + + assert wake.call_count == 1 + + # There is one queued transaction + assert len(machine.queued) == 1 + + real_method = w3.eth.send_raw_transaction + # make firing of transaction fail but with recoverable error + error = recoverable_error("recoverable error") + assert _is_recoverable_send_tx_error(error) + mocker.patch.object(w3.eth, "send_raw_transaction", side_effect=error) + + # repeat some cycles; tx fails then gets requeued since error is "recoverable" + machine.start(now=True) + for i in range(5): + yield clock.advance(1) + assert len(machine.queued) == 1 # remains in queue and not broadcasted + + # call real method from now on + mocker.patch.object(w3.eth, "send_raw_transaction", side_effect=real_method) + while machine.pending is None: + yield clock.advance(1) + + assert machine.current_state == machine._BUSY + + # The transaction is broadcasted and no longer queued + assert len(machine.queued) == 0 + + assert machine.pending == atx + assert isinstance(machine.pending, PendingTx) + + assert isinstance(atx, PendingTx) + assert not atx.final + assert atx.txhash + + # wait for the hook to be called + yield deferLater(reactor, 0.2, lambda: None) + assert broadcast_hook.call_count == 1 + assert broadcast_failure_hook.call_count == 0 + + # tx only broadcasted and not finalized, so we are still busy + assert machine.current_state == machine._BUSY + + assert len(state_observer.transitions) == 1 + assert state_observer.transitions[0] == (machine._IDLE, machine._BUSY) + + machine.stop() + + @pytest_twisted.inlineCallbacks @pytest.mark.usefixtures("disable_auto_mining") def test_finalize( From d6950787f3b282b46b6b2f4ad3afe93004eeb3ae Mon Sep 17 00:00:00 2001 From: derekpierre Date: Wed, 6 Mar 2024 20:23:00 -0500 Subject: [PATCH 39/70] Add test for unrecoverable errors encoutered when trying to retry a tx. --- tests/test_machine.py | 90 +++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 86 insertions(+), 4 deletions(-) diff --git a/tests/test_machine.py b/tests/test_machine.py index 99f8b6c..71fcc06 100644 --- a/tests/test_machine.py +++ b/tests/test_machine.py @@ -14,6 +14,7 @@ Web3Exception, ) +from atxm.exceptions import Fault from atxm.strategies import AsyncTxStrategy, TimeoutStrategy from atxm.tx import FutureTx, PendingTx from atxm.utils import _is_recoverable_send_tx_error @@ -774,10 +775,8 @@ def test_use_strategies_that_dont_make_updates( # params remained unchanged since strategies don't make updates assert atx.params == original_params, "params remain unchanged" - s1_call_count = strategy_1.execute.call_count - assert s1_call_count > 0, "strategy #1 was called" - s2_call_count = strategy_2.execute.call_count - assert s2_call_count > 0, "strategy #2 was called" + assert strategy_1.execute.call_count > 0, "strategy #1 was called" + assert strategy_2.execute.call_count > 0, "strategy #2 was called" assert atx.params == original_params, "params remain unchanged" assert update_spy.call_count == 0, "update never called because no retry" @@ -804,6 +803,89 @@ def test_use_strategies_that_dont_make_updates( machine.stop() +@pytest_twisted.inlineCallbacks +@pytest.mark.usefixtures("disable_auto_mining") +@pytest.mark.parametrize( + "non_recoverable_error", [ValidationError, ValueError, Web3Exception] +) +def test_retry_non_recoverable( + non_recoverable_error, + ethereum_tester, + w3, + machine, + state_observer, + clock, + eip1559_transaction, + account, + mocker, + mock_wake_sleep, +): + # TODO consider whether this should just be provided to constructor - #23 + machine._strategies.clear() + + # strategies that don't make updates + strategy_1 = mocker.Mock(spec=AsyncTxStrategy) + strategy_1.name = "mock_strategy" + # return non-None so retry is attempted + strategy_1.execute.return_value = dict(eip1559_transaction) + + machine._strategies = [strategy_1] + + update_spy = mocker.spy(machine._tx_tracker, "update_after_retry") + + machine.start() + assert machine.current_state == machine._IDLE + + fault_hook = mocker.Mock() + atx = machine.queue_transaction( + params=eip1559_transaction, signer=account, on_fault=fault_hook + ) + + # advance to broadcast the transaction + while machine.pending is None: + yield clock.advance(1) + + # ensure that hook is called + assert machine.current_state == machine._BUSY + + # make firing of transaction fail with non-recoverable error + error = non_recoverable_error("non-recoverable error") + assert not _is_recoverable_send_tx_error(error) + mocker.patch.object(w3.eth, "send_raw_transaction", side_effect=error) + + # run a cycle while tx unmined for strategies to kick in + yield clock.advance(1) + + assert strategy_1.execute.call_count > 0, "strategy #1 was called" + + # however retry failed, so params shouldn't have been updated + assert update_spy.call_count == 0, "update never called because no retry" + + # ensure that hook is called + yield deferLater(reactor, 0.2, lambda: None) + assert fault_hook.call_count == 1 + fault_hook.assert_called_with(atx) + assert atx.fault == Fault.ERROR + assert atx.error == "non-recoverable error" + + # ensure switch back to IDLE + yield clock.advance(1) + + assert len(machine.queued) == 0 + assert machine.pending is None + + assert not machine.busy + assert not atx.final + + assert machine.current_state == machine._IDLE + + assert len(state_observer.transitions) == 2 + assert state_observer.transitions[0] == (machine._IDLE, machine._BUSY) + assert state_observer.transitions[1] == (machine._BUSY, machine._IDLE) + + machine.stop() + + @pytest_twisted.inlineCallbacks def test_pause_when_idle(clock, machine, mocker): machine.start() From e000b64de9d6ea34978a70ac45edf690dda77e63 Mon Sep 17 00:00:00 2001 From: derekpierre Date: Thu, 7 Mar 2024 08:44:53 -0500 Subject: [PATCH 40/70] Retry failures should be treated differently than broadcast failures. Basically retries only happen after a broadcast has been successful, so we know that the base parameters were correct/fine. If we retry a tx with updated params and a failure occurs, the original broadcasted tx was still broadcasted so don't fault immediately; instead skip the round. However, at some point we have to decide whether or not to keep waiting for the original broadcasted tx. It is somewehat analogous to how many requeues we allow for a failed broadcast of a tx. Both can be explored as part of #12, #14. --- atxm/machine.py | 25 ++++-------------- tests/test_machine.py | 60 +++++++++++++++++++++++++++++++------------ 2 files changed, 48 insertions(+), 37 deletions(-) diff --git a/atxm/machine.py b/atxm/machine.py index a2292cf..8b8d4a4 100644 --- a/atxm/machine.py +++ b/atxm/machine.py @@ -12,7 +12,6 @@ from web3.types import TxParams from atxm.exceptions import ( - Fault, InsufficientFunds, TransactionFaulted, TransactionReverted, @@ -352,7 +351,6 @@ def __strategize(self) -> None: # (!) retry the transaction with the new parameters _names = " -> ".join(s.name for s in self._strategies) - # TODO try-except needed here (similar to broadcast) #14, #18, #20 try: txhash = self.__fire(tx=_active_copy, msg=_names) except InsufficientFunds: @@ -360,24 +358,11 @@ def __strategize(self) -> None: # TODO #13 raise except (ValidationError, Web3Exception, ValueError) as e: - if _is_recoverable_send_tx_error(e): - # TODO don't retry forever #12, #20 - log.warn( - f"[retry] transaction #atx-{_active_copy.id}|{_active_copy.params['nonce']} " - f"failed with updated params - {str(e)}; retry next round" - ) - else: - # non-recoverable - log.error( - f"[retry] transaction #atx-{_active_copy.id}|{_active_copy.params['nonce']} " - f"faulted non-recoverable failure - {str(e)}; tx will not be retried and removed" - ) - fault_error = TransactionFaulted( - tx=_active_copy, - fault=Fault.ERROR, - message=str(e), - ) - self._tx_tracker.fault(fault_error=fault_error) + # TODO don't retry forever #12, #14 + log.warn( + f"[retry] transaction #atx-{_active_copy.id}|{_active_copy.params['nonce']} " + f"failed with updated params - {str(e)}; retry again next round" + ) return _active_copy.txhash = txhash diff --git a/tests/test_machine.py b/tests/test_machine.py index 71fcc06..d25d127 100644 --- a/tests/test_machine.py +++ b/tests/test_machine.py @@ -14,7 +14,6 @@ Web3Exception, ) -from atxm.exceptions import Fault from atxm.strategies import AsyncTxStrategy, TimeoutStrategy from atxm.tx import FutureTx, PendingTx from atxm.utils import _is_recoverable_send_tx_error @@ -806,10 +805,18 @@ def test_use_strategies_that_dont_make_updates( @pytest_twisted.inlineCallbacks @pytest.mark.usefixtures("disable_auto_mining") @pytest.mark.parametrize( - "non_recoverable_error", [ValidationError, ValueError, Web3Exception] + "retry_error", + [ + ValidationError, + ValueError, + Web3Exception, + TooManyRequests, + ProviderConnectionError, + TimeExhausted, + ], ) -def test_retry_non_recoverable( - non_recoverable_error, +def test_retry_with_error( + retry_error, ethereum_tester, w3, machine, @@ -836,37 +843,56 @@ def test_retry_non_recoverable( machine.start() assert machine.current_state == machine._IDLE + broadcast_hook = mocker.Mock() fault_hook = mocker.Mock() atx = machine.queue_transaction( - params=eip1559_transaction, signer=account, on_fault=fault_hook + params=eip1559_transaction, + signer=account, + on_fault=fault_hook, + on_broadcast=broadcast_hook, ) # advance to broadcast the transaction while machine.pending is None: yield clock.advance(1) + # ensure that hook is not called + yield deferLater(reactor, 0.2, lambda: None) + assert broadcast_hook.call_count == 1 + # ensure that hook is called assert machine.current_state == machine._BUSY - # make firing of transaction fail with non-recoverable error - error = non_recoverable_error("non-recoverable error") - assert not _is_recoverable_send_tx_error(error) + real_method = w3.eth.send_raw_transaction + + # make firing of retry transaction fail with non-recoverable error + error = retry_error("retry error") mocker.patch.object(w3.eth, "send_raw_transaction", side_effect=error) # run a cycle while tx unmined for strategies to kick in - yield clock.advance(1) + for i in range(5): + yield clock.advance(1) + assert ( + machine.pending + ), "tx is being retried but encounters retry error and remains pending" assert strategy_1.execute.call_count > 0, "strategy #1 was called" - - # however retry failed, so params shouldn't have been updated + # retry failed, so params shouldn't have been updated assert update_spy.call_count == 0, "update never called because no retry" - # ensure that hook is called + # call real method from now on + mocker.patch.object(w3.eth, "send_raw_transaction", side_effect=real_method) + + while not machine.finalized: + yield ethereum_tester.mine_block() + yield clock.advance(1) + + yield clock.advance(1) + assert atx.final is True + + # ensure that hook is not called yield deferLater(reactor, 0.2, lambda: None) - assert fault_hook.call_count == 1 - fault_hook.assert_called_with(atx) - assert atx.fault == Fault.ERROR - assert atx.error == "non-recoverable error" + assert fault_hook.call_count == 0 # ensure switch back to IDLE yield clock.advance(1) @@ -875,7 +901,7 @@ def test_retry_non_recoverable( assert machine.pending is None assert not machine.busy - assert not atx.final + assert atx.final assert machine.current_state == machine._IDLE From b744a93d5cb3bf617494b6015ab77a3ca873dedc Mon Sep 17 00:00:00 2001 From: derekpierre Date: Thu, 7 Mar 2024 09:28:19 -0500 Subject: [PATCH 41/70] Make methods on TxTracker public instead of protected. --- atxm/machine.py | 6 +++--- atxm/tracker.py | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/atxm/machine.py b/atxm/machine.py index 8b8d4a4..f009abc 100644 --- a/atxm/machine.py +++ b/atxm/machine.py @@ -378,7 +378,7 @@ def __broadcast(self): Attempts to broadcast the next `FutureTx` in the queue. If the broadcast is not successful, it is re-queued. """ - future_tx = self._tx_tracker._pop() # popleft + future_tx = self._tx_tracker.pop() # popleft future_tx.params = _make_tx_params(future_tx.params) # update nonce as necessary @@ -405,7 +405,7 @@ def __broadcast(self): f"[broadcast] transaction #atx-{future_tx.id}|{future_tx.params['nonce']} " f"failed - {str(e)}; requeueing tx" ) - self._tx_tracker._requeue(future_tx) + self._tx_tracker.requeue(future_tx) else: # non-recoverable log.error( @@ -484,7 +484,7 @@ def queue_transaction( f"Mismatched 'from' value ({from_param}) and 'signer' account ({signer.address})" ) - tx = self._tx_tracker._queue(params=params_copy, *args, **kwargs) + tx = self._tx_tracker.queue_tx(params=params_copy, *args, **kwargs) if not previously_busy_or_paused: self._wake() diff --git a/atxm/tracker.py b/atxm/tracker.py index 9a989ac..575ace9 100644 --- a/atxm/tracker.py +++ b/atxm/tracker.py @@ -190,11 +190,11 @@ def queue(self) -> Tuple[FutureTx, ...]: """Return the queue of transactions.""" return tuple(self.__queue) - def _pop(self) -> FutureTx: + def pop(self) -> FutureTx: """Pop the next transaction from the queue.""" return self.__queue.popleft() - def _requeue(self, tx: FutureTx) -> None: + def requeue(self, tx: FutureTx) -> None: """Re-queue a transaction for broadcast and subsequent tracking.""" self.__queue.append(tx) self.commit() @@ -203,7 +203,7 @@ def _requeue(self, tx: FutureTx) -> None: f"priority {len(self.__queue)}" ) - def _queue( + def queue_tx( self, params: TxParams, info: Dict[str, str] = None, From 431462ebe67bdbcf20e10efffc5dc7740203d6db Mon Sep 17 00:00:00 2001 From: derekpierre Date: Thu, 7 Mar 2024 09:49:03 -0500 Subject: [PATCH 42/70] Use a counter for tracking number of requeue attempts for failed txs during broadcast. --- atxm/machine.py | 59 +++++++++++++++++++++++++++++++++++-------------- 1 file changed, 42 insertions(+), 17 deletions(-) diff --git a/atxm/machine.py b/atxm/machine.py index f009abc..1a7346c 100644 --- a/atxm/machine.py +++ b/atxm/machine.py @@ -1,5 +1,5 @@ from copy import copy, deepcopy -from typing import List, Optional, Type +from typing import Dict, List, Optional, Type from eth_account.signers.local import LocalAccount from eth_utils import ValidationError @@ -99,6 +99,8 @@ class _Machine(StateMachine): ExponentialSpeedupStrategy, ] + _REDO_ATTEMPTS = 3 + class LogObserver: """StateMachine observer for logging information about state/transitions.""" @@ -134,6 +136,9 @@ def __init__( self._task.clock = self.__CLOCK self._task.interval = self._IDLE_INTERVAL + # requeues + self._requeue_counter: Dict[int, int] = dict() + super().__init__() self.add_observer(_Machine.LogObserver()) @@ -399,29 +404,49 @@ def __broadcast(self): # TODO #13 raise except (ValidationError, Web3Exception, ValueError) as e: - if _is_recoverable_send_tx_error(e): - # TODO don't requeue forever #12, #20 - log.warn( - f"[broadcast] transaction #atx-{future_tx.id}|{future_tx.params['nonce']} " - f"failed - {str(e)}; requeueing tx" - ) - self._tx_tracker.requeue(future_tx) - else: - # non-recoverable - log.error( - f"[broadcast] transaction #atx-{future_tx.id}|{future_tx.params['nonce']} " - f"has non-recoverable failure - {str(e)}; tx will not be requeued and removed" - ) - hook = future_tx.on_broadcast_failure - if hook: - fire_hook(hook, future_tx, e) + # either requeue OR fail and move on to subsequent txs + self.__handle_broadcast_failure(future_tx, e) return + # clear requeue counter if necessary since successful + self._requeue_counter.pop(future_tx.id, None) + self._tx_tracker.morph(tx=future_tx, txhash=txhash) pending_tx = self._tx_tracker.pending if pending_tx.on_broadcast: fire_hook(hook=pending_tx.on_broadcast, tx=pending_tx) + def __handle_broadcast_failure(self, future_tx: FutureTx, e: Exception): + is_broadcast_failure = False + if _is_recoverable_send_tx_error(e): + num_requeues = self._requeue_counter.get(future_tx.id, 0) + if num_requeues >= self._REDO_ATTEMPTS: + is_broadcast_failure = True + log.error( + f"[broadcast] transaction #atx-{future_tx.id}|{future_tx.params['nonce']} " + f"failed for {num_requeues} attempts; tx will not be requeued" + ) + else: + log.warn( + f"[broadcast] transaction #atx-{future_tx.id}|{future_tx.params['nonce']} " + f"failed - {str(e)}; requeueing tx" + ) + self._tx_tracker.requeue(future_tx) + self._requeue_counter[future_tx.id] = num_requeues + 1 + else: + # non-recoverable + is_broadcast_failure = True + log.error( + f"[broadcast] transaction #atx-{future_tx.id}|{future_tx.params['nonce']} " + f"has non-recoverable failure - {str(e)}; tx will not be requeued" + ) + + if is_broadcast_failure: + self._requeue_counter.pop(future_tx.id, None) + hook = future_tx.on_broadcast_failure + if hook: + fire_hook(hook, future_tx, e) + # # Monitoring # From 290fd6598d192afba57fbac6ec2e34dd8f4265c8 Mon Sep 17 00:00:00 2001 From: derekpierre Date: Thu, 7 Mar 2024 10:46:31 -0500 Subject: [PATCH 43/70] Use a counter for tracking number of failed retry attempts when using strategies to retry a tx. --- atxm/machine.py | 46 +++++++++++++++++++++++++++++++++++++--------- 1 file changed, 37 insertions(+), 9 deletions(-) diff --git a/atxm/machine.py b/atxm/machine.py index 1a7346c..7233c12 100644 --- a/atxm/machine.py +++ b/atxm/machine.py @@ -12,6 +12,7 @@ from web3.types import TxParams from atxm.exceptions import ( + Fault, InsufficientFunds, TransactionFaulted, TransactionReverted, @@ -25,6 +26,7 @@ from atxm.tx import ( AsyncTx, FutureTx, + PendingTx, TxHash, ) from atxm.utils import ( @@ -99,7 +101,7 @@ class _Machine(StateMachine): ExponentialSpeedupStrategy, ] - _REDO_ATTEMPTS = 3 + _NUM_REDO_ATTEMPTS = 3 class LogObserver: """StateMachine observer for logging information about state/transitions.""" @@ -136,8 +138,9 @@ def __init__( self._task.clock = self.__CLOCK self._task.interval = self._IDLE_INTERVAL - # requeues + # requeues/retries self._requeue_counter: Dict[int, int] = dict() + self._retry_failure_counter: Dict[int, int] = dict() super().__init__() @@ -363,13 +366,12 @@ def __strategize(self) -> None: # TODO #13 raise except (ValidationError, Web3Exception, ValueError) as e: - # TODO don't retry forever #12, #14 - log.warn( - f"[retry] transaction #atx-{_active_copy.id}|{_active_copy.params['nonce']} " - f"failed with updated params - {str(e)}; retry again next round" - ) + self.__handle_retry_failure(_active_copy, e) return + # clear failed retries since successful + self._retry_failure_counter.pop(_active_copy.id, None) + _active_copy.txhash = txhash self._tx_tracker.update_after_retry(_active_copy) @@ -378,6 +380,31 @@ def __strategize(self) -> None: if pending_tx.on_broadcast: fire_hook(hook=pending_tx.on_broadcast, tx=pending_tx) + def __handle_retry_failure(self, pending_tx: PendingTx, e: Exception): + log.warn( + f"[retry] transaction #atx-{pending_tx.id}|{pending_tx.params['nonce']} " + f"failed with updated params - {str(e)}; retry again next round" + ) + num_failed_retries = self._retry_failure_counter.get(pending_tx.id, 0) + num_failed_retries += 1 + if num_failed_retries > self._NUM_REDO_ATTEMPTS: + log.error( + f"[retry] transaction #atx-{pending_tx.id}|{pending_tx.params['nonce']} " + f"failed for {num_failed_retries} attempts; tx will no longer be retried" + ) + + # remove entry since no longer needed + self._retry_failure_counter.pop(pending_tx.id, None) + + fault_error = TransactionFaulted( + tx=pending_tx, + fault=Fault.ERROR, + message=str(e), + ) + self._tx_tracker.fault(fault_error=fault_error) + else: + self._retry_failure_counter[pending_tx.id] = num_failed_retries + def __broadcast(self): """ Attempts to broadcast the next `FutureTx` in the queue. @@ -408,7 +435,7 @@ def __broadcast(self): self.__handle_broadcast_failure(future_tx, e) return - # clear requeue counter if necessary since successful + # clear requeue counter since successful self._requeue_counter.pop(future_tx.id, None) self._tx_tracker.morph(tx=future_tx, txhash=txhash) @@ -420,7 +447,7 @@ def __handle_broadcast_failure(self, future_tx: FutureTx, e: Exception): is_broadcast_failure = False if _is_recoverable_send_tx_error(e): num_requeues = self._requeue_counter.get(future_tx.id, 0) - if num_requeues >= self._REDO_ATTEMPTS: + if num_requeues >= self._NUM_REDO_ATTEMPTS: is_broadcast_failure = True log.error( f"[broadcast] transaction #atx-{future_tx.id}|{future_tx.params['nonce']} " @@ -442,6 +469,7 @@ def __handle_broadcast_failure(self, future_tx: FutureTx, e: Exception): ) if is_broadcast_failure: + # remove entry since no longer needed self._requeue_counter.pop(future_tx.id, None) hook = future_tx.on_broadcast_failure if hook: From 5c42f80c7c66ad831a4f7e4ae2e55a2aa3476fbd Mon Sep 17 00:00:00 2001 From: derekpierre Date: Thu, 7 Mar 2024 10:52:42 -0500 Subject: [PATCH 44/70] Update existing tests given limits on requeue/retry (redo) attempts. --- tests/test_machine.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/tests/test_machine.py b/tests/test_machine.py index d25d127..1276713 100644 --- a/tests/test_machine.py +++ b/tests/test_machine.py @@ -395,6 +395,9 @@ def test_broadcast_recoverable_error( mocker, mock_wake_sleep, ): + # need more freedom with redo attempts for test + mocker.patch.object(machine, "_NUM_REDO_ATTEMPTS", 10) + wake, _ = mock_wake_sleep assert machine.current_state == machine._IDLE @@ -815,7 +818,7 @@ def test_use_strategies_that_dont_make_updates( TimeExhausted, ], ) -def test_retry_with_error( +def test_retry_with_errors_but_recovers( retry_error, ethereum_tester, w3, @@ -827,6 +830,9 @@ def test_retry_with_error( mocker, mock_wake_sleep, ): + # need more freedom with redo attempts for test + mocker.patch.object(machine, "_NUM_REDO_ATTEMPTS", 10) + # TODO consider whether this should just be provided to constructor - #23 machine._strategies.clear() From 189b2dd4049e08b124ce07ada42a44fb446ab470 Mon Sep 17 00:00:00 2001 From: derekpierre Date: Thu, 7 Mar 2024 10:53:08 -0500 Subject: [PATCH 45/70] Add test for exceeding retries when encountering a broadcast error. --- tests/test_machine.py | 79 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 79 insertions(+) diff --git a/tests/test_machine.py b/tests/test_machine.py index 1276713..2f6f91f 100644 --- a/tests/test_machine.py +++ b/tests/test_machine.py @@ -462,6 +462,85 @@ def test_broadcast_recoverable_error( machine.stop() +@pytest_twisted.inlineCallbacks +@pytest.mark.usefixtures("disable_auto_mining") +@pytest.mark.parametrize( + "recoverable_error", [TooManyRequests, ProviderConnectionError, TimeExhausted] +) +def test_broadcast_recoverable_error_requeues_exceeded( + recoverable_error, + clock, + w3, + machine, + state_observer, + eip1559_transaction, + account, + mocker, + mock_wake_sleep, +): + wake, _ = mock_wake_sleep + + assert machine.current_state == machine._IDLE + assert not machine.busy + + # Queue a transaction + broadcast_failure_hook = mocker.Mock() + broadcast_hook = mocker.Mock() + atx = machine.queue_transaction( + params=eip1559_transaction, + signer=account, + on_broadcast=broadcast_hook, + on_broadcast_failure=broadcast_failure_hook, + info={"message": "something wonderful is happening..."}, + ) + + assert wake.call_count == 1 + + # There is one queued transaction + assert len(machine.queued) == 1 + + # make firing of transaction fail but with recoverable error + error = recoverable_error("recoverable error") + assert _is_recoverable_send_tx_error(error) + mocker.patch.object(w3.eth, "send_raw_transaction", side_effect=error) + + # repeat some cycles; tx fails then gets requeued since error is "recoverable" + machine.start(now=True) + # one less than attempts + for i in range(machine._NUM_REDO_ATTEMPTS - 1): + assert len(machine.queued) == 1 # remains in queue and not broadcasted + yield clock.advance(1) + assert machine._requeue_counter.get(atx.id, 0) >= i + + # push over the retry limit + yield clock.advance(1) + + # wait for the hook to be called + yield deferLater(reactor, 0.2, lambda: None) + assert broadcast_failure_hook.call_count == 1 + broadcast_failure_hook.assert_called_with(atx, error) + + assert machine._requeue_counter.get(atx.id) is None # no longer tracked + + # The transaction failed and is not requeued + assert len(machine.queued) == 0 + + # run a few cycles + for i in range(2): + yield clock.advance(1) + + assert broadcast_hook.call_count == 0 + + # tx failed and not requeued + assert machine.current_state == machine._IDLE + + assert len(state_observer.transitions) == 2 + assert state_observer.transitions[0] == (machine._IDLE, machine._BUSY) + assert state_observer.transitions[1] == (machine._BUSY, machine._IDLE) + + machine.stop() + + @pytest_twisted.inlineCallbacks @pytest.mark.usefixtures("disable_auto_mining") def test_finalize( From f4670ac9e8594e6e2c1e752449279d646a0a6df2 Mon Sep 17 00:00:00 2001 From: derekpierre Date: Thu, 7 Mar 2024 11:03:37 -0500 Subject: [PATCH 46/70] Add additional assertions for the requeue counter in existing tests. --- tests/test_machine.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/test_machine.py b/tests/test_machine.py index 2f6f91f..a1de15c 100644 --- a/tests/test_machine.py +++ b/tests/test_machine.py @@ -301,6 +301,8 @@ def test_broadcast( yield deferLater(reactor, 0.2, lambda: None) assert hook.call_count == 1 + assert machine._requeue_counter.get(atx.id) is None # not tracked + # tx only broadcasted and not finalized, so we are still busy assert machine.current_state == machine._BUSY @@ -369,6 +371,8 @@ def test_broadcast_non_recoverable_error( assert broadcast_hook.call_count == 0 + assert machine._requeue_counter.get(atx.id) is None # not tracked + # tx failed and not requeued assert machine.current_state == machine._IDLE @@ -453,6 +457,8 @@ def test_broadcast_recoverable_error( assert broadcast_hook.call_count == 1 assert broadcast_failure_hook.call_count == 0 + assert machine._requeue_counter.get(atx.id) is None # no longer tracked + # tx only broadcasted and not finalized, so we are still busy assert machine.current_state == machine._BUSY From c370deac43b7ed115220a1a5fe341e9dae3fff10 Mon Sep 17 00:00:00 2001 From: derekpierre Date: Thu, 7 Mar 2024 11:22:57 -0500 Subject: [PATCH 47/70] Existing retry test cleanup. --- tests/test_machine.py | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/tests/test_machine.py b/tests/test_machine.py index a1de15c..b03d21d 100644 --- a/tests/test_machine.py +++ b/tests/test_machine.py @@ -512,7 +512,7 @@ def test_broadcast_recoverable_error_requeues_exceeded( # repeat some cycles; tx fails then gets requeued since error is "recoverable" machine.start(now=True) - # one less than attempts + # one less than max attempts for i in range(machine._NUM_REDO_ATTEMPTS - 1): assert len(machine.queued) == 1 # remains in queue and not broadcasted yield clock.advance(1) @@ -947,11 +947,10 @@ def test_retry_with_errors_but_recovers( while machine.pending is None: yield clock.advance(1) - # ensure that hook is not called + # ensure that hook is called yield deferLater(reactor, 0.2, lambda: None) assert broadcast_hook.call_count == 1 - # ensure that hook is called assert machine.current_state == machine._BUSY real_method = w3.eth.send_raw_transaction @@ -968,8 +967,8 @@ def test_retry_with_errors_but_recovers( ), "tx is being retried but encounters retry error and remains pending" assert strategy_1.execute.call_count > 0, "strategy #1 was called" - # retry failed, so params shouldn't have been updated - assert update_spy.call_count == 0, "update never called because no retry" + # retries failed, so params shouldn't have been updated + assert update_spy.call_count == 0, "update never called because each retry failed" # call real method from now on mocker.patch.object(w3.eth, "send_raw_transaction", side_effect=real_method) From f59aa7453513a344309c12ffb40d55776ebd3c63 Mon Sep 17 00:00:00 2001 From: derekpierre Date: Thu, 7 Mar 2024 11:45:42 -0500 Subject: [PATCH 48/70] Add test for exceeding retries when encountering retry errors. --- tests/test_machine.py | 109 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 108 insertions(+), 1 deletion(-) diff --git a/tests/test_machine.py b/tests/test_machine.py index b03d21d..47e0a85 100644 --- a/tests/test_machine.py +++ b/tests/test_machine.py @@ -15,7 +15,7 @@ ) from atxm.strategies import AsyncTxStrategy, TimeoutStrategy -from atxm.tx import FutureTx, PendingTx +from atxm.tx import FaultedTx, FinalizedTx, FutureTx, PendingTx from atxm.utils import _is_recoverable_send_tx_error @@ -989,9 +989,116 @@ def test_retry_with_errors_but_recovers( assert len(machine.queued) == 0 assert machine.pending is None + assert machine._retry_failure_counter.get(atx.id) is None # no longer tracked assert not machine.busy assert atx.final + assert isinstance(atx, FinalizedTx) + + assert machine.current_state == machine._IDLE + + assert len(state_observer.transitions) == 2 + assert state_observer.transitions[0] == (machine._IDLE, machine._BUSY) + assert state_observer.transitions[1] == (machine._BUSY, machine._IDLE) + + machine.stop() + + +@pytest_twisted.inlineCallbacks +@pytest.mark.usefixtures("disable_auto_mining") +@pytest.mark.parametrize( + "retry_error", + [ + ValidationError, + ValueError, + Web3Exception, + TooManyRequests, + ProviderConnectionError, + TimeExhausted, + ], +) +def test_retry_with_errors_retries_exceeded( + retry_error, + ethereum_tester, + w3, + machine, + state_observer, + clock, + eip1559_transaction, + account, + mocker, + mock_wake_sleep, +): + # TODO consider whether this should just be provided to constructor - #23 + machine._strategies.clear() + + # strategies that don't make updates + strategy_1 = mocker.Mock(spec=AsyncTxStrategy) + strategy_1.name = "mock_strategy" + # return non-None so retry is attempted + strategy_1.execute.return_value = dict(eip1559_transaction) + + machine._strategies = [strategy_1] + + update_spy = mocker.spy(machine._tx_tracker, "update_after_retry") + + machine.start() + assert machine.current_state == machine._IDLE + + broadcast_hook = mocker.Mock() + fault_hook = mocker.Mock() + atx = machine.queue_transaction( + params=eip1559_transaction, + signer=account, + on_fault=fault_hook, + on_broadcast=broadcast_hook, + ) + + # advance to broadcast the transaction + while machine.pending is None: + yield clock.advance(1) + + # ensure that hook is called + yield deferLater(reactor, 0.2, lambda: None) + assert broadcast_hook.call_count == 1 + + assert machine.current_state == machine._BUSY + + # make firing of retry transaction fail with non-recoverable error + error = retry_error("retry error") + mocker.patch.object(w3.eth, "send_raw_transaction", side_effect=error) + + # retry max attempts + for i in range(machine._NUM_REDO_ATTEMPTS): + assert machine.pending is not None + yield clock.advance(1) + assert machine._retry_failure_counter.get(atx.id, 0) >= i + + # push over retry limit + yield clock.advance(1) + + assert strategy_1.execute.call_count > 0, "strategy #1 was called" + # retries failed, so params shouldn't have been updated + assert update_spy.call_count == 0, "update never called because each retry failed" + + # wait for the hook to be called + yield deferLater(reactor, 0.2, lambda: None) + assert fault_hook.call_count == 1 + fault_hook.assert_called_with(atx) + + assert machine._retry_failure_counter.get(atx.id) is None # no longer tracked + + assert len(machine.queued) == 0 + assert atx.final is False + assert isinstance(atx, FaultedTx) + + # ensure switch back to IDLE + yield clock.advance(1) + + assert len(machine.queued) == 0 + assert machine.pending is None + + assert not machine.busy assert machine.current_state == machine._IDLE From 86734c38c13dc9d55cf9f0e8c081e51d9c31fa4c Mon Sep 17 00:00:00 2001 From: derekpierre Date: Thu, 7 Mar 2024 11:54:44 -0500 Subject: [PATCH 49/70] Ensure that retry_failure_counter is cleared for all exit cases fault, success. --- atxm/machine.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/atxm/machine.py b/atxm/machine.py index 7233c12..a0f4a47 100644 --- a/atxm/machine.py +++ b/atxm/machine.py @@ -273,11 +273,17 @@ def __handle_active_transaction(self) -> None: # Outcome 2: the pending transaction was reverted (final error) except TransactionReverted as e: + # clear entry if exists + self._retry_failure_counter.pop(pending_tx.id, "None") + self._tx_tracker.fault(fault_error=e) return # Outcome 3: pending transaction is finalized (final success) if receipt: + # clear entry if exists + self._retry_failure_counter.pop(pending_tx.id, "None") + final_txhash = receipt["transactionHash"] confirmations = _get_confirmations(w3=self.w3, tx=pending_tx) self.log.info( From 190ed9484c9d56dbeadaae23825fae8f754b4fe7 Mon Sep 17 00:00:00 2001 From: derekpierre Date: Thu, 7 Mar 2024 11:58:22 -0500 Subject: [PATCH 50/70] Rename constant to _MAX_REDO_ATTEMPTS. --- atxm/machine.py | 7 ++++--- tests/test_machine.py | 8 ++++---- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/atxm/machine.py b/atxm/machine.py index a0f4a47..06769bf 100644 --- a/atxm/machine.py +++ b/atxm/machine.py @@ -101,7 +101,8 @@ class _Machine(StateMachine): ExponentialSpeedupStrategy, ] - _NUM_REDO_ATTEMPTS = 3 + # max requeues/retries + _MAX_REDO_ATTEMPTS = 3 class LogObserver: """StateMachine observer for logging information about state/transitions.""" @@ -393,7 +394,7 @@ def __handle_retry_failure(self, pending_tx: PendingTx, e: Exception): ) num_failed_retries = self._retry_failure_counter.get(pending_tx.id, 0) num_failed_retries += 1 - if num_failed_retries > self._NUM_REDO_ATTEMPTS: + if num_failed_retries > self._MAX_REDO_ATTEMPTS: log.error( f"[retry] transaction #atx-{pending_tx.id}|{pending_tx.params['nonce']} " f"failed for {num_failed_retries} attempts; tx will no longer be retried" @@ -453,7 +454,7 @@ def __handle_broadcast_failure(self, future_tx: FutureTx, e: Exception): is_broadcast_failure = False if _is_recoverable_send_tx_error(e): num_requeues = self._requeue_counter.get(future_tx.id, 0) - if num_requeues >= self._NUM_REDO_ATTEMPTS: + if num_requeues >= self._MAX_REDO_ATTEMPTS: is_broadcast_failure = True log.error( f"[broadcast] transaction #atx-{future_tx.id}|{future_tx.params['nonce']} " diff --git a/tests/test_machine.py b/tests/test_machine.py index 47e0a85..8420a74 100644 --- a/tests/test_machine.py +++ b/tests/test_machine.py @@ -400,7 +400,7 @@ def test_broadcast_recoverable_error( mock_wake_sleep, ): # need more freedom with redo attempts for test - mocker.patch.object(machine, "_NUM_REDO_ATTEMPTS", 10) + mocker.patch.object(machine, "_MAX_REDO_ATTEMPTS", 10) wake, _ = mock_wake_sleep @@ -513,7 +513,7 @@ def test_broadcast_recoverable_error_requeues_exceeded( # repeat some cycles; tx fails then gets requeued since error is "recoverable" machine.start(now=True) # one less than max attempts - for i in range(machine._NUM_REDO_ATTEMPTS - 1): + for i in range(machine._MAX_REDO_ATTEMPTS - 1): assert len(machine.queued) == 1 # remains in queue and not broadcasted yield clock.advance(1) assert machine._requeue_counter.get(atx.id, 0) >= i @@ -916,7 +916,7 @@ def test_retry_with_errors_but_recovers( mock_wake_sleep, ): # need more freedom with redo attempts for test - mocker.patch.object(machine, "_NUM_REDO_ATTEMPTS", 10) + mocker.patch.object(machine, "_MAX_REDO_ATTEMPTS", 10) # TODO consider whether this should just be provided to constructor - #23 machine._strategies.clear() @@ -1069,7 +1069,7 @@ def test_retry_with_errors_retries_exceeded( mocker.patch.object(w3.eth, "send_raw_transaction", side_effect=error) # retry max attempts - for i in range(machine._NUM_REDO_ATTEMPTS): + for i in range(machine._MAX_REDO_ATTEMPTS): assert machine.pending is not None yield clock.advance(1) assert machine._retry_failure_counter.get(atx.id, 0) >= i From f8204740745a5d5cae00e6cb50e703296945165b Mon Sep 17 00:00:00 2001 From: derekpierre Date: Thu, 7 Mar 2024 12:01:52 -0500 Subject: [PATCH 51/70] Add TODO. --- atxm/machine.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/atxm/machine.py b/atxm/machine.py index 06769bf..cae01a1 100644 --- a/atxm/machine.py +++ b/atxm/machine.py @@ -357,6 +357,9 @@ def __strategize(self) -> None: params_updated = True if not params_updated: + # TODO is this a potential forever wait - this is really controlled by strategies + # who can no longer do anything. if we limit the wait here then the TimeoutStrategy + # becomes useless - something to think about. #14 log.info( f"[wait] strategies made no suggested updates to " f"pending tx #{_active_copy.id} - skipping retry round" From a2ea01ccc54b9164279a2a2b85c07f5c5394cfc9 Mon Sep 17 00:00:00 2001 From: derekpierre Date: Thu, 7 Mar 2024 13:38:44 -0500 Subject: [PATCH 52/70] Track num requeues and num retries directly on FutureTx and PendingTx. This allows for not needing extra logic for clearing out the dictionary used in the original tracking logic. Instead requeues and retries can be tracked on the tx object itself. Requeues for FutureTx can easily be incremented via the requeue() call on the tracker.It's a little trickier with PendingTx since we only deal with copies in the machine, but added a method to the tracker to make this easier. Updated tests. --- atxm/machine.py | 41 +++++++++-------------------------------- atxm/tracker.py | 11 +++++++++++ atxm/tx.py | 2 ++ tests/test_machine.py | 17 ++++++++--------- 4 files changed, 30 insertions(+), 41 deletions(-) diff --git a/atxm/machine.py b/atxm/machine.py index cae01a1..738e741 100644 --- a/atxm/machine.py +++ b/atxm/machine.py @@ -1,5 +1,5 @@ from copy import copy, deepcopy -from typing import Dict, List, Optional, Type +from typing import List, Optional, Type from eth_account.signers.local import LocalAccount from eth_utils import ValidationError @@ -139,10 +139,6 @@ def __init__( self._task.clock = self.__CLOCK self._task.interval = self._IDLE_INTERVAL - # requeues/retries - self._requeue_counter: Dict[int, int] = dict() - self._retry_failure_counter: Dict[int, int] = dict() - super().__init__() self.add_observer(_Machine.LogObserver()) @@ -274,17 +270,11 @@ def __handle_active_transaction(self) -> None: # Outcome 2: the pending transaction was reverted (final error) except TransactionReverted as e: - # clear entry if exists - self._retry_failure_counter.pop(pending_tx.id, "None") - self._tx_tracker.fault(fault_error=e) return # Outcome 3: pending transaction is finalized (final success) if receipt: - # clear entry if exists - self._retry_failure_counter.pop(pending_tx.id, "None") - final_txhash = receipt["transactionHash"] confirmations = _get_confirmations(w3=self.w3, tx=pending_tx) self.log.info( @@ -374,14 +364,14 @@ def __strategize(self) -> None: except InsufficientFunds: # special case re-raise insufficient funds (for now) # TODO #13 + # TODO should the following also be done? + # self._tx_tracker.update_failed_retry_attempt(_active_copy) raise except (ValidationError, Web3Exception, ValueError) as e: + self._tx_tracker.update_failed_retry_attempt(_active_copy) self.__handle_retry_failure(_active_copy, e) return - # clear failed retries since successful - self._retry_failure_counter.pop(_active_copy.id, None) - _active_copy.txhash = txhash self._tx_tracker.update_after_retry(_active_copy) @@ -395,25 +385,19 @@ def __handle_retry_failure(self, pending_tx: PendingTx, e: Exception): f"[retry] transaction #atx-{pending_tx.id}|{pending_tx.params['nonce']} " f"failed with updated params - {str(e)}; retry again next round" ) - num_failed_retries = self._retry_failure_counter.get(pending_tx.id, 0) - num_failed_retries += 1 - if num_failed_retries > self._MAX_REDO_ATTEMPTS: + + if pending_tx.retries >= self._MAX_REDO_ATTEMPTS: log.error( f"[retry] transaction #atx-{pending_tx.id}|{pending_tx.params['nonce']} " - f"failed for {num_failed_retries} attempts; tx will no longer be retried" + f"failed for { pending_tx.retries} attempts; tx will no longer be retried" ) - # remove entry since no longer needed - self._retry_failure_counter.pop(pending_tx.id, None) - fault_error = TransactionFaulted( tx=pending_tx, fault=Fault.ERROR, message=str(e), ) self._tx_tracker.fault(fault_error=fault_error) - else: - self._retry_failure_counter[pending_tx.id] = num_failed_retries def __broadcast(self): """ @@ -445,9 +429,6 @@ def __broadcast(self): self.__handle_broadcast_failure(future_tx, e) return - # clear requeue counter since successful - self._requeue_counter.pop(future_tx.id, None) - self._tx_tracker.morph(tx=future_tx, txhash=txhash) pending_tx = self._tx_tracker.pending if pending_tx.on_broadcast: @@ -456,12 +437,11 @@ def __broadcast(self): def __handle_broadcast_failure(self, future_tx: FutureTx, e: Exception): is_broadcast_failure = False if _is_recoverable_send_tx_error(e): - num_requeues = self._requeue_counter.get(future_tx.id, 0) - if num_requeues >= self._MAX_REDO_ATTEMPTS: + if future_tx.requeues >= self._MAX_REDO_ATTEMPTS: is_broadcast_failure = True log.error( f"[broadcast] transaction #atx-{future_tx.id}|{future_tx.params['nonce']} " - f"failed for {num_requeues} attempts; tx will not be requeued" + f"failed for {future_tx.requeues} attempts; tx will not be requeued" ) else: log.warn( @@ -469,7 +449,6 @@ def __handle_broadcast_failure(self, future_tx: FutureTx, e: Exception): f"failed - {str(e)}; requeueing tx" ) self._tx_tracker.requeue(future_tx) - self._requeue_counter[future_tx.id] = num_requeues + 1 else: # non-recoverable is_broadcast_failure = True @@ -479,8 +458,6 @@ def __handle_broadcast_failure(self, future_tx: FutureTx, e: Exception): ) if is_broadcast_failure: - # remove entry since no longer needed - self._requeue_counter.pop(future_tx.id, None) hook = future_tx.on_broadcast_failure if hook: fire_hook(hook, future_tx, e) diff --git a/atxm/tracker.py b/atxm/tracker.py index 575ace9..40d5227 100644 --- a/atxm/tracker.py +++ b/atxm/tracker.py @@ -111,6 +111,16 @@ def update_after_retry(self, tx: PendingTx) -> PendingTx: return self.pending + def update_failed_retry_attempt(self, tx: PendingTx): + if tx.id != self.__active.id: + raise RuntimeError( + f"Trying to update unexpected active tx: from {self.__active.id} to {tx.id}" + ) + self.__active.retries += 1 + # safety check + if tx is not self.__active: + tx.retries += 1 + def morph(self, tx: FutureTx, txhash: TxHash) -> PendingTx: """ Morphs a future transaction into a pending transaction. @@ -196,6 +206,7 @@ def pop(self) -> FutureTx: def requeue(self, tx: FutureTx) -> None: """Re-queue a transaction for broadcast and subsequent tracking.""" + tx.requeues += 1 self.__queue.append(tx) self.commit() log.info( diff --git a/atxm/tx.py b/atxm/tx.py index 0aa1779..174ff0a 100644 --- a/atxm/tx.py +++ b/atxm/tx.py @@ -52,6 +52,7 @@ def from_dict(cls, data: Dict): @dataclass class FutureTx(AsyncTx): + requeues: int = field(default=0, init=False) final: bool = field(default=False, init=False) info: Optional[Dict] = None @@ -80,6 +81,7 @@ def from_dict(cls, data: Dict): @dataclass class PendingTx(AsyncTx): + retries: int = field(default=0, init=False) final: bool = field(default=False, init=False) txhash: TxHash created: int diff --git a/tests/test_machine.py b/tests/test_machine.py index 8420a74..9e4aa49 100644 --- a/tests/test_machine.py +++ b/tests/test_machine.py @@ -301,7 +301,7 @@ def test_broadcast( yield deferLater(reactor, 0.2, lambda: None) assert hook.call_count == 1 - assert machine._requeue_counter.get(atx.id) is None # not tracked + assert atx.retries == 0 # tx only broadcasted and not finalized, so we are still busy assert machine.current_state == machine._BUSY @@ -371,7 +371,7 @@ def test_broadcast_non_recoverable_error( assert broadcast_hook.call_count == 0 - assert machine._requeue_counter.get(atx.id) is None # not tracked + assert atx.requeues == 0 # tx failed and not requeued assert machine.current_state == machine._IDLE @@ -434,6 +434,7 @@ def test_broadcast_recoverable_error( for i in range(5): yield clock.advance(1) assert len(machine.queued) == 1 # remains in queue and not broadcasted + assert atx.requeues >= i # call real method from now on mocker.patch.object(w3.eth, "send_raw_transaction", side_effect=real_method) @@ -457,8 +458,6 @@ def test_broadcast_recoverable_error( assert broadcast_hook.call_count == 1 assert broadcast_failure_hook.call_count == 0 - assert machine._requeue_counter.get(atx.id) is None # no longer tracked - # tx only broadcasted and not finalized, so we are still busy assert machine.current_state == machine._BUSY @@ -516,7 +515,7 @@ def test_broadcast_recoverable_error_requeues_exceeded( for i in range(machine._MAX_REDO_ATTEMPTS - 1): assert len(machine.queued) == 1 # remains in queue and not broadcasted yield clock.advance(1) - assert machine._requeue_counter.get(atx.id, 0) >= i + assert atx.requeues >= i # push over the retry limit yield clock.advance(1) @@ -526,7 +525,7 @@ def test_broadcast_recoverable_error_requeues_exceeded( assert broadcast_failure_hook.call_count == 1 broadcast_failure_hook.assert_called_with(atx, error) - assert machine._requeue_counter.get(atx.id) is None # no longer tracked + assert atx.requeues == machine._MAX_REDO_ATTEMPTS # The transaction failed and is not requeued assert len(machine.queued) == 0 @@ -965,6 +964,7 @@ def test_retry_with_errors_but_recovers( assert ( machine.pending ), "tx is being retried but encounters retry error and remains pending" + assert atx.retries >= i assert strategy_1.execute.call_count > 0, "strategy #1 was called" # retries failed, so params shouldn't have been updated @@ -989,7 +989,6 @@ def test_retry_with_errors_but_recovers( assert len(machine.queued) == 0 assert machine.pending is None - assert machine._retry_failure_counter.get(atx.id) is None # no longer tracked assert not machine.busy assert atx.final @@ -1072,7 +1071,7 @@ def test_retry_with_errors_retries_exceeded( for i in range(machine._MAX_REDO_ATTEMPTS): assert machine.pending is not None yield clock.advance(1) - assert machine._retry_failure_counter.get(atx.id, 0) >= i + assert atx.retries >= i # push over retry limit yield clock.advance(1) @@ -1086,7 +1085,7 @@ def test_retry_with_errors_retries_exceeded( assert fault_hook.call_count == 1 fault_hook.assert_called_with(atx) - assert machine._retry_failure_counter.get(atx.id) is None # no longer tracked + assert atx.retries == machine._MAX_REDO_ATTEMPTS assert len(machine.queued) == 0 assert atx.final is False From 6f0e1503b28793642c6d4a446cc356fe1ef1859b Mon Sep 17 00:00:00 2001 From: derekpierre Date: Thu, 7 Mar 2024 14:48:43 -0500 Subject: [PATCH 53/70] Strategies must be explicitly provided by the user, if not, no strategies will be used by default. This way users totally control strategies used and their parameterized values instead of implicit defaults which have repercussions eg. cost for speed up strategy. Update tests accordingly. --- atxm/machine.py | 20 +++------ tests/conftest.py | 15 ++++++- tests/test_machine.py | 96 +++++++++++++++++++++++++++++++++++++------ 3 files changed, 103 insertions(+), 28 deletions(-) diff --git a/atxm/machine.py b/atxm/machine.py index 738e741..4a612f1 100644 --- a/atxm/machine.py +++ b/atxm/machine.py @@ -1,5 +1,5 @@ from copy import copy, deepcopy -from typing import List, Optional, Type +from typing import List, Optional from eth_account.signers.local import LocalAccount from eth_utils import ValidationError @@ -17,11 +17,7 @@ TransactionFaulted, TransactionReverted, ) -from atxm.strategies import ( - AsyncTxStrategy, - ExponentialSpeedupStrategy, - TimeoutStrategy, -) +from atxm.strategies import AsyncTxStrategy from atxm.tracker import _TxTracker from atxm.tx import ( AsyncTx, @@ -96,11 +92,6 @@ class _Machine(StateMachine): _BLOCK_INTERVAL = 20 # ~20 blocks _BLOCK_SAMPLE_SIZE = 10_000 # blocks - STRATEGIES: List[Type[AsyncTxStrategy]] = [ - TimeoutStrategy, - ExponentialSpeedupStrategy, - ] - # max requeues/retries _MAX_REDO_ATTEMPTS = 3 @@ -124,7 +115,7 @@ def __init__( self.w3 = w3 self.signers = {} self.log = log - self._strategies = [s(w3) for s in self.STRATEGIES] + self._strategies = list() if strategies: self._strategies.extend(list(strategies)) @@ -348,8 +339,9 @@ def __strategize(self) -> None: if not params_updated: # TODO is this a potential forever wait - this is really controlled by strategies - # who can no longer do anything. if we limit the wait here then the TimeoutStrategy - # becomes useless - something to think about. #14 + # who can no longer do anything OR if there are no strategies defined. Something + # to think about. If we simply limit retry here, then what does that mean if there is + # a TimeoutStrategy? #14 log.info( f"[wait] strategies made no suggested updates to " f"pending tx #{_active_copy.id} - skipping retry round" diff --git a/tests/conftest.py b/tests/conftest.py index ec51efc..15dd679 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -10,6 +10,7 @@ from atxm import AutomaticTxMachine from atxm.logging import log +from atxm.strategies import ExponentialSpeedupStrategy, TimeoutStrategy observer = textFileLogObserver(sys.stdout) globalLogPublisher.addObserver(observer) @@ -61,9 +62,19 @@ def w3(networks): @pytest.fixture -def machine(w3): +def strategies(w3): + _strategy_classes = [ + TimeoutStrategy, + ExponentialSpeedupStrategy, + ] + _strategies = [s(w3) for s in _strategy_classes] + return _strategies + + +@pytest.fixture +def machine(w3, strategies): clock = Clock() - _machine = AutomaticTxMachine(w3=w3) + _machine = AutomaticTxMachine(w3=w3, strategies=strategies) _machine._task.clock = clock yield _machine diff --git a/tests/test_machine.py b/tests/test_machine.py index 9e4aa49..8a41933 100644 --- a/tests/test_machine.py +++ b/tests/test_machine.py @@ -1,4 +1,5 @@ import math +from typing import List, Optional import pytest @@ -14,6 +15,7 @@ Web3Exception, ) +from atxm import AutomaticTxMachine from atxm.strategies import AsyncTxStrategy, TimeoutStrategy from atxm.tx import FaultedTx, FinalizedTx, FutureTx, PendingTx from atxm.utils import _is_recoverable_send_tx_error @@ -820,16 +822,13 @@ def test_use_strategies_that_dont_make_updates( mocker, mock_wake_sleep, ): - # TODO consider whether this should just be provided to constructor - #23 - machine._strategies.clear() - # strategies that don't make updates strategy_1 = mocker.Mock(spec=AsyncTxStrategy) strategy_1.execute.return_value = None strategy_2 = mocker.Mock(spec=AsyncTxStrategy) strategy_2.execute.return_value = None - machine._strategies = [strategy_1, strategy_2] + _configure_machine_strategies(machine, [strategy_1, strategy_2]) update_spy = mocker.spy(machine._tx_tracker, "update_after_retry") @@ -889,6 +888,77 @@ def test_use_strategies_that_dont_make_updates( machine.stop() +@pytest_twisted.inlineCallbacks +@pytest.mark.usefixtures("disable_auto_mining") +def test_dont_use_any_strategies( + ethereum_tester, + w3, + machine, + state_observer, + clock, + eip1559_transaction, + account, + mocker, + mock_wake_sleep, +): + # strategies that don't make updates + _configure_machine_strategies(machine, None) + + update_spy = mocker.spy(machine._tx_tracker, "update_after_retry") + + machine.start() + assert machine.current_state == machine._IDLE + + broadcast_hook = mocker.Mock() + atx = machine.queue_transaction( + params=eip1559_transaction, signer=account, on_broadcast=broadcast_hook + ) + + # advance to broadcast the transaction + while machine.pending is None: + yield clock.advance(1) + + # ensure that hook is called + yield deferLater(reactor, 0.2, lambda: None) + assert broadcast_hook.call_count == 1 + broadcast_hook.assert_called_with(atx), "called with correct parameter" + + original_params = dict(atx.params) + + assert machine.current_state == machine._BUSY + + # need some cycles while tx unmined for strategies to kick in + num_cycles = 4 + for i in range(num_cycles): + yield clock.advance(1) + # params remained unchanged since strategies don't make updates + assert atx.params == original_params, "params remain unchanged" + + assert atx.params == original_params, "params remain unchanged" + assert update_spy.call_count == 0, "update never called because no retry" + + # mine tx + ethereum_tester.mine_block() + yield clock.advance(1) + + # ensure switch back to IDLE + yield clock.advance(1) + + assert len(machine.queued) == 0 + assert machine.pending is None + + assert not machine.busy + assert atx.final + + assert machine.current_state == machine._IDLE + + assert len(state_observer.transitions) == 2 + assert state_observer.transitions[0] == (machine._IDLE, machine._BUSY) + assert state_observer.transitions[1] == (machine._BUSY, machine._IDLE) + + machine.stop() + + @pytest_twisted.inlineCallbacks @pytest.mark.usefixtures("disable_auto_mining") @pytest.mark.parametrize( @@ -917,16 +987,13 @@ def test_retry_with_errors_but_recovers( # need more freedom with redo attempts for test mocker.patch.object(machine, "_MAX_REDO_ATTEMPTS", 10) - # TODO consider whether this should just be provided to constructor - #23 - machine._strategies.clear() - # strategies that don't make updates strategy_1 = mocker.Mock(spec=AsyncTxStrategy) strategy_1.name = "mock_strategy" # return non-None so retry is attempted strategy_1.execute.return_value = dict(eip1559_transaction) - machine._strategies = [strategy_1] + _configure_machine_strategies(machine, [strategy_1]) update_spy = mocker.spy(machine._tx_tracker, "update_after_retry") @@ -1028,16 +1095,13 @@ def test_retry_with_errors_retries_exceeded( mocker, mock_wake_sleep, ): - # TODO consider whether this should just be provided to constructor - #23 - machine._strategies.clear() - # strategies that don't make updates strategy_1 = mocker.Mock(spec=AsyncTxStrategy) strategy_1.name = "mock_strategy" # return non-None so retry is attempted strategy_1.execute.return_value = dict(eip1559_transaction) - machine._strategies = [strategy_1] + _configure_machine_strategies(machine, [strategy_1]) update_spy = mocker.spy(machine._tx_tracker, "update_after_retry") @@ -1251,3 +1315,11 @@ def test_simple_state_transitions( assert machine.current_state == machine._IDLE assert wake.call_count == wake_call_count, "wake call count remains unchanged" assert sleep.call_count == sleep_call_count, "wake call count remains unchanged" + + +def _configure_machine_strategies( + machine: AutomaticTxMachine, strategies: Optional[List[AsyncTxStrategy]] = None +): + machine._strategies.clear() + if strategies: + machine._strategies.extend(strategies) From 008c3085763cee07bb524adbfc2d158631c86561 Mon Sep 17 00:00:00 2001 From: derekpierre Date: Thu, 7 Mar 2024 17:05:29 -0500 Subject: [PATCH 54/70] There will always be one TimeoutStrategy used by the machine. The timeout is provided in the constructor for the machine and is either the default timeout value from the TimeoutStrategy class or a user provided timeout. This is a guardrail to prevent misconfigured strategies from never timing out causing the machine to forever wait on a long running tx. Removed tests regarding no strategies since no longer applicable. --- atxm/machine.py | 11 +++--- atxm/strategies.py | 5 +-- tests/conftest.py | 3 +- tests/test_machine.py | 82 ++++-------------------------------------- tests/test_strategy.py | 4 +-- 5 files changed, 17 insertions(+), 88 deletions(-) diff --git a/atxm/machine.py b/atxm/machine.py index 4a612f1..79e2d4b 100644 --- a/atxm/machine.py +++ b/atxm/machine.py @@ -17,7 +17,7 @@ TransactionFaulted, TransactionReverted, ) -from atxm.strategies import AsyncTxStrategy +from atxm.strategies import AsyncTxStrategy, TimeoutStrategy from atxm.tracker import _TxTracker from atxm.tx import ( AsyncTx, @@ -108,6 +108,7 @@ def on_transition(self, source, target): def __init__( self, w3: Web3, + tx_exec_timeout: int = TimeoutStrategy.TIMEOUT, strategies: Optional[List[AsyncTxStrategy]] = None, disk_cache: bool = False, ): @@ -115,7 +116,8 @@ def __init__( self.w3 = w3 self.signers = {} self.log = log - self._strategies = list() + # default TimeoutStrategy using provided timeout - guardrail for users + self._strategies = [TimeoutStrategy(w3, timeout=tx_exec_timeout)] if strategies: self._strategies.extend(list(strategies)) @@ -338,10 +340,7 @@ def __strategize(self) -> None: params_updated = True if not params_updated: - # TODO is this a potential forever wait - this is really controlled by strategies - # who can no longer do anything OR if there are no strategies defined. Something - # to think about. If we simply limit retry here, then what does that mean if there is - # a TimeoutStrategy? #14 + # mandatory default timeout strategy prevents this from being a forever wait log.info( f"[wait] strategies made no suggested updates to " f"pending tx #{_active_copy.id} - skipping retry round" diff --git a/atxm/strategies.py b/atxm/strategies.py index 9ac5b33..892ddeb 100644 --- a/atxm/strategies.py +++ b/atxm/strategies.py @@ -69,13 +69,14 @@ class TimeoutStrategy(AsyncTxStrategy): _NAME = "timeout" - _TIMEOUT = 60 * 60 # 1 hour in seconds + # TODO is this too long? + TIMEOUT = 60 * 60 # 1 hour in seconds _WARN_FACTOR = 0.15 # 15% of timeout remaining def __init__(self, w3: Web3, timeout: Optional[int] = None): super().__init__(w3) - self.timeout = timeout or self._TIMEOUT + self.timeout = timeout or self.TIMEOUT # use 30s as default in case timeout is too small for warn factor self._warn_threshold = max(30, self.timeout * self._WARN_FACTOR) diff --git a/tests/conftest.py b/tests/conftest.py index 15dd679..b492db4 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -10,7 +10,7 @@ from atxm import AutomaticTxMachine from atxm.logging import log -from atxm.strategies import ExponentialSpeedupStrategy, TimeoutStrategy +from atxm.strategies import ExponentialSpeedupStrategy observer = textFileLogObserver(sys.stdout) globalLogPublisher.addObserver(observer) @@ -64,7 +64,6 @@ def w3(networks): @pytest.fixture def strategies(w3): _strategy_classes = [ - TimeoutStrategy, ExponentialSpeedupStrategy, ] _strategies = [s(w3) for s in _strategy_classes] diff --git a/tests/test_machine.py b/tests/test_machine.py index 8a41933..3889050 100644 --- a/tests/test_machine.py +++ b/tests/test_machine.py @@ -1,5 +1,5 @@ import math -from typing import List, Optional +from typing import List import pytest @@ -782,7 +782,7 @@ def test_use_strategies_timeout_used( num_cycles = 4 for i in range(num_cycles): # reduce creation time by timeout to force timeout - atx.created -= math.ceil(TimeoutStrategy._TIMEOUT / (num_cycles - 1)) + atx.created -= math.ceil(TimeoutStrategy.TIMEOUT / (num_cycles - 1)) yield clock.advance(1) # ensure switch back to IDLE @@ -888,77 +888,6 @@ def test_use_strategies_that_dont_make_updates( machine.stop() -@pytest_twisted.inlineCallbacks -@pytest.mark.usefixtures("disable_auto_mining") -def test_dont_use_any_strategies( - ethereum_tester, - w3, - machine, - state_observer, - clock, - eip1559_transaction, - account, - mocker, - mock_wake_sleep, -): - # strategies that don't make updates - _configure_machine_strategies(machine, None) - - update_spy = mocker.spy(machine._tx_tracker, "update_after_retry") - - machine.start() - assert machine.current_state == machine._IDLE - - broadcast_hook = mocker.Mock() - atx = machine.queue_transaction( - params=eip1559_transaction, signer=account, on_broadcast=broadcast_hook - ) - - # advance to broadcast the transaction - while machine.pending is None: - yield clock.advance(1) - - # ensure that hook is called - yield deferLater(reactor, 0.2, lambda: None) - assert broadcast_hook.call_count == 1 - broadcast_hook.assert_called_with(atx), "called with correct parameter" - - original_params = dict(atx.params) - - assert machine.current_state == machine._BUSY - - # need some cycles while tx unmined for strategies to kick in - num_cycles = 4 - for i in range(num_cycles): - yield clock.advance(1) - # params remained unchanged since strategies don't make updates - assert atx.params == original_params, "params remain unchanged" - - assert atx.params == original_params, "params remain unchanged" - assert update_spy.call_count == 0, "update never called because no retry" - - # mine tx - ethereum_tester.mine_block() - yield clock.advance(1) - - # ensure switch back to IDLE - yield clock.advance(1) - - assert len(machine.queued) == 0 - assert machine.pending is None - - assert not machine.busy - assert atx.final - - assert machine.current_state == machine._IDLE - - assert len(state_observer.transitions) == 2 - assert state_observer.transitions[0] == (machine._IDLE, machine._BUSY) - assert state_observer.transitions[1] == (machine._BUSY, machine._IDLE) - - machine.stop() - - @pytest_twisted.inlineCallbacks @pytest.mark.usefixtures("disable_auto_mining") @pytest.mark.parametrize( @@ -1318,8 +1247,9 @@ def test_simple_state_transitions( def _configure_machine_strategies( - machine: AutomaticTxMachine, strategies: Optional[List[AsyncTxStrategy]] = None + machine: AutomaticTxMachine, strategies: List[AsyncTxStrategy] ): + # create updated strategies list with the default TimeoutStrategy machine._strategies.clear() - if strategies: - machine._strategies.extend(strategies) + machine._strategies.append(TimeoutStrategy(machine.w3)) + machine._strategies.extend(strategies) diff --git a/tests/test_strategy.py b/tests/test_strategy.py index 53c1c2a..facd541 100644 --- a/tests/test_strategy.py +++ b/tests/test_strategy.py @@ -18,10 +18,10 @@ def test_timeout_strategy(w3, mocker): # default timeout timeout_strategy = TimeoutStrategy(w3) - assert timeout_strategy.timeout == TimeoutStrategy._TIMEOUT + assert timeout_strategy.timeout == TimeoutStrategy.TIMEOUT assert ( timeout_strategy._warn_threshold - == TimeoutStrategy._TIMEOUT * TimeoutStrategy._WARN_FACTOR + == TimeoutStrategy.TIMEOUT * TimeoutStrategy._WARN_FACTOR ) # specific timeout - low timeout From b76111f9865c1fd41c123b8b98a941777fd48054 Mon Sep 17 00:00:00 2001 From: derekpierre Date: Thu, 7 Mar 2024 21:19:57 -0500 Subject: [PATCH 55/70] Update error messages when non-active tx provided for retry update/failed attempt. --- atxm/tracker.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/atxm/tracker.py b/atxm/tracker.py index 40d5227..7c29b23 100644 --- a/atxm/tracker.py +++ b/atxm/tracker.py @@ -101,9 +101,11 @@ def __set_active(self, tx: PendingTx) -> None: log.debug(f"[tracker] tracked active transaction {tx.txhash.hex()}") def update_after_retry(self, tx: PendingTx) -> PendingTx: + if not self.__active: + raise RuntimeError("No active transaction to update") if tx.id != self.__active.id: raise RuntimeError( - f"Trying to update unexpected active tx: from {self.__active.id} to {tx.id}" + f"Mismatch between active tx ({self.__active.id}) and provided tx ({tx.id})" ) self.__active.txhash = tx.txhash @@ -112,9 +114,11 @@ def update_after_retry(self, tx: PendingTx) -> PendingTx: return self.pending def update_failed_retry_attempt(self, tx: PendingTx): + if not self.__active: + raise RuntimeError("No active transaction to update") if tx.id != self.__active.id: raise RuntimeError( - f"Trying to update unexpected active tx: from {self.__active.id} to {tx.id}" + f"Mismatch between active tx ({self.__active.id}) and provided tx ({tx.id})" ) self.__active.retries += 1 # safety check From 3f521216604920d9e2441a8538b0887bc4f903f1 Mon Sep 17 00:00:00 2001 From: derekpierre Date: Thu, 7 Mar 2024 21:21:02 -0500 Subject: [PATCH 56/70] Get hook after ensuring active tx is set. --- atxm/tracker.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/atxm/tracker.py b/atxm/tracker.py index 7c29b23..94e6fae 100644 --- a/atxm/tracker.py +++ b/atxm/tracker.py @@ -172,9 +172,10 @@ def finalize_active_tx(self, receipt: TxReceipt) -> None: Finalizes a pending transaction. Use polymorphism to transform the pending transaction into a finalized transaction. """ - hook = self.__active.on_finalized if not self.__active: raise RuntimeError("No pending transaction to finalize") + + hook = self.__active.on_finalized self.__active.receipt = receipt self.__active.__class__ = FinalizedTx tx = self.__active From 7d5b060f8b2377daffe4155b5f697ff2fbe8823d Mon Sep 17 00:00:00 2001 From: derekpierre Date: Thu, 7 Mar 2024 21:21:55 -0500 Subject: [PATCH 57/70] Inital tests for tracker. --- tests/test_tracker.py | 398 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 398 insertions(+) create mode 100644 tests/test_tracker.py diff --git a/tests/test_tracker.py b/tests/test_tracker.py new file mode 100644 index 0000000..586ec4f --- /dev/null +++ b/tests/test_tracker.py @@ -0,0 +1,398 @@ +import time + +import pytest +from hexbytes import HexBytes +from web3.types import TxReceipt + +from atxm.exceptions import Fault, TransactionFaulted +from atxm.tracker import _TxTracker +from atxm.tx import FaultedTx, FinalizedTx, FutureTx, PendingTx, TxHash + + +def test_queue(eip1559_transaction, legacy_transaction, mocker): + tx_tracker = _TxTracker(disk_cache=False) + assert len(tx_tracker.queue) == 0 + assert tx_tracker.pending is None + assert len(tx_tracker.finalized) == 0 + + tx = tx_tracker.queue_tx(params=eip1559_transaction) + assert len(tx_tracker.queue) == 1 + assert isinstance(tx, FutureTx) + assert tx_tracker.queue[0] == tx + assert tx_tracker.queue[0].params == eip1559_transaction + assert tx.params == eip1559_transaction + assert tx.info is None + assert tx.final is False + assert tx.fault is None + assert tx.id == 0 + + assert tx_tracker.pending is None + assert len(tx_tracker.finalized) == 0 + + tx_2_info = {"description": "it's me!", "message": "me who?"} + tx_2 = tx_tracker.queue_tx(params=legacy_transaction, info=tx_2_info) + assert len(tx_tracker.queue) == 2 + assert isinstance(tx_2, FutureTx) + assert tx_tracker.queue[1] == tx_2 + assert tx_tracker.queue[1].params == legacy_transaction + assert tx_2.params == legacy_transaction + assert tx_2.info == tx_2_info + assert tx_2.final is False + assert tx_2.fault is None + assert tx_2.id == 1 + + assert tx_tracker.pending is None + assert len(tx_tracker.finalized) == 0 + + # check hooks + assert tx.on_broadcast is None + assert tx.on_broadcast_failure is None + assert tx.on_fault is None + assert tx.on_finalized is None + + broadcast_hook = mocker.Mock() + broadcast_failure_hook = mocker.Mock() + fault_hook = mocker.Mock() + finalized_hook = mocker.Mock() + tx_3 = tx_tracker.queue_tx( + params=eip1559_transaction, + on_broadcast=broadcast_hook, + on_broadcast_failure=broadcast_failure_hook, + on_fault=fault_hook, + on_finalized=finalized_hook, + ) + assert tx_3.params == eip1559_transaction + assert tx_3.info is None + assert tx_3.final is False + assert tx_3.fault is None + assert tx_3.id == 2 + assert tx_3.on_broadcast == broadcast_hook + assert tx_3.on_broadcast_failure == broadcast_failure_hook + assert tx_3.on_fault == fault_hook + assert tx_3.on_finalized == finalized_hook + + assert len(tx_tracker.queue) == 3 + assert isinstance(tx_3, FutureTx) + assert tx_tracker.queue[2] == tx_3 + assert tx_tracker.queue[2].params == eip1559_transaction + assert tx_tracker.pending is None + assert len(tx_tracker.finalized) == 0 + + +def test_pop(eip1559_transaction, legacy_transaction): + tx_tracker = _TxTracker(disk_cache=False) + tx_1 = tx_tracker.queue_tx(params=eip1559_transaction) + tx_2 = tx_tracker.queue_tx(params=legacy_transaction) + tx_3 = tx_tracker.queue_tx(params=eip1559_transaction) + + assert len(tx_tracker.queue) == 3 + + for tx in [tx_1, tx_2, tx_3]: + popped_tx = tx_tracker.pop() + assert popped_tx is tx + + with pytest.raises(IndexError): + tx_tracker.pop() + + +def test_requeue(eip1559_transaction, legacy_transaction): + tx_tracker = _TxTracker(disk_cache=False) + tx_1 = tx_tracker.queue_tx(params=eip1559_transaction) + assert tx_1.requeues == 0 + tx_2 = tx_tracker.queue_tx(params=legacy_transaction) + assert tx_2.requeues == 0 + tx_3 = tx_tracker.queue_tx(params=eip1559_transaction) + assert tx_3.requeues == 0 + + assert len(tx_tracker.queue) == 3 + + base_num_requeues = 4 + for i in range(1, base_num_requeues + 1): + prior_pop = None + for _ in tx_tracker.queue: + popped_tx = tx_tracker.pop() + assert popped_tx is not prior_pop, "requeue was an append, not a prepend" + + tx_tracker.requeue(popped_tx) + prior_pop = popped_tx + assert popped_tx.requeues == i + + assert len(tx_tracker.queue) == 3, "remains the same length" + + assert tx_1.requeues == base_num_requeues + assert tx_2.requeues == base_num_requeues + assert tx_3.requeues == base_num_requeues + + _ = tx_tracker.pop() # remove tx_1 + _ = tx_tracker.pop() # remove tx_2 + + tx_tracker.requeue(tx_2) + assert tx_2.requeues == base_num_requeues + 1 + assert tx_1.requeues == base_num_requeues + assert tx_3.requeues == base_num_requeues + + +def test_morph(eip1559_transaction, legacy_transaction, mocker): + tx_tracker = _TxTracker(disk_cache=False) + broadcast_hook = mocker.Mock() + broadcast_failure_hook = mocker.Mock() + fault_hook = mocker.Mock() + finalized_hook = mocker.Mock() + tx_1 = tx_tracker.queue_tx( + params=eip1559_transaction, + on_broadcast=broadcast_hook, + on_broadcast_failure=broadcast_failure_hook, + on_fault=fault_hook, + on_finalized=finalized_hook, + ) + tx_2 = tx_tracker.queue_tx(params=legacy_transaction) + assert tx_1.id != tx_2.id + + tx_hash = TxHash("0xdeadbeef") + pending_tx = tx_tracker.morph(tx_1, tx_hash) + + assert isinstance(pending_tx, PendingTx) + assert pending_tx is tx_1, "same underlying object" + assert pending_tx.params == eip1559_transaction + assert pending_tx.txhash == tx_hash + assert pending_tx.created >= int(time.time()) + assert pending_tx.retries == 0 + assert pending_tx.final is False + assert pending_tx.fault is None + assert tx_tracker.pending.params == pending_tx.params + assert tx_1.on_broadcast == broadcast_hook + assert tx_1.on_broadcast_failure == broadcast_failure_hook + assert tx_1.on_fault == fault_hook + assert tx_1.on_finalized == finalized_hook + + assert isinstance(tx_2, FutureTx), "unaffected by the morph" + assert tx_tracker.pending is not tx_2 + + tx_2_hash = TxHash("0xdeadbeef2") + pending_tx_2 = tx_tracker.morph(tx_2, tx_2_hash) + assert isinstance(pending_tx_2, PendingTx) + assert pending_tx_2 is tx_2, "same underlying object" + assert tx_tracker.pending.params == pending_tx_2.params + assert tx_tracker.pending.params != pending_tx.params + assert tx_tracker.pending.txhash == pending_tx_2.txhash + + assert ( + tx_tracker.pending is not tx_tracker.pending + ), "copy of object always returned" + + +def test_fault(eip1559_transaction, legacy_transaction, mocker): + tx_tracker = _TxTracker(disk_cache=False) + broadcast_hook = mocker.Mock() + broadcast_failure_hook = mocker.Mock() + fault_hook = mocker.Mock() + finalized_hook = mocker.Mock() + tx = tx_tracker.queue_tx( + params=eip1559_transaction, + on_broadcast=broadcast_hook, + on_broadcast_failure=broadcast_failure_hook, + on_fault=fault_hook, + on_finalized=finalized_hook, + ) + tx_2 = tx_tracker.queue_tx(params=legacy_transaction) + + assert len(tx_tracker.queue) == 2 + + assert tx_tracker.pending is None + with pytest.raises(RuntimeError, match="No active transaction"): + # there is no active tx + fault_error = TransactionFaulted( + tx=tx_2, fault=Fault.ERROR, message="not active" + ) + tx_tracker.fault(fault_error) + + tx_hash = TxHash("0xdeadbeef") + pending_tx = tx_tracker.morph(tx, tx_hash) + assert tx_tracker.pending.params == tx.params + + fault_message = "Don't find fault, find a remedy" # - Henry Ford + + assert tx_tracker.pending is not tx_2 + with pytest.raises(RuntimeError, match="Mismatch between active tx"): + # tx_2 is not the active tx + fault_error = TransactionFaulted( + tx=tx_2, fault=Fault.ERROR, message=fault_message + ) + tx_tracker.fault(fault_error) + + fault_error = TransactionFaulted( + tx=pending_tx, fault=Fault.ERROR, message=fault_message + ) + assert fault_hook.call_count == 0 + + tx_tracker.fault(fault_error) + assert isinstance(tx, FaultedTx) + assert tx.fault == Fault.ERROR + assert tx.error == fault_message + + # check that fault hook was called + assert fault_hook.call_count == 1 + fault_hook.assert_called_with(tx) + + # check that other hooks were not called + assert broadcast_hook.call_count == 0 + assert broadcast_failure_hook.call_count == 0 + assert finalized_hook.call_count == 0 + + # no active tx + assert tx_tracker.pending is None + + # repeat with no hook + tx_hash_2 = TxHash("0xdeadbeef2") + pending_tx_2 = tx_tracker.morph(tx_2, tx_hash_2) + assert tx_tracker.pending.params == tx_2.params + + assert pending_tx_2 is tx_2, "same underlying object" + assert tx_tracker.pending.params == tx_2.params + + fault_error = TransactionFaulted( + tx=pending_tx_2, fault=Fault.REVERT, message=fault_message + ) + tx_tracker.fault(fault_error) + assert isinstance(tx_2, FaultedTx) + assert tx_2.fault == Fault.REVERT + assert tx_2.error == fault_message + + # no active tx + assert tx_tracker.pending is None + + +def test_update_after_retry(eip1559_transaction, legacy_transaction, mocker): + tx_tracker = _TxTracker(disk_cache=False) + tx = tx_tracker.queue_tx(params=eip1559_transaction) + + assert tx_tracker.pending is None + with pytest.raises(RuntimeError, match="No active transaction"): + # there is no active tx + tx_tracker.update_after_retry(mocker.Mock(spec=PendingTx)) + + tx_hash = TxHash("0xdeadbeef") + + tx_tracker.morph(tx, tx_hash) + assert isinstance(tx, PendingTx) + assert tx_tracker.pending.params == tx.params + + with pytest.raises(RuntimeError, match="Mismatch between active tx"): + mocked_tx = mocker.Mock(spec=PendingTx) + mocked_tx.id = 20 + tx_tracker.update_after_retry(mocked_tx) + + # first update + new_params = legacy_transaction + new_tx_hash = TxHash("0xdeadbeef2") + assert tx.params != new_params + pending_tx = tx_tracker.pending # obtain fresh copy + pending_tx.params = new_params + pending_tx.txhash = new_tx_hash + tx_tracker.update_after_retry(pending_tx) + assert tx.params == new_params + assert tx.txhash == new_tx_hash + + # update again + new_params = eip1559_transaction + new_tx_hash = TxHash("0xdeadbeef3") + assert tx.params != new_params + pending_tx = tx_tracker.pending # obtain fresh copy + pending_tx.params = new_params + pending_tx.txhash = new_tx_hash + tx_tracker.update_after_retry(pending_tx) + assert tx.params == new_params + assert tx.txhash == new_tx_hash + + +def test_update_failed_retry_attempt(eip1559_transaction, legacy_transaction, mocker): + tx_tracker = _TxTracker(disk_cache=False) + tx = tx_tracker.queue_tx(params=eip1559_transaction) + + assert tx_tracker.pending is None + with pytest.raises(RuntimeError, match="No active transaction"): + # there is no active tx + tx_tracker.update_failed_retry_attempt(mocker.Mock(spec=PendingTx)) + + tx_hash = TxHash("0xdeadbeef") + tx_tracker.morph(tx, tx_hash) + assert isinstance(tx, PendingTx) + pending_tx = tx_tracker.pending + assert pending_tx is not None + assert pending_tx.params == tx.params + + with pytest.raises(RuntimeError, match="Mismatch between active tx"): + mocked_tx = mocker.Mock(spec=PendingTx) + mocked_tx.id = 20 + tx_tracker.update_failed_retry_attempt(mocked_tx) + + assert tx.retries == 0 + + for i in range(1, 5): + tx_tracker.update_failed_retry_attempt(tx_tracker.pending) + assert tx.retries == i + assert tx_tracker.pending.retries == i + + +def test_finalize_active_tx(eip1559_transaction, mocker): + tx_tracker = _TxTracker(disk_cache=False) + broadcast_hook = mocker.Mock() + broadcast_failure_hook = mocker.Mock() + fault_hook = mocker.Mock() + finalized_hook = mocker.Mock() + tx = tx_tracker.queue_tx( + params=eip1559_transaction, + on_broadcast=broadcast_hook, + on_broadcast_failure=broadcast_failure_hook, + on_fault=fault_hook, + on_finalized=finalized_hook, + ) + + tx_hash = TxHash("0xdeadbeef") + tx_tracker.morph(tx, tx_hash) + assert isinstance(tx, PendingTx) + pending_tx = tx_tracker.pending + assert pending_tx is not None + assert pending_tx.params == tx.params + + assert len(tx_tracker.finalized) == 0 + + tx_receipt = TxReceipt( + { + "blockHash": HexBytes( + "0xf8be31c3eecd1f58432b211e906463b97c3cbfbe60c947c8700dff0ae7348299" + ), + "blockNumber": 1, + "contractAddress": None, + "cumulativeGasUsed": 21000, + "effectiveGasPrice": 1875000000, + "from": "0x1e59ce931B4CFea3fe4B875411e280e173cB7A9C", + "gasUsed": 21000, + "logs": [], + "state_root": b"\x01", + "status": 1, + "to": "0x1e59ce931B4CFea3fe4B875411e280e173cB7A9C", + "transactionHash": HexBytes( + "0x4798799f1cf30337b72381434d3ff56c43ee1fdfa1f812b8262069b7fb2f5a95" + ), + "transactionIndex": 0, + "type": 2, + } + ) + tx_tracker.finalize_active_tx(tx_receipt) + + assert isinstance(tx, FinalizedTx) + assert tx.final is True + assert tx.receipt == tx_receipt + assert finalized_hook.call_count == 1 + # other hooks not called + assert broadcast_hook.call_count == 0 + assert broadcast_failure_hook.call_count == 0 + assert fault_hook.call_count == 0 + + # active tx cleared + assert tx_tracker.pending is None + + assert len(tx_tracker.finalized) == 1 + for t in tx_tracker.finalized: + assert t == tx From b01782045f3720d0f379f4f2f48891414f02f82b Mon Sep 17 00:00:00 2001 From: derekpierre Date: Fri, 8 Mar 2024 08:26:58 -0500 Subject: [PATCH 58/70] Set retries value of copy instead of incrementing it - just safer. --- atxm/tracker.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/atxm/tracker.py b/atxm/tracker.py index 94e6fae..654872f 100644 --- a/atxm/tracker.py +++ b/atxm/tracker.py @@ -123,7 +123,7 @@ def update_failed_retry_attempt(self, tx: PendingTx): self.__active.retries += 1 # safety check if tx is not self.__active: - tx.retries += 1 + tx.retries = self.__active.retries def morph(self, tx: FutureTx, txhash: TxHash) -> PendingTx: """ From 153161f0c5cc4647265266e64a1b6d01c03be4b5 Mon Sep 17 00:00:00 2001 From: derekpierre Date: Fri, 8 Mar 2024 08:27:24 -0500 Subject: [PATCH 59/70] Rename handle retry failure parameter for clarity. --- atxm/machine.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/atxm/machine.py b/atxm/machine.py index 79e2d4b..abf3ce7 100644 --- a/atxm/machine.py +++ b/atxm/machine.py @@ -371,20 +371,20 @@ def __strategize(self) -> None: if pending_tx.on_broadcast: fire_hook(hook=pending_tx.on_broadcast, tx=pending_tx) - def __handle_retry_failure(self, pending_tx: PendingTx, e: Exception): + def __handle_retry_failure(self, attempted_tx: PendingTx, e: Exception): log.warn( - f"[retry] transaction #atx-{pending_tx.id}|{pending_tx.params['nonce']} " + f"[retry] transaction #atx-{attempted_tx.id}|{attempted_tx.params['nonce']} " f"failed with updated params - {str(e)}; retry again next round" ) - if pending_tx.retries >= self._MAX_REDO_ATTEMPTS: + if attempted_tx.retries >= self._MAX_REDO_ATTEMPTS: log.error( - f"[retry] transaction #atx-{pending_tx.id}|{pending_tx.params['nonce']} " - f"failed for { pending_tx.retries} attempts; tx will no longer be retried" + f"[retry] transaction #atx-{attempted_tx.id}|{attempted_tx.params['nonce']} " + f"failed for { attempted_tx.retries} attempts; tx will no longer be retried" ) fault_error = TransactionFaulted( - tx=pending_tx, + tx=attempted_tx, fault=Fault.ERROR, message=str(e), ) From e8e704c05371145164b5446f319ce4e3957b68e7 Mon Sep 17 00:00:00 2001 From: derekpierre Date: Fri, 8 Mar 2024 08:27:49 -0500 Subject: [PATCH 60/70] Ensure that hook is given time to be called by using a deferred with the reactor. --- tests/test_tracker.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/test_tracker.py b/tests/test_tracker.py index 586ec4f..1cc3dc4 100644 --- a/tests/test_tracker.py +++ b/tests/test_tracker.py @@ -2,6 +2,8 @@ import pytest from hexbytes import HexBytes +from twisted.internet import reactor +from twisted.internet.task import deferLater from web3.types import TxReceipt from atxm.exceptions import Fault, TransactionFaulted @@ -384,7 +386,11 @@ def test_finalize_active_tx(eip1559_transaction, mocker): assert isinstance(tx, FinalizedTx) assert tx.final is True assert tx.receipt == tx_receipt + + # check hook called + yield deferLater(reactor, 0.2, lambda: None) assert finalized_hook.call_count == 1 + # other hooks not called assert broadcast_hook.call_count == 0 assert broadcast_failure_hook.call_count == 0 From f36fb9dba0b4299bfcf0a1537622702df9c6af3f Mon Sep 17 00:00:00 2001 From: derekpierre Date: Fri, 8 Mar 2024 08:54:27 -0500 Subject: [PATCH 61/70] Don't forget to pop the tx before morphing in tests. Also use inlineCallbacks now that reactor is used. --- tests/test_tracker.py | 61 +++++++++++++++++++++++++++---------------- 1 file changed, 38 insertions(+), 23 deletions(-) diff --git a/tests/test_tracker.py b/tests/test_tracker.py index 1cc3dc4..54abbab 100644 --- a/tests/test_tracker.py +++ b/tests/test_tracker.py @@ -1,6 +1,7 @@ import time import pytest +import pytest_twisted from hexbytes import HexBytes from twisted.internet import reactor from twisted.internet.task import deferLater @@ -11,6 +12,34 @@ from atxm.tx import FaultedTx, FinalizedTx, FutureTx, PendingTx, TxHash +@pytest.fixture +def tx_receipt(): + _tx_receipt = TxReceipt( + { + "blockHash": HexBytes( + "0xf8be31c3eecd1f58432b211e906463b97c3cbfbe60c947c8700dff0ae7348299" + ), + "blockNumber": 1, + "contractAddress": None, + "cumulativeGasUsed": 21000, + "effectiveGasPrice": 1875000000, + "from": "0x1e59ce931B4CFea3fe4B875411e280e173cB7A9C", + "gasUsed": 21000, + "logs": [], + "state_root": b"\x01", + "status": 1, + "to": "0x1e59ce931B4CFea3fe4B875411e280e173cB7A9C", + "transactionHash": HexBytes( + "0x4798799f1cf30337b72381434d3ff56c43ee1fdfa1f812b8262069b7fb2f5a95" + ), + "transactionIndex": 0, + "type": 2, + } + ) + + return _tx_receipt + + def test_queue(eip1559_transaction, legacy_transaction, mocker): tx_tracker = _TxTracker(disk_cache=False) assert len(tx_tracker.queue) == 0 @@ -151,6 +180,7 @@ def test_morph(eip1559_transaction, legacy_transaction, mocker): assert tx_1.id != tx_2.id tx_hash = TxHash("0xdeadbeef") + assert tx_tracker.pop() == tx_1 pending_tx = tx_tracker.morph(tx_1, tx_hash) assert isinstance(pending_tx, PendingTx) @@ -171,6 +201,7 @@ def test_morph(eip1559_transaction, legacy_transaction, mocker): assert tx_tracker.pending is not tx_2 tx_2_hash = TxHash("0xdeadbeef2") + assert tx_tracker.pop() == tx_2 pending_tx_2 = tx_tracker.morph(tx_2, tx_2_hash) assert isinstance(pending_tx_2, PendingTx) assert pending_tx_2 is tx_2, "same underlying object" @@ -209,6 +240,7 @@ def test_fault(eip1559_transaction, legacy_transaction, mocker): tx_tracker.fault(fault_error) tx_hash = TxHash("0xdeadbeef") + assert tx_tracker.pop() == tx pending_tx = tx_tracker.morph(tx, tx_hash) assert tx_tracker.pending.params == tx.params @@ -246,6 +278,7 @@ def test_fault(eip1559_transaction, legacy_transaction, mocker): # repeat with no hook tx_hash_2 = TxHash("0xdeadbeef2") + assert tx_tracker.pop() == tx_2 pending_tx_2 = tx_tracker.morph(tx_2, tx_hash_2) assert tx_tracker.pending.params == tx_2.params @@ -275,6 +308,7 @@ def test_update_after_retry(eip1559_transaction, legacy_transaction, mocker): tx_hash = TxHash("0xdeadbeef") + assert tx_tracker.pop() == tx tx_tracker.morph(tx, tx_hash) assert isinstance(tx, PendingTx) assert tx_tracker.pending.params == tx.params @@ -317,6 +351,7 @@ def test_update_failed_retry_attempt(eip1559_transaction, legacy_transaction, mo tx_tracker.update_failed_retry_attempt(mocker.Mock(spec=PendingTx)) tx_hash = TxHash("0xdeadbeef") + assert tx_tracker.pop() == tx tx_tracker.morph(tx, tx_hash) assert isinstance(tx, PendingTx) pending_tx = tx_tracker.pending @@ -336,7 +371,8 @@ def test_update_failed_retry_attempt(eip1559_transaction, legacy_transaction, mo assert tx_tracker.pending.retries == i -def test_finalize_active_tx(eip1559_transaction, mocker): +@pytest_twisted.inlineCallbacks +def test_finalize_active_tx(eip1559_transaction, mocker, tx_receipt): tx_tracker = _TxTracker(disk_cache=False) broadcast_hook = mocker.Mock() broadcast_failure_hook = mocker.Mock() @@ -351,6 +387,7 @@ def test_finalize_active_tx(eip1559_transaction, mocker): ) tx_hash = TxHash("0xdeadbeef") + assert tx_tracker.pop() == tx tx_tracker.morph(tx, tx_hash) assert isinstance(tx, PendingTx) pending_tx = tx_tracker.pending @@ -359,28 +396,6 @@ def test_finalize_active_tx(eip1559_transaction, mocker): assert len(tx_tracker.finalized) == 0 - tx_receipt = TxReceipt( - { - "blockHash": HexBytes( - "0xf8be31c3eecd1f58432b211e906463b97c3cbfbe60c947c8700dff0ae7348299" - ), - "blockNumber": 1, - "contractAddress": None, - "cumulativeGasUsed": 21000, - "effectiveGasPrice": 1875000000, - "from": "0x1e59ce931B4CFea3fe4B875411e280e173cB7A9C", - "gasUsed": 21000, - "logs": [], - "state_root": b"\x01", - "status": 1, - "to": "0x1e59ce931B4CFea3fe4B875411e280e173cB7A9C", - "transactionHash": HexBytes( - "0x4798799f1cf30337b72381434d3ff56c43ee1fdfa1f812b8262069b7fb2f5a95" - ), - "transactionIndex": 0, - "type": 2, - } - ) tx_tracker.finalize_active_tx(tx_receipt) assert isinstance(tx, FinalizedTx) From 018b0e2b35c2317a19d6ba215c40d8d3193c0405 Mon Sep 17 00:00:00 2001 From: derekpierre Date: Fri, 8 Mar 2024 10:54:24 -0500 Subject: [PATCH 62/70] Remove the storage of txdata from PendingTx; it isn't used at all. --- atxm/tx.py | 5 +---- atxm/utils.py | 1 - 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/atxm/tx.py b/atxm/tx.py index 174ff0a..ac2c618 100644 --- a/atxm/tx.py +++ b/atxm/tx.py @@ -5,7 +5,7 @@ from eth_typing import ChecksumAddress from eth_utils import encode_hex from hexbytes import HexBytes -from web3.types import TxData, TxParams, TxReceipt +from web3.types import TxParams, TxReceipt from atxm.exceptions import Fault @@ -85,7 +85,6 @@ class PendingTx(AsyncTx): final: bool = field(default=False, init=False) txhash: TxHash created: int - data: Optional[TxData] = None def __hash__(self) -> int: return hash(self.txhash) @@ -95,7 +94,6 @@ def to_dict(self) -> Dict: "id": self.id, "txhash": self.txhash.hex(), "created": self.created, - "data": self.data, } @classmethod @@ -104,7 +102,6 @@ def from_dict(cls, data: Dict): id=int(data["id"]), txhash=HexBytes(data["txhash"]), created=int(data["created"]), - data=dict(data) if data else dict(), ) diff --git a/atxm/utils.py b/atxm/utils.py index 1159d12..e43e567 100644 --- a/atxm/utils.py +++ b/atxm/utils.py @@ -62,7 +62,6 @@ def _get_receipt(w3: Web3, pending_tx: PendingTx) -> Optional[TxReceipt]: """ try: txdata = w3.eth.get_transaction(pending_tx.txhash) - pending_tx.data = txdata except TransactionNotFound: log.error(f"[error] Transaction {pending_tx.txhash.hex()} not found") return From a0f2c748266412832ad9e6d64dbdf9500f0032b7 Mon Sep 17 00:00:00 2001 From: derekpierre Date: Fri, 8 Mar 2024 10:55:28 -0500 Subject: [PATCH 63/70] Fix bug in restoring queue values for _TxTracker. --- atxm/tracker.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/atxm/tracker.py b/atxm/tracker.py index 654872f..e5e018a 100644 --- a/atxm/tracker.py +++ b/atxm/tracker.py @@ -78,7 +78,7 @@ def restore(self) -> bool: # deserialize & restore self.__active = PendingTx.from_dict(active) if active else None - self.__queue.extend(FutureTx.from_dict(tx) for tx in queue) + self.__queue.extend([FutureTx.from_dict(tx) for tx in queue]) self.finalized = {FinalizedTx.from_dict(tx) for tx in final} log.debug( f"[tracker] restored {len(queue)} transactions from cache file {self.__filepath}" From 1914e6bc1bc55c440ae2ee545e9b86be975cf56d Mon Sep 17 00:00:00 2001 From: derekpierre Date: Fri, 8 Mar 2024 11:19:26 -0500 Subject: [PATCH 64/70] Move params out of AsyncTx and into FutureTx and PendingTx, the other tx types (FinalizedTx/FaultedTx) don't care typified by their serializations. Force subclasses of AsyncTx to implement their own __eq__ and __hash__ methods; added implementations to each subclass. --- atxm/tx.py | 54 +++++++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 45 insertions(+), 9 deletions(-) diff --git a/atxm/tx.py b/atxm/tx.py index ac2c618..8af9168 100644 --- a/atxm/tx.py +++ b/atxm/tx.py @@ -1,3 +1,4 @@ +import json from abc import ABC, abstractmethod from dataclasses import dataclass, field from typing import Callable, Dict, Optional @@ -15,7 +16,6 @@ @dataclass class AsyncTx(ABC): id: int - params: TxParams final: bool = field(default=None, init=False) fault: Optional[Fault] = field(default=None, init=False) on_broadcast: Optional[Callable[["PendingTx"], None]] = field( @@ -32,11 +32,13 @@ class AsyncTx(ABC): def __repr__(self): return f"<{self.__class__.__name__} id={self.id} final={self.final}>" + @abstractmethod def __hash__(self): - return hash(self.id) + raise NotImplementedError + @abstractmethod def __eq__(self, other): - return self.id == other.id + raise NotImplementedError def __ne__(self, other): return not self.__eq__(other) @@ -52,12 +54,20 @@ def from_dict(cls, data: Dict): @dataclass class FutureTx(AsyncTx): + params: TxParams + info: Optional[Dict] = None requeues: int = field(default=0, init=False) final: bool = field(default=False, init=False) - info: Optional[Dict] = None def __hash__(self): - return hash(self.id) + return hash(json.dumps(self.to_dict())) + + def __eq__(self, other): + return ( + self.id == other.id + and self.params == other.params + and self.info == other.info + ) @property def _from(self) -> ChecksumAddress: @@ -75,7 +85,7 @@ def from_dict(cls, data: Dict): return cls( id=int(data["id"]), params=TxParams(data["params"]), - info=dict(data["info"]), + info=dict(data["info"]) if data["info"] else None, ) @@ -83,15 +93,27 @@ def from_dict(cls, data: Dict): class PendingTx(AsyncTx): retries: int = field(default=0, init=False) final: bool = field(default=False, init=False) + params: TxParams txhash: TxHash created: int def __hash__(self) -> int: - return hash(self.txhash) + # don't use "params" or "txhash" for hash because they can change + # over the object's lifetime + return hash((self.id, self.created)) + + def __eq__(self, other): + return ( + self.id == other.id + and self.params == other.params + and self.txhash == other.txhash + and self.created == other.created + ) def to_dict(self) -> Dict: return { "id": self.id, + "params": _serialize_tx_params(self.params), "txhash": self.txhash.hex(), "created": self.created, } @@ -100,6 +122,7 @@ def to_dict(self) -> Dict: def from_dict(cls, data: Dict): return cls( id=int(data["id"]), + params=TxParams(data["params"]), txhash=HexBytes(data["txhash"]), created=int(data["created"]), ) @@ -111,7 +134,13 @@ class FinalizedTx(AsyncTx): receipt: TxReceipt def __hash__(self) -> int: - return hash(self.receipt["transactionHash"]) + return hash((self.id, self.receipt["transactionHash"])) + + def __eq__(self, other): + return ( + self.id == other.id + and self.receipt["transactionHash"] == other.receipt["transactionHash"] + ) def to_dict(self) -> Dict: return {"id": self.id, "receipt": _serialize_tx_receipt(self.receipt)} @@ -133,7 +162,14 @@ class FaultedTx(AsyncTx): error: Optional[str] = None def __hash__(self) -> int: - return hash(self.id) + return hash(json.dumps(self.to_dict())) + + def __eq__(self, other): + return ( + self.id == other.id + and self.fault == other.fault + and self.error == other.error + ) def to_dict(self) -> Dict: return {"id": self.id, "error": str(self.error), "fault": self.fault.value} From 7185531d61ed20a7191e01c63492271dc9801533 Mon Sep 17 00:00:00 2001 From: derekpierre Date: Fri, 8 Mar 2024 11:32:04 -0500 Subject: [PATCH 65/70] TxParams which has Union data types can take different forms so to ensure consistency with equality check, use serialized versions for comparison. TxReceipt on FinalizedTx we only care about specific values; extra values get lost during serialization/deserialization. So to ensure consistency with equality check, use serialized versions for comparison. --- atxm/tx.py | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/atxm/tx.py b/atxm/tx.py index 8af9168..660f598 100644 --- a/atxm/tx.py +++ b/atxm/tx.py @@ -65,7 +65,7 @@ def __hash__(self): def __eq__(self, other): return ( self.id == other.id - and self.params == other.params + and _serialize_tx_params(self.params) == _serialize_tx_params(other.params) and self.info == other.info ) @@ -105,7 +105,7 @@ def __hash__(self) -> int: def __eq__(self, other): return ( self.id == other.id - and self.params == other.params + and _serialize_tx_params(self.params) == _serialize_tx_params(other.params) and self.txhash == other.txhash and self.created == other.created ) @@ -137,10 +137,9 @@ def __hash__(self) -> int: return hash((self.id, self.receipt["transactionHash"])) def __eq__(self, other): - return ( - self.id == other.id - and self.receipt["transactionHash"] == other.receipt["transactionHash"] - ) + return self.id == other.id and _serialize_tx_receipt( + self.receipt + ) == _serialize_tx_receipt(other.receipt) def to_dict(self) -> Dict: return {"id": self.id, "receipt": _serialize_tx_receipt(self.receipt)} From faa51ed4b294988f8f32c7f1cd2350f11611eca3 Mon Sep 17 00:00:00 2001 From: derekpierre Date: Fri, 8 Mar 2024 11:34:30 -0500 Subject: [PATCH 66/70] Fix tests that used hook to wait using reactor, and enable inlineCallbacks. --- tests/test_tracker.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/test_tracker.py b/tests/test_tracker.py index 54abbab..1496095 100644 --- a/tests/test_tracker.py +++ b/tests/test_tracker.py @@ -214,6 +214,7 @@ def test_morph(eip1559_transaction, legacy_transaction, mocker): ), "copy of object always returned" +@pytest_twisted.inlineCallbacks def test_fault(eip1559_transaction, legacy_transaction, mocker): tx_tracker = _TxTracker(disk_cache=False) broadcast_hook = mocker.Mock() @@ -265,6 +266,7 @@ def test_fault(eip1559_transaction, legacy_transaction, mocker): assert tx.error == fault_message # check that fault hook was called + yield deferLater(reactor, 0.2, lambda: None) assert fault_hook.call_count == 1 fault_hook.assert_called_with(tx) @@ -405,6 +407,7 @@ def test_finalize_active_tx(eip1559_transaction, mocker, tx_receipt): # check hook called yield deferLater(reactor, 0.2, lambda: None) assert finalized_hook.call_count == 1 + finalized_hook.assert_called_with(tx) # other hooks not called assert broadcast_hook.call_count == 0 From 084e1e2a880a275e556b39dc2f4b98b8a4f35401 Mon Sep 17 00:00:00 2001 From: derekpierre Date: Fri, 8 Mar 2024 11:37:52 -0500 Subject: [PATCH 67/70] Add tests for commit/restore functionality for _TxTracker. --- tests/conftest.py | 42 +++++++++++++++ tests/test_tracker.py | 117 ++++++++++++++++++++++++++++++------------ 2 files changed, 127 insertions(+), 32 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index b492db4..0ec0cfd 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,12 +1,17 @@ +import os import sys +import tempfile +from pathlib import Path from typing import List, Tuple import pytest from eth_account import Account from eth_tester import EthereumTester +from hexbytes import HexBytes from statemachine import State from twisted.internet.task import Clock from twisted.logger import globalLogPublisher, textFileLogObserver +from web3.types import TxReceipt from atxm import AutomaticTxMachine from atxm.logging import log @@ -124,3 +129,40 @@ def state_observer(machine): machine.add_observer(_observer) return _observer + + +@pytest.fixture +def tx_receipt(): + _tx_receipt = TxReceipt( + { + "blockHash": HexBytes( + "0xf8be31c3eecd1f58432b211e906463b97c3cbfbe60c947c8700dff0ae7348299" + ), + "blockNumber": 1, + "contractAddress": None, + "cumulativeGasUsed": 21000, + "effectiveGasPrice": 1875000000, + "from": "0x1e59ce931B4CFea3fe4B875411e280e173cB7A9C", + "gasUsed": 21000, + "logs": [], + "root": "0x01", + "status": 1, + "to": "0x1e59ce931B4CFea3fe4B875411e280e173cB7A9C", + "transactionHash": HexBytes( + "0x4798799f1cf30337b72381434d3ff56c43ee1fdfa1f812b8262069b7fb2f5a95" + ), + "transactionIndex": 0, + "type": 2, + } + ) + + return _tx_receipt + + +@pytest.fixture +def tempfile_path(): + fd, path = tempfile.mkstemp() + path = Path(path) + yield path + os.close(fd) + path.unlink() diff --git a/tests/test_tracker.py b/tests/test_tracker.py index 1496095..2d32dd4 100644 --- a/tests/test_tracker.py +++ b/tests/test_tracker.py @@ -2,44 +2,14 @@ import pytest import pytest_twisted -from hexbytes import HexBytes from twisted.internet import reactor from twisted.internet.task import deferLater -from web3.types import TxReceipt from atxm.exceptions import Fault, TransactionFaulted from atxm.tracker import _TxTracker from atxm.tx import FaultedTx, FinalizedTx, FutureTx, PendingTx, TxHash -@pytest.fixture -def tx_receipt(): - _tx_receipt = TxReceipt( - { - "blockHash": HexBytes( - "0xf8be31c3eecd1f58432b211e906463b97c3cbfbe60c947c8700dff0ae7348299" - ), - "blockNumber": 1, - "contractAddress": None, - "cumulativeGasUsed": 21000, - "effectiveGasPrice": 1875000000, - "from": "0x1e59ce931B4CFea3fe4B875411e280e173cB7A9C", - "gasUsed": 21000, - "logs": [], - "state_root": b"\x01", - "status": 1, - "to": "0x1e59ce931B4CFea3fe4B875411e280e173cB7A9C", - "transactionHash": HexBytes( - "0x4798799f1cf30337b72381434d3ff56c43ee1fdfa1f812b8262069b7fb2f5a95" - ), - "transactionIndex": 0, - "type": 2, - } - ) - - return _tx_receipt - - def test_queue(eip1559_transaction, legacy_transaction, mocker): tx_tracker = _TxTracker(disk_cache=False) assert len(tx_tracker.queue) == 0 @@ -418,5 +388,88 @@ def test_finalize_active_tx(eip1559_transaction, mocker, tx_receipt): assert tx_tracker.pending is None assert len(tx_tracker.finalized) == 1 - for t in tx_tracker.finalized: - assert t == tx + assert tx in tx_tracker.finalized + + +def test_commit_restore( + eip1559_transaction, legacy_transaction, tx_receipt, tempfile_path +): + tx_tracker = _TxTracker(disk_cache=True, filepath=tempfile_path) + + # check commit restore + tx_tracker.commit() + restored_tracker = _TxTracker(disk_cache=True, filepath=tempfile_path) + _compare_trackers(tx_tracker, restored_tracker) + + tx_1 = tx_tracker.queue_tx(params=eip1559_transaction, info={"name": "tx_1"}) + tx_2 = tx_tracker.queue_tx(params=legacy_transaction) + tx_3 = tx_tracker.queue_tx(params=eip1559_transaction, info={"name": "tx_3"}) + tx_4 = tx_tracker.queue_tx(params=legacy_transaction) + tx_5 = tx_tracker.queue_tx(params=eip1559_transaction, info={"name": "tx_5"}) + tx_6 = tx_tracker.queue_tx(params=legacy_transaction) + + # max tx_1 finalized + tx_hash = TxHash("0xdeadbeef") + assert tx_tracker.pop() == tx_1 + tx_tracker.morph(tx_1, tx_hash) + + tx_tracker.finalize_active_tx(tx_receipt) + assert len(tx_tracker.finalized) == 1 + + # check commit restore + tx_tracker.commit() + restored_tracker = _TxTracker(disk_cache=True, filepath=tempfile_path) + _compare_trackers(tx_tracker, restored_tracker) + + # make tx_2 finalized + tx_hash_2 = TxHash("0xdeadbeef2") + assert tx_tracker.pop() == tx_2 + tx_tracker.morph(tx_2, tx_hash_2) + + tx_tracker.finalize_active_tx(tx_receipt) + assert len(tx_tracker.finalized) == 2 + assert tx_1 in tx_tracker.finalized + assert tx_2 in tx_tracker.finalized + + # check commit restore + tx_tracker.commit() + restored_tracker = _TxTracker(disk_cache=True, filepath=tempfile_path) + _compare_trackers(tx_tracker, restored_tracker) + + # make tx_3 active + tx_hash_3 = TxHash("0xdeadbeef3") + assert tx_tracker.pop() == tx_3 + tx_tracker.morph(tx_3, tx_hash_3) + assert tx_tracker.pending == tx_3 + + # check commit restore + tx_tracker.commit() + restored_tracker = _TxTracker(disk_cache=True, filepath=tempfile_path) + _compare_trackers(tx_tracker, restored_tracker) + + # tx_4,5,6 still queued + assert len(tx_tracker.queue) == 3 + assert tx_4 in tx_tracker.queue + assert tx_5 in tx_tracker.queue + assert tx_6 in tx_tracker.queue + + # check commit restore + tx_tracker.commit() + restored_tracker = _TxTracker(disk_cache=True, filepath=tempfile_path) + _compare_trackers(tx_tracker, restored_tracker) + + +def _compare_trackers(tracker_1: _TxTracker, tracker_2: _TxTracker): + # compare individual tx objects at each stage since restore doesn't care about + # prior state value eg. FinalizedTx doesn't care about params, because the tx was finalized + assert len(tracker_1.queue) == len(tracker_2.queue) + for i, tx in enumerate(list(tracker_1.queue)): + assert tx == tracker_2.queue[i] + + assert tracker_1.pending == tracker_2.pending + if tracker_1.pending: + assert tracker_1.pending.id == tracker_2.pending.id + + assert len(tracker_1.finalized) == len(tracker_2.finalized) + for tx in tracker_1.finalized: + assert tx in tracker_2.finalized From 2b52a48f294d2440b92e34b149e0f477bc8e1203 Mon Sep 17 00:00:00 2001 From: derekpierre Date: Fri, 8 Mar 2024 12:04:40 -0500 Subject: [PATCH 68/70] Ensure that hash comparisons of txs are also made as part of tests. --- tests/test_tracker.py | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/tests/test_tracker.py b/tests/test_tracker.py index 2d32dd4..0eeb9e1 100644 --- a/tests/test_tracker.py +++ b/tests/test_tracker.py @@ -460,16 +460,21 @@ def test_commit_restore( def _compare_trackers(tracker_1: _TxTracker, tracker_2: _TxTracker): - # compare individual tx objects at each stage since restore doesn't care about - # prior state value eg. FinalizedTx doesn't care about params, because the tx was finalized + # 1. check FutureTxs assert len(tracker_1.queue) == len(tracker_2.queue) for i, tx in enumerate(list(tracker_1.queue)): assert tx == tracker_2.queue[i] + # ensure __hash__ is tested + assert hash(tx) == hash(tracker_2.queue[i]) + # 2. check PendingTx assert tracker_1.pending == tracker_2.pending if tracker_1.pending: - assert tracker_1.pending.id == tracker_2.pending.id + # ensure __hash__ is tested + assert hash(tracker_2.pending) == hash(tracker_1.pending) + # 3. check FinalizedTxs assert len(tracker_1.finalized) == len(tracker_2.finalized) + # finalized already in a set for tx in tracker_1.finalized: assert tx in tracker_2.finalized From e1f312eab8d72183823e7967f69ee9d4be9eb91b Mon Sep 17 00:00:00 2001 From: derekpierre Date: Fri, 8 Mar 2024 12:08:06 -0500 Subject: [PATCH 69/70] Add serialization/deserialization test for FaultedTx. May/may not really be needed since it isn't tracked by the tracker. In any case because we sub-class from AsyncTx we test it anyway since implemented. --- tests/test_tracker.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/tests/test_tracker.py b/tests/test_tracker.py index 0eeb9e1..aa04201 100644 --- a/tests/test_tracker.py +++ b/tests/test_tracker.py @@ -248,6 +248,11 @@ def test_fault(eip1559_transaction, legacy_transaction, mocker): # no active tx assert tx_tracker.pending is None + # serialization/deserialization of FaultedTx - not tracked so not done anywhere else + deserialized_fault_tx = FaultedTx.from_dict(tx.to_dict()) + assert deserialized_fault_tx == tx + assert hash(deserialized_fault_tx) == hash(tx) + # repeat with no hook tx_hash_2 = TxHash("0xdeadbeef2") assert tx_tracker.pop() == tx_2 @@ -268,6 +273,14 @@ def test_fault(eip1559_transaction, legacy_transaction, mocker): # no active tx assert tx_tracker.pending is None + # serialization/deserialization of FaultedTx - not tracked so not done anywhere else + deserialized_fault_tx_2 = FaultedTx.from_dict(tx_2.to_dict()) + assert deserialized_fault_tx_2 == tx_2 + assert hash(deserialized_fault_tx_2) == hash(tx_2) + + assert tx_2 != tx + assert hash(tx_2) != hash(tx) + def test_update_after_retry(eip1559_transaction, legacy_transaction, mocker): tx_tracker = _TxTracker(disk_cache=False) From 3245c0fff0813e1070e9eef572c170650e43e444 Mon Sep 17 00:00:00 2001 From: derekpierre Date: Fri, 8 Mar 2024 12:11:35 -0500 Subject: [PATCH 70/70] Add test about trying to perform finalize when there isn't an existing active tx. --- tests/test_tracker.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/test_tracker.py b/tests/test_tracker.py index aa04201..1454d74 100644 --- a/tests/test_tracker.py +++ b/tests/test_tracker.py @@ -359,6 +359,11 @@ def test_update_failed_retry_attempt(eip1559_transaction, legacy_transaction, mo @pytest_twisted.inlineCallbacks def test_finalize_active_tx(eip1559_transaction, mocker, tx_receipt): tx_tracker = _TxTracker(disk_cache=False) + + with pytest.raises(RuntimeError, match="No pending transaction to finalize"): + # there is no active tx + tx_tracker.finalize_active_tx(mocker.Mock()) + broadcast_hook = mocker.Mock() broadcast_failure_hook = mocker.Mock() fault_hook = mocker.Mock()