Skip to content
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

Open
hexoscott opened this issue Jul 16, 2024 · 17 comments
Open

Verify AccInputHash #794

hexoscott opened this issue Jul 16, 2024 · 17 comments
Assignees
Labels
bug Something isn't working Gateway FM

Comments

@hexoscott
Copy link
Collaborator

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

@Sharonbc01
Copy link

Hey @mandrigin who can work on this issue please to support Banana changes?

@Sharonbc01
Copy link

Ideal if we can get done sooner rather than later to have 100% coverage on all rollback cases

@V-Staykov V-Staykov linked a pull request Aug 8, 2024 that will close this issue
@V-Staykov
Copy link
Collaborator

We can't calculate this in the sequencer, because we don't have the sequence timestamp, so nothing to do on our side currently.

@cffls
Copy link

cffls commented Aug 9, 2024

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.

@hexoscott
Copy link
Collaborator Author

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.

@cffls
Copy link

cffls commented Aug 9, 2024

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.

@cffls
Copy link

cffls commented Aug 9, 2024

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.

@hexoscott
Copy link
Collaborator Author

Thanks @cffls - that definitely looks like it's calculated on a batch by batch basis

@cffls
Copy link

cffls commented Aug 15, 2024

Thanks for confirming. Looks like we have all information required to proceed with the implementation.

@hexoscott
Copy link
Collaborator Author

I've dropped a comment on the l1 info root changes which affect this here #793 (comment)

@Sharonbc01
Copy link

Hi @cffls @hexoscott can this be closed now?

@cffls
Copy link

cffls commented Sep 24, 2024

Looks like #1161 addressed this. Maybe @hexoscott or @V-Staykov could confirm.

@Sharonbc01
Copy link

Hi @hexoscott Can you please review and confirm if we can close this please. Thanks

cc: @ToniRamirezM

@V-Staykov
Copy link
Collaborator

No this is not done. The mentioned PR still queries L1 for all the data needed to calculate it.

@Sharonbc01
Copy link

@xavier-romero to test Monday Nov 11 am

@Sharonbc01
Copy link

Sharonbc01 commented Nov 12, 2024

Works if data on L1, but given CDK work around we can live with this in order to get v2.60x to Bali

@Sharonbc01
Copy link

@revitteth from Gateway FM will look into this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Gateway FM
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants