Skip to content

cl/phase1/forkchoice: fix block_importing_latency never recording on Fulu+ - #21842

Merged
lystopad merged 3 commits into
mainfrom
feature/lystopad/fix-block-importing-latency-metric
Jun 16, 2026
Merged

lystopad merged 3 commits into
mainfrom
feature/lystopad/fix-block-importing-latency-metric

Conversation

@lystopad

Copy link
Copy Markdown
Member

Problem

block_importing_latency (Caplin) reads a flat 0 for hours in Grafana — it's effectively never recorded.

It's a gauge set only by ObserveBlockImportingLatency, whose only caller (collectOnBlockLatencyToUnixTime) was nested inside the Deneb-only blob data-availability branch of OnBlock, behind:
checkDataAvaiability && BlobKzgCommitments.Len() > 0 && !elHasBlobs && version is Deneb/Electra (not Fulu) && highestSeen < slot.

Two of those gates kill it:

  • Fulu+ (current mainnet) takes the Fulu/PeerDAS branch, which never calls it → the gauge stays at its 0 default. This is the flat line.
  • Even pre-Fulu it only fired for a current-slot block carrying blobs the EL didn't already have — a rare race at tip — and skipped blob-less blocks entirely.

So the panel value is not a real "0 ms"; the setter just isn't reached.

Fix

Move the single call to the fork-agnostic highestSeen tip-advance in OnBlock, which runs for every new head block on all forks (Fulu/Gloas included). The helper's existing slot == GetCurrentSlot() guard still skips backfill/sync blocks, so the gauge measures head-arrival latency rather than block age.

if block.Block.Slot > f.highestSeen.Load() {
    f.highestSeen.Store(block.Block.Slot)
    f.highestSeenRoot.Store(common.Hash(blockRoot))
    collectOnBlockLatencyToUnixTime(f.ethClock, block.Block.Slot) // fork-agnostic
}

Net effect: block_importing_latency now populates every slot the node imports a current-slot head block, on all forks.

Tests

TestCollectOnBlockLatency covers the helper's contract (records for a current-slot block, skips a past/backfill slot).

This change is a one-call relocation — the emitting helper itself is unchanged. A full cross-fork OnBlock integration test was intentionally not added, as exercising it requires substantial fork-choice/engine/PeerDAS scaffolding to make a test block "current"; flagging per the repo's TDD-pragmatism guidance. make lint and make erigon clean.

Scope

Affects release/3.4, release/3.5, and main (the call placement is identical on all three). Will cherry-pick to the release branches.

…Fulu+

The block_importing_latency gauge was updated from a single call site nested
inside the Deneb-only blob data-availability branch of OnBlock, gated on the
block carrying blobs the EL didn't already have. So it almost never fired even
pre-Fulu, and on Fulu+ (current mainnet) the Deneb branch is never taken — the
gauge stayed at its 0 default for hours, which reads as a flat zero in Grafana.

Move the call to the fork-agnostic highestSeen tip-advance, so it records once
per new head block on every fork. The helper's current-slot guard still skips
backfill/sync blocks.

Note: this is a one-call relocation (the emitting helper is unchanged). Added a
unit test covering the helper's current-slot contract; a full cross-fork OnBlock
integration test was not added as it needs substantial forkchoice/engine/peerDAS
scaffolding.
@lystopad
lystopad requested a review from domiwei as a code owner June 16, 2026 09:23
@lystopad lystopad self-assigned this Jun 16, 2026
@lystopad lystopad added the Caplin Caplin: Consensus Layer, Beacon API label Jun 16, 2026

domiwei commented Jun 16, 2026

Copy link
Copy Markdown
Member

Pushed a small follow-up commit to address the timing issue we discussed.

What changed:

  • Capture currentSlotOnEntry early in OnBlock, after the cheap early rejects but before DA / engine work.
  • Pass that captured slot into collectOnBlockLatencyToUnixTime instead of having the helper re-read GetCurrentSlot() at emit time.
  • Keep the highestSeen gate, so backfill / old / duplicate blocks still do not update block_importing_latency.
  • Adjust the unit test so it verifies the helper uses the supplied entry slot and does not depend on a late current-slot lookup.

Why:
A block can arrive during its own slot but cross into the next slot while DA / PeerDAS / NewPayload work runs. If the metric checks GetCurrentSlot() only at the later emit point, those higher-latency current-slot imports can be silently skipped. Capturing the slot at OnBlock entry preserves the intended head-arrival semantics while still avoiding backfill updates.

Verification run locally:

GOCACHE=/private/tmp/erigon-pr21842-gocache go test -count=1 ./cl/phase1/forkchoice
GOCACHE=/private/tmp/erigon-pr21842-gocache GOLANGCI_LINT_CACHE=/private/tmp/erigon-pr21842-golangci-cache make lint

Both passed. The first make lint attempt picked up stale golangci-lint cache entries from another deleted worktree, so I reran it with a clean GOLANGCI_LINT_CACHE; that completed with 0 issues.

@lystopad
lystopad added this pull request to the merge queue Jun 16, 2026
Merged via the queue into main with commit 3b54bec Jun 16, 2026
92 checks passed
@lystopad
lystopad deleted the feature/lystopad/fix-block-importing-latency-metric branch June 16, 2026 14:03
domiwei added a commit that referenced this pull request Jun 17, 2026
…ing on Fulu+ (#21843)

Cherry-pick of #21842 to release/3.5.

`block_importing_latency` was only recorded from a call nested in the
Deneb-only blob data-availability branch of `OnBlock`, so it never fired
on Fulu+ (current mainnet) and read a flat 0 in Grafana. Moved to the
fork-agnostic `highestSeen` tip-advance so it records once per new head
block on all forks. See #21842 for full analysis.

Clean cherry-pick.

---------

Co-authored-by: kewei <kewei.train@gmail.com>
domiwei added a commit that referenced this pull request Jun 17, 2026
…ing on Fulu+ (#21845)

Cherry-pick of #21842 to release/3.4 (applied manually — r3.4 OnBlock
layout differs from main).

`block_importing_latency` only fired from a call nested in the
Deneb-only blob branch of `OnBlock`, so it never recorded on Fulu+ (flat
0 in Grafana). Moved to the fork-agnostic `highestSeen` tip-advance. See
#21842 for full analysis.

---------

Co-authored-by: kewei <kewei.train@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Caplin Caplin: Consensus Layer, Beacon API

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants