PeerDAS: Implement core. #15192

nalepae · 2025-04-17T14:01:20Z

Please read commit by commit.

Implement the specification: https://github.com/ethereum/consensus-specs/tree/dev/specs/fulu

What type of PR is this?
Feature

Other notes for review
Depends on:

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-04-18T09:28:34Z

consensus-types/blocks/kzg.go

@@ -80,6 +80,37 @@ func MerkleProofKZGCommitment(body interfaces.ReadOnlyBeaconBlockBody, index int
 	return proof, nil
 }

+// MerkleProofKZGCommitments constructs a Merkle proof of inclusion of the KZG
+// commitments into the Beacon Block with the given `body`
+// TODO: Add missing tests.


Add missing tests.

Added in 43ecbda.

terencechain · 2025-04-20T23:45:20Z

beacon-chain/blockchain/kzg/validation_test.go

+}
+
+// Returns a serialized random field element in big-endian
+func getRandFieldElement(seed int64) [32]byte {


There are two identical implementations, could we reuse?

prysm/testing/util/deneb.go

Line 220 in 774b9a7

func GetRandFieldElement(seed int64) [32]byte {

prysm/beacon-chain/core/peerdas/utils_test.go

Line 38 in 5207750

func getRandFieldElement(seed int64) [32]byte {

This is explained in the corresponding commit message: 972a7a6

Aka, for this test only code, duplicating, while not ideal, is, IMO, better than trying to solve the cyclical dependencies.

Hm I think we might want to fix this dependency. We should be able to import testing/util anywhere so there is a bad design issue outside of this PR. Someone else will probably run into a similar problem. It is not clear to me what the exact cause is. We can postpone this one.

compilepkg: dependency cycle detected between "github.com/OffchainLabs/prysm/v6/beacon-chain/blockchain/kzg" and "github.com/OffchainLabs/prysm/v6/testing/util" in file "/private/var/tmp/_bazel_t/3a219d5e47df5947c1056b3e139abb7c/sandbox/darwin-sandbox/627/execroot/_main/beacon-chain/blockchain/kzg/validation_test.go"

OK, I created this ticket: #15231 for book keeping.

terencechain · 2025-04-20T23:47:06Z

beacon-chain/blockchain/kzg/validation_test.go

+}
+
+// Returns a random blob using the passed seed as entropy
+func getRandBlob(seed int64) GoKZG.Blob {


Same, the implementations are duplicated in deneb.go and util_test.go. We should find a way to consolidate them to avoid copy-pasting each time

This is explained in the corresponding commit message: 972a7a6

Aka, for this test only code, duplicating, while not ideal, is, IMO, better than trying to solve the cyclical dependencies.

beacon-chain/core/peerdas/das_core.go

terencechain · 2025-04-20T23:54:07Z

beacon-chain/core/peerdas/das_core.go

+		}
+
+		sidecar := &ethpb.DataColumnSidecar{
+			Index:                        columnIndex,


Need a rebase to fix these errors

As explained in the PR description, we need #15187 to be merged before rebasing. This dependent PR not be merged is the main reason why this PR is still in draft.

terencechain · 2025-04-20T23:54:35Z

beacon-chain/core/peerdas/das_core.go

+
+var (
+	// Custom errors
+	ErrCustodyGroupTooLarge           = errors.New("custody group too large")


ErrCustodyGroupTooLarge could be private, and maybe exported later

This error in the peerdas_test package. As a consequence, we need to export it else the build is broken.

terencechain · 2025-04-20T23:56:20Z

beacon-chain/core/peerdas/das_core.go

+		// Convert to little endian.
+		currentIdBytesLittleEndian := bytesutil.ReverseByteOrder(currentIdBytesBigEndian[:])
+
+		// Hash the result.


Just a personal preference and a minor nit — I don't think we need comments for code that's already self-explanatory, like hash.Hash

terencechain · 2025-04-21T00:21:17Z

beacon-chain/core/peerdas/das_core.go

+		if currentId.Cmp(maxUint256) == 0 {
+			currentId = uint256.NewInt(0)
+		}


This has a bug where currentId = 0 and currentId.Add(currentId, one) can occur in the same loop iteration due to the for loop’s post-expression. This causes the currentId == 0 case to be skipped entirely.
Spec:

if current_id == UINT256_MAX: # Overflow prevention current_id = uint256(0) else: current_id += 1

I'd implement it like this:

custodyGroups := make(map[uint64]bool, custodyGroupCount) currentID := new(uint256.Int).SetBytes(nodeID.Bytes()) one := uint256.NewInt(1) for uint64(len(custodyGroups)) < custodyGroupCount { input := bytesutil.ReverseByteOrder(currentID.Bytes32()[:]) hashBytes := hash.Hash(input) groupID := binary.LittleEndian.Uint64(hashBytes[:8]) % totalGroups custodyGroups[groupID] = true if currentID.Cmp(maxUint256) == 0 { currentID = uint256.NewInt(0) } else { currentID.Add(currentID, one) } }

😱 Nice catch, thanks!
This bug is even not caught by spec tests, because all spec tests implying the maximum node ID (or close to):

get_custody_groups__max_node_id_max_custody_group_count

get_custody_groups__max_node_id_max_custody_group_count_minus_1

get_custody_groups__max_node_id_min_custody_group_count

either imply 128 custody group counts or 0.

For the test with 0 cgc, we trivially get the expected result.
For tests with 128 cgc, we get the expected result, because the only possible solution is all possible groups.

==> Asked to modify spec tests to have at least one test with max node id but custody group count nor min neither max.

Fixed in fc772d5.

Also, added a shortcut if custodyGroupCount == params.BeaconConfig().NumberOfCustodyGroups - 1, since in this case there is no other solution than using all possible groups (done in c5dbcc9.)

terencechain · 2025-04-21T01:58:20Z

beacon-chain/core/peerdas/das_core.go

+		return nil, errWrongComputedCustodyGroupCount
+	}
+
+	return custodyGroups, nil


The spec returns a sorted list, but this implementation doesn't. It’s likely not consensus-breaking since we're converting from a map, but worth calling out

Fixed in ec56d54.

beacon-chain/core/peerdas/das_core.go

terencechain · 2025-04-21T02:09:33Z

beacon-chain/core/peerdas/das_core.go

+		columns = append(columns, column)
+	}
+
+	return columns, nil


spec sorts columns at the end but I dont see we need to do this as the for loop already sorts it by induction

Agree, that's the reason why I did not sort them.

ethereum/consensus-specs#4289

terencechain · 2025-04-21T02:12:12Z

beacon-chain/core/peerdas/das_core.go

+// https://github.com/ethereum/consensus-specs/blob/v1.5.0-beta.3/specs/fulu/das-core.md#get_data_column_sidecars
+func DataColumnSidecars(signedBlock interfaces.ReadOnlySignedBeaconBlock, cellsAndProofs []kzg.CellsAndProofs) ([]*ethpb.DataColumnSidecar, error) {
+	start := time.Now()
+	if signedBlock == nil || len(cellsAndProofs) == 0 {


I'd recommend signedBlock.IsNil() for nil check

Fixed in 346e7c5.

Actually we need to do both, as done in other parts of the code base.
signedBlock.IsNil() will panic if signedBlock == nil.

terencechain · 2025-04-21T02:32:57Z

beacon-chain/core/peerdas/das_core.go

+	blobsCount := len(cellsAndProofs)
+	sidecars := make([]*ethpb.DataColumnSidecar, 0, numberOfColumns)
+	for columnIndex := range numberOfColumns {
+		column := make([]kzg.Cell, 0, blobsCount)


I think you can preallocate to blobsCount

Hum it is already done thanks to the third argument in

make([]kzg.Cell, 0, blobsCount)

right?

I see. I was thinking about setting the length and assigning values directly by index. I thought it might be slightly faster since there is no append bookkeeping and it avoids accidental appends beyond the intended number. But the difference is extremely small so using make([]T, 0, n) with append is perfectly fine.

terencechain · 2025-04-21T02:33:54Z

beacon-chain/core/peerdas/das_core.go

+			kzgProofOfColumn = append(kzgProofOfColumn, kzgProof)
+		}
+
+		columnBytes := make([][]byte, 0, blobsCount)


I think you can pre allocate here and below as well

Same, the pre allocation is done thanks to the third allocation of

columnBytes := make([][]byte, 0, blobsCount)

right?

third value is the capacity, the second is the allocation I think. instead of having append .
can't you do something like

columnBytes := make([][]byte, blobsCount) // len == cap for i, cell := range column { columnBytes[i] = cell[:] // slice header only – no copy }

See #15192 (comment) and #15192 (comment).

terencechain · 2025-04-21T02:34:25Z

beacon-chain/core/peerdas/das_core.go

+			KzgCommitmentsInclusionProof: kzgCommitmentsInclusionProof,
+		}
+
+		sidecars = append(sidecars, sidecar)


sidecars array can also be pre allocated as well

Sidecar is already pre allocated with

sidecars := make([]*ethpb.DataColumnSidecar, 0, numberOfColumns)

(third argument)

same with the other comment can't you do.

sidecars := make([]*ethpb.DataColumnSidecar, numberOfColumns)
and then

sidecar[I] = sidecar

See #15192 (comment) and #15192 (comment).

terencechain · 2025-04-21T15:34:50Z

beacon-chain/core/peerdas/das_core.go

+// `dataColumnsSidecar` needs to contain the datacolumns corresponding to the non-extended matrix,
+// else an error will be returned.
+// (`dataColumnsSidecar` can contain extra columns, but they will be ignored.)
+func Blobs(indices map[uint64]bool, dataColumnsSidecar []*ethpb.DataColumnSidecar) ([]*blocks.VerifiedROBlob, error) {


How will this function be used?

This can be seen in this PR: #14129.

It is only used in the /eth/v1/beacon/blob_sidecars/{block_id} beacon API handler, where we need to convert data column sidecars into blob sidecars.

terencechain · 2025-04-21T15:53:08Z

beacon-chain/core/peerdas/das_core.go

+	}
+
+	// Filter blobs index higher than the blob count.
+	filteredIndices := make(map[uint64]bool, len(indices))


couldn't you do this in one for loop and without the extra allocations for filtered indices. Example:

indicesSlice := make([]uint64, 0, len(indices)) for i := range indices { if i < blobCount { indicesSlice = append(indicesSlice, i) } } slices.Sort(indicesSlice)

Fixed in 059b33e.

terencechain · 2025-04-21T16:00:43Z

beacon-chain/core/peerdas/info.go

+	// If the value is already in the cache, return it.
+	if value, ok := nodeInfoCache.Get(key); ok {
+		peerInfo, ok := value.(*info)
+		if !ok {
+			return nil, false, errors.New("failed to cast peer info (should never happen)")
+		}
+
+		return peerInfo, true, nil


Disclaimer: I'm pessimistic about caching, it's easy to get wrong and hard to debug. The real question is whether caching is necessary here. What's the bottleneck that justifies it? It doesn't hurt to merge without a cache first, monitor live performance, identify real bottlenecks, and then design a cache around that.

The real question is whether caching is necessary here

I think, yes.
We particularly want to avoid to re-run again an again this function, which is quite heavy for 2 reasons:

It uses a lot of hashes in a while loop:

... while len(custody_groups) < custody_group_count: custody_group = CustodyIndex( bytes_to_uint64(hash(uint_to_bytes(current_id))[0:8]) % NUMBER_OF_CUSTODY_GROUPS ) ...

And this loop is pure brute force until we like the result because of the if condition. It can be quite expensive for high custody_group_count:

... while len(custody_groups) < custody_group_count: ... if custody_group not in custody_groups: custody_groups.append(custody_group) ...

Note: I implemented a hack for the case custody_group_count == NUMBER_OF_CUSTODY_GROUPS, so it is now very cheap to compute. But the case custody_group_count == NUMBER_OF_CUSTODY_GROUPS - 1 is still very expensive to compute.

Also, two additional points:

The used cache (lru_cache from Hashicorp) is well tested and already (heavily) used in the Prysm codebase.

func Info(nodeID enode.ID, custodyGroupCount uint64) is a pure mathematical function: It will always return the same result for the same inputs: No side effects. So, IMO, there is no fear to have using a cache in this case.

Or maybe am I missing something?

Nah, I just don't like caches because they make bugs easy, but your explanation makes sense. I rest my case

terencechain · 2025-04-21T16:02:57Z

beacon-chain/core/peerdas/p2p_interface.go

+
+// VerifyDataColumnSidecar verifies if the data column sidecar is valid.
+// https://github.com/ethereum/consensus-specs/blob/v1.5.0-beta.5/specs/fulu/p2p-interface.md#verify_data_column_sidecar
+func VerifyDataColumnSidecar(sidecar blocks.RODataColumn) error {


We would typically have this under the verification package

The content of the peerdas packages is consistent with the spec.
For example, functions in https://github.com/ethereum/consensus-specs/blob/v1.5.0-beta.5/specs/fulu/p2p-interface.md are implemented in peerdas/p2p_interface.go.

But, indeed, this function (VerifyDataColumnSidecar) is used in the verification package. (To be merged later.)

Ya, I think we typically have p2p-interface validation logic in verification package or in sync directly but dont feel strongly here

terencechain · 2025-04-23T17:34:45Z

beacon-chain/core/peerdas/info.go

+		CustodyGroups      map[uint64]bool
+		CustodyColumns     map[uint64]bool
+		DataColumnsSubnets map[uint64]bool


Two open questions

Do we really need all three sets? At a minimum, custody groups or columns might be sufficient since everything else can be derived from them

Is a map the right structure for this, or would a list or sorted uint64 work better?

DataColumnsSubnets can be recomputed from CustodyColumns, and CustodyColumns can be recomputed from CustodyGroups. But as said here, computing CustodyGroups is quite heavy. And also, computing CustodyColumns from CustodyGroups and DataColumnsSubnets from CustodyColumns is done with pure mathematical functions. So, once we have CustodyGroups cached, it's quite convenient to cache the 2 others, to avoid recomputing them again and again in multiple locations in the codebase.

The advantage of using an uint64 set (which is represented by a map[uint64]bool in go since there is no builtin set), is to be able to know if a value is in this set in an O(1) fashion. For a sorted list, is is O(log(n)). (So up to 7x more operations for 128 items.) IMO, using a set is strictly better in our case, since we often need to look if a given value is in the struct.

Right, but I think although map gives O(1) lookup, it is not free in practice because of memory overhead and GC pressure compared to slices. Lookup time for small n is likely negligible, and []uint64 is a flat contiguous block of memory. I think maps are optimized for unknown or large-scale sets, and for small predictable sets, slices might be better
Don't feel strongly about this one, just my 2c

terencechain · 2025-04-23T17:35:38Z

beacon-chain/core/peerdas/p2p_interface.go

+	commitments := make([]kzg.Bytes48, 0, count)
+	indices := make([]uint64, 0, count)
+	cells := make([]kzg.Cell, 0, count)
+	proofs := make([]kzg.Bytes48, 0, count)


I think we can just pre-allocate the slice here

They are actually pre allocated thanks to the third argument.

Extract of the go doc for make:

The make built-in function allocates and initializes an object of type slice, map, or chan (only). Like new, the first argument is a type, not a value. Unlike new, make's return type is the same as the type of its argument, not a pointer to it. The specification of the result depends on the type:

Slice: The size specifies the length. The capacity of the slice is equal to its length. A second integer argument may be provided to specify a different capacity; it must be no smaller than the length. For example, make([]int, 0, 10) allocates an underlying array of size 10 and returns a slice of length 0 and capacity 10 that is backed by this underlying array.

...

ya I think you are right here, it's my misunderstanding, please ignore all the relevant feedback about pre alloctaions

terencechain · 2025-04-23T17:44:45Z

beacon-chain/core/peerdas/p2p_interface.go

+	columnsPerGroup := numberOfColumns / numberOfCustodyGroups
+	return columnIndex / columnsPerGroup, nil


This implementation is not consistent with ComputeColumnsForCustodyGroup which uses
column := numberOfCustodyGroup*i + custodyGroup
That means column c belongs to custody group c % numberOfCustodyGroups
So to compute the custody group for a column index, the logic should be:
return columnIndex % numberOfCustodyGroups, nil

😱 Nice catch, thanks!

Actually unit tests are OK as well, since my logic was both wrong for the function itself and the tests...
(Note this bug luckily does not currently impact the devnet, since there is 128 groups and 128 columns, so columnIndex / columnsPerGroup == columnIndex % numberOfCustodyGroups == columnIndex.)

Fixed in abe4831.

terencechain · 2025-04-23T17:45:39Z

beacon-chain/core/peerdas/peer_sampling.go

+	"github.com/OffchainLabs/prysm/v6/config/params"
+)
+
+// ExtendedSampleCount computes, for a given number of samples per slot and allowed failures the


Should we wait to merge these and focus on custody sampling first?

The whole peer sampling code is currently not used at all.
However, it is already implemented and well unit tested, so there is no harm to merge it as is.
Note I'm not strongly against not to merge it right now. (No strong opinion about it.)

I'm not against merging it, but I probably won't review it here because this PR is already big enough and already contains a lot of critical logic to review. We can either merge it to develop and hope someone who uses it later will review it, or merge it later in another peerDAS to develop PR. Up to you!

Removed in 2543120.

Peer sampling is not lost, since still living in the peerDAS branch.
For now, let's not merge it into develop and postpone the decision to later.

terencechain · 2025-04-23T17:58:45Z

beacon-chain/core/peerdas/util.go

+		var proofs []kzg.Proof
+		for idx := i * numColumns; idx < (i+1)*numColumns; idx++ {
+			proofs = append(proofs, kzg.Proof(cellProofs[idx]))
+		}
+
+		cellsAndProofs = append(cellsAndProofs, kzg.CellsAndProofs{
+			Cells:  cells,
+			Proofs: proofs,
+		})
+	}


The whole idea of constructCellsAndProofs and the cellsAndProofs struct feels a bit extra to me. We could just compute the cells directly in ConstructDataColumnSidecars and pass (block, cells, cellProofs) into DataColumnSidecars, which can handle everything internally. Is there any benefit to keeping the cells and proofs in their own struct again?

For the cellsAndProofs struct, it is at least needed if we want to stick to the spec for get_data_column_sidecars.

The Python spec uses a single cells_and_kzg_proofs argument which is a list of tuples of cells and proofs.
Tuples do not exist in go. The closer we can do is the CellsAndProofs struct.

Now this CellsAndProofs is defined to stick to the spec, IMO it's better to re-use it in other locations in the codebase rather than using []Cell and []Proof separately.

Removed the constructCellsAndProofs function in 34236e2.

james-prysm · 2025-05-01T14:54:24Z

beacon-chain/core/peerdas/p2p_interface.go

+// VerifyDataColumnSidecarInclusionProof verifies if the given KZG commitments included in the given beacon block.
+// https://github.com/ethereum/consensus-specs/blob/v1.5.0-beta.5/specs/fulu/p2p-interface.md#verify_data_column_sidecar_inclusion_proof
+func VerifyDataColumnSidecarInclusionProof(sidecar blocks.RODataColumn) error {
+	if sidecar.SignedBlockHeader == nil || sidecar.SignedBlockHeader.Header == nil {


do you not need to verify the sidecar itself? i think we've used IsEmpty() function in the past for these

There is no IsEmpty (or IsNil) function defined for blocks.RODataColumn).

do you think there should be?

Hum not sure.
A IsNil function would check if every field is non nil right?
Here we just need the Header (and all its parents) to be non nil.

james-prysm · 2025-05-01T15:21:29Z

consensus-types/blocks/kzg.go

-func leavesFromCommitments(commitments [][]byte) [][]byte {
+// MerkleProofKZGCommitments constructs a Merkle proof of inclusion of the KZG
+// commitments into the Beacon Block with the given `body`
+func MerkleProofKZGCommitments(body interfaces.ReadOnlyBeaconBlockBody) ([][]byte, error) {


i'm surprised we didn't have something like this for deneb is this new?

For Deneb we had MerkleProofKZGCommitment(without the terminal s) defined. This function is defined just before.

james-prysm · 2025-05-01T16:29:00Z

beacon-chain/core/peerdas/util.go

+
+// ConstructDataColumnSidecars constructs data column sidecars from a block, blobs and their cell proofs.
+// This is a convenience method as blob and cell proofs are common inputs.
+func ConstructDataColumnSidecars(block interfaces.SignedBeaconBlock, blobs [][]byte, cellProofs [][]byte) ([]*ethpb.DataColumnSidecar, error) {


I don't think this function is used anywhere, should we include this right now ( I get it's in core )

It is not. It is used on on the peerDAS branch. Will be merged in a future PR.

james-prysm · 2025-05-02T19:17:22Z

beacon-chain/core/peerdas/das_core.go

+		return nil, errors.Wrap(err, "blob KZG commitments")
+	}
+
+	if len(blobKzgCommitments) != len(cellsAndProofs) {


is 0 ok here? or do we need to exit early ever? i think if it's 0 here then we still set the metric for datacolumn time

Yeah appart for the wrong metric it's OK to have 0.
However:

as you said the metric is now misleading, and

we run some functions for nothing: Ex signedBlock.Header.

==> Better to exit early if 0.

Actually I was already checked at the beginning of the function:

if signedBlock == nil || signedBlock.IsNil() || len(cellsAndProofs) == 0 { return nil, nil }

but not tested.

I added a test for that (and one extra test for sizes mismatch) in 1d8fb51.

james-prysm · 2025-05-02T19:41:11Z

beacon-chain/core/peerdas/das_core.go

+
+		kzgProofOfColumnBytes := make([][]byte, 0, blobsCount)
+		for _, kzgProof := range kzgProofOfColumn {
+			copiedProof := kzgProof


do we need to do this?

Hum it was more a safety feature (at but at the cost of a copy).

I removed it and added a warning in the godoc, in ea5e0e8.

james-prysm · 2025-05-02T20:03:37Z

beacon-chain/core/peerdas/das_core.go

+
+	// Get missing columns.
+	if actualColumnCount < neededColumnCount {
+		missingColumns := make(map[uint64]bool, neededColumnCount-actualColumnCount)


do you actually need this map? you could do

for I :=range neededColumnCount { if _, ok := sliceIndexFromColumnIndex[i]; !ok { missing = append(missing, i) } } if len(missing) > 0 { return nil, fmt.Errorf("some columns are missing: %v", missing) } ```

Fixed in 8dac003.

james-prysm · 2025-05-02T20:16:00Z

beacon-chain/core/peerdas/reconstruction.go

+	recoveredCellsAndProofs := make([]kzg.CellsAndProofs, blobCount)
+
+	for blobIndex := 0; blobIndex < blobCount; blobIndex++ {
+		bIndex := blobIndex


do we need this?

See #15192 (comment).

james-prysm · 2025-05-02T20:17:30Z

beacon-chain/core/peerdas/reconstruction.go

+	return recoveredCellsAndProofs, nil
+}
+
+// DataColumnSidecarsForReconstruct is a TEMPORARY function until there is an official specification for it.


should we add like a todo on removal? i missed that this is temporary

Fixed in c87701e.

Actually in the (not yet merged) code using the peerDAS package, we need both to reconstruct data column sidecars from a signed block (as the spec specifies) and from signed block header, blob KZG commitments and KZG commitment inclusion proofs, which can be extracted from the signed block.

==> The heavy duty is now done by DataColumnsSidecarsFromItems, which is called by DataColumnSidecars.

james-prysm · 2025-05-02T20:19:07Z

beacon-chain/core/peerdas/util.go

+
+	for i, blob := range blobs {
+		var b kzg.Blob
+		copy(b[:], blob)


do we actually need to do this?

Yes, because b is of kzg.Blob type, and blob is of []byte type.
An other way to do this would be to write:

b := kzg.Blob(blob)

but the later can panic, contrary to the former (and a copy is done in both cases).

james-prysm · 2025-05-02T20:20:27Z

beacon-chain/core/peerdas/validator.go

+func ValidatorsCustodyRequirement(state beaconState.ReadOnlyBeaconState, validatorsIndex map[primitives.ValidatorIndex]bool) (uint64, error) {
+	totalNodeBalance := uint64(0)
+	for index := range validatorsIndex {
+		validator, err := state.ValidatorAtIndex(index)


this should be ValidatorAtIndexReadOnly

Fixed in 2d4d8d1.

james-prysm · 2025-05-02T20:51:35Z

beacon-chain/core/peerdas/das_core.go

+
+	// Filter blobs index higher than the blob count.
+	indicesSlice := make([]uint64, 0, len(indices))
+	for i := range indices {


what do we do if indices is less than blobcount? or is that not possible?

Sorry not sure to understand.
Here we want to only keep indices lower than blobCount.

So if an index is lower than blobCount, we keep it, else we filter it out.

I meant what do we do if the number of indices keys in the map in the argument is less than the number for blobCount, and if that's possible.

nvm i misunderstood, as long as there is 1 its ok

james-prysm · 2025-05-05T16:38:16Z

beacon-chain/core/peerdas/das_core.go

+			dataColumnSideCar := dataColumnsSidecar[sliceIndex]
+			cell := dataColumnSideCar.Column[blobIndex]
+
+			for i := range cell {


what if this code was changed to the following?

cell := dataColumnsSidecar[sliceIdx].Column[blobIndex] if len(cell) != kzg.BytesPerCell { return nil, fmt.Errorf("column %d: expected %d‑byte cell, got %d", columnIndex, kzg.BytesPerCell, len(cell)) } offset := uint64(columnIndex)*uint64(kzg.BytesPerCell) end := offset + uint64(len(cell)) if end > uint64(kzg.BytesPerBlob) { return nil, fmt.Errorf("column %d overflows blob (%d > %d)", columnIndex, end, kzg.BytesPerBlob) } copy(blob[offset:end], cell) ``` do you think we need these checks?

I don't think we beed these checks, since all values are already trusted, either because recomputed by ourself, either because verified in data column sidecars pipeline if coming from a peer.

james-prysm · 2025-05-05T17:00:19Z

beacon-chain/core/peerdas/das_core.go

+func CustodyColumns(custodyGroups []uint64) (map[uint64]bool, error) {
+	numberOfCustodyGroups := params.BeaconConfig().NumberOfCustodyGroups
+
+	custodyGroupCount := len(custodyGroups)


is this correct? you loop over groupColumns that are calculated and set to true, not the custodyColumns length.

I think it is correct:

For each group, we compute columns.

For each column (belonging to a group), we add it to the columns map.

it's not possible for the columns from the groupColumns to be a different number than custodyGroupsCount? i guess it will just add to the map but it would look strange

james-prysm · 2025-05-05T19:13:12Z

beacon-chain/core/peerdas/reconstruction.go

+// RecoverCellsAndProofs recovers the cells and proofs from the data column sidecars.
+func RecoverCellsAndProofs(
+	dataColumnSideCars []*ethpb.DataColumnSidecar,
+	blockRoot [fieldparams.RootLength]byte,


why is blockRoot not used in this instance?

Because we don't need it :)

Fixed in 182b5d7.

james-prysm

That's all I was able to comment on, LGTM since some others have reviewed for architecture specific

terencechain · 2025-05-06T21:30:29Z

Approved based on the issues I found and fixed, but feel free to get few more eyes on it. This was a long and critical PR to review, and given the size and constraints, I’m not confident that myself is enough for a thorough, high-confidence review

nalepae · 2025-05-06T21:36:55Z

We have @nisdas who reviewed it before merging in the peerdas branch, then @terencechain and @james-prysm .

I guess we can merge now .

nalepae added the peerDAS label Apr 17, 2025

nalepae marked this pull request as draft April 17, 2025 14:01

nalepae mentioned this pull request Apr 17, 2025

Implement all needed KZG wrappers for peerDAS in the kzg package. #15186

Merged

3 tasks

nalepae force-pushed the peerdas-core branch 6 times, most recently from bd44664 to a0d0b93 Compare April 18, 2025 09:27

nalepae commented Apr 18, 2025

View reviewed changes

nalepae force-pushed the peerdas-core branch 3 times, most recently from 690c1ed to 5207750 Compare April 18, 2025 10:08

terencechain requested changes Apr 21, 2025

View reviewed changes

terencechain reviewed Apr 21, 2025

View reviewed changes

beacon-chain/core/peerdas/das_core.go Outdated Show resolved Hide resolved

terencechain reviewed Apr 21, 2025

View reviewed changes

terencechain reviewed Apr 23, 2025

View reviewed changes

james-prysm reviewed May 1, 2025

View reviewed changes

james-prysm reviewed May 2, 2025

View reviewed changes

nalepae added 2 commits May 5, 2025 11:38

DataColumnSidecars: Add missing tests.

1d8fb51

Fix Jame's comment.

ea5e0e8

nalepae force-pushed the peerdas-core branch from ddb47e4 to ea5e0e8 Compare May 5, 2025 10:03

nalepae added 3 commits May 5, 2025 12:20

Fix James' comment.

8dac003

Fix James' comment.

c87701e

Fix James' coment.

2d4d8d1

nalepae requested a review from terencechain May 5, 2025 12:13

james-prysm reviewed May 5, 2025

View reviewed changes

Fix James' comment.

182b5d7

james-prysm approved these changes May 6, 2025

View reviewed changes

terencechain approved these changes May 6, 2025

View reviewed changes

nalepae added this pull request to the merge queue May 6, 2025

Merged via the queue into develop with commit 7da7019 May 6, 2025
15 checks passed

nalepae deleted the peerdas-core branch May 6, 2025 23:39

		columnsPerGroup := numberOfColumns / numberOfCustodyGroups
		return columnIndex / columnsPerGroup, nil

PeerDAS: Implement core. #15192

PeerDAS: Implement core. #15192

Uh oh!

Conversation

nalepae commented Apr 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

nalepae Apr 26, 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!

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!

nalepae Apr 27, 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!

nalepae Apr 27, 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nalepae Apr 28, 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!

nalepae Apr 26, 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!

nalepae Apr 26, 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!

nalepae May 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

nalepae commented Apr 17, 2025 •

edited

Loading

nalepae Apr 26, 2025 •

edited

Loading

nalepae Apr 27, 2025 •

edited

Loading

nalepae Apr 27, 2025 •

edited

Loading

nalepae Apr 28, 2025 •

edited

Loading

nalepae Apr 26, 2025 •

edited

Loading

nalepae Apr 26, 2025 •

edited

Loading

nalepae May 4, 2025 •

edited

Loading

nalepae May 4, 2025 •

edited

Loading

nalepae Apr 26, 2025 •

edited

Loading