peerDAS: Public methods must accept raw bytes#3579
Conversation
asn-d6
force-pushed
the
peerdas_public_method_bytes
branch
from
789e24e to
a4331e7
Compare
jtraglia
left a comment
jtraglia
left a comment
There was a problem hiding this comment.
Looking good. Here's my initial review.
| def verify_cell_proof(commitment_bytes: Bytes48, | ||
| cell_id: uint64, | ||
| cell_bytes: Vector[Bytes32, FIELD_ELEMENTS_PER_CELL], | ||
| proof: Bytes48) -> bool: |
| bytes_to_kzg_commitment(commitment_bytes), | ||
| coset, | ||
| bytes_to_cell(cell_bytes), | ||
| bytes_to_kzg_proof(proof)) |
There was a problem hiding this comment.
Could you ensure that all the docstrings have proper punctuation? Lots of the *_polynomialcoeff methods and other funcs, like verify_kzg_proof_multi_impl, are missing punctuation.
There was a problem hiding this comment.
So I think the documentation of the entire file is a bit lacking to the point that I would suggest we do it in another PR since it requires quite a bit of work.
(PS: That also made me look at the 4844 documentation, and it also seems severely incomplete or outdated)
| cell: Cell, | ||
| proof: KZGProof) -> bool: | ||
| def verify_cell_proof(commitment_bytes: Bytes48, | ||
| cell_id: uint64, |
There was a problem hiding this comment.
We should use the new CellID type here, right?
| proofs: Sequence[KZGProof]) -> bool: | ||
| def verify_cell_proof_batch(row_commitments_bytes: Sequence[Bytes48], | ||
| row_ids: Sequence[uint64], | ||
| column_ids: Sequence[uint64], |
There was a problem hiding this comment.
Probably not IMO. The ID of a row or column is semantically different from a cell ID, even if the underlying type is the same.
Specifically, we expect to have just a few rows. We also expect the amount of columns to be equal to the amount of cells per blob.
However, due to the semantic difference, I would be hesitant in reusing CellID here. IMO another approach would be to define a RowID and ColumnID type for these.
There was a problem hiding this comment.
Related commit that will cause conflicts: 665e6fa
There was a problem hiding this comment.
I went address it in #3574 -> RowIndex and ColumnIndex. IMO "index" implies they are consecutive numbers but ID is not.
We can fix conflict later whichever merge late.
| column_ids=[0, 1], | ||
| cells=cells[0:1], | ||
| proofs=proofs, | ||
| cells_bytes=cells_bytes[0:1], |
There was a problem hiding this comment.
How many cells are we trying to verify here? Since column_ids has two values, it appears to be two. But [0:1] will result in a single value. Think there's a bit of an accident here.
There was a problem hiding this comment.
Yeah that unittest was a bit working by accident. I made it more robust in 2000a4f.
In the future we will want certainly want test_verify_cell_proof_batch() to do a multi-blob cell verification, but to do that we need to figure out a solution to compute_cells_and_proofs() taking multiple minutes (perhaps load them directly from a file?).
| cells=cells[0:1], | ||
| proofs=proofs, | ||
| cells_bytes=cells_bytes[0:1], | ||
| proofs_bytes=proofs, |
There was a problem hiding this comment.
We're sending all the proofs when only the first one is required.
|
|
||
| ```python | ||
| def coset_for_cell(cell_id: int) -> Cell: | ||
| def coset_for_cell(cell_id: CellID) -> Cell: |
There was a problem hiding this comment.
Hmm, I think cell_id should be column_index (or column_id) here.
There was a problem hiding this comment.
In practice, column_id and cell_id can be used interchangeably in this function, since each cell corresponds to a column in a one-to-one fashion.
I think it's better for us to choose which notion is more intuitive in a separate PR.
|
Merged. Thanks all! |
3 participants
The cryptography public methods should take as input raw bytes. This is a lesson from the 4844 cryptography specs. It makes it clear that cryptographic validation of inputs is a responsibility of the cryptography spec and not the rest of the client code.
(Also switched
inttouint64after discussion in the specs call.)Furthermore, merging this PR will likely break the currently active peerDAS PR #3574.