Add DA Check For Data Columns #13938

nisdas · 2024-04-30T06:27:41Z

What type of PR is this?

Feature Work

What does this PR do? Why is it needed?

As part of the peerDAS P.O.C, this PR modifies our data availability check to ensure that we see the correct amount of data columns from gossip that we are custodied to. In the event we do not, the block fails block processing and is dropped.

Which issues(s) does this PR fix?

N.A

Other notes for review

beacon-chain/blockchain/process_block.go

nalepae · 2024-04-30T09:00:52Z

beacon-chain/blockchain/process_block.go

+	if len(expected) > int(params.BeaconConfig().NumberOfColumns) {
+		return nil, errMaxDataColumnsExceeded
+	}
+	indices, err := bs.Indices(root)


Not sure it works as expected here: indices is of type [fieldparams.MaxBlobsPerBlock]bool and fieldparams.MaxBlobsPerBlock == 6.

Ah, great catch will need to fix it

nalepae · 2024-04-30T09:27:11Z

beacon-chain/blockchain/process_block.go

 // isDataAvailable blocks until all BlobSidecars committed to in the block are available,
 // or an error or context cancellation occurs. A nil result means that the data availability check is successful.
 // The function will first check the database to see if all sidecars have been persisted. If any
 // sidecars are missing, it will then read from the blobNotifier channel for the given root until the channel is
 // closed, the context hits cancellation/timeout, or notifications have been received for all the missing sidecars.
 func (s *Service) isDataAvailable(ctx context.Context, root [32]byte, signed interfaces.ReadOnlySignedBeaconBlock) error {
+	if features.Get().EnablePeerDAS {
+		return s.isDataAvailableDataColumns(ctx, root, signed)
+	}
 	if signed.Version() < version.Deneb {


Maybe create a s.isDataAbiableBlobs so blobs and data columns are treated equally?

Not straightforward due to how we only need a subset of the columns vs all of the blobs. But i can see if its possible

beacon-chain/blockchain/process_block.go

Co-authored-by: Manu NALEPA <enalepa@offchainlabs.com>

nalepae · 2024-04-30T12:57:03Z

config/fieldparams/mainnet.go

@@ -33,4 +33,5 @@ const (
 	BlobSize                              = 131072        // defined to match blob.size in bazel ssz codegen
 	KzgCommitmentInclusionProofDepth      = 17            // Merkle proof depth for blob_kzg_commitments list item
 	NextSyncCommitteeBranchDepth          = 5             // NextSyncCommitteeBranchDepth defines the depth of the next sync committee branch.
+	NumberOfColumns                       = 128           // NumberOfColumns refers to the specified number of data columns that can exist in a network.


Defined in cKzg4844.CellsPerExtBlob. (Should we redefine it here?)

nalepae · 2024-04-30T12:57:09Z

config/fieldparams/minimal.go

@@ -33,4 +33,5 @@ const (
 	BlobSize                              = 131072        // defined to match blob.size in bazel ssz codegen
 	KzgCommitmentInclusionProofDepth      = 17            // Merkle proof depth for blob_kzg_commitments list item
 	NextSyncCommitteeBranchDepth          = 5             // NextSyncCommitteeBranchDepth defines the depth of the next sync committee branch.
+	NumberOfColumns                       = 128           // NumberOfColumns refers to the specified number of data columns that can exist in a network.


Defined in cKzg4844.CellsPerExtBlob. (Should we redefine it here?)

* Add new DA check * Exit early in the event no commitments exist. * Gazelle * Fix Mock Broadcaster * Fix Test Setup * Update beacon-chain/blockchain/process_block.go Co-authored-by: Manu NALEPA <enalepa@offchainlabs.com> * Manu's Review * Fix Build --------- Co-authored-by: Manu NALEPA <enalepa@offchainlabs.com>

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

nisdas requested a review from a team as a code owner April 30, 2024 06:27

nisdas requested review from potuz, terencechain and rkapka and removed request for a team April 30, 2024 06:27

nalepae reviewed Apr 30, 2024

View reviewed changes

beacon-chain/blockchain/process_block.go Outdated Show resolved Hide resolved

nalepae reviewed Apr 30, 2024

View reviewed changes

beacon-chain/blockchain/process_block.go Show resolved Hide resolved

nisdas and others added 7 commits April 30, 2024 20:47

Add new DA check

117037e

Exit early in the event no commitments exist.

cc939fa

Gazelle

41ddd0d

Fix Mock Broadcaster

2ccf258

Fix Test Setup

9453e9e

Update beacon-chain/blockchain/process_block.go

bf42972

Co-authored-by: Manu NALEPA <enalepa@offchainlabs.com>

Manu's Review

d5501ac

nisdas force-pushed the modifyDACheck branch from b114cbe to d5501ac Compare April 30, 2024 12:48

Fix Build

983c874

nalepae reviewed May 2, 2024

View reviewed changes

nalepae approved these changes May 2, 2024

View reviewed changes

nisdas merged commit f2d956d into peerDAS May 2, 2024
15 of 16 checks passed

nisdas deleted the modifyDACheck branch May 2, 2024 07:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add DA Check For Data Columns #13938

Add DA Check For Data Columns #13938

nisdas commented Apr 30, 2024

nalepae Apr 30, 2024

nisdas Apr 30, 2024

nalepae Apr 30, 2024

nisdas Apr 30, 2024

nalepae Apr 30, 2024

nalepae Apr 30, 2024

Add DA Check For Data Columns #13938

Add DA Check For Data Columns #13938

Conversation

nisdas commented Apr 30, 2024

nalepae Apr 30, 2024

Choose a reason for hiding this comment

nisdas Apr 30, 2024

Choose a reason for hiding this comment

nalepae Apr 30, 2024

Choose a reason for hiding this comment

nisdas Apr 30, 2024

Choose a reason for hiding this comment

nalepae Apr 30, 2024

Choose a reason for hiding this comment

nalepae Apr 30, 2024

Choose a reason for hiding this comment