Skip to content
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 #1016

Draft
wants to merge 59 commits into
base: develop
Choose a base branch
from

Conversation

vladimir-trifonov
Copy link

Purpose or design rationale of this PR

Describe your change. Make sure to answer these three questions: What does this PR do? Why does it do it? How does it do it?

Updating scroll monorepo to support l1 block hashes POC.

PR title

Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:

  • build: Changes that affect the build system or external dependencies (example scopes: yarn, eslint, typescript)
  • ci: Changes to our CI configuration files and scripts (example scopes: vercel, github, cypress)
  • docs: Documentation-only changes
  • feat: A new feature
  • fix: A bug fix
  • perf: A code change that improves performance
  • refactor: A code change that doesn't fix a bug, or add a feature, or improves performance
  • style: Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc)
  • test: Adding missing tests or correcting existing tests

Deployment tag versioning

Has tag in common/version.go been updated or have you added bump-version label to this PR?

  • No, this PR doesn't involve a new deployment, git tag, docker image tag
  • Yes

Breaking change label

Does this PR have the breaking-change label?

  • No, this PR is not a breaking change
  • Yes

@vladimir-trifonov vladimir-trifonov changed the title Lime chain/block hashes poc 2 L1 Block Hashes Nov 20, 2023
@vladimir-trifonov vladimir-trifonov force-pushed the LimeChain/block-hashes-poc-2 branch from 64f4cdd to 24f9020 Compare November 20, 2023 14:34
@vladimir-trifonov vladimir-trifonov force-pushed the LimeChain/block-hashes-poc-2 branch from 33b2481 to 49ecf53 Compare November 20, 2023 14:37
@vladimir-trifonov vladimir-trifonov changed the title L1 Block Hashes wip: l1 block hashes poc Nov 21, 2023
@zimpha zimpha self-requested a review November 27, 2023 11:37
@reo101 reo101 force-pushed the LimeChain/block-hashes-poc-2 branch from b38c73b to 7fafc28 Compare December 11, 2023 13:46

import {L1Blocks} from "../../src/L2/L1Blocks.sol";

contract DeployL2L1BlocksContract is Script {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we need a hardfork to add this change, I think it's better to make it a predployed contract.

* @return hash_ The keccak hash of all blockhashes in the provided range.
*/
function blockRangeHash(uint256 _from, uint256 _to) external view returns (bytes32 hash_) {
require(_to >= _from, "Incorrect from/to range");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is better to use custom error here.


for (uint256 i = _from; i <= _to; i++) {
bytes32 blockHash = blockhash(i);
require(blockHash != 0, "Blockhash not available");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here.

// stack too deep
struct ChunkResult {
// _totalNumL1MessagesInChunk The total number of L1 messages popped in current chunk
uint256 _totalNumL1MessagesInChunk;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove the bai in the variable name.

BatchHeaderV0Codec.storeParentBatchHash(batchPtr, _parentBatchHash);
BatchHeaderV0Codec.storeSkippedBitmap(batchPtr, _skippedL1MessageBitmap);
BatchHeaderV0Codec.storeLastAppliedL1Block(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since we change the structure of BatchHeader, it is better to use another version.

@@ -165,13 +190,13 @@ contract ScrollChain is OwnableUpgradeable, PausableUpgradeable, IScrollChain {
uint8 _version,
bytes calldata _parentBatchHeader,
bytes[] memory _chunks,
bytes calldata _skippedL1MessageBitmap
bytes calldata _skippedL1MessageBitmap,
uint64 _prevLastAppliedL1Block
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the first value of _prevLastAppliedL1Block? And except the first one, all _prevLastAppliedL1Block can be found in the _parentBatchHeader, right?

uint64 lastAppliedL1Block_ = lastAppliedL1Block;

/// @dev It handles the case where the block is in the future.
require(_number <= lastAppliedL1Block_, "L1Blocks: hash number out of bounds");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same, use custom error here.

@vladimir-trifonov vladimir-trifonov force-pushed the LimeChain/block-hashes-poc-2 branch from 2585426 to ee021f7 Compare December 12, 2023 09:15
@vladimir-trifonov vladimir-trifonov force-pushed the LimeChain/block-hashes-poc-2 branch from ee021f7 to 72fe2d2 Compare December 12, 2023 09:20
failfmi and others added 24 commits December 13, 2023 14:15
…lock hashes specific fields

add missing chunk setters
@Thegaram Thegaram removed their request for review June 5, 2024 12:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants