Skip to content

Avoid dropping sidecars tracker on block import in Gloas#10788

Open
zilm13 wants to merge 5 commits into
Consensys:masterfrom
zilm13:deferred-payload-fix
Open

Avoid dropping sidecars tracker on block import in Gloas#10788
zilm13 wants to merge 5 commits into
Consensys:masterfrom
zilm13:deferred-payload-fix

Conversation

@zilm13
Copy link
Copy Markdown
Contributor

@zilm13 zilm13 commented Jun 4, 2026

PR Description

Keep DAS sampling trackers alive after block import when DA is deferred to the execution payload envelope.

Adds block/payload import lifecycle hooks so Gloas cleanup happens after payload import, not after block import, without milestone checks in BlockManager.

This prevents cancelling the in-flight DA check before recovered data columns can satisfy it.

Fixed Issue(s)

Documentation

  • I thought about documentation and added the doc-change-required label to this PR if updates are required.

Changelog

  • I thought about adding a changelog entry, and added one if I deemed necessary.

Note

Medium Risk
Changes when fork-choice DA completes and when sampling state is cleared on Gloas; incorrect timing could fail or prematurely pass DA checks. Payload gossip dedup behavior on failed imports also changes.

Overview
Defers DAS tracker teardown for Gloas so data-column sampling is not cancelled when the beacon block imports before the execution payload envelope finishes DA.

ForkChoiceUtil exposes isDataAvailabilityCheckDeferredToExecutionPayloadEnvelope() (true on Gloas); Gloas runs the data-column availability checker on payload envelope import and passes SignedExecutionPayloadEnvelope into the checker factory. BlockManager routes block import through BlockEventsListener.onBlockImported and notifies ExecutionPayloadEventsListener after payload import; DasSamplerBasic skips removeAllForBlock on block import when deferral applies, prunes trackers on payload import, and avoids slot-based cleanup for imported blocks until then.

DefaultExecutionPayloadManager drops recentSeenExecutionPayloads when fork-choice payload import fails or errors so gossip can retry. AvailabilityChecker adds getAndLogAvailabilityCheckResult used from ForkChoice instead of inline DA logging.

Reviewed by Cursor Bugbot for commit 386aa94. Bugbot is set up for automated code reviews on this repo. Configure here.

StefanBratanov
StefanBratanov previously approved these changes Jun 4, 2026
Copy link
Copy Markdown
Contributor

@StefanBratanov StefanBratanov left a comment

Choose a reason for hiding this comment

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

LGTM but added few comments which I think would make it better

return factory.createAvailabilityChecker(block);
}

@Override
Copy link
Copy Markdown
Contributor

@StefanBratanov StefanBratanov Jun 4, 2026

Choose a reason for hiding this comment

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

I feel like we can get rid of createAvailabilityCheckerOnBlock and createAvailabilityCheckerOnExecutionPayloadEnvelope and have only createAvailabilityChecker and this boolean method and have a simple if in on_block, I think better naming is isDataAvailabilityDeferredUntilExecutionPayloadProcessing.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

So we move all logic in ForkChoice. I don't think it's good.
image

}
}

@Override
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we should have instead of the two new methods added to BlockEventsListener:

public interface DataAvailabilitySampler extends BlockEventsListener, ReceivedBlockEventsChannel, ReceivedExecutionPayloadEventsChannel

To avoid repetition and delegation.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not sure about this as well
So we keep onNewBlock, removeAllForBlock and enableBlockImportOnCompletion in BlockEventsListener and still use it in DasSamplerBasic and BlockBlobSidecarsTrackersPoolImpl
But we also subscribe DasSamplerBasic and BlockBlobSidecarsTrackersPoolImpl to events in ReceivedBlockEventsChannel so it's spread between 2 sources

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ive moved only payload event
looks good

block.getSlot(),
block.getRoot(),
result.toLogString());
if (!forkChoiceUtil.isDataAvailabilityCheckDeferredToExecutionPayloadEnvelope()) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think it's fine to print it, it would just say not required

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

that's why i've removed it
it is confusing
you have 2 da checks on one block and one says not required, and you start requesting sidecars after that
what's happening?
not required = no blobs

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

yeah actually it's ok it's just an if

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

moved it to availability checker

Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

Reviewed by Cursor Bugbot for commit b155024. Configure here.

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