fix: handle revert errors in gas estimation for executable transactions

[afa7789]

I added a result.Revert() check and returned it if that's the error when we find the location amidst the binary search

[IvanBelyakoff]

@afa7789 great job! Curious why you removed the check for error being vm.ErrExecutionReverted and treat all errors here as revert errors. Could you please elaborate on that?

[afa7789]

this is almost the same piece of code we had in the ending of the function.

something could happen before we wouldn't know that it revert/failed, so now we first test the code for reverts/failures with max gas, and then look for gas estimation.

so any type of errors/reverts we try to get that first, and then we look for the estimate gas.

[IvanBelyakoff]

These lines returned revert error in case it was a revert error, otherwise, just an error:

				if len(result.Revert()) > 0 {
					return 0, ethapi2.NewRevertError(result)
				}
				return 0, result.Err

This is what I am talking about, now all errors here are revert errors. Idk if that is correct, I'd leave the old way.
Next, code error is left as is:

		if err != nil {
			return 0, err
		}

Which means we still return it the same old way it was before, I dont see any change here, if it fails. If there is no extra reason in the err, then we still dont know what happened.
Overall, I believe this is a good PR that optimizes a case for exceeding the gas limit, I just don't see if the original issue is resolved.
@revitteth Can you please give us some more clues on the issue? There is not much description.

[afa7789]

we could move the err check, or even the failed.

but it's not 100% related, an error is related to a failure in the code itself and not the contract which would revert.

as you can see in executable, when we error we don't return a result.

	// Create a helper to check if a gas allowance results in an executable transaction
	executable := func(gas uint64) (bool, *core.ExecutionResult, error) {
		result, err := caller.DoCallWithNewGas(ctx, gas)
		if err != nil {
			if errors.Is(err, core.ErrIntrinsicGas) {
				// Special case, raise gas limit
				return true, nil, nil
			}
			// Bail out
			return true, nil, err
		}

		return result.Failed(), result, nil
	}

[afa7789]

func (r *ReusableCaller) DoCallWithNewGas(
	ctx context.Context,
	newGas uint64,
) (*core.ExecutionResult, error) {
	var cancel context.CancelFunc
	if r.callTimeout > 0 {
		ctx, cancel = context.WithTimeout(ctx, r.callTimeout)
	} else {
		ctx, cancel = context.WithCancel(ctx)
	}

	// Make sure the context is cancelled when the call has completed
	// this makes sure resources are cleaned up.
	defer cancel()

	r.message.ChangeGas(r.gasCap, newGas)

	// reset the EVM so that we can continue to use it with the new context
	txCtx := core.NewEVMTxContext(r.message)
	r.intraBlockState = state.New(r.stateReader)
	r.evm.Reset(txCtx, r.intraBlockState)

	timedOut := false
	go func() {
		<-ctx.Done()
		timedOut = true
	}()

	gp := new(core.GasPool).AddGas(r.message.Gas()).AddBlobGas(r.message.BlobGas())

	result, err := core.ApplyMessage(r.evm, r.message, gp, true /* refunds */, false /* gasBailout */)
	if err != nil {
		return nil, err
	}

	// If the timer caused an abort, return an appropriate error message
	if timedOut {
		return nil, fmt.Errorf("execution aborted (timeout = %v)", r.callTimeout)
	}

	return result, nil
}