Skip to content

Distributed cell proofs#9343

Merged
lucassaldanha merged 12 commits into
Consensys:dasfrom
zilm13:distributed-cell-proofs
Apr 15, 2025
Merged

Distributed cell proofs#9343
lucassaldanha merged 12 commits into
Consensys:dasfrom
zilm13:distributed-cell-proofs

Conversation

@zilm13

@zilm13 zilm13 commented Apr 11, 2025

Copy link
Copy Markdown
Contributor

PR Description

Distributed cell proofs as of:
ethereum/execution-apis#630
ethereum/EIPs#9378
ethereum/consensus-specs#4128
ethereum/consensus-specs#4187

Fixed Issue(s)

Documentation

  • I thought about documentation and added the doc-change-required label to this PR if updates are required.

Changelog

  • I thought about adding a changelog entry, and added one if I deemed necessary.

@zilm13 zilm13 marked this pull request as ready for review April 14, 2025 22:45
import tech.pegasys.teku.spec.datastructures.type.SszKZGCommitment;
import tech.pegasys.teku.spec.datastructures.type.SszKZGProof;

// TODO: FULU with cell proofs

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

My understanding is that the only meaningful change in this object was the now strict length of all list fields? What is this TODO about?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Also, do we need to update BlobsBundleSchema with the update size for the proofs (FIELD_ELEMENTS_PER_EXT_BLOB * MAX_BLOB_COMMITMENTS_PER_BLOCK)?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

we don't have builder specs (or maybe I've not seen it yet), not sure
I don't want to change BlobsBundleSchema, I've separate BlobsCellBundleSchema

public List<DataColumnSidecar> constructDataColumnSidecars(
@VisibleForTesting
@Deprecated
public List<DataColumnSidecar> constructDataColumnSidecarsOld(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is only used by tests now. Can we get rid of it?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

In separate pr please

BeaconBlock block = blockResolver.addBlock(10, blobCount);
List<DataColumnSidecar> sidecars =
miscHelpers.constructDataColumnSidecars(createSigned(block), blobs, kzg);
miscHelpers.constructDataColumnSidecarsOld(createSigned(block), blobs, kzg);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

?

BeaconBlock block = blockResolver.addBlock(10, 1);
List<DataColumnSidecar> sidecars =
miscHelpers.constructDataColumnSidecars(createSigned(block), blobs, kzg);
miscHelpers.constructDataColumnSidecarsOld(createSigned(block), blobs, kzg);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we need this?

@zilm13 zilm13 Apr 15, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't see other way to construct proofs for fixtures. And we need proofs to construct valid DataColumnSidecars. Did I miss other way to do this? Deprecated mark is mostly to avoid anyone to call this in production

throw new RuntimeException("LibPeerDASKZG library doesn't support computeBlobKzgProof");
}

// TODO: test

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do you plan to test it as part of this PR? Otherwise it would be good to add a ticket link here so we know we won't miss it.

Comment on lines +117 to 118
@Deprecated(since = "Use computeCells instead, computeCellsAndProof is not for production")
List<KZGCellAndProof> computeCellsAndProofs(Bytes blob);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this method needed? Why can't we delete it?


@Override
public List<KZGCell> computeCells(final Bytes blob) {
// TODO: replace with RustKZG when available

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Where are we tracking this? We are probably gonna forget...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

it's done, just outdated todo, thank you for catching it

@zilm13

zilm13 commented Apr 15, 2025

Copy link
Copy Markdown
Contributor Author

So debts, I want to do in separate PR:

  • cleanup tests, removing computeCellsAndProofs
  • add computeCells tests
  • add fulu block production tests
  • builder bid fulu compatibleness with cell kzg proofs

@lucassaldanha lucassaldanha merged commit ec1f8f4 into Consensys:das Apr 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants