-
Notifications
You must be signed in to change notification settings - Fork 182
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
Add verification tool for evm offchain replay #6755
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6755 +/- ##
==========================================
- Coverage 41.17% 41.07% -0.11%
==========================================
Files 2065 2071 +6
Lines 182905 183431 +526
==========================================
+ Hits 75316 75336 +20
- Misses 101284 101794 +510
+ Partials 6305 6301 -4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
103f257
to
d379a68
Compare
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.
Nice!
return nil | ||
} | ||
|
||
func parseEVMEvents(evts flow.EventsList) (*events.BlockEventPayload, []events.TransactionEventPayload, error) { |
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.
This does not seem to handle the case where there was a period that did not have blocks executed events, but did have transaction executed.
The solution is to accumulate the transaction executed events, until you hit the next block executed event.
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.
Yes, it's handled by the NewEVMEventsAccumulator
module
return nil | ||
} | ||
|
||
func initStorages(chainID flow.ChainID, dataDir string, executionDataDir string) ( |
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.
chain ID is not needed
8f6ed3b
to
5db925d
Compare
4fe749e
to
27c0f3a
Compare
a725780
to
f564161
Compare
27c0f3a
to
0cfcb7d
Compare
887419e
to
a41e107
Compare
transactionEvents []events.TransactionEventPayload, | ||
blockEvent *events.BlockEventPayload, | ||
) (types.ReplayResultCollector, error) { | ||
) (types.ReplayResultCollector, []*types.Result, error) { |
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.
Added the result for comparing trie updates, but the original ReplayBlock
is unchanged, so that gateway doesn't need to update its usage.
if chainID == flow.Testnet { | ||
return flowHeight == 211176670 | ||
} else if chainID == flow.Mainnet { | ||
return flowHeight == 85981135 |
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 believe that the proper Flow Height should be 85981134
. This is where the EVM
contract was first deployed on mainnet, see https://www.flowscan.io/contract/A.e467b9dd11fa00df.EVM/deployments?deployment=85981134
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.
This height is used to skip the root height where there is no execution data to read EVM events from.
Technically, you are right, but I can't use 85981134
with mainnet25's data, because 85981134
is a block from mainnet24. And this doesn't matter much as both 85981134
and 85981135
have no EVM events.
It only matters if we are verifying the state change at register level, which is not the case at the moment.
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.
LGTM! Just one correction on the mainnet
first EVM height.
556d903
to
1bdc486
Compare
This PR adds a util cmd for verifying the evm offchain replay.
No API change for gateway.