WIP: Backfill data columns #15580

kasey · 2025-08-12T00:56:00Z

What type of PR is this?

Feature

What does this PR do? Why is it needed?

Adds data column support to backfill.

Other notes for review

WIP - needs some additional cleanup and testing.
In the case where custody_group_count is the minimal CUSTODY_REQUIREMENT, we actually download SAMPLES_PER_SLOT for simplicity (otherwise DA check code needs to get more complicated). This is an easy thing to improve in a follow up if we decide to.

Acknowledgements

I have read CONTRIBUTING.md.
I have included a uniquely named changelog fragment file.
I have added a description to this PR with sufficient context for reviewers to understand this PR.

nalepae · 2025-08-15T07:46:35Z

beacon-chain/sync/data_column_assignment.go

+		}
+		dasInfo, _, err := peerdas.Info(nodeID, p2pSvc.CustodyGroupCountFromPeer(peer))
+		if err != nil {
+			log.WithField("peerID", peer).WithField("nodeID", nodeID).WithError(err).Debug("Failed to derive custody groups from peer.")


Why both logging and returning an error?
The caller already logs the error.

nalepae · 2025-08-15T08:02:10Z

beacon-chain/sync/data_column_assignment.go

+	return 1 / float64(f.freq())
+}
+
+func (f colFreq) freq() int {


freq is unclear. Why not simply custodianCount or an other name indicating the number of peers effectively custodying this column?

freq is an abbreviatio for "frequency". Will think about the naming some more.

nalepae · 2025-08-15T08:03:01Z

beacon-chain/sync/data_column_assignment.go

+}
+
+func (f colFreq) rarity() float64 {
+	if f.freq() == 0 {


The issue here is a column without any custodian and a column with exactly one custodian is considered as the same.

We don't need rarity. We can use popularity which is simply freq.

nalepae · 2025-08-15T08:05:10Z

beacon-chain/sync/data_column_assignment.go

+	return len(f.custodians)
+}
+
+func columnScarcity(cf []colFreq) []float64 {


Why not using columnPopularity instead of columnRarity and so we can avoid the division by 0 hack?

nalepae · 2025-08-15T08:37:01Z

beacon-chain/sync/data_column_assignment.go

+	freq         []colFreq
+	scarcity     []float64
+	rg           rand.Rand
+	covScoreRank []*peerColumnCoverage


why uint64 instead of bool?

nalepae · 2025-08-15T08:44:17Z

beacon-chain/sync/data_column_assignment.go

+		ra[f.col] = f.rarity()
+	}
+	return ra
+}


I think it's a waste of resource here to add peerColumnCoverage in freqByColumn[c].custodians instead just the peerID or the nodeID, because here each item in custodians will have exactly one non-zero item in its custodied list, which is the information already present in the col attribue of freqByColumn.

nalepae · 2025-08-15T08:49:59Z

beacon-chain/sync/backfill/columns.go

+
+	// Note that in the case where custody_group_count is the minimum CUSTODY_REQUIREMENT, we will
+	// still download the extra columns dictated by SAMPLES_PER_SLOT. This is a hack to avoid complexity in the DA check.
+	// We may want to revisit this to reduce bandwidth and storage for nodes with 0 validators attached.


This is not a hack, this is a feature.
https://github.com/ethereum/consensus-specs/blob/dev/specs/fulu/das-core.md#custody-sampling

At each slot, a node advertising custody_group_count downloads a minimum of sampling_size = max(SAMPLES_PER_SLOT, custody_group_count) custody groups, selected by groups = get_custody_groups(node_id, sampling_size), to which correspond the columns compute_columns_for_custody_group(group) for group in groups. The custody groups to custody, selected by get_custody_groups(node_id, custody_group_count), are then in particular a subset of those to sample. Sampling is considered successful if the node manages to retrieve all selected columns.

This also applies for initial sync and backfill.

This does not imply that the behavior in the specs is a hack. It is talking about the implementation in the backfill service. Because backfill is only concerned with finalized data it does not need to perform oversampling as we would want to do when range syncing or in gossip. The hack is that backfill simply does the same thing as these other syncing modes, even though it doesn't need to, and it would be more resource efficient to avoid doing so. However it would require more complexity in the DA check code, so it is simpler to just act as if we were in one of these other sync modes.

nalepae · 2025-08-15T08:52:46Z

beacon-chain/p2p/peers/assigner.go

@@ -42,7 +42,7 @@ func (a *Assigner) freshPeers() ([]peer.ID, error) {
 	if flags.Get().MinimumSyncPeers < required {
 		required = flags.Get().MinimumSyncPeers
 	}
-	_, peers := a.ps.BestFinalized(params.BeaconConfig().MaxPeersToSync, a.fc.FinalizedCheckpoint().Epoch)
+	_, peers := a.ps.BestFinalized(-1, a.fc.FinalizedCheckpoint().Epoch)


Not clear what -1 means.

I meant to comment that, thanks!

nalepae · 2025-08-15T08:58:54Z

beacon-chain/sync/backfill/worker.go

+	if cfg == nil {
+		cfg = &workerCfg{}
+	}
+	cfg.c = c


To ease the readability, it would be useful to use more descriptive names.
c, v, cm, bfs, cfs, nbv, ndcv etc... are quite are to understand.

nalepae · 2025-08-15T08:59:41Z

beacon-chain/sync/backfill/worker.go

 }

 func (w *p2pWorker) run(ctx context.Context) {
 	for {
 		select {
 		case b := <-w.todo:
+			if err := b.waitUntilReady(ctx); err != nil {
+				log.WithField("batch_id", b.id()).WithError(ctx.Err()).Info("worker context canceled while waiting to retry")


No camel case in fields. Capital letter in log.

nalepae · 2025-08-15T09:00:16Z

beacon-chain/sync/backfill/worker.go

 			log.WithFields(b.logFields()).WithField("backfillWorker", w.id).Debug("Backfill worker received batch")
-			if b.state == batchSidecarSync {
+			if b.state == batchSyncBlobs {


What if we have a batch with both blobs and columns?

nalepae · 2025-08-15T09:10:50Z

beacon-chain/sync/backfill/fulu_transition.go

+
+// IsDataAvailable implements the das.AvailabilityStore interface.
+func (m *multiStore) IsDataAvailable(ctx context.Context, current primitives.Slot, blk blocks.ROBlock) error {
+	if blk.Block().Slot() < m.fuluStart {


Probably it's better to switch the if and to consider the fulu case as the most common.

nalepae · 2025-08-15T09:13:07Z

beacon-chain/sync/backfill/fulu_transition.go

+	if blk.Block().Slot() < m.fuluStart {
+		return m.checkAvailabilityWithFallback(ctx, m.blobStore, current, blk)
+	}
+	return m.checkAvailabilityWithFallback(ctx, m.columnStore, current, blk)


This checks sidecars block by block, with is non efficient regarding KZG verification, especially when low custody count. (8 by 8 check).
If a sidecar does not respect a rule (ex: KZG verification), where is the peer that gave us this sidecar downscored?

kasey changed the title ~~Backfill data columns~~ WIP: Backfill data columns Aug 12, 2025

james-prysm added fulu sync peer-das labels Aug 13, 2025

kasey changed the base branch from develop to fusaka-devnet-3 August 14, 2025 19:17

kasey force-pushed the backfill-data-columns branch 3 times, most recently from 8634cee to e3f9617 Compare August 14, 2025 23:06

nalepae reviewed Aug 15, 2025

View reviewed changes

kasey force-pushed the backfill-data-columns branch from e3f9617 to 2b8384f Compare August 16, 2025 03:40

kasey requested a review from prestonvanloon as a code owner August 16, 2025 03:40

kasey changed the base branch from fusaka-devnet-3 to peerdas-sync August 16, 2025 03:41

Base automatically changed from peerdas-sync to develop August 18, 2025 14:50

kasey added 2 commits August 20, 2025 08:48

DataColumnSidecar backfill

c16ccc7

adapt to all the divergent changes between peerDAS and peerdas-sync

271a393

kasey force-pushed the backfill-data-columns branch from 2b8384f to 271a393 Compare August 20, 2025 13:48

WIP: Backfill data columns #15580

Are you sure you want to change the base?

WIP: Backfill data columns #15580

Uh oh!

Conversation

kasey commented Aug 12, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kasey Aug 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kasey Aug 15, 2025 •

edited

Loading