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

feat: enforced messages and enforces messages #61

Closed
wants to merge 3 commits into from

Conversation

zimpha
Copy link
Member

@zimpha zimpha commented Nov 6, 2024

No description provided.

@zimpha zimpha requested review from Thegaram and jonastheis November 7, 2024 00:48
L2BaseFeeParameters memory parameters = l2BaseFeeParameters;
// this is unlikely to happen, use unchecked here
unchecked {
return (block.basefee * parameters.scalar) / PRECISION + parameters.overhead;
Copy link
Member

@colinlyguo colinlyguo Nov 14, 2024

Choose a reason for hiding this comment

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

we may set a backlog task to decommission part of the gas-oracle as a reminder, removing "updating l2 base fee in l1 contract".

after this new contract is deployed, although there shouldn't be any problems if we don't remove the code, just keeps printing "fee estimation failed, execution reverted" when simulating the txn.

/// @param postStateRoot The state root of current batch.
/// @param withdrawRoot The withdraw trie root of current batch.
/// @param version The version of current batch.
/// @param parentBatchHeader The header of parent batch, see the comments of `BatchHeaderV0Codec`.
Copy link
Member

@colinlyguo colinlyguo Nov 14, 2024

Choose a reason for hiding this comment

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

Suggested change
/// @param parentBatchHeader The header of parent batch, see the comments of `BatchHeaderV0Codec`.
/// @param parentBatchHeader The header of parent batch, see the comments of `BatchHeaderV0Codec`, `BatchHeaderV1Codec` and `BatchHeaderV3Codec.sol`.

src/L1/rollup/L1MessageQueue.sol Show resolved Hide resolved

/// @param The struct for batch committing.
/// @param version The version of current batch.
/// @param parentBatchHeader The header of parent batch, see the comments of `BatchHeaderV0Codec`.
Copy link
Member

@colinlyguo colinlyguo Nov 16, 2024

Choose a reason for hiding this comment

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

Suggested change
/// @param parentBatchHeader The header of parent batch, see the comments of `BatchHeaderV0Codec`.
/// @param parentBatchHeader The header of parent batch, see the comments of `BatchHeaderV0Codec`, `BatchHeaderV1Codec` and `BatchHeaderV3Codec.sol`.

/// timestamp `[t+maxFinalizeDelay, t+maxDelayEnterEnforcedMode]`.
struct EnforcedBatchParameters {
uint64 maxInclusionDelay;
uint64 maxFinalizeDelay;
Copy link
Member

Choose a reason for hiding this comment

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

how to resolve the issue when prover cannot finalize any batches before maxFinalizeDelay? Since finalizeBundleWithProof and finalizeBundleWithTeeProof are blocked by:

_checkFinalizationAllowed(enforcedBatchParameters.maxFinalizeDelay);

batchPtr := mload(0x40)
_totalL1MessagesPoppedOverall := add(_totalL1MessagesPoppedOverall, _totalL1MessagesPoppedInBatch)
/// @inheritdoc IScrollChain
function commitBatchWithBlobProof(
Copy link
Member

Choose a reason for hiding this comment

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

Is this interface used for rollup-relayer upgrading commit interface asynchronously?

/// @inheritdoc IScrollChain
function commitAndFinalizeBatch(CommitStruct calldata commitStruct, FinalizeStruct calldata finalizeStruct)
external
whenFinalizeNotPaused
Copy link
Member

@colinlyguo colinlyguo Nov 17, 2024

Choose a reason for hiding this comment

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

this interface checks whenFinalizeNotPaused, iiuc it will be paused when security council is resolving zk and tee proof divergence, not sure if it will break some conditions of "permissionless exit" in stage 1. e.g. unresolvedState has never been cleared, then commitAndFinalizeBatch cannot be triggered forever.

Comment on lines +115 to +116
/// @param overhead The address of new whitelist checker contract.
/// @param scalar The address of new whitelist checker contract.

Choose a reason for hiding this comment

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

wrong comment

/// @return _parentBatchHash The batch hash of parent batch header.
/// @return _batchIndex The index of current batch.
/// @return _totalL1MessagesPoppedOverall The total number of L1 messages popped before current batch.
function _beforeCommitBatch(bytes calldata _parentBatchHeader, bytes[] memory _chunks)
function _beforeCommitBatch(

Choose a reason for hiding this comment

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

There's only 1 call site for _beforeCommitBatch in line 567 and we always call it with lastCommittedBatchIndex=0. Why is that?

(bytes32 _parentBatchHash, uint256 _batchIndex, uint256 _totalL1MessagesPoppedOverall) = _beforeCommitBatch(
            _parentBatchHeader,
            _chunks,
            0
);

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed, one of the left todo.

);
BatchHeaderV0Codec.storeBatchIndex(batchPtr, _batchIndex);

// versions 2 and 3 both use ChunkCodecV1
(bytes32 _dataHash, uint256 _totalL1MessagesPoppedInBatch) = _commitChunksV1(
_totalL1MessagesPoppedOverall,
_chunks,
_skippedL1MessageBitmap
0 // parameters.maxInclusionDelay

Choose a reason for hiding this comment

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

is this a TODO?
since we also call _commitBatchWithBlobProof from commitAndFinalizeBatch we apply the same logic when in permissionless mode. Just to double check: This means that if there's lots of L1 messages then a permissionless batch will need to include all L1 messages first before any other tx created on L2 (via permissionless batch production toolkit)?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, it's a todo. fixed

EnforcedBatchParameters memory parameters = enforcedBatchParameters;
if (!parameters.enforcedModeEnabled) {
uint256 timestamp = IL1MessageQueue(messageQueue).getFirstUnfinalizedMessageTimestamp();
if (timestamp > 0 && timestamp + parameters.maxDelayEnterEnforcedMode < block.timestamp) {

Choose a reason for hiding this comment

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

What happens if there's no L1 message (eg because no user posted one on L1)? iiuc then permissionless mode will never be activated. we could make the sending of L1 messages a requirement but so far I thought permissionless mode would be either triggered by a L1 message or just no finalization for too long.

Copy link
Member Author

Choose a reason for hiding this comment

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

oh, yeah you are correct. forget to add this check.

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.

3 participants