Implement Data Columns By Range Request And Response Methods #13972

nisdas · 2024-05-09T10:54:19Z

What type of PR is this?

Feature

What does this PR do? Why is it needed?

This adds support for the newly added RPC method for columns by range here:
ethereum/consensus-specs#3750

Prysm nodes are now able to serve column by range methods along with all the relevant streaming, and validation of these columns.
During syncing we can now start utilizing the column by range request method to get the respective custodied columns from other peers.

Which issues(s) does this PR fix?

Requires #13945 to be merged first

Other notes for review

beacon-chain/core/peerdas/helpers.go

nalepae · 2024-05-10T08:10:47Z

beacon-chain/rpc/prysm/v1alpha1/validator/proposer.go

+		// See https://golang.org/doc/faq#closures_and_goroutines for more details.
+		colIdx := i
+		sidecar := sd
+		eg.Go(func() error {


Do we still need the to run s.internalBroadcastDataColumn in a goroutine here?

func (s *Service) BroadcastDataColumn(ctx context.Context, columnSubnet uint64, dataColumnSidecar *ethpb.DataColumnSidecar) error { // Add tracing to the function. ctx, span := trace.StartSpan(ctx, "p2p.BroadcastBlob") defer span.End() // Ensure the data column sidecar is not nil. if dataColumnSidecar == nil { return errors.Errorf("attempted to broadcast nil data column sidecar at subnet %d", columnSubnet) } // Retrieve the current fork digest. forkDigest, err := s.currentForkDigest() if err != nil { err := errors.Wrap(err, "current fork digest") tracing.AnnotateError(span, err) return err } // Non-blocking broadcast, with attempts to discover a column subnet peer if none available. go s.internalBroadcastDataColumn(ctx, columnSubnet, dataColumnSidecar, forkDigest) return nil }

(We now have a goroutine calling an other goroutine.)

nalepae · 2024-05-10T08:19:40Z

beacon-chain/sync/rpc_data_column_sidecars_by_range.go

+
+// dataColumnSidecarsByRangeRPCHandler looks up the request data columns from the database from a given start slot index
+func (s *Service) dataColumnSidecarsByRangeRPCHandler(ctx context.Context, msg interface{}, stream libp2pcore.Stream) error {
+	var err error


nalepae · 2024-05-10T08:33:51Z

beacon-chain/sync/rpc_data_column_sidecars_by_range.go

+	maxRequest := params.MaxRequestBlock(slots.ToEpoch(current))
+	// Allow some wiggle room, up to double the MaxRequestBlocks past the current slot,
+	// to give nodes syncing close to the head of the chain some margin for error.
+	maxStart, err := current.SafeAdd(maxRequest * 2)


I don't really understand this. Why this would be ever the case?

The caller knows the current slot. So why would it send a request with a start > currentSlot?

nalepae · 2024-05-10T08:42:34Z

beacon-chain/sync/rpc_data_column_sidecars_by_range.go

+	if rp.end > current {
+		rp.end = current
+	}
+	if rp.end < rp.start {


Should not this be considered as an invalid request?

nalepae · 2024-05-10T08:52:20Z

beacon-chain/sync/rpc_send_request.go

+	return roDataColumns, nil
+}
+
+func SendDataColumnsByRangeRequest(ctx context.Context, tor blockchain.TemporalOracle, p2pApi p2p.P2P, pid peer.ID, ctxMap ContextByteVersions, req *pb.DataColumnSidecarsByRangeRequest) ([]blocks.RODataColumn, error) {


Is the following condition checked?

The following data column sidecars, where they exist, MUST be sent in (slot, column_index) order.

nalepae · 2024-05-10T14:27:20Z

Some entry is probably missing here.

And probably here as well.

nalepae · 2024-05-12T07:00:23Z

consensus-types/blocks/roblock.go

@@ -98,9 +98,11 @@ func (s ROBlockSlice) Len() int {

 // BlockWithROBlobs is a wrapper that collects the block and blob values together.
 // This is helpful because these values are collated from separate RPC requests.
+// TODO: Use a more generic name


Proposal: BlockWithROSidecars?

nalepae · 2024-05-12T07:05:15Z

beacon-chain/sync/initial-sync/blocks_fetcher.go

+	if len(commits) == 0 {
+		return bw, errDidntPopulate
+	}
+	if len(columns) != int(params.BeaconConfig().CustodyRequirement) {


Could not this condition be tested at the beginning of the function as this condition only involves functions parameters and params?

nalepae · 2024-05-12T07:08:53Z

beacon-chain/sync/verify/blob.go

+		return nil
+	}
+	if col.ColumnIndex >= fieldparams.NumberOfColumns {
+		return errors.Wrapf(ErrIncorrectColumnIndex, "index %d exceeds NUMBERS_OF_COLUMN %d", col.ColumnIndex, fieldparams.MaxBlobsPerBlock)


Is fieldparams.MaxBlobsPerBlock the wanted value?

nalepae · 2024-05-12T07:11:03Z

beacon-chain/sync/initial-sync/blocks_fetcher.go

+func (f *blocksFetcher) fetchColumnsFromPeer(ctx context.Context, bwb []blocks2.BlockWithROBlobs, pid peer.ID, peers []peer.ID) ([]blocks2.BlockWithROBlobs, error) {
+	ctx, span := trace.StartSpan(ctx, "initialsync.fetchColumnsFromPeer")
+	defer span.End()
+	if slots.ToEpoch(f.clock.CurrentSlot()) < params.BeaconConfig().DenebForkEpoch {


f.clock.CurrentSlot() is called several times in the function.
It can be factorized in a currentSlot variable.

nalepae · 2024-05-12T07:13:37Z

beacon-chain/sync/initial-sync/blocks_fetcher.go

+	if err != nil {
+		return nil, err
+	}
+	var colIdxs []uint64


Could be a make with fixed size/capacity to avoid possible eventual re-allocation.

nalepae · 2024-05-12T07:18:13Z

beacon-chain/das/availbility_columns.go

@@ -0,0 +1,151 @@
+package das


Typo in file name. (a missing)

nalepae · 2024-05-12T07:23:31Z

beacon-chain/das/availbility_columns.go

+	return nil
+}
+
+// IsDataAvailable returns nil if all the commitments in the given block are persisted to the db and have been verified.


Some references to blob while they should probably be references to data columns.
I find disturbing the fact that a function named IsDataAvailable actually saves something into the database.

* Add Data Structure for New Request Type * Add Data Column By Range Handler * Add Data Column Request Methods * Add new validation for columns by range requests * Fix Build * Allow Prysm Node To Fetch Data Columns * Allow Prysm Node To Fetch Data Columns And Sync * Bug Fixes For Interop * GoFmt * Use different var * Manu's Review

nisdas requested a review from a team as a code owner May 9, 2024 10:54

nisdas requested review from kasey, nalepae and prestonvanloon and removed request for a team May 9, 2024 10:54

nisdas added Ready For Review A pull request ready for code review peerDAS labels May 9, 2024

nalepae reviewed May 10, 2024

View reviewed changes

nalepae reviewed May 12, 2024

View reviewed changes

beacon-chain/das/availbility_columns.go Outdated

@@ -0,0 +1,151 @@

package das

Copy link

Contributor

nalepae May 12, 2024

Choose a reason for hiding this comment

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

Typo in file name. (a missing)

nalepae reviewed May 12, 2024

View reviewed changes

nisdas added 7 commits May 13, 2024 16:14

Add Data Structure for New Request Type

0e34219

Add Data Column By Range Handler

363d7fd

Add Data Column Request Methods

64e57b9

Add new validation for columns by range requests

66e6f22

Fix Build

a987995

Allow Prysm Node To Fetch Data Columns

e13a518

Allow Prysm Node To Fetch Data Columns And Sync

7b0eeab

nisdas force-pushed the dataColumnsbyRangeRequest branch from dc59fdf to 7b0eeab Compare May 13, 2024 08:16

nisdas added 4 commits May 14, 2024 23:01

Bug Fixes For Interop

d6f9db6

GoFmt

c6a312d

Use different var

03781e0

Manu's Review

2f0d998

nisdas merged commit e2c64e6 into peerDAS May 14, 2024
8 of 12 checks passed

nisdas deleted the dataColumnsbyRangeRequest branch May 14, 2024 16:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Data Columns By Range Request And Response Methods #13972

Implement Data Columns By Range Request And Response Methods #13972

nisdas commented May 9, 2024

nalepae May 10, 2024

nalepae May 10, 2024

nalepae May 10, 2024

nalepae May 10, 2024

nalepae May 10, 2024

nalepae May 10, 2024

nalepae May 10, 2024

nalepae commented May 10, 2024 •

edited

Loading

nalepae May 12, 2024

nalepae May 12, 2024

nalepae May 12, 2024

nalepae May 12, 2024

nalepae May 12, 2024 •

edited

Loading

nalepae May 12, 2024

nalepae May 12, 2024

Implement Data Columns By Range Request And Response Methods #13972

Implement Data Columns By Range Request And Response Methods #13972

Conversation

nisdas commented May 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nalepae commented May 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nalepae May 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nalepae commented May 10, 2024 •

edited

Loading

nalepae May 12, 2024 •

edited

Loading