feat: update scroll consensus #314
Conversation
Signed-off-by: Gregory Edison <[email protected]>
Signed-off-by: Gregory Edison <[email protected]>
Signed-off-by: Gregory Edison <[email protected]>
* feat: update execute_with_state_closure closure Signed-off-by: Gregory Edison <[email protected]> * feat: LoadWithdrawRoot Signed-off-by: Gregory Edison <[email protected]> * chore: fix clippy docs (paradigmxyz#17726) Co-authored-by: Alexey Shekhirin <[email protected]> * fix: clippy Signed-off-by: Gregory Edison <[email protected]> --------- Signed-off-by: Gregory Edison <[email protected]> Co-authored-by: Matthias Seitz <[email protected]> Co-authored-by: Alexey Shekhirin <[email protected]>
CodSpeed Performance ReportMerging #314 will not alter performanceComparing Summary
|
Signed-off-by: Gregory Edison <[email protected]>
|
Rust logic looks sound, deferring to the subject matter experts for consnsus logic review. |
| // Check if the block contains L1 messages. | ||
| if !txs.iter().any(ScrollTransaction::is_l1_message) { | ||
| return Ok(()) | ||
| } |
There was a problem hiding this comment.
Doesn't this check ultimately imply that if there is an L1 message present in the block, then we end up iterating transactions twice? Is there any value in this?
There was a problem hiding this comment.
yes can remove this, it added it because l2geth has it, but it makes sense for them since they also have some db access, whereas here we just iterate.
Signed-off-by: Gregory Edison <[email protected]>
| parent: &H, | ||
| ) -> Result<(), ConsensusError> { | ||
| let diff = header.gas_limit().abs_diff(parent.gas_limit()); | ||
| let limit = parent.gas_limit().saturating_div(GAS_LIMIT_BOUND_DIVISOR); |
There was a problem hiding this comment.
May checked_div be better here?
There was a problem hiding this comment.
I replaced it with plain div /, saturating div here wasn't useful and checked_div returns None if the divisor is 0, which is guaranteed not to be the case here.
There was a problem hiding this comment.
I think I can even just use plain div, checked will return None if the divisor is 0, which in this case it wouldn't be.
Signed-off-by: Gregory Edison <[email protected]>
| // Check index is strictly increasing. | ||
| if tx.is_l1_message() { | ||
| let tx_queue_index = tx.queue_index().expect("is_l1_message"); | ||
| if tx_queue_index < queue_index { |
There was a problem hiding this comment.
From EuclidV2 onwards there can't be any skipped L1 messages, so l1 message indexes after EuclidV2 should be consecutive. I guess here we only make sure it's increasing rather than consecutively increasing?
some context here: https://github.com/scroll-tech/go-ethereum/blob/8bee78a76ca728cc37dd65d3b5e76e7f6386e2f6/core/block_validator.go#L150
There was a problem hiding this comment.
you're right, will check that we don't skip any for EuclidV2
Signed-off-by: Gregory Edison <[email protected]>
Updates the scroll consensus checks based on the l2geth clique and system config consensus. Successfully synced to block 18663346 on Mainnet with the consensus updates.
Resolves #290.
Resolves #296.
Note: Clique consensus signer verification isn't performed. I don't see any issues with skipping it, but do tell me if I'm wrong. This check would require some work to save signer snapshots.