-
Notifications
You must be signed in to change notification settings - Fork 4
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
fix panic issue when ExcessDataGas is nil #37
Conversation
bf6679c
to
23b22a1
Compare
core/state_transition.go
Outdated
if st.evm.ChainConfig().IsSharding(st.evm.Context.BlockNumber) { | ||
if st.evm.Context.ExcessDataGas == nil { | ||
// this edge-case will happen when the parent block is pre EIP-4844 | ||
st.evm.Context.ExcessDataGas = new(big.Int) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems awkward to update the State Context inside buyGas. Could we move this initialization elsewhere, perhaps when the context is created?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I considered making it always non-nil in NewEVMBlockContext but wasn't sure if I should make the MaxFeePerDataGas field be handled differently than BaseFee, which is left as nil when unspecified. I can't really think of any reasons why checking for nil ExcessDataGas would be important/informative though so perhaps that is the best place for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to always set to non-nil in NewEVMBlockContext.
2dfedc8
to
a0a36aa
Compare
core/evm.go
Outdated
@@ -44,7 +44,7 @@ func NewEVMBlockContext(header *types.Header, excessDataGas *big.Int, chain Chai | |||
beneficiary common.Address | |||
baseFee *big.Int | |||
random *common.Hash | |||
edg *big.Int | |||
edg *big.Int = new(big.Int) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add a comment here explaining the edge case we're covering.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done!
a0a36aa
to
36e1168
Compare
This issue may arise when the current block is a Sharding-enabled block but the parent is not.