internal/ethapi: fix prev hashes in eth_simulate #31122
Conversation
internal/ethapi/simulate.go
Outdated
| @@ -73,6 +73,19 @@ func (r *simCallResult) MarshalJSON() ([]byte, error) { | |||
| return json.Marshal((*callResultAlias)(r)) | |||
| } | |||
|
|
|||
| // simBlockResult is the result of a simulated block. | |||
| type simBlockResult struct { | |||
| sim *simulator | |||
There was a problem hiding this comment.
Feels weird to me to include a pointer to the simulator when you really only need the config + fullTx. But even still, I don't see why you need to return these at all. execute is a method on sim so the caller will always have sim around to do the RPCMarshalBlock correctly.
There was a problem hiding this comment.
I removed sim from simBlockResult and replaced it with fullTx + chainConfig.
You're right that this is not needed for the fix. It was a refactoring that made the fix nicer (but also generally the code I think). I disliked returning a map[string]interface{} and preferred to return a strict type. And moving the encoding logic from processBlock to its own method felt like a good change.
internal/ethapi/simulate.go
Outdated
| // simBlockResult is the result of a simulated block. | ||
| type simBlockResult struct { | ||
| sim *simulator | ||
| Block *types.Block |
There was a problem hiding this comment.
I'm wondering if storing the block here will increase the memory usage of eth simulate over a longer period of blocks significantly
There was a problem hiding this comment.
Why should this be different? The encoded blocks were previously stored in the map[string]interface{}.
|
@s1na please address the comments and let's move this forward |
|
I have now also added a test case that covers both issues (parentHash of returned object is wrong, and blockhash opcode returns wrong hash). |
…mulateV1ChainLinkage
lightclient
force-pushed
the
simulate/fix-prev-hashes
branch
from
224de51 to
1cf96f6
Compare
|
|
||
| func (r *simBlockResult) MarshalJSON() ([]byte, error) { | ||
| blockData := RPCMarshalBlock(r.Block, true, r.fullTx, r.chainConfig) | ||
| blockData["calls"] = r.Calls |
There was a problem hiding this comment.
it feels a bit weird naming it "calls", but i guess we have to follow the spec anyway
Shout-out to @Gabriel-Trintinalia for discovering this issue. The gist of it as follows:
When processing a block, we should provide the parent block as well as the last 256 block hashes. Some of these parents data (specifically the hash) was incorrect because even though during the processing of the parent block we have updated the header, that header was not updating the TransactionsRoot and ReceiptsRoot fields (types.NewBlock makes a new copy of the header and changes it only on that instance).