-
Notifications
You must be signed in to change notification settings - Fork 391
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
[WIP] L1 block hashes POC #1040
base: develop
Are you sure you want to change the base?
[WIP] L1 block hashes POC #1040
Conversation
0cf70f2
to
c59c48e
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.
Completed first pass. Going through the PI circuit changes again for in-depth review. Will post asap
geth-utils/l2geth/trace.go
Outdated
@@ -190,6 +190,7 @@ func Trace(config TraceConfig) (*types.BlockTrace, error) { | |||
// Random: &randao, | |||
BaseFee: toBigInt(config.Block.BaseFee), | |||
GasLimit: blockGasLimit, | |||
// TODO: add last_applied_l1_block and l1_block_hashes |
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.
TODO
let mut num_all_l1_block_hashes_in_blocks = 0; | ||
for block_ctx in self.block_ctxs.ctxs.values() { | ||
if let Some(l1_block_hashes) = &block_ctx.l1_block_hashes { | ||
num_all_l1_block_hashes_in_blocks += l1_block_hashes.len(); | ||
} | ||
} | ||
|
||
num_all_l1_block_hashes_in_blocks |
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.
can't we just do last_applied_l1_block - prev_last_applied_l1_block
to get the same 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.
we want the exact count of l1 block hashes so we can constraint it against last_applied_l1_block - prev_last_applied_l1_block
zkevm-circuits/src/pi_circuit.rs
Outdated
self.block_ctxs.ctxs | ||
.last_key_value() | ||
.map(|(_, blk)| blk.eth_block.last_applied_l1_block.unwrap_or(U64::zero())) |
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.
similarly here, shouldn't this simply be last_applied_l1_block
?
zkevm-circuits/src/pi_circuit.rs
Outdated
if public_data.prev_last_applied_l1_block != 0 { | ||
assert!( | ||
public_data.last_applied_l1_block >= public_data.prev_last_applied_l1_block, | ||
"last_applied_l1_block should be greater or equal than prev_last_applied_l1_block"); | ||
l1_block_hashes_count = | ||
public_data.last_applied_l1_block - public_data.prev_last_applied_l1_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.
shouldn't this simply be:
let l1_block_hashes_count = public_data.last_applied_l1_block - public_data.prev_last_applied_l1_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.
the first time we have l1 block hashes in a block the prev_last_applied_l1_block
will be 0 and then we don't want to have let l1_block_hashes_count = public_data.last_applied_l1_block - public_data.prev_last_applied_l1_block;
because it will be an incorrect value, so in that case we just have an unuseful constraint that num_all_l1_block_hashes == num_all_l1_block_hashes
c3e03a2
to
8787180
Compare
Current status, I added L1BlockHashes Tx and am trying to fix the Tx flow to not fail. We added an L1BlockHashes Tx to the test but currently, there are failing parts of the circuit. |
64274ff
to
9ab71ab
Compare
This reverts commit c6af8ba17344c5bbde59505e0257c269b5594414.
This reverts commit 2761520.
This reverts commit 8acb71d.
* reproduce phase issues in sha256_circuit * fix issues * fmt and clippy * fix gate issue --------- Co-authored-by: kunxian xia <[email protected]>
Description
lastAppliedL1Block
,blockRangeHash
lastAppliedL1Block
in the block trace matches the last applied block viaL1Blockhashes
transaction.L1Blockhashes
in a batch and check them against the publicblockRangeHash
.lastAppliedL1Block
matches the last block tracelastAppliedL1Block
.Issue Link
[link issue here]
Type of change
Contents
Rationale
[design decisions and extended information]
How Has This Been Tested?
[explanation]
How to fill a PR description
Please give a concise description of your PR.
The target readers could be future developers, reviewers, and auditors. By reading your description, they should easily understand the changes proposed in this pull request.
MUST: Reference the issue to resolve
Single responsability
Is RECOMMENDED to create single responsibility commits, but not mandatory.
Anyway, you MUST enumerate the changes in a unitary way, e.g.
Design choices
RECOMMENDED to: