fix(providers): Propogate gas limit with access list

[wolflo]

Motivation

Previously, Provider's fill_transaction() would override the gas price set by the caller if using an access list changed the gas estimate. This led to transactions consistently running out of gas with no way to increase the gas limit.

Solution

Update the transaction gas limit only if the caller did not set it. If including an access list will decrease the gas limit, we use the access list but leave the caller-supplied gas limit.

This may lead to over-estimating the gas limit, but it should never cause a transaction that would have otherwise succeeded to run out of gas, because we only include the access list if:

1. It decreases the estimated gas used
2. eth_estimateGas fails and the caller has not specified a gas limit

PR Checklist

• Added Tests
• Added Documentation X - no api changes
• Updated the changelog

[gakonst]

nice work - are you sure it's correctly implemented? the test seems to indicate otherwise?

[gakonst]

If this worked correctly, wouldn't this be gas_with_al?

[wolflo]

See below, but my thought is that in this case we want the access list because it saves us gas, but we want to leave the gas limit untouched because we don't want to override any values set upstream.

[wolflo]

Yeah, the code is a bit convoluted in the interesting of minimizing rpc calls. Not resetting the gas even if we include the access list is actually the big change I was going for, as this means you can't manually set the gas limit when the estimated gas with an access list is inacurrate.

For clarity, here are some of the properties I was going for. Whether these properties are actually desirable is worth a discussion.

• We never override an access list or a gas limit that was set upstream (whether by the user or an outer middleware).
  • This is the big change from the current behavior, where the gas limit is overriden if we attempt to populate the access list and the access list saves gas.
  • For gas limit, Some(_) indicates that it was previously set.
  • For the access list, Some(_) indicates that the transaction has an access list (i.e. is an Eip2930 or Eip1559 transaction). Some(AccessList( vec![] )) indicates that the transaction has an access list but it has not yet been populated.
  • There is a case where the access list may have been explicitly set to the empty access list. If we want to support this we probably need to add a separate has_access_list() method and use tx.access_list().is_none() to indicate that it has not yet been set.
• We recover from any rpc errors as long as we can still create a transaction that makes sense.
  • If eth_estimateGas fails, but we need to create an access list anyway, we use the gas with access list
    • Failure of eth_estimateGas might not actually be something we want to recover from, since it could indicate a problem with the transaction. Should we reconsider this?
  • If eth_createAccessList fails, we ignore the access list as long as the gas was either set upstream or eth_estimateGas succeeds.
• If the gas limit is set already, we never request eth_estimateGas.
  • We may want to compare the gas required with an access list to the result of eth_estimateGas rather than to the gas set upstream. It's possible the gas set upstream is an overestimation, and including an access list results in a gas limit lower than what's set upstream but higher than eth_estimateGas, in which case we don't actually want to include the access list, even if we aren't changing the gas limit. But, this means an extra rpc call in this case.

[meetmangukiya]

We never override an access list or a gas limit that was set upstream (whether by the user or an outer middleware).

I think this is reasonable. Ideally we should split out the filling operations out in their separate functions on a builder to give the maximum composability. If the user doesn't care to customise / override any of the behaviour, we go with what makes most sense for generic cases in fill_transaction. If the user wants to customise, they should not use fill_transaction, rather call fill_access_lists, fill_gas, fill_nonce, etc. separately and it will be entirely up to the user to fill everything. Thoughts? @gakonst @wolflo

• Failure of eth_estimateGas might not actually be something we want to recover from, since it could indicate a problem with the transaction. Should we reconsider this?

I agree, this shouldn't be ignored, leave it up to the user to deal with it. estimateGas failure saves many failed txs imo. I might also add that maybe try estimating gas with from set, some of the access controlled txs will fail with no from set.

[wolflo]

I like the idea of separate fill methods. fill_transaction() will always be called by the innermost provider, so to support these separate fill methods sticking to the strategy of not replacing any previously set values should be sufficient. Another question is where these special fill methods exist. We could add them to the middleware trait itself (probably unnecessary clutter), use a new trait, or just leave them as an inherent impl on the Provider struct and allow users to implement them for their own middleware if they have more specific needs.

I think propogating failure of eth_estimateGas makes sense. I'm in favor of maintaining the current behavior of swallowing failures of eth_createAccessList, though, since this commonly fails if your rpc endpoint doesn't support it. Should we call eth_estimateGas to ensure its success even if we know we won't use the result, e.g. if the gas limit was set upstream?

Also a great point about setting transaction values before estimating gas. We should probably do the same with gas price.

[wolflo]

Thinking about this more, an important property fill_transaction() should have in my mind is that for any combination of transaction inputs, it should be possible to prevent fill_transaction() from making any rpc calls at all.

That probably means not calling eth_estimateGas if the gas limit is already set. This also allows "forcing" a transaction to be sent even if eth_estimateGas fails, which is sometimes need. The other change needed to achieve this is allowing an empty access list to be considered "set", rather than having the empty access list represent a transaction which can contain an access list but does not yet.

[gakonst]

@wolflo what do you suggest as next steps to getting this PR over the line? I think we def want to remove recovering from eth_estimateGas

[wolflo]

@gakonst Currently it only recovers from failure of eth_estimateGas if either:

1. the gas limit is set upstream of the provider
2. eth_createAccessList succeeds

I see some advantages to maintaining the behavior in one or both of these cases:

1. This allows forcing a transaction to be sent even if eth_estimateGas fails for some reason. For instance, you may want to send a failing transaction for some reason. This one seems less necessary than the below case.
2. There are some transactions that require an access list to succeed (see the motivation for EIP-2930). For these, eth_estimateGas will always fail, but eth_createAccessList will succeed and should give us the right gas estimate to use.

I'm happy to update to whatever behavior sounds most intuitive, though. Other than that, I will do a pass and make sure we fill as much info as we can before estimating gas. The separate fill_nonce, fill_gas, etc. methods @meetmangukiya suggested probably make the most sense as a separate PR.

[wolflo]

@gakonst This should be ready to merge, with the caveat that the behavior around eth_estimateGas is still as described above. If different behavior is preferred, I can update that as well.

[gakonst]

Hey sorry missed the message above (please ping me if you see me being non-responsive!). To get this over the line I'd really want us to make transactions return an error if eth_estimateGas fails, as mentioned above

Motivation being: I've never needed to send a failing transaction and EIP-2930-only transactions are few enough that shouldn't create a behavior change requirement

[wolflo]

@gakonst Updated to propogate any failure of eth_estimateGas.

But, it still doesn't call self.estimate_gas() if the gas has been set upstream. Is there a reason to try to estimate in this case just to see if eth_estimateGas fails?

Edit: Also worth noting that we still swallow failures of eth_createAccessList as long as we can still get a gas limit from somewhere (eth_estimateGas or set upstream). I have found eth_createAccessList to be unreliable at best, especially with test rpc endpoints, so I'm in favor of keeping this behavior.

[gakonst]

SGTM. Nice work. Let's hope this fixes ppl's issues!