Update | Fix | Refactor | Unit Tests

[roynalnaruto]

Major Updates

• In the blob's metadata, we use number_valid_snarks (or num_valid_chunks) instead of num_nonempty_chunks (which was kind of useless given that we already know the sizes of every chunk). This is currently used as a placeholder as there is no real use for this. We add an equality constraint between thisnum_valid_chunks and the num_valid_snarks exported by the conditional constraints in core.rs
• We use the exports chunks_are_padding from conditional constraints in core.rs to do the below listed
• chunk_data_digest for a padded chunk (i.e. not real snark) will be the chunk_data_digest of the last valid chunk, instead of using keccak256([]). This brings more consistency between how the Aggregation Circuit expects a padded chunk's snark to just be a copy of the last valid chunk's snark.
• If a chunk is valid (not a padded chunk) but empty (no L2 transactions), then its chunk_data_digest is keccak256([])
• In the case that all chunks are empty, i.e. no L2 transactions, then the first row of the "chunk data" section should be a padding row and must have accumulator==0. In the case that at least one non-empty chunk exists, the first row of "chunk data" section is not a padding row, and the accumulator==1.

Refactoring

• Remove inconsistency between blob.rs and batch.rs
• Remove Blob struct and exclusively use BlobData. Implement From<T> for various T that a BlobData can be constructed from
• More comments/documentation for the new codes

Tests

• Integration tests are passing: cargo test --release --lib test_aggregation_circuit
• Unit tests are failing (only 1 test failure): cargo test --release --lib blob_circuit_completeness

Pending Tasks:

• The only unit test that is failing currently is: "all empty except last chunk", i.e. We have 15 valid chunks, 14 are empty and the 15th chunk has some data. The reason why this is failing is because the chunk_idx for the first non-empty chunk is 15. But the range table used in BlobDataConfig is initialised as: RangeTable<MAX_AGG_SNARKS> hence the maximum value in the range table is 14. So we encounter a lookup failure
• This range table also includes 0, which allows an attacker to set chunk_idx' == chunk_idx at a chunk boundary, thereby never increasing the chunk idx at this row. This is an issue. We want to constrain:

  (chunk_idx' - chunk_idx) in [1, MAX_AGG_SNARKS]
  

  One solution is to exclude 0 from this range table (change RangeTable<MAX> to RangeTable<EXCLUDE_ZERO, MAX>), but this won't work as on the rows where the lookup condition is not met, 0 wouldn't lie in the range table.
  Another solution is to simply use RangeTable<{ MAX_AGG_SNARKS + 1 }> and add an additional constraint that on is_boundary, we do not allow chunk_idx' == chunk_idx.

[kunxian-xia]

We want to merge this pr before all comments are fixed because we want to run integration test ASAP. @roynalnaruto

[kunxian-xia]

should we require chunk_sizes.iter().sum() to less than N_ROWS_DATA?

[kunxian-xia]

this renaming may cause confussion as hash / digest are often used in interchangeably.

[kunxian-xia]

We also do lookup into keccak table for last metadata row. Shall we add it in the lookup's name?

[kunxian-xia]

Do we have mechanism to prevent malicious prover from skipping valid and non-empty chunk? For example, if non-empty chunk i is skipped in "data section", then the i-th digest_rlc row in hash section does not necessarily be correct.