Fix some nits associated with data column sidecar verification (#15521)

* Fix some nits associated with data column sidecar verification

* Add changelog fragment

beacon-chain/blockchain/receive_data_column.go

@@ -15,7 +15,6 @@ func (s *Service) ReceiveDataColumns(dataColumnSidecars []blocks.VerifiedRODataC
 }
 
 // ReceiveDataColumn receives a single data column.
-// (It is only a wrapper around ReceiveDataColumns.)
 func (s *Service) ReceiveDataColumn(dataColumnSidecar blocks.VerifiedRODataColumn) error {
 	if err := s.dataColumnStorage.Save([]blocks.VerifiedRODataColumn{dataColumnSidecar}); err != nil {
 		return errors.Wrap(err, "save data column sidecars")

beacon-chain/das/availability_columns.go

@@ -130,7 +130,7 @@ func (s *LazilyPersistentStoreColumn) IsDataAvailable(ctx context.Context, curre
 	verifier := s.newDataColumnsVerifier(roDataColumns, verification.ByRangeRequestDataColumnSidecarRequirements)
 
 	if err := verifier.ValidFields(); err != nil {
-		return errors.Wrap(err, "valid")
+		return errors.Wrap(err, "valid fields")
 	}
 
 	if err := verifier.SidecarInclusionProven(); err != nil {
@@ -164,7 +164,7 @@ func (s *LazilyPersistentStoreColumn) fullCommitmentsToCheck(nodeID enode.ID, bl
 	blockSlot := block.Block().Slot()
 	blockEpoch := slots.ToEpoch(blockSlot)
 
-	// Compute the current spoch.
+	// Compute the current epoch.
 	currentEpoch := slots.ToEpoch(currentSlot)
 
 	// Return early if the request is out of the MIN_EPOCHS_FOR_DATA_COLUMN_SIDECARS_REQUESTS window.

beacon-chain/db/filesystem/data_column.go

@@ -251,7 +251,7 @@ func (dcs *DataColumnStorage) Summary(root [fieldparams.RootLength]byte) DataCol
 }
 
 // Save saves data column sidecars into the database and asynchronously performs pruning.
-// The returned chanel is closed when the pruning is complete.
+// The returned channel is closed when the pruning is complete.
 func (dcs *DataColumnStorage) Save(dataColumnSidecars []blocks.VerifiedRODataColumn) error {
 	startTime := time.Now()
 
@@ -266,8 +266,7 @@ func (dcs *DataColumnStorage) Save(dataColumnSidecars []blocks.VerifiedRODataCol
 		return errWrongNumberOfColumns
 	}
 
-	highestEpoch := primitives.Epoch(0)
-	dataColumnSidecarsbyRoot := make(map[[fieldparams.RootLength]byte][]blocks.VerifiedRODataColumn)
+	dataColumnSidecarsByRoot := make(map[[fieldparams.RootLength]byte][]blocks.VerifiedRODataColumn)
 
 	// Group data column sidecars by root.
 	for _, dataColumnSidecar := range dataColumnSidecars {
@@ -278,23 +277,20 @@ func (dcs *DataColumnStorage) Save(dataColumnSidecars []blocks.VerifiedRODataCol
 
 		// Group data column sidecars by root.
 		root := dataColumnSidecar.BlockRoot()
-		dataColumnSidecarsbyRoot[root] = append(dataColumnSidecarsbyRoot[root], dataColumnSidecar)
+		dataColumnSidecarsByRoot[root] = append(dataColumnSidecarsByRoot[root], dataColumnSidecar)
 	}
 
-	for root, dataColumnSidecars := range dataColumnSidecarsbyRoot {
+	for root, dataColumnSidecars := range dataColumnSidecarsByRoot {
 		// Safety check all data column sidecars for this root are from the same slot.
-		firstSlot := dataColumnSidecars[0].SignedBlockHeader.Header.Slot
+		slot := dataColumnSidecars[0].Slot()
 		for _, dataColumnSidecar := range dataColumnSidecars[1:] {
-			if dataColumnSidecar.SignedBlockHeader.Header.Slot != firstSlot {
+			if dataColumnSidecar.Slot() != slot {
 				return errDataColumnSidecarsFromDifferentSlots
 			}
 		}
 
-		// Set the highest epoch.
-		epoch := slots.ToEpoch(dataColumnSidecars[0].Slot())
-		highestEpoch = max(highestEpoch, epoch)
-
 		// Save data columns in the filesystem.
+		epoch := slots.ToEpoch(slot)
 		if err := dcs.saveFilesystem(root, epoch, dataColumnSidecars); err != nil {
 			return errors.Wrap(err, "save filesystem")
 		}
@@ -306,7 +302,7 @@ func (dcs *DataColumnStorage) Save(dataColumnSidecars []blocks.VerifiedRODataCol
 		}
 
 		// Compute the data columns ident.
-		dataColumnsIdent := DataColumnsIdent{Root: root, Epoch: slots.ToEpoch(dataColumnSidecars[0].Slot()), Indices: indices}
+		dataColumnsIdent := DataColumnsIdent{Root: root, Epoch: epoch, Indices: indices}
 
 		// Set data columns in the cache.
 		if err := dcs.cache.set(dataColumnsIdent); err != nil {

beacon-chain/sync/validate_data_column.go

@@ -74,7 +74,7 @@ func (s *Service) validateDataColumn(ctx context.Context, pid peer.ID, msg *pubs
 	verifier := s.newColumnsVerifier(roDataColumns, verification.GossipDataColumnSidecarRequirements)
 
 	// Start the verification process.
-	// https://github.com/ethereum/consensus-specs/blob/master/specs/fulu/p2p-interface.md#the-gossip-domain-gossipsub
+	// https://github.com/ethereum/consensus-specs/blob/master/specs/fulu/p2p-interface.md#data_column_sidecar_subnet_id
 
 	// [REJECT] The sidecar is valid as verified by `verify_data_column_sidecar(sidecar)`.
 	if err := verifier.ValidFields(); err != nil {
@@ -86,7 +86,7 @@ func (s *Service) validateDataColumn(ctx context.Context, pid peer.ID, msg *pubs
 		return pubsub.ValidationReject, err
 	}
 
-	// [IGNORE] The sidecar is not from a future slot (with a `MAXIMUM_GOSSIP_CLOCK_DISPARITY`` allowance
+	// [IGNORE] The sidecar is not from a future slot (with a `MAXIMUM_GOSSIP_CLOCK_DISPARITY` allowance)
 	//  -- i.e. validate that `block_header.slot <= current_slot` (a client MAY queue future sidecars for processing at the appropriate slot).
 	if err := verifier.NotFromFutureSlot(); err != nil {
 		return pubsub.ValidationIgnore, err
@@ -132,13 +132,13 @@ func (s *Service) validateDataColumn(ctx context.Context, pid peer.ID, msg *pubs
 		return pubsub.ValidationReject, err
 	}
 
-	// [REJECT] The current finalized_checkpoint is an ancestor of the sidecar's block
+	// [REJECT] The current `finalized_checkpoint` is an ancestor of the sidecar's block
 	// -- i.e. `get_checkpoint_block(store, block_header.parent_root, store.finalized_checkpoint.epoch) == store.finalized_checkpoint.root`.
 	if err := verifier.SidecarDescendsFromFinalized(); err != nil {
 		return pubsub.ValidationReject, err
 	}
 
-	// [REJECT] The sidecar's kzg_commitments field inclusion proof is valid as verified by `verify_data_column_sidecar_inclusion_proof(sidecar)`.
+	// [REJECT] The sidecar's `kzg_commitments` field inclusion proof is valid as verified by `verify_data_column_sidecar_inclusion_proof(sidecar)`.
 	if err := verifier.SidecarInclusionProven(); err != nil {
 		return pubsub.ValidationReject, err
 	}
@@ -154,7 +154,7 @@ func (s *Service) validateDataColumn(ctx context.Context, pid peer.ID, msg *pubs
 		return pubsub.ValidationIgnore, nil
 	}
 
-	// [REJECT] The sidecar is proposed by the expected `proposer_index` for the block's slot in the context of the current shuffling (defined by block_header.parent_root/block_header.slot).
+	// [REJECT] The sidecar is proposed by the expected `proposer_index` for the block's slot in the context of the current shuffling (defined by `block_header.parent_root`/`block_header.slot`).
 	// If the `proposer_index` cannot immediately be verified against the expected shuffling, the sidecar MAY be queued for later processing while proposers for the block's branch are calculated
 	// -- in such a case do not REJECT, instead IGNORE this message.
 	if err := verifier.SidecarProposerExpected(ctx); err != nil {

beacon-chain/verification/data_column.go

@@ -165,7 +165,7 @@ func (dv *RODataColumnsVerifier) NotFromFutureSlot() (err error) {
 		// Extract the data column slot.
 		dataColumnSlot := dataColumn.Slot()
 
-		// Skip if the data column slotis the same as the current slot.
+		// Skip if the data column slot is the same as the current slot.
 		if currentSlot == dataColumnSlot {
 			continue
 		}
@@ -174,7 +174,7 @@ func (dv *RODataColumnsVerifier) NotFromFutureSlot() (err error) {
 		// We lower the time by MAXIMUM_GOSSIP_CLOCK_DISPARITY in case system time is running slightly behind real time.
 		earliestStart, err := dv.clock.SlotStart(dataColumnSlot)
 		if err != nil {
-			return fmt.Errorf("failed to determine slot start time from clock waiter: %w", err)
+			return columnErrBuilder(errors.Wrap(err, "failed to determine slot start time from clock waiter"))
 		}
 		earliestStart = earliestStart.Add(-maximumGossipClockDisparity)
 
@@ -204,11 +204,8 @@ func (dv *RODataColumnsVerifier) SlotAboveFinalized() (err error) {
 	}
 
 	for _, dataColumn := range dv.dataColumns {
-		// Extract the data column slot.
-		dataColumnSlot := dataColumn.Slot()
-
 		// Check if the data column slot is after first slot of the epoch corresponding to the finalized checkpoint.
-		if dataColumnSlot <= startSlot {
+		if dataColumn.Slot() <= startSlot {
 			return columnErrBuilder(errSlotNotAfterFinalized)
 		}
 	}
@@ -271,10 +268,8 @@ func (dv *RODataColumnsVerifier) SidecarParentSeen(parentSeen func([fieldparams.
 	defer dv.recordResult(RequireSidecarParentSeen, &err)
 
 	for _, dataColumn := range dv.dataColumns {
-		// Extract the root of the parent block corresponding to the data column.
-		parentRoot := dataColumn.ParentRoot()
-
 		// Skip if the parent root has been seen.
+		parentRoot := dataColumn.ParentRoot()
 		if parentSeen != nil && parentSeen(parentRoot) {
 			continue
 		}
@@ -295,10 +290,7 @@ func (dv *RODataColumnsVerifier) SidecarParentValid(badParent func([fieldparams.
 	defer dv.recordResult(RequireSidecarParentValid, &err)
 
 	for _, dataColumn := range dv.dataColumns {
-		// Extract the root of the parent block corresponding to the data column.
-		parentRoot := dataColumn.ParentRoot()
-
-		if badParent != nil && badParent(parentRoot) {
+		if badParent != nil && badParent(dataColumn.ParentRoot()) {
 			return columnErrBuilder(errSidecarParentInvalid)
 		}
 	}
@@ -314,21 +306,15 @@ func (dv *RODataColumnsVerifier) SidecarParentSlotLower() (err error) {
 	defer dv.recordResult(RequireSidecarParentSlotLower, &err)
 
 	for _, dataColumn := range dv.dataColumns {
-		// Extract the root of the parent block corresponding to the data column.
-		parentRoot := dataColumn.ParentRoot()
-
 		// Compute the slot of the parent block.
-		parentSlot, err := dv.fc.Slot(parentRoot)
+		parentSlot, err := dv.fc.Slot(dataColumn.ParentRoot())
 		if err != nil {
 			return columnErrBuilder(errors.Wrap(err, "slot"))
 		}
 
-		// Extract the slot of the data column.
-		dataColumnSlot := dataColumn.Slot()
-
 		// Check if the data column slot is after the parent slot.
-		if parentSlot >= dataColumnSlot {
-			return errSlotNotAfterParent
+		if parentSlot >= dataColumn.Slot() {
+			return columnErrBuilder(errSlotNotAfterParent)
 		}
 	}
 
@@ -435,7 +421,7 @@ func (dv *RODataColumnsVerifier) SidecarProposerExpected(ctx context.Context) (e
 		// Compute the target root for the epoch.
 		targetRoot, err := dv.fc.TargetRootForEpoch(parentRoot, dataColumnEpoch)
 		if err != nil {
-			return [fieldparams.RootLength]byte{}, errors.Wrap(err, "target root from epoch")
+			return [fieldparams.RootLength]byte{}, columnErrBuilder(errors.Wrap(err, "target root from epoch"))
 		}
 
 		// Store the target root in the cache.
@@ -534,7 +520,7 @@ func inclusionProofKey(c blocks.RODataColumn) ([160]byte, error) {
 
 	root, err := c.SignedBlockHeader.HashTreeRoot()
 	if err != nil {
-		return [160]byte{}, errors.Wrap(err, "hash tree root")
+		return [160]byte{}, columnErrBuilder(errors.Wrap(err, "hash tree root"))
 	}
 
 	for i := range c.KzgCommitmentsInclusionProof {

changelog/jtraglia_nits-dcsc-verification.md

@@ -0,0 +1,3 @@
+### Changed
+
+- Fix some nits associated with data column sidecar verification