search

0xPolygonHermez / zkevm-node

Go implementation of a node that operates the Polygon zkEVM Network
Other
531 stars 685 forks source link

synchronizer: check l1blocks #3546

Closed joanestebanr closed 5 months ago

joanestebanr commented 5 months ago

Closes #3540 #3561

What does this PR do?

Reviewers

Main reviewers:

Codeowner reviewers:

joanestebanr commented 5 months ago

I don't know the scope of this task, but the PR changes seem like overkill.

I expected it to be just a single method running concurrently to check for blocks related to the consolidated point on Ethereum.

I have added many comments, but I confess I got exhausted while reviewing it because I always thought, "Is this necessary?"

I understand this PR can achieve the goal, but assuming the price we are paying to maintain this implementation in the future, I prefer to drop everything and come up with a more straightforward solution that gets the consolidated point from Ethereum and checks how we are from there.

Proposal:

Assuming the Synchronizer must be able to recover by itself when a reorg is detected and this mechanism is a secondary protection to help the Synchronizer identify the reorg with a different strategy, I would suggest implementing the following in the simplest way possible, like:

  • at the start of the application, get the current consolidated point block from Ethereum

  • get all the blocks we have from the consolidated point and check all of them for a reorg

    • if reorg is detected, flag it and wait for the synchronizer to fix it. Once fixed
  • stores the last block we checked in memory
  • keep monitoring the consolidated point on Ethereum until the last block we checked matches
  • repeat

Integration with the Synchronizer can be done via a simple channel, which can be appended to the current synchronization process.

Advantages of this approach: no changes in the DB faster, runs only on memory takes advantage of the network consolidation point instead of verifying everything with less checks, it can load all the blocks in a single shot instead of one by one and check them concurrently way less code to maintain way less changes in the real code due to this extra protection

Reasoning:

We assume the consolidation point on Ethereum is where we trust a reorg will never happen; from this point, we do our own check to make sure all the blocks not consolidated yet are matching with Ethereum; once we guarantee this, the synchronizer takes place and continues his job synchronizing block by block until all the blocks we have checked are now part of the consolidated part of Ethereum, then we start over. If, for some reason, the synchronizer is not able to detect a reorg in the regular synchronization process, our next check will start from the last time we checked until the latest synchronized block, and we will find it, flagging it to the synchronizer to allow the reorg process to be executed in the next synchronizer reorg check.

Conclusion:

I don't feel comfortable merging this whole PR, and I'm open to discussing it if you consider it worth it. Otherwise, you can check my comments in the PR and follow with this implementation.

I double-check the scope of the task and it's the expected behaviour