fix: constrain proposed block header#13693
Conversation
LeilaWang
force-pushed
the
lw/constrain_proposed_block
branch
from
8d76e49 to
ece49b0
Compare
| bytes calldata _blobPublicInputs | ||
| ) internal view returns (bytes32[] memory) { | ||
| RollupStore storage rollupStore = STFLib.getStorage(); | ||
| // Args are defined as an array because Solidity complains with "stack too deep" otherwise |
There was a problem hiding this comment.
Removing the comment as it's not an array anymore.
| uint256 pendingBlockNumber = STFLib.getEffectivePendingBlockNumber(_args.currentTime); | ||
|
|
||
| require( | ||
| _args.header.globalVariables.blockNumber == pendingBlockNumber + 1, |
There was a problem hiding this comment.
We don't need to check the block number here because in the block merge rollup circuit we ensure that consecutive block numbers are incrementing by 1. And when proving an epoch, we check that the first block number is correct, by checking previousArchive.nextAvailableLeafIndex. So the block numbers after the first block are also guaranteed to be correct.
| // @note: not view as sampling validators uses tstore | ||
| function validateHeader(ValidateHeaderArgs memory _args) internal { | ||
| require( | ||
| block.chainid == _args.header.globalVariables.chainId, |
There was a problem hiding this comment.
chainId and version are propagated to root and checked once per epoch.
MirandaWood
left a comment
MirandaWood
left a comment
There was a problem hiding this comment.
Thought I'd have more to say considering the number of files, but all looks great and very clean! LGTM
| + 1 /* protocol_contract_tree_root */ | ||
| + 1 /* prover_id */ | ||
| + AZTEC_MAX_EPOCH_DURATION * BLOB_PUBLIC_INPUTS * BLOBS_PER_BLOCK; | ||
| // + 6 for end_timestamp, end_block_number, out_hash, vk_tree_root, protocol_contract_tree_root, prover_id |
There was a problem hiding this comment.
I think this referred to ROOT_ROLLUP_PUBLIC_INPUTS_LENGTH which you've nicely broken up below so can be removed!
There was a problem hiding this comment.
Thanks for catching this!
| function updateHeaderArchive(bytes memory _header, bytes32 _archive) | ||
| internal | ||
| pure | ||
| returns (bytes memory) | ||
| { | ||
| assembly { | ||
| mstore(add(_header, 0x20), _archive) | ||
| } | ||
| return _header; | ||
| } | ||
|
|
There was a problem hiding this comment.
Thank you for adding these, I always hated editing the assembly for these use cases!!
| }; | ||
|
|
||
| retrievedBlocks.push({ ...block, l1 }); | ||
| const chainId = new Fr(await publicClient.getChainId()); |
There was a problem hiding this comment.
This should be called outside the loop
There was a problem hiding this comment.
That makes way more sense! Will change this.
| private sender: EthAddress | undefined; | ||
|
|
||
| constructor( | ||
| /** The block number of the attestation. */ |
There was a problem hiding this comment.
Having block number outside the consensus payload here means that attesting nodes are not directly checking it, which means it will only be caught at proving time. Im not sure we want this
There was a problem hiding this comment.
I don't know if I miss anything, but this block number is only used in debug logs so I thought it should be fine throwing it in via constructor?
| retrievedBlocks.push({ ...block, l1 }); | ||
| const chainId = new Fr(await publicClient.getChainId()); | ||
|
|
||
| const version = new Fr(await rollup.read.getVersion({ blockNumber: log.blockNumber })); |
There was a problem hiding this comment.
When this is passing along blockNumber as this, it will likely try to run at that point in time, which could make it explode as historical execution. You can remove the blockNumber part, for the same contract the value will not change. Also as @Maddiaa0 said above, can be done outside the loop.
4 participants
#12928