-
Notifications
You must be signed in to change notification settings - Fork 607
refactor!: remove unneeded root rollup public inputs #13929
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -49,8 +49,6 @@ impl BlockMergeRollupInputs { | |
| // ^ Where instead of num_txs, use num_blocks = (end_global_variables.block_number - start_global_variables.block_number) + 1 | ||
| components::assert_prev_block_rollups_follow_on_from_each_other(left, right); | ||
|
|
||
| let out_hash = components::compute_blocks_out_hash(self.previous_rollup_data); | ||
|
|
||
| let proposed_block_header_hashes = | ||
| components::accumulate_proposed_block_header_hashes(left, right); | ||
|
|
||
|
|
@@ -67,7 +65,7 @@ impl BlockMergeRollupInputs { | |
| new_archive: right.new_archive, | ||
| 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. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it not still useful to see that it is accumulated correctly?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't accumulate the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Agree that it's confusing. I struggled to find a good name for it, so kept the name I removed Whether 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 But if you think a single type of block header is better, I will add the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Re |
||
| proposed_block_header_hashes, | ||
| fees, | ||
| vk_tree_root: left.vk_tree_root, | ||
|
|
@@ -83,7 +81,6 @@ mod tests { | |
| use dep::types::constants::{ | ||
| BLOCK_MERGE_ROLLUP_INDEX, BLOCK_ROOT_ROLLUP_INDEX, ROOT_PARITY_INDEX, | ||
| }; | ||
| use dep::types::hash::accumulate_sha256; | ||
| use dep::types::tests::fixtures; | ||
| use types::merkle_tree::merkle_tree::MerkleTree; | ||
|
|
||
|
|
@@ -122,15 +119,6 @@ mod tests { | |
| let _output = inputs.block_merge_rollup_circuit(); | ||
| } | ||
|
|
||
| #[test] | ||
| fn out_hash() { | ||
| let mut inputs = default_block_merge_rollup_inputs(); | ||
| let expected_hash = accumulate_sha256([1, 2]); | ||
| let outputs = inputs.block_merge_rollup_circuit(); | ||
|
|
||
| assert_eq(outputs.out_hash, expected_hash); | ||
| } | ||
|
|
||
| #[test] | ||
| fn block_fees_are_accumulated() { | ||
| let mut inputs = default_block_merge_rollup_inputs(); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,5 @@ | ||
| use crate::abis::{ | ||
| block_root_or_block_merge_public_inputs::{BlockRootOrBlockMergePublicInputs, FeeRecipient}, | ||
| previous_rollup_block_data::PreviousRollupBlockData, | ||
| use crate::abis::block_root_or_block_merge_public_inputs::{ | ||
| BlockRootOrBlockMergePublicInputs, FeeRecipient, | ||
| }; | ||
| use super::abis::tx_effect::TxEffect; | ||
| use dep::types::{ | ||
|
|
@@ -18,7 +17,7 @@ use dep::types::{ | |
| PUBLIC_DATA_UPDATE_REQUESTS_PREFIX, PUBLIC_LOG_SIZE_IN_FIELDS, PUBLIC_LOGS_PREFIX, | ||
| REVERT_CODE_PREFIX, TX_FEE_PREFIX, TX_START_PREFIX, | ||
| }, | ||
| hash::{accumulate_sha256, compute_contract_class_log_hash}, | ||
| hash::compute_contract_class_log_hash, | ||
| merkle_tree::VariableMerkleTree, | ||
| traits::{Empty, ToField}, | ||
| utils::arrays::{array_length, array_length_until, array_merge, array_padded_with}, | ||
|
|
@@ -82,12 +81,6 @@ pub fn accumulate_blocks_fees( | |
| left: BlockRootOrBlockMergePublicInputs, | ||
| right: BlockRootOrBlockMergePublicInputs, | ||
| ) -> [FeeRecipient; AZTEC_MAX_EPOCH_DURATION] { | ||
| let left_len = array_length(left.fees); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| let right_len = array_length(right.fees); | ||
| assert( | ||
| left_len + right_len <= AZTEC_MAX_EPOCH_DURATION, | ||
| "too many fee payment structs accumulated in rollup", | ||
| ); | ||
| // TODO(Miranda): combine fees with same recipient depending on rollup structure | ||
| // Assuming that the final rollup tree (block root -> block merge -> root) has max 32 leaves (TODO: constrain in root), then | ||
| // in the worst case, we would be checking the left 16 values (left_len = 16) against the right 16 (right_len = 16). | ||
|
|
@@ -102,11 +95,6 @@ pub fn accumulate_blob_public_inputs( | |
| right: BlockRootOrBlockMergePublicInputs, | ||
| ) -> [BlockBlobPublicInputs; AZTEC_MAX_EPOCH_DURATION] { | ||
| let left_len = array_length(left.blob_public_inputs); | ||
| let right_len = array_length(right.blob_public_inputs); | ||
| assert( | ||
| left_len + right_len <= AZTEC_MAX_EPOCH_DURATION, | ||
| "too many blob public input structs accumulated in rollup", | ||
| ); | ||
| // NB: The below is cheaper than array_merge because assigning BlockBlobPublicInputs is cheaper than calling .equals | ||
| let mut add_from_left = true; | ||
| let mut result = [BlockBlobPublicInputs::empty(); AZTEC_MAX_EPOCH_DURATION]; | ||
|
|
@@ -121,17 +109,6 @@ pub fn accumulate_blob_public_inputs( | |
| result | ||
| } | ||
|
|
||
| pub fn compute_blocks_out_hash(previous_rollup_data: [PreviousRollupBlockData; 2]) -> Field { | ||
| if previous_rollup_data[1].block_root_or_block_merge_public_inputs.is_padding() { | ||
| previous_rollup_data[0].block_root_or_block_merge_public_inputs.out_hash | ||
| } else { | ||
| accumulate_sha256([ | ||
| previous_rollup_data[0].block_root_or_block_merge_public_inputs.out_hash, | ||
| previous_rollup_data[1].block_root_or_block_merge_public_inputs.out_hash, | ||
| ]) | ||
| } | ||
| } | ||
|
|
||
| pub fn compute_kernel_out_hash(l2_to_l1_msgs: [Field; MAX_L2_TO_L1_MSGS_PER_TX]) -> Field { | ||
| let non_empty_items = array_length(l2_to_l1_msgs); | ||
| let merkle_tree = VariableMerkleTree::new_sha(l2_to_l1_msgs, non_empty_items); | ||
|
|
||
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.
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
assertAcceptablewill pass, but someheaderHashes here will be set as 0s when they shouldn't be and fail verification. If it's a longer a range of blocks (andassertAcceptablesomehow passes) then it's the reverse - someheaderHashes 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 setendas a block not yet proposed - so it's all good?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.
Exactly :)