Do not try to publish reconstructed columns while syncing #10033
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| dataColumnSidecarPublisher.accept(dataColumnSidecar, RemoteOrigin.RECOVERED); | ||
| } | ||
| recoveredColumnSidecarSubscribers.forEach( | ||
| subscriber -> subscriber.onRecoveredColumnSidecar(dataColumnSidecar, LOCAL_EL)); |
There was a problem hiding this comment.
Bug: Inconsistent Origin Label for Recovered Sidecars
The RemoteOrigin for recovered sidecars is LOCAL_EL when notifying subscribers, which is inconsistent with other calls in the method using RemoteOrigin.RECOVERED. Since these sidecars are reconstructed, not from the local execution layer, LOCAL_EL misrepresents their true origin and could lead to downstream misinterpretation.
|
added a |
There was a problem hiding this comment.
You need to merge latest changes with DasSamplerBasic/ DasPreSampler. I will recheck after this.
New solution looks better with some flows explicitely defined in BeaconChainController, but we still keep have this ring DataColumnSidecarComponent -> Gossip -> DataColumnSidecarManager -> DataColumnSidecarComponent. Maybe add the test to each component in ring:
onNewValidatedSidecar(sidecar);
onNewValidatedSidecar(sameSidecar);
verifyNoInteraction(everything);
just to be sure we always do nothing on the same sidecar submitted again, just thoughts. Anyway you cut some unnneeded links and have not added new so it's definitely better.
| dataColumnSidecarELRecoveryManager::onNewDataColumnSidecar); | ||
|
|
||
| // RecoveringCustody -> EL Recovery | ||
| dataColumnSidecarRecoveringCustody.subscribeToRecoveredColumnSidecar( |
There was a problem hiding this comment.
we don't need this, EL recovery manager needs only trigger to start, if we already recovered everything we don't need to feed it to him. It has ignore for RECOVERED sidecars in logic.
| dataColumnSidecarPublisher.accept(dataColumnSidecar, RemoteOrigin.RECOVERED); | ||
| } | ||
| recoveredColumnSidecarSubscribers.forEach( | ||
| subscriber -> subscriber.onRecoveredColumnSidecar(dataColumnSidecar, LOCAL_EL)); |
There was a problem hiding this comment.
cursor is right about incorrect origin
| import tech.pegasys.teku.statetransition.blobs.RemoteOrigin; | ||
|
|
||
| public interface RecoveredColumnSidecarSubscriber { | ||
| void onRecoveredColumnSidecar(DataColumnSidecar dataColumnSidecar, RemoteOrigin origin); |
There was a problem hiding this comment.
as we still have origin, it is confusing to have separate subscriber type. I'd reuse other DataColumnSidecar subscriber or default with RemoteOrigin.RECOVERED
3 participants
fixes #9976
Documentation
doc-change-requiredlabel to this PR if updates are required.Changelog
Note
Stop publishing reconstructed data column sidecars while syncing; add a recovered-sidecar subscriber API and wire Gossip, EL recovery, recovering custody, and retriever together.
DataColumnSidecars on node sync status inDataColumnSidecarRecoveringCustodyImplandDataColumnSidecarELRecoveryManagerImpl(newonSyncingStatusChangedhooks; only publish wheninSync).SyncServiceto both components.RecoveredColumnSidecarSubscriberandsubscribeToRecoveredColumnSidecartoDataColumnSidecarELRecoveryManagerandDataColumnSidecarRecoveringCustody; notify subscribers on recovered sidecars.completeDasClassesPluming()inBeaconChainControllerto connect flows:DataColumnSidecarRetriever.DataColumnSidecarManager.onDataColumnSidecarPublishand related NOOP/usage.Written by Cursor Bugbot for commit 4c0e64e. This will update automatically on new commits. Configure here.