experiment: remove bloom filter from header#3854
Conversation
EgeCaner
added
the
disable-deploy-test
6cfbdcd to
e385bc7
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3854 +/- ##
==========================================
- Coverage 76.45% 75.55% -0.91%
==========================================
Files 442 446 +4
Lines 39767 39856 +89
==========================================
- Hits 30405 30114 -291
- Misses 7221 7653 +432
+ Partials 2141 2089 -52 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
b7d9bae to
e070c39
Compare
e070c39 to
aea2729
Compare
|
Claude finished @EgeCaner's task in 6m 53s —— View job PR Review: remove bloom filter from headerI reviewed the full diff against Three things worth addressing, posted inline: Important
Nit / perf
Everything else (statebackend plumbing, genesis, builder/sequencer, adapters, p2p sync, RPC v8/v9/v10 subscriptions, |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| PruneModeFlag, | ||
| ). | ||
| WithOptional(&headstate.Migrator{}, cfg.NewState, "new-state") | ||
| With(&blockeventsbloom.Migrator{}) |
There was a problem hiding this comment.
Important — looks like an accidental drop during the rebase. This PR removes the previously-registered optional headstate.Migrator{} (WithOptional(&headstate.Migrator{}, cfg.NewState, "new-state")) and its import, replacing it with the new blockeventsbloom.Migrator. But cfg.NewState / new-state is still wired up everywhere else (node/node.go:149,240,264,785, node/genesis.go), and the migration/state/headstate package still exists and is otherwise unregistered anywhere in the tree.
The net effect: nodes started with --new-state will no longer run the head-state migration. Since this is unrelated to removing the bloom filter (and the last commit is "rebase fix"), please confirm whether the intent was to also register blockeventsbloom alongside the headstate migrator rather than in place of it:
registry := migration.NewRegistry().
With(&blocktransactions.Migrator{}).
WithOptional(
historyprunner.New(cfg.RetainedBlocks, cfg.PruneMinAge),
cfg.Prune,
PruneModeFlag,
).
WithOptional(&headstate.Migrator{}, cfg.NewState, "new-state").
With(&blockeventsbloom.Migrator{})
| if !e.matcher.TestBloom(entryWithBloom.Bloom) { | ||
| continue | ||
| } |
There was a problem hiding this comment.
Important — behavioral regression: the skippedEvents = 0 reset in the bloom-skip branch was dropped. On origin/main this branch read:
if !e.matcher.TestBloom(header.EventsBloom) {
// Skipped events are scoped to the resume block; once we step past
// it, reset so later blocks aren't under-counted.
skippedEvents = 0
continue
}Now the reset is gone, so skippedEvents (seeded from cToken.processedEvents, which is scoped to the resume block only) survives a bloom miss. Scenario: a continuation token resumes at fromBlock=X with processedEvents=5. Pre-confirmed blocks are volatile — a delta/replace-slot can change block X's content between requests. If X's new bloom no longer matches, we continue without resetting, and the next matching block X+1 then gets AppendBlockEvents(..., skippedEvents=5, ...), silently dropping its first 5 events.
Restore the reset in the skip path:
| if !e.matcher.TestBloom(entryWithBloom.Bloom) { | |
| continue | |
| } | |
| if !e.matcher.TestBloom(entryWithBloom.Bloom) { | |
| // Skipped events are scoped to the resume block; once we step past | |
| // it, reset so later blocks aren't under-counted. | |
| skippedEvents = 0 | |
| continue | |
| } |
| return nil, nil, err | ||
| } | ||
| newNode := &node{preconfirmed: &next, parent: target.parent} | ||
| newNode := &node{entry: pending.NewPreConfirmedWithBloom(&next), parent: target.parent} |
There was a problem hiding this comment.
Performance — delta updates now recompute the full block bloom instead of an incremental merge. NewPreConfirmedWithBloom(&next) calls core.EventsBloom(next.Block.Receipts) over all receipts. Previously (adapters/sn2core/sn2core.go AdaptPreConfirmedWithDelta, now removed) only the newly-appended receipts were bloomed and merged into the existing filter:
addedBloom := core.EventsBloom(mergedReceipts[len(existingTxs):])
addedBloom.Merge(current.Block.Header.EventsBloom)Delta updates arrive on every poll while a pre-confirmed block grows, so a block that ends with N events now costs O(N) bloom work per delta → ~O(N²) over the block's lifetime, on the poller hot path. Worth confirming this is acceptable for this experiment, or keeping the incremental merge (e.g. threading the parent's bloom through the delta path) if it shows up in profiles.
1 participant
No description provided.