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

fix: toBlock argument in L1 getLogs is inclusive #10828

Merged
merged 1 commit into from
Dec 18, 2024

Conversation

spalladino
Copy link
Collaborator

As @alexghr identified, we got a spurious reorg on a node in the exp1 network. This was caused by the node getting a current l1BlockNumber=245, but then fetching an L2 block mined at 246.

This caused the canPrune check to fail:

const canPrune =
      localPendingBlockNumber > provenBlockNumber &&
      (await this.rollup.read.canPruneAtTime([time], { blockNumber: currentL1BlockNumber }));

The canPruneAtTime was evaluated at L1 block number 245, and it correctly returned true, since there had been a reorg shortly before (at 240), and no new L2 block had been mined so the rollup hadn't reset its state by then. However, the localPendingBlockNumber was incorrectly increased due to the block mined at 246, which caused the archiver to incorrectly reorg it.

This PR fixes the L1 event queries so the toBlock is inclusive. A quick test with cast shows that this is the case:

$ cast logs -r https://mainnet.infura.io/v3/$INFURA_API_KEY --from-block 0x146eade --to-block 0x146eadf --address 0xa0b86991c6218b36c1d19d4a2e9eb0ce3606eb48 --json | jq .[].blockNumber | uniq
"0x146eade"
"0x146eadf"

And just for good measure, we also filter the logs returned by the block range searched.

Copy link
Contributor

Changes to circuit sizes

Generated at commit: 6f1fb5761bd7c53fc1513cafc6a20bd0b7da91af, compared to commit: a3fba8442fdd62f429054c3367984fd4206bbbeb

🧾 Summary (100% most significant diffs)

Program ACIR opcodes (+/-) % Circuit size (+/-) %
rollup_base_public -6 ✅ -0.00% 0 ➖ 0.00%

Full diff report 👇
Program ACIR opcodes (+/-) % Circuit size (+/-) %
rollup_base_public 3,851,246 (-6) -0.00% 12,654,734 (0) 0.00%

@spalladino spalladino merged commit 970ad77 into master Dec 18, 2024
72 checks passed
@spalladino spalladino deleted the palla/to-block-is-inclusive branch December 18, 2024 12:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants