-
Notifications
You must be signed in to change notification settings - Fork 50
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
Verify AccInputHash #794
Comments
Hey @mandrigin who can work on this issue please to support Banana changes? |
Ideal if we can get done sooner rather than later to have 100% coverage on all rollback cases |
We can't calculate this in the sequencer, because we don't have the sequence timestamp, so nothing to do on our side currently. |
Sequence sender sets the timestamp of the batch the same as the timestamp of the last block in that batch: https://github.com/0xPolygonHermez/zkevm-sequence-sender/blob/develop/sequencesender/sequencesender.go#L1036. If this assumption is correct, cdk-erigon will have everything it needs to compute accinputhash. |
If my understanding is correct though, it is the last block timestamp of the last batch in the "sequence", because this is determined by the sequence sender the sequencer will always need to look this up from the L1 as it doesn't control what goes into a sequence. |
Agreed. Thanks for the insights. Only sequence sender will be able to calculate this first. However, I think it will be still beneficial to have this calculation in cdk-erigon, with the timestamp retrieved from L1. For example, in a rollback event, only L1Info index has changed, a RPC node will be able to detect the inconsistency when it verifies the AccInputHash for a range of batches. |
Looking into the implementation of AccInputHash, it seems to me that AccInputHash is calculate per batch, not per range of batches. Also see this diagram from this page. |
Thanks @cffls - that definitely looks like it's calculated on a batch by batch basis |
Thanks for confirming. Looks like we have all information required to proceed with the implementation. |
I've dropped a comment on the l1 info root changes which affect this here #793 (comment) |
Hi @cffls @hexoscott can this be closed now? |
Looks like #1161 addressed this. Maybe @hexoscott or @V-Staykov could confirm. |
Hi @hexoscott Can you please review and confirm if we can close this please. Thanks cc: @ToniRamirezM |
No this is not done. The mentioned PR still queries L1 for all the data needed to calculate it. |
@xavier-romero to test Monday Nov 11 am |
Works if data on L1, but given CDK work around we can live with this in order to get v2.60x to Bali |
@revitteth from Gateway FM will look into this. |
Because the L1InfoRoot is now deterministic, the sequencer will be able to calculate the AccInputHash without needing to call the L1. There are places in the RPC where we call the L1 for this information that we need to switch over to calculating locally.
Details here: https://hackmd.io/O4t3Xvj9TIydY9UsXfXHiA?view#Verify-accumulate-Input-Hash
The text was updated successfully, but these errors were encountered: