Conversation
Note: merging ts methods into this branch because many nr tests use the same inputs and process as the ts versions. It's useful (to me!) to have them side by side in the same branch. Hopefully it's not too difficult to review - the new unreviewed files in this PR are .nr code only, ignore all .ts. If that's not useful, I can recommit the ts files to `mw/blob-batching`, leaving only the nr ones here. TODOs: - [x] Remove copied/pasted fns by bumping bigcurve and `noir-protocol-circuits` to bignum 0.7.0 (blocked by bignum/bigcurve) - [x] Remove visibility warnings (blocked by mostly bigcurve, as required imports are marked as private in the repo) EDIT 16.5: my bigcurve branch `mw/bump` temporarily resolves these - [ ] Explore whether it's safe to use the output of a BN Poseidon2 hash as part of `gamma` (the 'challenge' for a random linear combination on BLS12 elts) - #13608 (not blocked, but requires some cryptography thinking) - [x] Decide whether to keep `finalize` as a separate fn once accumulators are complete (the only thing finalize actually does is hash the final `gamma_acc` with `z`). TODOs which can only be completed once batching is integrated: - [ ] Remove temp `pub`s all over the place - [ ] Remove old `BlobCommitment` type and replace entirely with the properties of `BatchingBlobCommitment` --- ## PR Stack - [ ] `mw/blob-batching` <- main feature - [x] ^ `mw/blob-batching-bls-utils` <- BLS12-381 bigcurve and bignum utils (noir) - [ ] ^ `mw/blob-batching-bls-utils-ts` <- BLS12-381 bigcurve and bignum utils (ts) - [ ] ^ `mw/blob-batching-integration` <- Integrate batching into noir protocol circuits - [ ] ^ `mw/blob-batching-integration-ts-sol` <- Integrate batching into ts and solidity
Ts only blob batching methods plus tests. Points to the parent methods PR: #13583. TODOs (Marked in files as `TODO(MW)`): - [ ] Remove the large trusted setup file? Not sure if it's required, but it is currently the only way I show in tests that our BLS12 methods match those in c-kzg. - [x] Add nr fixture where we can use `updateInlineTestData` for point compression. Other TODOs must wait until we actually integrate batching, otherwise I will break the repo. NB: The files `bls12_fields.ts` and `bls12_point.ts` and their tests are essentially copies of `./fields.ts` and `./point.ts`. When reviewing please keep that in mind and double check the original file if you see an issue before commenting (@iAmMichaelConnor ;) ). --- ## PR Stack - [ ] `mw/blob-batching` <- main feature - [ ] ^ `mw/blob-batching-bls-utils` <- BLS12-381 bigcurve and bignum utils (noir) (#13583) - [x] ^ `mw/blob-batching-bls-utils-ts` <- BLS12-381 bigcurve and bignum utils (ts) (#13606) - [ ] ^ `mw/blob-batching-integration` <- Integrate batching into noir protocol circuits (#13817) - [ ] ^ `mw/blob-batching-integration-ts-sol` <- Integrate batching into ts and solidity (#14329)
WIP TODOs - [ ] Compress BLS12 fq and fr values to fewer native fields to reduce number of public inputs (somewhat blocked by #13608 since that dictates how large bls12fr value gamma is) - [ ] Delete old `blob.nr` files and remove `pub`s w/o batching (will do this later so it's easier to review) - [x] Rework `RootRollupPublicInputs` so it doesn't contain unnecessary values not needed for L1 verification --- ## PR Stack - [ ] `mw/blob-batching` <- main feature - [ ] ^ `mw/blob-batching-bls-utils` <- BLS12-381 bigcurve and bignum utils (noir) (#13583) - [ ] ^ `mw/blob-batching-bls-utils-ts` <- BLS12-381 bigcurve and bignum utils (ts) (#13606) - [x] ^ `mw/blob-batching-integration` <- Integrate batching into noir protocol circuits (#13817) - [ ] ^ `mw/blob-batching-integration-ts-sol` <- Integrate batching into ts and solidity (#14329) --------- Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com>
## Finalises integration of batched blobs `mw/blob-batching-integration` adds batching to the rollup .nr circuits only (=> will not run in the repo). This PR brings those changes downstream to the typescript and L1 contracts. Main changes: - L1 Contracts: - No longer calls the point evaluation precompile on `propose`, instead injects the blob commitments, check they correspond to the broadcast blobs, and stores them in the `blobCommitmentsHash` - Does not store any blob public inputs apart from the `blobCommitmentsHash` (no longer required) - Calls the point evaluation precompile once on `submitEpochRootProof` for ALL blobs in the epoch - Uses the same precompile inputs as pubic inputs to the root proof verification alonge with the `blobCommitmentsHash` to link the circuit batched blob, real L1 blobs, and the batched blob verified on L1 - Refactors mock blob oracle - Injects the final blob challenges used on each blob into all block building methods in `orchestrator` - Accumulates blobs in ts when building blocks and uses as inputs to each rollup circuit - Returns the blob inputs required for `submitEpochRootProof` on `finaliseEpoch()` - Updates nr structs in ts plus fixtures and tests ## TODOs/Current issues - ~When using real proofs (e.g. `yarn-project/prover-client/src/test/bb_prover_full_rollup.test.ts`), the root rollup proof is generated correctly but fails verification checks in `bb` due to incorrect number of public inputs. Changing the number correctly updates vks and all constants elsewhere, but `bb` does not change.~ EDIT: solved - must include the `is_inf` point member for now (see below TODO) - ~The `Prover.toml` for block-root is not executing. The error manifests in the same way as that in #12540 (but may be different).~ EDIT: temporarily fixed - details in this repro (#14381) and noir issue (noir-lang/noir#8563). - BLS points in noir take up 9 fields (4 for each coordinate as a limbed bignum, 1 for the `is_inf` flag) but can be compressed to only 2. For recursive verification in block root and above, would it be worth the gates to compress these? It depends whether the gate cost of compression is more/less than gate cost of recursively verifying 7 more public inputs. ## PR Stack - [ ] `mw/blob-batching` <- main feature - [ ] ^ `mw/blob-batching-bls-utils` <- BLS12-381 bigcurve and bignum utils (noir) (#13583) - [ ] ^ `mw/blob-batching-bls-utils-ts` <- BLS12-381 bigcurve and bignum utils (ts) (#13606) - [ ] ^ `mw/blob-batching-integration` <- Integrate batching into noir protocol circuits (#13817) - [x] ^ `mw/blob-batching-integration-ts-sol` <- Integrate batching into ts and solidity (#14329) --------- Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com>
LHerskind
left a comment
There was a problem hiding this comment.
Did a pretty high level look here. Seems pretty sane, but have reservations about some of the todo as I think they don't account for how blocks are produced and at the same time would provide negligable saving compared to some of the other changes we might have to make anyway.
| return STFLib.getStorage().blocks[_blockNumber].blobCommitmentsHash; | ||
| } | ||
|
|
||
| function getCurrentBlobCommitmentsHash() external view override(IRollup) returns (bytes32) { |
There was a problem hiding this comment.
Are we using this to have a slightly easier way to fetch it? Not a big thing atm, but it takes up extra space in the contract, and put us with more code paths which can be kinda annoying. We have a good bunch of these flowing around.
Created #14854, no need to address here. Guess there will be plenty at once as part of #14854.
There was a problem hiding this comment.
Yeah, just an easier way to fetch - thanks for adding an issue!
| bytes32 archive; | ||
| bytes32 headerHash; // hash of the proposed block header | ||
| bytes32 headerHash; | ||
| bytes32 blobCommitmentsHash; // TODO(#14646): Keep a running hash we iteratively overwrite per epoch, instead of per block. |
There was a problem hiding this comment.
I don't think you would really be able to do it per epoch, for the reason that it is not guaranteed that the whole epoch lands. This is because we have partial epochs.
Ignoring that, I think a more likely outcome is that the BlockLog struct won't be saved in its entirety, but that we instead store just a hash, and whenever reading need to provide data. It is less neat and a bit of a pain to handle, but it takes at least a good chunk of the cost out of the question. And at that point it is not much more expensive to read more values in the BlockLog than just one.
I think I would lean towards, we get the things you have in. And then more or less leave the contracts alone so we can apply optimisations and things for gas without running into big changes etc.
TL;DR: I would probably close the part of per epoch, because it is not sound given we have partial epochs.
There was a problem hiding this comment.
Makes sense - considering partial epochs, how would the running hash work? No need to reply now, was just my assumption that the reason we could overwrite one value was that we could 'start fresh' with each epoch (whether it goes through or is pruned).
| * @notice Validate an L2 block's blobs and return the blobHashes, the hashed blobHashes, and blob commitments. | ||
| * @notice We assume that this propose transaction contains only Aztec blobs | ||
| * Input bytes: | ||
| * input[:1] - num blobs in block |
There was a problem hiding this comment.
Insanity caused by original blob PR 5 months ago. Will change
| } | ||
| blobHashes[i] = blobHash; | ||
| } | ||
| // Ensure no non-Aztec blobs have been emitted in this tx: |
There was a problem hiding this comment.
Oh, my comment related to this one (on the previous pr) was because people have been talking about same actor sending a tx that propose blocks for multiple rollups at the same time, so there could be a blob before the aztec specific, or after.
I'm ok with us checking like this, but would expect that we could get away with just checking blobhash(numBlobs) == 0 🤔.
There was a problem hiding this comment.
Ah ok I misunderstood! I was thinking that it might be possible to submit blobs like blob_0, blob_1, blob_2 where blob_1 is empty and wanted to cover that case. I wasn't sure whether blob submission would count blob_1 as 'blank' (with blobhash 0, covered by new check) or assign a blobhash which would just be the hash of the (0, 0) commitment (covered by protocol).
so there could be a blob before the aztec specific, or after.
We can't have one before the aztec specific since any commitment before numBlobs would be 'stored' and fail the epoch verification. I don't think there is a way to 'attack' this by injecting a commitment that matches the non-aztec blob both to this fn and in the rollup circuits (@iAmMichaelConnor?)
| + 16 /* gas_fees.fee_per_da_gas */ | ||
| + 16 /* gas_fees.fee_per_l2_gas */ | ||
| ; | ||
| + 16 /* gas_fees.fee_per_l2_gas */; |
There was a problem hiding this comment.
Note: This is an unrelated change - contants.in.ts was unable to read this constant because the ; was on a new line
## The blobs are back in town. This PR reworks blobs so that instead of calling the point evaluation precompile for each blob (currently up to 3 per block => up to 96 (?) calls per epoch), we call it once per epoch by batching blobs to a single kzg commitment, opening, challenge, and proof. How we can be sure that this one pairing check is equivalent to a check per blob is covered in the maths by @iAmMichaelConnor [here](https://hackmd.io/WUtNusQxS5KAw-af3gxycA?view) 🎉 ## Overview Instead of pushing to a long array of `BlobPublicInputs`, which are then individually checked on L1, we batch each blob together to a single set of `BlobAccumulatorPublicInputs`. The `start` accumulator state is fed into each block root circuit, where the block's blobs are accumulated and the `end` state is set. Each block merge circuit checks that the state follows on correctly and, finally, the root circuit checks that the very `start` state was empty and finalises the last `end` state. This last `end` state makes up the set of inputs for the point evaluation precompile. If the pairing check in that precompile passes, we know that all blobs for all blocks in the epoch are valid and contain only the tx effects validated by the rollup. ### Circuits Key changes: - Integrate BLS12-381 curve operations with `bignum` and `bigcurve` libraries, plus tests. - Rework the `blob` package to batch blobs and store in reworked structs, plus tests. - Rework the rollup circuits from `block_root` above to handle blob accumulation state rather than a list of individual blob inputs, plus (you guessed it) tests. ### Contracts The contracts: - No longer call the point evaluation precompile on `propose`, instead inject the blob commitments, check they correspond to the broadcast blobs, and stores them in the `blobCommitmentsHash`. - Do not store any blob public inputs apart from the `blobCommitmentsHash`. - Call the point evaluation precompile once on `submitEpochRootProof` for ALL blobs in the epoch. - Use the same precompile inputs as pubic inputs to the root proof verification along with the `blobCommitmentsHash` to link the circuit batched blob, real L1 blobs, and the batched blob verified on L1. ### TypeScript Key changes: - Edit all the structs and methods reliant on the circuits/contracts to match the above changes. - Inject the final blob challenges used on each blob into all block building methods in `orchestrator`. - Accumulate blobs in ts when building blocks and use as inputs to each rollup circuit, plus tests. - Return the blob inputs required for `submitEpochRootProof` on `finaliseEpoch()`. ### TODOs/Related Issues - Choose field for hashing challenge: AztecProtocol#13608 - Instead of exponentiating `gamma` (expensive!), hash it for each iteration: AztecProtocol#13740 - Number of public inputs: BLS points in noir take up 9 fields (4 for each coordinate as a limbed bignum, 1 for the is_inf flag) but can be compressed to only 2. For recursive verification in block root and above, would it be worth the gates to compress these? It depends whether the gate cost of compression is more/less than gate cost of recursively verifying 7 more public inputs. - Remove the large trusted setup file from `yarn-project/blob-lib/src/trusted_setup_bit_reversed.json`? Used in testing, but may not be worth keeping (see code comments). - Cleanup old, unused blob stuff in AztecProtocol#14637. ## PR Stack - [x] `mw/blob-batching` <- main feature - [x] ^ `mw/blob-batching-bls-utils` <- BLS12-381 bigcurve and bignum utils (noir) (AztecProtocol#13583) - [x] ^ `mw/blob-batching-bls-utils-ts` <- BLS12-381 bigcurve and bignum utils (ts) (AztecProtocol#13606) - [x] ^ `mw/blob-batching-integration` <- Integrate batching into noir protocol circuits (AztecProtocol#13817) - [x] ^ `mw/blob-batching-integration-ts-sol` <- Integrate batching into ts and solidity (AztecProtocol#14329) - [ ] ^ `mw/blob-batching-cleanup` <- Remove old blob code --------- Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com>
The blobs are back in town.
This PR reworks blobs so that instead of calling the point evaluation precompile for each blob (currently up to 3 per block => up to 96 (?) calls per epoch), we call it once per epoch by batching blobs to a single kzg commitment, opening, challenge, and proof.
How we can be sure that this one pairing check is equivalent to a check per blob is covered in the maths by @iAmMichaelConnor here 🎉
Overview
Instead of pushing to a long array of
BlobPublicInputs, which are then individually checked on L1, we batch each blob together to a single set ofBlobAccumulatorPublicInputs. Thestartaccumulator state is fed into each block root circuit, where the block's blobs are accumulated and theendstate is set. Each block merge circuit checks that the state follows on correctly and, finally, the root circuit checks that the verystartstate was empty and finalises the lastendstate.This last
endstate makes up the set of inputs for the point evaluation precompile. If the pairing check in that precompile passes, we know that all blobs for all blocks in the epoch are valid and contain only the tx effects validated by the rollup.Circuits
Key changes:
bignumandbigcurvelibraries, plus tests.blobpackage to batch blobs and store in reworked structs, plus tests.block_rootabove to handle blob accumulation state rather than a list of individual blob inputs, plus (you guessed it) tests.Contracts
The contracts:
propose, instead inject the blob commitments, check they correspond to the broadcast blobs, and stores them in theblobCommitmentsHash.blobCommitmentsHash.submitEpochRootProoffor ALL blobs in the epoch.blobCommitmentsHashto link the circuit batched blob, real L1 blobs, and the batched blob verified on L1.TypeScript
Key changes:
orchestrator.submitEpochRootProofonfinaliseEpoch().TODOs/Related Issues
gamma(expensive!), hash it for each iteration: feat(blobs): explore hashing gamma instead of using powers to form linear combination of y, C, Q #13740yarn-project/blob-lib/src/trusted_setup_bit_reversed.json? Used in testing, but may not be worth keeping (see code comments).PR Stack
mw/blob-batching<- main featuremw/blob-batching-bls-utils<- BLS12-381 bigcurve and bignum utils (noir) (feat: blob batching methods #13583)mw/blob-batching-bls-utils-ts<- BLS12-381 bigcurve and bignum utils (ts) (feat: blob batching methods (ts) #13606)mw/blob-batching-integration<- Integrate batching into noir protocol circuits (feat: WIP batch blobs and validate in rollup #13817)mw/blob-batching-integration-ts-sol<- Integrate batching into ts and solidity (feat: WIP integrate batched blobs into l1 contracts + ts #14329)mw/blob-batching-cleanup<- Remove old blob code