refactor!: remove unneeded root rollup public inputs#13929
Conversation
| left: BlockRootOrBlockMergePublicInputs, | ||
| right: BlockRootOrBlockMergePublicInputs, | ||
| ) -> [FeeRecipient; AZTEC_MAX_EPOCH_DURATION] { | ||
| let left_len = array_length(left.fees); |
There was a problem hiding this comment.
fees and blob_public_inputs and the above proposed_block_header_hashes are inserted 1 item per block in the block root. Instead of checking the array lengths for all of them here, we can just check the number of blocks in the root rollup.
LeilaWang
changed the title
LHerskind
left a comment
LHerskind
left a comment
There was a problem hiding this comment.
I only looked at the solidity part here.
I think the PR looks fine. I think some of the changes on the parent one should probably be cleaned up a bit.
It is for example a bit confusing that we have
headerbytes, but it is not actually the header but only some subset of it, where the missing piecestateReferenceis passed separately as another blob of bytes. What is the reason that it was not kept inside that "header" blob?The
headerHashis also a bit weird as it is not really a hash of the header, but a hash of this subset of the header.
| start_global_variables: left.start_global_variables, | ||
| end_global_variables: right.end_global_variables, | ||
| out_hash, | ||
| out_hash: 0, // Not needed for block merge. This is only required for block root since the value is inserted on L1 per block. |
There was a problem hiding this comment.
Is it not still useful to see that it is accumulated correctly?
There was a problem hiding this comment.
We don't accumulate the out_hash of all the blocks to build a tree anymore. When proposing a block, the out_hash of that block is set to the outbox. After this point it is pretty much irrelevant.
There was a problem hiding this comment.
It is for example a bit confusing that we have header bytes, but it is not actually the header but only some subset of it, where the missing piece stateReference is passed separately as another blob of bytes. What is the reason that it was not kept inside that "header" blob?
The headerHash is also a bit weird as it is not really a hash of the header, but a hash of this subset of the header.
Agree that it's confusing. I struggled to find a good name for it, so kept the name header because it's a "proposed header". Maybe proposed "args" could be a better name? Then headerHash will become proposedArgsHash? Naming is hard...
I removed stateReference out of the proposed header as discussed because the values are committed to by archive. I know it seems stupid to have stateReference passed to L1 as separate bytes. But I'm hoping that it's just here temporary.
Whether stateReference is separated or included in the proposed header, we have to double check the values against those in world state: we build the trees from blob data, then get all the tree roots and archive and check that they all match the proposed values. Without stateReference onchain, it feels cleaner, we only need to check the archive.
But that also means that the archiver won't have the full header for proving. We could maybe have this condensed proposed header as the formal l2 block header. And the full header is only required upon proving, when world state is available and can reconstruct the stateReference.
But if you think a single type of block header is better, I will add the stateReference to the proposed header.
There was a problem hiding this comment.
Re out_hash, we have one per block and insert it as the root of the outbox tree in propose. It's part of the content_commitment which is inside the new header hash, so if a proposed L1 out_hash does not match the one in the circuit, the final roof proof verification will fail. Nice to be able to remove it above block level!
MirandaWood
left a comment
MirandaWood
left a comment
There was a problem hiding this comment.
Looks great thanks! Contract stuff LGTM but will leave the final approval to the team
| uint256 numBlocks = _end - _start + 1; | ||
|
|
||
| for (uint256 i = 0; i < numBlocks; i++) { | ||
| publicInputs[7 + i] = rollupStore.blocks[_start + i].headerHash; | ||
| publicInputs[2 + i] = rollupStore.blocks[_start + i].headerHash; | ||
| } |
There was a problem hiding this comment.
Without a start and end block number in the root I was wondering if its possible to submit an epoch proof with the 'wrong' start and end values - but I don't think it's possible.
Just to make sure I'm understanding, if someone tries to submit a proof for a shorter range of blocks then assertAcceptable will pass, but some headerHashes here will be set as 0s when they shouldn't be and fail verification. If it's a longer a range of blocks (and assertAcceptable somehow passes) then it's the reverse - some headerHashes will be non-0 when they are expected to be 0. I think they must be non-0 because L1 will not allow us to set end as a block not yet proposed - so it's all good?
Remove: - `previous_archive.next_available_leaf_index` - `end_archive.next_available_leaf_index` - `end_timestamp` - `end_block_number` - `out_hash` The first 4 are committed to by start and end block number, which are constrained by `proposed_block_header_hashes`. `out_hash` is not used at all beyond block root. Each block's `out_hash` is also constrained by its `proposed_block_header_hash`.
3 participants
Remove:
previous_archive.next_available_leaf_indexend_archive.next_available_leaf_indexend_timestampend_block_numberout_hashThe first 4 are committed to by start and end block number, which are constrained by
proposed_block_header_hashes.out_hashis not used at all beyond block root. Each block'sout_hashis also constrained by itsproposed_block_header_hash.