Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

A race condition sometimes causes a transaction to have an invalid pair of maxFeePerGas/maxPriorityFeePerGas #3052

Open
evgeny-osipenko opened this issue Jul 19, 2023 · 1 comment

Comments

@evgeny-osipenko
Copy link

When Web3 is asked to construct a transaction without gas fees set explicitly, the values of the maxFeePerGas and maxPriorityFeePerGas fields are filled as part of the defaulting process, governed by this code:

TRANSACTION_DEFAULTS = {
"value": 0,
"data": b"",
"gas": lambda w3, tx: w3.eth.estimate_gas(tx),
"gasPrice": lambda w3, tx: w3.eth.generate_gas_price(tx),
"maxFeePerGas": (
lambda w3, tx: w3.eth.max_priority_fee
+ (2 * w3.eth.get_block("latest")["baseFeePerGas"])
),
"maxPriorityFeePerGas": lambda w3, tx: w3.eth.max_priority_fee,
"chainId": lambda w3, tx: w3.eth.chain_id,
}

@curry
def fill_transaction_defaults(w3: "Web3", transaction: TxParams) -> TxParams:
"""
if w3 is None, fill as much as possible while offline
"""
strategy_based_gas_price = w3.eth.generate_gas_price(transaction)
is_dynamic_fee_transaction = strategy_based_gas_price is None and (
"gasPrice" not in transaction # default to dynamic fee transaction
or any_in_dict(DYNAMIC_FEE_TXN_PARAMS, transaction)
)
defaults = {}
for key, default_getter in TRANSACTION_DEFAULTS.items():
if key not in transaction:
if (
is_dynamic_fee_transaction
and key == "gasPrice"
or not is_dynamic_fee_transaction
and key in DYNAMIC_FEE_TXN_PARAMS
):
# do not set default max fees if legacy txn or
# gas price if dynamic fee txn
continue
if callable(default_getter):
if w3 is None:
raise ValueError(
f"You must specify a '{key}' value in the transaction"
)
default_val = default_getter(w3, transaction)
else:
default_val = default_getter
defaults[key] = default_val
return merge(defaults, transaction)

If a new block is created right as the defaulting process takes place, then w3.eth.max_priority_fee will return different values for different fields of the same transaction. This can cause the transaction to end up with tx['maxFeePerGas'] < tx['maxPriorityFeePerGas'], which is invalid and will cause it to fail further down the line with something like

{ "jsonrpc": "2.0"
, "id": 1383832
, "error":
    { "code": -32000
    , "message": "max priority fee per gas higher than max fee per gas"
    }
}

As for the fix, I propose to change the behavior like this:

  1. Swap the order of the fields in the dictionary, so that "maxPriorityFeePerGas" goes before "maxFeePerGas ". Since Python dictionaries remember the insertion order of their keys and respect it during iteration, changing their order in the TRANSACTION_DEFAULTS map will correspondingly change the order the initializers are actually run.
  2. In the loop, pass the defaults accumulator to the initializers together with w3 and the initial transaction. Again, since Python guarantees to iterate over maps in the key insertion order, it's perfectly fine to carry state in-between iterations and rely on some actions being done before others.
  3. In the "maxFeePerGas" initializer, instead of asking for w3.eth.max_priority_fee again, use tx.get('maxPriorityFeePerGas', defaults['maxPriorityFeePerGas']).

So, with these considerations, the fix in code would look somewhat like this:

# >>> add 3rd parameter `defs` to the lambda
TRANSACTION_DEFAULTS = {
    "value": 0,
    "data": b"",
    "gas": lambda w3, tx, defs: w3.eth.estimate_gas(tx),
    "gasPrice": lambda w3, tx, defs: w3.eth.generate_gas_price(tx),
# >>> swap the order of `maxFeePerGas` and `maxPriorityFeePerGas`
    "maxPriorityFeePerGas": lambda w3, tx, defs: w3.eth.max_priority_fee,
    # maxFeePerGas depends on maxPriorityFeePerGas having already run
    "maxFeePerGas": lambda w3, tx, defs: (
        tx.get("maxPriorityFeePerGas", defs["maxPriorityFeePerGas"])
        + (2 * w3.eth.get_block("latest")["baseFeePerGas"])
    ),
    "chainId": lambda w3, tx: w3.eth.chain_id,
}

<...>

@curry
def fill_transaction_defaults(w3: "Web3", transaction: TxParams) -> TxParams:
    """
    if w3 is None, fill as much as possible while offline
    """
    strategy_based_gas_price = w3.eth.generate_gas_price(transaction)
    is_dynamic_fee_transaction = strategy_based_gas_price is None and (
        "gasPrice" not in transaction  # default to dynamic fee transaction
        or any_in_dict(DYNAMIC_FEE_TXN_PARAMS, transaction)
    )

    defaults = {}
    for key, default_getter in TRANSACTION_DEFAULTS.items():
        if key not in transaction:
            if (
                is_dynamic_fee_transaction
                and key == "gasPrice"
                or not is_dynamic_fee_transaction
                and key in DYNAMIC_FEE_TXN_PARAMS
            ):
                # do not set default max fees if legacy txn or
                # gas price if dynamic fee txn
                continue

            if callable(default_getter):
                if w3 is None:
                    raise ValueError(
                        f"You must specify a '{key}' value in the transaction"
                    )
# >>> pass current `defaults` to the getter
                default_val = default_getter(w3, transaction, defaults)
            else:
                default_val = default_getter

            defaults[key] = default_val
    return merge(defaults, transaction)
@evgeny-osipenko evgeny-osipenko changed the title A race condition sometimes causes a transaction to have invalid pair of maxFeePerGas/maxPriorityFeePerGas A race condition sometimes causes a transaction to have an invalid pair of maxFeePerGas/maxPriorityFeePerGas Jul 19, 2023
@evgeny-osipenko
Copy link
Author

On the probability and severity of this bug: we actually encountered it in the wild when working with the Polygon Mumbai testnet. It has two important properties with regards to this bug:

  1. New blocks are created relatively often: currently, the average block time on Mumbai is 4 seconds.
  2. Specifically on Mumbai, the gas fees are heavily dominated by the priority fee. The base fee here usually stays in the range of 16..20 wei, while the priority fee starts at 1'000'000'000 wei and can jump several times up from there. So, if the priority max and the total max are calculated at different blocks, it's entirely possible to end up with the maxFeePerGas much less than the maxPriorityFeePerGas.
    The failed transaction that we actually got, as an example, had maxFeePerGas of 2.25 Gwei versus maxPriorityFeePerGas of 30 Gwei.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants