chore: Encapsulate all kzg functionality for PeerDAS into the kzg package #14136

kevaundray · 2024-06-24T15:22:22Z

What type of PR is this?

Other (refactor)

What does this PR do? Why is it needed?

This PR removes ckzg4844 from the public API and puts it behind a kzg API. It means that the rest of the code becomes more modular as it is not tied to the implementation details of a particular cryptographic library.

This could also be done for Go4844, though I left it as is since that would need to be done on the develop branch.

Status

This PR still needs a bit of cleanup, in some places we accept Bytes48 and in others Commitment for example. Putting it up to get thoughts on this direction. I've also put everything in validation.go for now

CLAassistant · 2024-06-24T15:22:28Z

All committers have signed the CLA.

kevaundray · 2024-06-24T15:59:30Z

beacon-chain/sync/data_columns_reconstruct.go

-		recoveredCells, err := cKzg4844.RecoverAllCells(cellsId, cKzgCells)
+		recoveredCells, err := kzg.RecoverAllCells(cellsId, cKzgCells)
 		if err != nil {
 			return nil, errors.Wrapf(err, "recover all cells for blob %d", blobIndex)
 		}

-		recoveredBlob, err := cKzg4844.CellsToBlob(recoveredCells)
+		recoveredBlob, err := kzg.CellsToBlob(recoveredCells)


The usecase for calling these two methods is to eventually compute the CellsAndKZGProofs -- I would recommend adding a RecoverCellsAndComputeKZGProofs method which does this, so that when libraries are updated there is not much that needs to be modified.

Pseudocode to clarify

func RecoverCellsAndKZGProofs(cells []Cell) ([]Cells, []Proofs, error) { recoveredCells, err := ckzg.RecoverAllCells(cellsIds, cells) // This is no longer called directly in beacon-chain recoveredBlob, err := ckzg.CellsToBlob(recoveredCells) // This is no longer called directly in beacon-chain cells, proofs, err := ckzg.ComputeCellsAndProofs(recoveredBlob) return cells, proofs, err }

This is addressed in #14160

kevaundray · 2024-06-25T13:33:06Z

Setting it as ready to get feedback, before polishing it

beacon-chain/blockchain/kzg/validation.go

nalepae · 2024-06-27T10:52:27Z

In general, blob, cells etc... are quite heavy.
Instead of returning a Blob or Cell object, it's better to return a pointer to these objects.
This way, no copy of these objects are creted.

nalepae · 2024-06-27T10:52:58Z

Setting it as ready to get feedback, before polishing it
Thanks a lot for your PR! Please go ahead with polishing :)

nalepae · 2024-06-27T10:54:04Z

You need to run gazelle:

bazel run //:gazelle -- fix

kevaundray · 2024-06-28T14:34:24Z

Thanks for the review! I'll clean up the PR since it seems you are fine with the general PR.

RE having pointers to large objects, I agree -- In this PR, I was aiming to solely apply the abstraction layer with little modifications to the other code, and then apply modify the abstraction layer to do things like not pass around large objects and sloely deprecate some functions in another PR. Let me know what you think of this.

…etween types

kevaundray · 2024-06-28T15:07:13Z

Note: Had to manually modify the build.Bazel in fdedf2d

kevaundray · 2024-06-28T15:30:17Z

beacon-chain/blockchain/kzg/kzg.go

+func RecoverCellsAndKZGProofs(cellIds []uint64, _cells []Cell) ([ckzg4844.CellsPerExtBlob]Cell, [ckzg4844.CellsPerExtBlob]Proof, error) {
+	// First recover all of the cells
+	recoveredCells, err := RecoverAllCells(cellIds, _cells)
+	if err != nil {
+		return [ckzg4844.CellsPerExtBlob]Cell{}, [ckzg4844.CellsPerExtBlob]Proof{}, err
+	}
+
+	// Extract the Blob from all of the Cells
+	blob, err := CellsToBlob(recoveredCells)
+	if err != nil {
+		return [ckzg4844.CellsPerExtBlob]Cell{}, [ckzg4844.CellsPerExtBlob]Proof{}, err
+	}
+
+	// Compute all of the cells and KZG proofs
+	return ComputeCellsAndKZGProofs(&blob)
+}


This method is not being used -- the most recent commit of the crypto libraries will no longer expose RecoverAllCells and CellsToBlob, so I've added this method tentatively, so that we can modify the codebase preemptively in anticipation for that breaking change

kevaundray · 2024-06-28T19:36:01Z

I have pulled out the last deepsource error into #14162 since it is not related to any changes to kzg/peerdas

nalepae · 2024-07-03T11:39:45Z

bazel run //:gazelle -- fix

needs to be run.

EDIT: I did not pull.

beacon-chain/core/peerdas/helpers_test.go

nalepae · 2024-07-03T11:54:10Z

After running gazelle, there is some build errors:

bazel build //cmd/beacon-chain

...
beacon-chain/blockchain/kzg/validation.go:93:12: nil dereference in index operation (nilness)
beacon-chain/blockchain/kzg/validation.go:103:12: nil dereference in index operation (nilness)
beacon-chain/blockchain/kzg/validation.go:106:59: Expression is already a slice. (slicedirect)

EDIT: I did not pull.

…kage (#14136) * chore: move all ckzg related functionality into kzg package * refactor code to match * run: bazel run //:gazelle -- fix * chore: add some docs and stop copying large objects when converting between types * fixes * manually add kzg.go dep to Build.Hazel * move kzg methods to kzg.go * chore: add RecoverCellsAndProofs method * bazel run //:gazelle -- fix * use BytesPerBlob constant * chore: fix some deepsource issues * one declaration for commans and blobs

kevaundray added 2 commits June 24, 2024 16:16

chore: move all ckzg related functionality into kzg package

5d04e14

refactor code to match

d28f97c

kevaundray commented Jun 24, 2024

View reviewed changes

kevaundray marked this pull request as ready for review June 25, 2024 13:32

kevaundray requested a review from a team as a code owner June 25, 2024 13:32

kevaundray requested review from kasey, nalepae and rauljordan and removed request for a team June 25, 2024 13:32

nalepae added the peerDAS label Jun 25, 2024