fix: block.timestamp is not accurate

[thomas-nguy]

What ❔

Related to zkSync-Community-Hub/zksync-developers#820

Change the l2 block creation logic to start a new l2 block only when a transaction is ready to be executed.

Why ❔

Current logic start a new l2 block as soon as the previous one is sealed.

A contract that relies on block.timestamp would be able to predict the time correctly because if the l2 block goes stale (no transaction), then it will be open indefinitely and the timestamp will not be accurate anymore

Solution has been tested locally but any feedbacks would be appreciated

Checklist

• PR title corresponds to the body of PR (we generate changelog entries from PRs).
• Tests for the changes have been added / updated.
• Documentation comments have been added / updated.
• Code has been formatted via zkstack dev fmt and zkstack dev lint.

[popzxc]

@slowli PTAL

[slowli]

I think it could be worth exploring a slightly differing approach: decoupling setting a new block in the UpdatesManager and in BatchExecutor. Namely, as soon as a new block is added in the current workflow, it is still added in UpdatesManager, but is not sent to BatchExecutor. Instead, it is only sent to BatchExecutor after receiving the first transaction in the block with the updated timestamp (obviously, the timestamp needs to be updated in UpdatesManager as well). IMO, this would make it slightly easier to reason about correctness.

[slowli]

AFAIU, this implementation is incomplete at least mathematically. Namely, if a node is restarted in the middle of a batch, logic in restore_state will request new block params at the end; i.e., the timestamp of the created new block may be outdated as previously. I think this could be fixed by passing the initial is_sealed / is_last_block_sealed value to process_l1_batch, and creating this new block (if appropriate) inside process_l1_batch.

[slowli]

Sorry for not noticing this before, but wait_for_next_tx() usage is bogus, since updates_manager.l2_block.timestamp may refer to the timestamp of the previous block. This timestamp is used in the MempoolIO implementation to check timestamp_asserter_range of the transaction. I think the easiest solution would be to create a tentative block timestamp before calling wait_for_next_tx() if necessary. This circles back to the idea we've discussed about creating a block in UpdatesManager early on, but not passing it to the VM until the first tx in the block (and adjusting the block timestamp with time before that). The adjustment likely has to be expressed as a StateKeeperIO method since we don't want to rely on any sort of I/O (like getting the wall clock time) unconditionally. E.g., for ExternalIO, block timestamps must never be adjusted.

To make invariants clearer, it may make sense to change StateKeeperIO to have 2 methods to create blocks:

• Creating an ordinary block, which returns a transaction together with a block.
• Creating a fictive block.

I think this would make the intended workflow obvious on the type system level.