-
Notifications
You must be signed in to change notification settings - Fork 511
feat!: checkpoint circuits #16187
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat!: checkpoint circuits #16187
Conversation
pub fn evaluate_blobs_and_batch( | ||
blobs_as_fields: [Field; FIELDS_PER_BLOB * BLOBS_PER_BLOCK], | ||
kzg_commitments_points: [BLSPoint; BLOBS_PER_BLOCK], | ||
pub fn evaluate_blobs_and_batch<let NumBlobs: u32>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing it to generic because I don't like it being hard coded to BLOBS_PER_BLOCK
in the blob lib. We might want to have different circuits that work with different number of blobs.
* - final_blob_challenges: Final values z and gamma, shared across the epoch. | ||
*/ | ||
#[derive(Deserialize, Eq, Serialize)] | ||
pub struct BlockBlobPublicInputs { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't need this struct anymore, the fields are now defined in CheckpointRollupPublicInputs
. (Probably shouldn't be in the blob-lib
in the first place.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved to rollup_lib/abis/root_parity_proof_data
because it's the private inputs for the rollups and should be defined there what data is in the struct. The public inputs of the parity circuit stays in the parity-lib
.
archive_root_membership_witness: MembershipWitness<ARCHIVE_HEIGHT>, | ||
contract_class_log_fields: [[Field; CONTRACT_CLASS_LOG_SIZE_IN_FIELDS]; MAX_CONTRACT_CLASS_LOGS_PER_TX], | ||
|
||
prover_id: Field, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make the prover_id
propagate from the base to root through all the constants (BlockConstantData
-> CheckpointConstantData
-> EpochConstantData
).
(Previously it was set from the block root.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? Was it a bug previously (and we always should have been attributing each proof in the rollup to a prover)?
Should the AVM and Tube circuits also be attributed to a prover_id?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes sense to attribute each proof in the rollup to a prover! Will change it in another pr.
For this pr, it's easier to set it from the base's constants, so that we don't have to pass the prover id to the 5 different private inputs of all the block root variations. And the existing logic already checks that it's the same everywhere (left.constants == right.constants).
} | ||
|
||
/// Asserts that the tree formed by rollup circuits is filled greedily from left to right. | ||
pub fn assert_rollups_filled_from_left(num_left_rollups: u32, num_right_rollups: u32) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adjust and expose this function so that we can reuse it to validate the tree shape of blocks (in validate_consecutive_block_rollups.nr
) and checkpoints (in validate_consecutive_checkpoint_rollups.nr
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got confused by the word "rollups" here, because:
- when called from the block_root circuit, it represents "number of left/right txs"
- when called from the checkpoint_root circuit, it represents "number of left/right blocks".
I can't think of a better name (because the name depends on the context in which it's called). So maybe add a doc comment those two bullet points that I've written.
Or maybe a name could be "num_left_leaves" and "num_right_leaves", but "leaf" is ambiguous too, because it depends how far down the tree you go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe also rename the function to assert_rollups_filled_greedily
or something
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I named it rollups because for txs they are also called base "rollups". I will add a comment to explain what the "rollups" can be!
pub(crate) mod previous_rollup_block_data; | ||
pub(crate) mod rollup_fixture_builder; | ||
// pub(crate) mod previous_rollup_block_data; | ||
// pub(crate) mod rollup_fixture_builder; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will fix the tests and add these back later!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the logic looks good to me.
The only things I highlight in my comments are pedantic things.
// - Checks that any fields above expected_fields are empty (inside poseidon2_hash_subarray()) | ||
fn check_block_blob_sponge( | ||
blobs_as_fields: [Field; FIELDS_PER_BLOB * BLOBS_PER_BLOCK], | ||
fn check_block_blob_sponge<let N: u32>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doc comment to say that N = FIELDS_PER_BLOB * NumBlobs
, or (probably more clear:) change the generic to be NumBlobs
, and then compute FIELDS_PER_BLOB * NumBlobs
within the function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This util doesn't need to always take FIELDS_PER_BLOB * NumBlobs
fields. It checks the sponge blob against the fields used to create it. The tests for it can work with any number of fields (10, 32, etc).
Consider renaming to check_checkpoint_blob_sponge
I was actually thinking about changing its name to be more general :p Or move the part that check the lengths out of the blob lib and put it with the circuit that will make more sense to have the specific error messages.
// - Checks that any fields above expected_fields are empty (inside poseidon2_hash_subarray()) | ||
fn check_block_blob_sponge( | ||
blobs_as_fields: [Field; FIELDS_PER_BLOB * BLOBS_PER_BLOCK], | ||
fn check_block_blob_sponge<let N: u32>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider renaming to check_checkpoint_blob_sponge
pub struct BlockRollupPublicInputs { | ||
pub constants: CheckpointConstantData, | ||
|
||
// Archive tree root immediately before this block range. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's a block range
? Does it just mean "immediately before this block"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't name this but I think it means a range of blocks, because there could be more than one block. If it's the public inputs for block merge, then they are the aggregated values of all the blocks.
Naming suggestion welcome!
archive_root_membership_witness: MembershipWitness<ARCHIVE_HEIGHT>, | ||
contract_class_log_fields: [[Field; CONTRACT_CLASS_LOG_SIZE_IN_FIELDS]; MAX_CONTRACT_CLASS_LOGS_PER_TX], | ||
|
||
prover_id: Field, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? Was it a bug previously (and we always should have been attributing each proof in the rollup to a prover)?
Should the AVM and Tube circuits also be attributed to a prover_id?
"crates/rollup-block-root-first-single-tx", | ||
"crates/rollup-block-root-first-single-tx-simulated", | ||
"crates/rollup-block-root-first-empty-tx", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it reduce the amount of code & files if we don't have a separate "first" circuit, but instead just have one block root circuit, with a comptime bool to trigger the extra "first" logic. Kind of like what we do for the Reset circuits: we have one circuit, but then we create lots of versions of reset circuit with generics.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The private inputs are different for the "first" circuits, so the script will be slightly more complicated. Will be nice to reduce it from 5 files to 1. But I will make it a low priority for now 😛
} | ||
|
||
/// Asserts that the tree formed by rollup circuits is filled greedily from left to right. | ||
pub fn assert_rollups_filled_from_left(num_left_rollups: u32, num_right_rollups: u32) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe also rename the function to assert_rollups_filled_greedily
or something
pub constants: CheckpointConstantData, | ||
|
||
// Archive tree root immediately before this block range. | ||
pub previous_archive: AppendOnlyTreeSnapshot, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm seeing the terms last
and previous
get used for last_archive
/previous_archive
in various places. We should try to be consistent with one or the other.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed some to previous
, but left the rest as last
because I don't want to touch too many files - changing it in the block header means updating aztec.nr and many files in ts.
It's probably less obtrusive to change previous
to last
. But I think previous
vs new
sound better than last
vs new
. wdyt?
pub(crate) mod previous_rollup_block_data; | ||
pub(crate) mod tx_effect; | ||
|
||
pub use base_or_merge_rollup_public_inputs::BaseOrMergeRollupPublicInputs; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BaseOrMergeRollupPublicInputs.start
and .end
should be renamed to start_partial_state
and end_partial_state
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related: why do we have a "partial" reference, which is a snapshot of all trees except for the L1->L2 tree? Could we instead combine those into a single state_tree_snapshots
struct and pass that around most places?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can do it in another pr.
pub struct BlockHeader { | ||
pub last_archive: AppendOnlyTreeSnapshot, | ||
pub content_commitment: ContentCommitment, | ||
pub state: StateReference, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest rename to state_tree_snapshots: StateTreeSnapshots
/// Check that the hash of the previous block header is the last leaf in the previous archive. | ||
fn validate_previous_block_header_in_archive(self) { | ||
let previous_archive = self.previous_rollups[0].public_inputs.previous_archive; | ||
let last_leaf_index = previous_archive.next_available_leaf_index - 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where do we check that this previous_archive
is indeed the correct "last nonzero archive leaf in the tree", and hence that the last_leaf_index
is correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, perhaps it's in the way we insert a new archive leaf into the tree, as part of the block root rollup circuit. There, we assert that each block header is being inserted into an empty position. Ah, but we're trusting that the next_available_leaf index of the "previous archive" is correct.
Perhaps we only check that the "previous_archive" is indeed the correct one all the way on L1?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes the previous_archive
is checked on L1 when proposing a checkpoint. And the new_archive
of the proposed checkpoint will be stored on L1, so that it can be used to check the next checkpoint.
✅ Deploy Preview for aztec-docs-temp-you-can-delete ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
) => { | ||
span.setAttribute(Attributes.TX_HASH, tx.hash.toString()); | ||
// Get trees info before any changes hit | ||
const lastArchive = await getTreeSnapshot(MerkleTreeId.ARCHIVE, db); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Passing the lastArchive
to this function instead, because it's the same for all txs in the same block and is more efficient to fetch it just once. Besides, we are already passing in the lastL1ToL2MessageTreeSnapshot
so it doesn't seem weird to treat the archive the same.
|
||
// Append new data to startSpongeBlob | ||
const inputSpongeBlob = startSpongeBlob.clone(); | ||
await startSpongeBlob.absorb(tx.txEffect.toBlobFields()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sponge needs to absorb the block end marker in the parent function. So moving this out because I think it's cleaner to only mutate the sponge in the parent function.
return this.txs.map(t => t.processedTx); | ||
} | ||
|
||
public isProvingBase(txIndex: number) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a variable blockRootRollupStarted
used to check if the block root proving has started. Thought I'd generalize it and include a isProving
flag to the proving state for all types of proofs. Then we can use isProving_
and startProving_
to ensure that we don't trigger proving the same proof more than once.
|
||
const blobCommitmentsFields = blobCommitments[0].toBN254Fields(); | ||
|
||
// Run with AZTEC_GENERATE_TEST_DATA=1 to update noir test data. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing these as there are no hard-coded values for empty blob in noir anymore.
We still have the below test to make sure both the ts and noir produce the same results.
[Attributes.BLOCK_NUMBER]: blockNumber, | ||
})) | ||
public async setBlockCompleted(blockNumber: number, expectedHeader?: BlockHeader): Promise<L2Block> { | ||
public async setBlockCompleted(blockNumber: number, expectedHeader?: BlockHeader): Promise<BlockHeader> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing it to return the BlockHeader
instead of an L2Block
because L2Block
is tied to the L2BlockHeader
, which contains content commitment. Computing it will be a faff now that in the orchestrator the concepts of block and checkpoint are separated.
The only time we need setBlockCompleted
to return an L2Block
is from using the LightweightBlockFactory
. And luckily it implements IBlockFactory
and can have a different interface.
* @param tx - The transaction whose proving we wish to commence | ||
* @param provingState - The proving state being worked on | ||
*/ | ||
private async prepareTransaction(tx: ProcessedTx, provingState: BlockProvingState) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing this and make the calling function call prepareBaseRollupInputs
directly.
NUMBER_OF_L1_L2_MESSAGES_PER_ROLLUP, | ||
'Too many L1 to L2 messages', | ||
); | ||
const baseParityInputs = times(NUM_BASE_PARITY_PER_ROOT_PARITY, i => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The base parity (private) inputs are now built in the checkpoint proving state, to make it consistent everywhere - the trees are progressed in the orchestrator, and the private inputs are built in the corresponding proving state.
logger.debug(`Completed ${rollupType} proof for block ${provingState.blockNumber}`); | ||
|
||
logger.debug(`Completed ${rollupType} proof for block ${provingState.block!.number}`); | ||
// validatePartialState(result.inputs.end, tx.treeSnapshots); // TODO(palla/prover) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't need to be called anymore, because in verifyBuiltBlockAgainstSyncedState
we will verify the block header built from circuit output and from db, it basically checks that the entire state (partial + l1tol2) matches.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We only needed to run tests to check the proof counts because they were manually written. The tests can be removed now that the config is built by running code.
return; | ||
} | ||
|
||
// TODO(palla/prover): This closes the fork only on the happy path. If this epoch orchestrator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved from checkAndEnqueueBlockRootRollup
to here because it's the last place we use the db.
The issue still exists that the db is not closed on the unhappy path. Will leave it for you (palla) to fix :p
9edf04a
to
29c9fab
Compare
29c9fab
to
e2cb7f4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed stdlib. Gotta step out for a bit, will review the rest of ts when back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious what changed that this is now needed here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bug fix that's not related to the checkpoint 😅 We used to propagate the proverId
from the block root's private inputs. But all the proofs generated by the prover should attribute to a proverId
. While separating block root and checkpoint root, I figured adding this to the base rollups now will make my life easier than adding to the new variants of the block root rollups.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's keep this one in mind so we backport it in case this PR does not make the cut for the ignition version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've created #17081 and assigned to you, let me know if that's ok!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good. I need to make another pass at the orchestrator
on Monday with fresher eyes though. Got two general comments, none for this PR, but that came to mind while reviewing:
- I'm shocked at the amount of boilerplate we need for every new rollup circuit we add. New enums, types, mappings, conversions, methods, etc. Seems like we should be able to simplify A LOT of it.
- How can we make sure that every single circuit is being exercised (both simulated and proven) on CI? Especially since we now have a lot of circuits (vk tree height is 7!).
yarn-project/noir-protocol-circuits-types/src/conversion/server.ts
Outdated
Show resolved
Hide resolved
yarn-project/prover-client/src/orchestrator/checkpoint-proving-state.ts
Outdated
Show resolved
Hide resolved
yarn-project/prover-client/src/orchestrator/checkpoint-proving-state.ts
Outdated
Show resolved
Hide resolved
yarn-project/prover-client/src/orchestrator/epoch-proving-state.ts
Outdated
Show resolved
Hide resolved
5d7b68d
to
1137cc1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Thanks for all the effort that went into this @LeilaWang!
Just to double check some comments on parallelism, with this implementation:
- We can call all
startCheckpoint
s for an epoch in parallel, since our world state already has all blocks processed for the epoch being proven, so we just fork at each checkpoint (assuming we add the extra call tocheckAndEnqueueCheckpointRootRollup
from the comment below). - Within a checkpoint, we have to
startBlock
andaddTxs
(ie simulate the txs) in order, but can then prove in parallel.
if (!provingState.isReadyForCheckpointRoot()) { | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC this checks if all its children have been proven and if the blob accumulator from the previous checkpoint is available. If any of those is not true, we don't start proving, and wait until a future call.
Should we then add a call to checkAndEnqueueCheckpointRootRollup
for checkpoint N+1 once we set the endBlobAccumulator
for checkpoint N? This should allow us to call startNewCheckpoint
out of order if we want to, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually the end blob accumulator must've been set when all the block root/merge rollups for a checkpoint are proven. Will add a comment in the function to explain this and throw if not set!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately we can't run startCheckpoint
at random or in parallel at the moment. There isn't a way to identify the index of a checkpoint in the epoch when it's added. And the index of the checkpoint is used to decide the order when accumulating blobs and for rolling up. So they must be added in the correct order.
We can maybe pass a firstCheckpointNumber
to startEpoch
to enable it. But I'll leave it for now and can be done later if we decide we want to start checkpoints in parallel. (It's forking db and updating the l1-to-l2 tree so maybe it won't be too bad to run in sequence)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately we can't run startCheckpoint at random or in parallel at the moment
Then we have a race condition that needs fixing: the epoch-proving-job kicks off all blocks/checkpoints in an asyncPool
. Though they start in order, the first time we hit an async operation another "thread" starts. This means that if any of the async operations in Orchestrator.startNewCheckpoint
take too long for a given checkpoint, a different one may end up reaching the provingState.startNewCheckpoint
, which means they get added out of order.
1137cc1
to
2142ff2
Compare
provingState.setEndSpongeBlob(spongeBlobState); | ||
|
||
// Txs have been added to the block. Now try to accumulate the blobs as far as we can: | ||
await this.provingState.setBlobAccumulators(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved here to unblock the checkpoint root proving sooner if a later checkpoint is proved first.
It was done before proving a block root, which means there's a chance the later checkpoint would need to wait for an earlier checkpoint to reach to their block root before the proving can resume.
This will make awaiting addTxs
(and startNewBlock
for an empty block) a bit longer. But addTxs
is called in parallel in the epoch prover job, and the proving has been kick off before starting accumulating, so the impact is less serious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change to start checkpoints in parallel in the asyncPool looks good, thanks @LeilaWang!
17e7bd8
to
e7031aa
Compare
e7031aa
to
30ad781
Compare
30ad781
to
a3e23b9
Compare
Adding new checkpoint circuits as describe in the design doc.
Things different to the doc:
CheckpointHeader
is the same as the previousProposedBlockHeader
- we will still be validating the same values on L1 when submitting a checkpoint.To make the e2e tests pass without changing too many things, each checkpoint currently only contains one block. The existing "L2Block" class represents a block and its checkpoint. And a temporary
L2BlockHeader
is created for it, which has all the fields required to construct aBlockHeader
and aCheckpointHeader
.The orchestrator should already work correctly for multiple blocks per checkpoint.