-
Notifications
You must be signed in to change notification settings - Fork 578
feat!: aggregate out hashes and set on L1 per epoch #17712
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
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,7 +16,6 @@ import {STFLib} from "@aztec/core/libraries/rollup/STFLib.sol"; | |
| import {ValidatorSelectionLib} from "@aztec/core/libraries/rollup/ValidatorSelectionLib.sol"; | ||
| import {Timestamp, Slot, Epoch, TimeLib} from "@aztec/core/libraries/TimeLib.sol"; | ||
| import {CompressedSlot, CompressedTimeMath} from "@aztec/shared/libraries/CompressedTimeMath.sol"; | ||
| import {Math} from "@oz/utils/math/Math.sol"; | ||
| import {SafeCast} from "@oz/utils/math/SafeCast.sol"; | ||
|
|
||
| /** | ||
|
|
@@ -117,7 +116,20 @@ library EpochProofLib { | |
| require(verifyEpochRootProof(_args), Errors.Rollup__InvalidProof()); | ||
|
|
||
| RollupStore storage rollupStore = STFLib.getStorage(); | ||
| rollupStore.tips = rollupStore.tips.updateProven(Math.max(rollupStore.tips.getProven(), _args.end)); | ||
|
|
||
| // Advance the proven block number and insert the out hash if the chain is extended. | ||
| if (_args.end > rollupStore.tips.getProven()) { | ||
| rollupStore.tips = rollupStore.tips.updateProven(_args.end); | ||
|
|
||
| // Handle L2->L1 message processing. | ||
| // The circuit outputs a zero out hash if the epoch contains no messages. It is also impossible for a partial | ||
| // epoch to produce a non-zero out hash, then later produce a zero out hash once more checkpoints are included. | ||
| // Therefore, we can safely skip the insertion for a zero out hash here. | ||
| if (_args.args.outHash != bytes32(0)) { | ||
| // Insert L2->L1 messages root into outbox for consumption. | ||
| rollupStore.config.outbox.insert(endEpoch, _args.args.outHash); | ||
| } | ||
| } | ||
|
|
||
| RewardLib.handleRewardsAndFees(_args, endEpoch); | ||
|
|
||
|
|
@@ -171,30 +183,33 @@ library EpochProofLib { | |
| // struct RootRollupPublicInputs { | ||
| // previous_archive_root: Field, | ||
| // end_archive_root: Field, | ||
| // out_hash: Field, | ||
| // checkpointHeaderHashes: [Field; Constants.AZTEC_MAX_EPOCH_DURATION], | ||
| // fees: [FeeRecipient; Constants.AZTEC_MAX_EPOCH_DURATION], | ||
| // chain_id: Field, | ||
| // version: Field, | ||
| // vk_tree_root: Field, | ||
| // protocol_contracts_hash: Field, | ||
| // prover_id: Field, | ||
| // blob_public_inputs: FinalBlobAccumulatorPublicInputs, | ||
| // blob_public_inputs: FinalBlobAccumulator, | ||
| // } | ||
| { | ||
| // previous_archive.root: the previous archive tree root | ||
| publicInputs[0] = _args.previousArchive; | ||
|
|
||
| // end_archive.root: the new archive tree root | ||
| publicInputs[1] = _args.endArchive; | ||
|
|
||
| publicInputs[2] = _args.outHash; | ||
|
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. As mentioned elsewhere this one seem to be essentially unconstrained (on L1) to the outhashes that was published during the proposals. |
||
| } | ||
|
|
||
| uint256 numCheckpoints = _end - _start + 1; | ||
|
|
||
| for (uint256 i = 0; i < numCheckpoints; i++) { | ||
| publicInputs[2 + i] = STFLib.getHeaderHash(_start + i); | ||
| publicInputs[3 + i] = STFLib.getHeaderHash(_start + i); | ||
| } | ||
|
|
||
| uint256 offset = 2 + Constants.AZTEC_MAX_EPOCH_DURATION; | ||
| uint256 offset = 3 + Constants.AZTEC_MAX_EPOCH_DURATION; | ||
|
|
||
| uint256 feesLength = Constants.AZTEC_MAX_EPOCH_DURATION * 2; | ||
| // fees[2n to 2n + 1]: a fee element, which contains of a recipient and a value | ||
|
|
@@ -249,7 +264,6 @@ library EpochProofLib { | |
| publicInputs[offset] = bytes32(uint256(uint248(bytes31((_blobPublicInputs[96:127]))))); | ||
| // c[1] | ||
| publicInputs[offset + 1] = bytes32(uint256(uint136(bytes17((_blobPublicInputs[127:144]))))); | ||
| offset += 2; | ||
|
|
||
| return publicInputs; | ||
| } | ||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check 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.
As long as the circuits are sound, there should be no case where it is possible for us to end up needing to overwrite an outhash with 0 if it was previously non-zero.
This is kinda more a comment for myself.
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 would be good to also make a note that if the tree is empty it will return 0 here, as that is not always the case. But having that makes it possible for us to skip the "isIgnition" check.
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 you still have the issue last discussed. There are outhashes in the proposalHeader again, but I'm not sure that they are actually linked to the eventual
outHashthat one seem to be provided by the prover? So if the circuits are broken what is stopping someone from proposing A_1, A_2, A_n, and then the prover posting B_1 as the outhash? I'm probably just missing something 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.
Yeah you are right, if the circuits are broken, there's nothing stopping someone to post a completely different out hash via the root rollup proof. To actually use the out hashes in the proposed headers to validate the root out hash, I think we have to store the out hash in the block log. And when proving an epoch, we could either:
I think the second one is better because it's cheaper. Although it means the checkpoint header now depends on the state of the previous checkpoints in the same epoch, making it slightly complicated to compute and verify. But when we move to multiple blocks per checkpoint, the similar situation will apply to the block headers anyway.
Are we good with adding the out hash to the block log?
Or any other ideas?
On a similar note, if the circuits are broken, it would allow a prover to set a high fee in the root rollup circuit's public inputs. Do we want to do something about this?
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 storing it in the block log is acceptable. Since we are doing circular storage there it would add ~3K on a propose, but it is one of the things that can be optimized by storing just the hash of the blocklog.
I think as you say, that solution 2 is probably the way to go if we anyway would need to do that for multiple blocks per checkpoint.
If circuits are broken, it is acceptable that the fee asset controlled by them run into issues. The damage is much less troublesome than the outbox generally