ethapi: add block override to estimateGas #30695
Conversation
antonydenyer
requested review from
lightclient,
gballet and
MariusVanDerWijden
as code owners
antonydenyer
force-pushed
the
estimate-gas-block-overrides
branch
from
b135064 to
b7a664e
Compare
antonydenyer
force-pushed
the
estimate-gas-block-overrides
branch
from
b7a664e to
7702e9e
Compare
antonydenyer
force-pushed
the
estimate-gas-block-overrides
branch
from
7702e9e to
b304b13
Compare
internal/ethapi/api.go
Outdated
| Chain: NewChainContext(ctx, b), | ||
| Header: header, | ||
| Chain: chainContext, | ||
| Header: blockOverrides.MakeHeader(header), |
There was a problem hiding this comment.
Please revert this line. The EVM doesn't have access to the header directly anyway. It only accesses block fields via the block context object.
There was a problem hiding this comment.
Yeah it took me a while to find this, I was taking most of the code fromDoCall path which doesn't do this. But the problem with the estimate path is that it uses the header in https://github.com/ethereum/go-ethereum/blob/master/eth/gasestimator/gasestimator.go#L221
Without this line the block overrides are not available in solidity code when running the unit test; this could be because is the test isn't setup correctly or it could genuinely be needed.
There was a problem hiding this comment.
Riighhtt this made me realize we should do this differently. Right now you're effectively only applying the block overrides to mutate the header and pass that to the gasestimator.
IMO we should pass in the overrides to gasestimator and apply them there just before execution. But that will create an import cycle. Hmm...
There was a problem hiding this comment.
I was surprised by the number of differences between the two endpoints. They are basically the same thing, except when they are not!
There was a problem hiding this comment.
Another option is to move the gasestimator package up into the ethapi package to prevent this.
let me know what you think ...
c5af175
There was a problem hiding this comment.
This was a deliberate change to move gas estimator out of rpc: #28600. let me think about it
There was a problem hiding this comment.
Alternatively, I could pass it in on the options, but that didn't feel right.
antonydenyer
requested review from
karalabe,
holiman and
rjl493456442
as code owners
|
I moved the overrides to their own package so we can pass it down to gasestimator. @antonydenyer can you try this patch see if it works for you? |
| @@ -50,8 +51,8 @@ const ( | |||
|
|
|||
| // simBlock is a batch of calls to be simulated sequentially. | |||
| type simBlock struct { | |||
| BlockOverrides *BlockOverrides | |||
| StateOverrides *StateOverride | |||
| BlockOverrides *override.BlockOverrides | |||
There was a problem hiding this comment.
override.BlockOverrides kinda duplicates the "Overrides" part. Maybe we could rename it to override.Block and overrides.State wdyt? cc @fjl
There was a problem hiding this comment.
Sure, we could change the name. All of this is in internal/ anyway.
There was a problem hiding this comment.
I do think that introducing types Block and State is a bit weird, even though it's technically possible, because these words have a very definitive meaning already. I personally don't mind the duplication in naming for these 'override' types, since they are so low-use.
Add block overrides to
eth_estimateGasto align consistency witheth_call#27800 (comment)
Fixes #28175