draft PR for chained l1 message queue #69
Conversation
| *********************/ | ||
|
|
||
| /// @dev The list of queued cross domain messages. | ||
| mapping(uint256 => bytes32) private messageRollingHashes; |
There was a problem hiding this comment.
Would be good to have the memory layout here in the description
| @@ -3,7 +3,8 @@ | |||
| pragma solidity =0.8.24; | |||
|
|
|||
| import {IScrollChain} from "./rollup/IScrollChain.sol"; | |||
| import {IL1MessageQueue} from "./rollup/IL1MessageQueue.sol"; | |||
| import {IL1MessageQueueV1} from "./rollup/IL1MessageQueueV1.sol"; | |||
| import {IL1MessageQueueV2} from "./rollup/IL1MessageQueueV2.sol"; | |||
There was a problem hiding this comment.
IL1MessageQueueV2 is not used in this file
There was a problem hiding this comment.
yes, it will be added together with changes in ScrollChain, which is not started yet.
| function estimatedL2BaseFee() public view returns (uint256) { | ||
| L2BaseFeeParameters memory parameters = l2BaseFeeParameters; | ||
| // this is unlikely to happen, use unchecked here | ||
| unchecked { | ||
| return (block.basefee * parameters.scalar) / PRECISION + parameters.overhead; | ||
| } | ||
| } |
There was a problem hiding this comment.
Is the idea here that we will no longer use gas-oracle to write l2 base fee here?
If yes, would it make sense to move configs such as L2BaseFeeParameters into a SystemConfig contract (the same that @ranchalp will use to store the current authorized L2 signer)?
There was a problem hiding this comment.
Yes, we use formula to compute the l2 base fee. Sure, it is better to add it into SystemConfig contract.
| ) external { | ||
| if (_msgSender() != enforcedTxGateway) revert ErrorCallerIsNotEnforcedTxGateway(); | ||
| // We will check it in EnforcedTxGateway, just in case. | ||
| if (_sender.code.length > 0) revert ErrorCallerIsNotEOA(); |
There was a problem hiding this comment.
This is not compatible with EIP-7702. Do we need this restriction?
There was a problem hiding this comment.
we can remove this and also remove check in EnforcedTxGateway.
| _updateL2BaseFeeParameters(_params.overhead, _params.scalar); | ||
| _updateMaxGasLimit(_maxGasLimit); | ||
|
|
||
| uint256 _nextCrossDomainMessageIndex = IL1MessageQueueV1(messageQueueV1).nextCrossDomainMessageIndex(); |
There was a problem hiding this comment.
As a convenience, we could consider storing this as something like firstMessageIndex, to know which contract a specific index belongs to.
| /// @notice Update the parameters for l2 base fee formula. | ||
| /// @param overhead The value of overhead in l2 base fee formula. | ||
| /// @param scalar The value of scalar in l2 base fee formula. | ||
| function updateL2BaseFeeParameters(uint128 overhead, uint128 scalar) external onlyOwner { |
There was a problem hiding this comment.
Note to self: These will need to be configured on ScrollOwner
There was a problem hiding this comment.
will move this function SystemConfig contract.
| /// @param _value The value passed | ||
| /// @param _gasLimit The maximum gas should be used for this transaction in L2. | ||
| /// @param _data The calldata passed to target contract. | ||
| function _queueTransaction( |
There was a problem hiding this comment.
What will be the impact of this added hashing on ETH and ERC20 deposit gas costs?
There was a problem hiding this comment.
my estimation is adding 2000~5000 gas.
| /// @param hash The rolling hash. | ||
| /// @param enqueueTimestamp The enqueue timestamp. | ||
| /// @return The encoded rolling hash for storage. | ||
| function _encodeRollingHash(bytes32 hash, uint256 enqueueTimestamp) internal pure returns (bytes32) { |
There was a problem hiding this comment.
Should we mention the use case of enqueueTimestamp somewhere?
There was a problem hiding this comment.
It should combine with enforces messages PR #61. I'll add comments about this.
| function _encodeRollingHash(bytes32 hash, uint256 enqueueTimestamp) internal pure returns (bytes32) { | ||
| assembly { | ||
| // clear last 36 bits and then encode timestamp to it. | ||
| hash := or(enqueueTimestamp, shl(36, shr(36, hash))) |
There was a problem hiding this comment.
no particular reason, just pick a number sufficient enough. 36 bits is about 2000 years, 34 bits is about 489 years, 32 bits is about 89 year.
we can definitely use 32 bits if we want.
| hash = messageRollingHashes[index]; | ||
| assembly { | ||
| enqueueTimestamp := and(hash, 0xfffffffff) | ||
| hash := shl(36, shr(36, hash)) |
There was a problem hiding this comment.
So the rolling hash only has a domain of 220 bits (not 256), but that should be secure enough, is that the idea?
There was a problem hiding this comment.
yes, 220 bits are secure enough.
| @@ -121,7 +122,7 @@ contract ScrollChain is OwnableUpgradeable, PausableUpgradeable, IScrollChain { | |||
| uint64 public immutable layer2ChainId; | |||
|
|
|||
| /// @notice The address of L1MessageQueue contract. | |||
| address public immutable messageQueue; | |||
| address public immutable messageQueueV1; | |||
There was a problem hiding this comment.
How will we switch from V1 to V2 in ScrollChain and in L1ScrollMessenger?
There was a problem hiding this comment.
It is not done yet. I have discussed with @jonastheis, quote the results here
after queue v2 is initialized, every new message goes to queue v2
for remaining messages in queue v1: we keep it in queue v1 and continue consuming them until the upgrade happens on L2
there will be multiple hours of time between upgrade tx on L1 and the actual upgrade time on L2 -> we should have consumed and finalized all messages from queue v1
once the upgrade happened on L2: in l2geth we need to implement relaying the messages from queue v2 anyway. We add a condition that we only do that after the upgrade is activated.
once we commit with new commit method we check that all messages of queue v1 are finalized. otherwise reject or alternatively migrate to queue v2. since we left enough time in step 3 there should be no message anyway.
No description provided.