Skip to content

core/filtermaps: only use common ancestor snapshots #2334

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

Merged
merged 1 commit into from
May 4, 2025
Merged

core/filtermaps: only use common ancestor snapshots #2334

merged 1 commit into from
May 4, 2025

Conversation

ucwong
Copy link
Member

@ucwong ucwong commented May 4, 2025

No description provided.

This PR makes the conditions for using a map rendering snapshot stricter
so that whenever a reorg happens, only a snapshot of a common ancestor
block can be used. The issue fixed in
ethereum/go-ethereum#31642 originated from using
a snapshot that wasn't a common ancestor. For example in the following
reorg scenario: `A->B`, then `A->B2`, then `A->B2->C2`, then `A->B->C`
the last reorg triggered a render from snapshot `B` saved earlier. Now
this is possible under certain conditions but extra care is needed, for
example if block `B` crosses a map boundary then it should not be
allowed. With the latest fix the checks are sufficient but I realized I
would just feel safer if we disallowed this rare and risky scenario
altogether and just render from snapshot `A` after the last reorg in the
example above. The performance difference if a few milliseconds and it
occurs rarely (about once a day on Holesky, probably much more rare on
Mainnet).
Note that this PR only makes the snapshot conditions stricter and
`TestIndexerRandomRange` does check that snapshots are still used
whenever it's obviously possible (adding blocks after the current head
without a reorg) so this change can be considered safe. Also I am
running the unit tests and the fuzzer and everything seems to be fine.
@ucwong ucwong marked this pull request as ready for review May 4, 2025 09:26
@ucwong ucwong merged commit 5de9364 into master May 4, 2025
1 check failed
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