Skip to content

Conversation

@DiogoSantoss
Copy link
Contributor

@DiogoSantoss DiogoSantoss commented Nov 13, 2025

Fetch attestation data on receiving SSE block event or after 1/3 + 300ms as a fallback, possibly fetching a stale head.
Includes a small fix to featureset/config.go which had a data race on clean up after a test.

category: feature
ticket: #4027
feature_flag: fetch_att_on_block

@DiogoSantoss DiogoSantoss self-assigned this Nov 13, 2025
@codecov
Copy link

codecov bot commented Nov 13, 2025

Codecov Report

❌ Patch coverage is 63.85542% with 30 lines in your changes missing coverage. Please review.
✅ Project coverage is 56.43%. Comparing base (426ba79) to head (14add24).
⚠️ Report is 69 commits behind head on main.

Files with missing lines Patch % Lines
core/scheduler/scheduler.go 55.38% 22 Missing and 7 partials ⚠️
app/app.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4095      +/-   ##
==========================================
+ Coverage   56.37%   56.43%   +0.06%     
==========================================
  Files         245      245              
  Lines       31169    31238      +69     
==========================================
+ Hits        17571    17629      +58     
- Misses      11284    11296      +12     
+ Partials     2314     2313       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR implements early attestation data fetching triggered by SSE block events from the beacon node, with a fallback to the traditional 1/3 slot + 300ms timeout if no block event is received. This aims to reduce attestation delays and improve validator performance.

Key changes:

  • Added HandleBlockEvent method to scheduler that triggers attestation duties immediately upon receiving block events
  • Introduced waitForBlockEventOrTimeout function that implements the fallback timeout logic
  • Refactored duty triggering logic into a shared triggerDuty method
  • Added SSE block event subscription and notification infrastructure
  • Fixed data race in featureset test helper functions

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
core/scheduler/scheduler.go Core implementation of block event handling, early attestation triggering, and fallback timeout logic
core/scheduler/scheduler_test.go Test coverage for block event triggered attestation duties
app/sse/listener.go Added block event subscription mechanism and notification to subscribers
app/sse/listener_internal_test.go Test coverage for block event handling and notification
app/featureset/featureset.go Added FetchAttOnBlock feature flag definition
app/featureset/config.go Fixed data race by adding mutex protection in test cleanup functions
app/app.go Wired block event subscription from SSE listener to scheduler

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@DiogoSantoss DiogoSantoss requested a review from pinebit November 17, 2025 16:25
@DiogoSantoss DiogoSantoss force-pushed the diogo/fetch-att-on-block-event branch from 2bade22 to 136ac08 Compare November 17, 2025 16:38
dutyCtx := log.WithCtx(ctx, z.Any("duty", duty))
if duty.Type == core.DutyProposer {
var span trace.Span
dutyCtx, span = core.StartDutyTrace(dutyCtx, duty, "core/scheduler.scheduleSlot")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure the trace name is right .scheduleSlot..

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't modify this name, it was already like that in scheduleSlot

dutyCtx, span = core.StartDutyTrace(dutyCtx, duty, "core/scheduler.scheduleSlot")
but I extracted that logic to a separate function to reuse in HandleBlockEvent
Are you suggesting changing to .triggerDuty ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No suggestion, whatever you think will be useful when browsing trace data in Grafana.

Copy link
Collaborator

@pinebit pinebit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple minor comments and it's good

@sonarqubecloud
Copy link

@KaloyanTanev KaloyanTanev added the do not merge Indicate to bulldozer bot that this PR should not be merged label Dec 3, 2025
@KaloyanTanev KaloyanTanev removed the do not merge Indicate to bulldozer bot that this PR should not be merged label Dec 13, 2025
@KaloyanTanev
Copy link
Collaborator

KaloyanTanev commented Dec 16, 2025

Let's rework this one after Christmas as we've discussed in the past week - separate the logic into 2 flags:

  • fetch_att_on_block
  • fetch_att_on_block_with_delay

If both are or only the second is enabled, delay will be used. If only the first is enabled, no delay will be added.

To be fair, the first one does not make a huge amount of sense. We can achieve only triggering early fetching, which will save us some time, but submitting will still be in 1/3 > x > 2/3 of slot time. There are 2 blockers for the attestation to be actually submitted early:

  1. Consensus starts at 1/3 slot time. This is hard and scary to change. Does not really make sense to me to put that much effort in it for such a small reward.
  2. VC asks for data at 1/3 slot time. This can be changed relatively easy, as most VCs have an option to use the SSE endpoint to get notified when there is a new block and ask for attestation data (what we do). As we have SSE proxying, it should work.

@KaloyanTanev KaloyanTanev added the do not merge Indicate to bulldozer bot that this PR should not be merged label Dec 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do not merge Indicate to bulldozer bot that this PR should not be merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants