minor/cosmetic improvements to data gas handling in state transition #83
Conversation
|
just some minor issues I spotted while porting this code to erigon. Nothing that should affect your testing as it's mostly cosmetic. |
core/state_transition.go
Outdated
| if st.evm.Context.ExcessDataGas == nil { | ||
| return fmt.Errorf("sharding is active but ExcessDataGas is nil. Time: %v", st.evm.Context.Time.Uint64()) | ||
| return fmt.Errorf("%w: sharding is active but ExcessDataGas is nil. Time: %v", ErrInsufficientFunds, st.evm.Context.Time.Uint64()) |
There was a problem hiding this comment.
This is more of an "internal error" rather than an ErrInsufficientFunds. I think the previous error was fine as-is.
There was a problem hiding this comment.
That was my thinking when I first wrote the code, but as I looked at the other error handling it appears if you don't wrap some error then executionResult.Failed() will return false, which struck me as potentially problematic. Perhaps we should create an "ErrInternalFailure" type to wrap for these oddball & unexpected failures?
There was a problem hiding this comment.
Updated to use a new "ErrInternalFailure" for this.
There was a problem hiding this comment.
Using an ErrInternalFailure is fine. Though I don't see how this affects the ExecutionResult, which only yields errors as bytecode execution faults.
roberto-bayardo
force-pushed
the
cleanup
branch
from
f70630c
to
fa0d9c6
Compare
roberto-bayardo
force-pushed
the
cleanup
branch
from
fa0d9c6
to
92cf4af
Compare
fix error that wasn't wrapping one of the underlying execution errors as expected by the error handler
change dataGasUsed to return type uint64 which is more convenient than big int
remove confusing comment I had added earlier around gas refunds