make EstimateGas bail early when a tx reverts for any reason other than Out of Gas

[roberto-bayardo]

EstimateGas currently conducts a binary search to determine a suitable gas limit for a tx, and continues searching with a higher gas limit whenever the transaction's execution results in revert. While this makes sense if the reason for revert is out of gas, increasing the gas limit is highly unlikely to allow a non-OOG reverting transaction to succeed (unless it is making unusual use of the GAS opcode).

This PR makes EstimateGas return immediately when a transaction reverts for any reason other than out of gas. This has two benefits:

1. Saves compute overhead.

2. Eliminates a race condition where a reverting transaction causes the gas limit lowerbound to increase repeatedly during gas estimation, only for a state change to allow the transaction to later succeed later in the loop, resulting in an unreasonably high estimate being returned. (We believe this is happening in practice in one of our applications)

[joohhnnn]

In the case where the error is not due to insufficient gas, it will return an error. Here is the code snippet:

if err != nil {
	if errors.Is(err, core.ErrIntrinsicGas) {
		return true, nil, nil // Special case, raise gas limit
	}
	return true, nil, err // Bail out
}

Additionally, when an error is encountered, it will immediately return. Here is the code snippet:

if err != nil {
	return 0, err
}

Therefore, I believe the functionality you mentioned has already been implemented.

[roberto-bayardo]

In the case where the error is not due to insufficient gas, it will return an error. Here is the code snippet:

My concern is not errors (which as you note are handled correctly) but transactions reverting. This case does not appear to result in an error being returned from DoCall(), but rather a result being returned with a non.nil Err field. At least that is what is implied by the implementation of Call() that wraps DoCall() to convert tx revert outcomes into errors (but is not called here):

	result,_ err := DoCall(...)
	if err != nil {
		return nil, err
	}
	// If the result contains a revert reason, try to unpack and return it.
	if len(result.Revert()) > 0 {
		return nil, newRevertError(result)
	}
	return result.Return(), result.Err

[roberto-bayardo]

@mdehoog has noted that because of the GAS opcode, transactions could revert for non-out of gas reasons based on gas remaining, therefore one might need to continue the binary search in such cases to get an accurate gaslimit.

Another fix would be to lock the VM during the entire binary search, but that would result in one hell of a blocking call.

[jwasinger]

If I understand correctly, the racey behavior you're describing is caused by Geth retrieving the state at each iteration of the loop inside estimate gas. If the pending/latest state is used, this could change mid-call causing strange results.

I opened a PR to address that issue.

[roberto-bayardo]

If I understand correctly, the racey behavior you're describing is caused by Geth retrieving the state at each iteration of the loop inside estimate gas. If the pending/latest state is used, this could change mid-call causing strange results.

I opened a PR to address that issue.

yes, brilliant!

[roberto-bayardo]

Closing in favor of @jwasinger's fix above.