Skip to content

Optimisations in the sequencer inbox #68

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

Closed
wants to merge 41 commits into from
Closed
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
ee52a74
Added test sequencer inbox opt
yahgwai Oct 4, 2023
cb72410
Made state vars immutable
yahgwai Oct 4, 2023
2b17df0
Added commentson max time variation
yahgwai Oct 4, 2023
ad06124
Removed deprecated method
yahgwai Oct 4, 2023
a4c3800
Updated tests
yahgwai Oct 4, 2023
90de978
Updated comments
yahgwai Oct 4, 2023
b7414fb
Formatted files
yahgwai Oct 4, 2023
87aafcf
Test file lint
yahgwai Oct 4, 2023
5890bde
Merge from develop
yahgwai Oct 5, 2023
998fe16
Updated max time variation comments
yahgwai Oct 5, 2023
a60f7ff
Updated tests
yahgwai Oct 5, 2023
b7b2f49
Storage layout checks no longer required for sequencer inbox
yahgwai Oct 5, 2023
0b9ba23
Merge from develop
yahgwai Oct 25, 2023
cd0d302
Build updates
yahgwai Oct 25, 2023
75215ba
Updated tests and added bridge.rollup() initialization tests
yahgwai Oct 25, 2023
a3111c4
Formatting
yahgwai Oct 25, 2023
90f2262
Updated arbrollup tests
yahgwai Oct 25, 2023
fe97273
Removed test events
yahgwai Oct 25, 2023
f75a8fc
Moved delayed messages read and batch delivered event into the bridge
yahgwai Oct 26, 2023
887cc47
Removed tests for now
yahgwai Oct 26, 2023
d06f2a0
Added encoder v2
yahgwai Oct 26, 2023
ce8fb88
Updated bridge storage dot
yahgwai Oct 26, 2023
3609765
Merge branch 'develop' into seq-inbox-opt
gzeoneth Oct 27, 2023
275d75b
Merge remote-tracking branch 'origin/develop' into seq-inbox-opt
gzeoneth Oct 30, 2023
22c9c75
Merge remote-tracking branch 'origin/seq-inbox-opt' into seq-inbox-br…
gzeoneth Oct 31, 2023
a4337e1
Updates from CR
yahgwai Oct 31, 2023
f3b1ceb
Merge branch 'seq-inbox-bridge' of https://github.com/OffchainLabs/ni…
yahgwai Oct 31, 2023
285b6bf
Merge from remote
yahgwai Oct 31, 2023
085e0e9
Removed old dot files
yahgwai Oct 31, 2023
3646a78
Merge pull request #87 from OffchainLabs/seq-inbox-bridge
yahgwai Oct 31, 2023
31fd3c6
Added internal max time variation func
yahgwai Nov 17, 2023
05e311d
Updated max time variation to be u64
yahgwai Nov 17, 2023
773e1ec
Merge branch 'develop' into seq-inbox-opt
Tristan-Wilson Dec 7, 2023
f3cfc60
fix: backward compatible maxTimeVariation
gzeoneth Dec 28, 2023
1fd0958
Formatting
yahgwai Jan 9, 2024
ab6855c
Updated tests
yahgwai Jan 9, 2024
a8f206b
Merge pull request #106 from OffchainLabs/g-patch-1
yahgwai Jan 9, 2024
2dda137
Update src/bridge/ISequencerInbox.sol
yahgwai Jan 9, 2024
e728f1f
Updated gas refunder to account for proxy usage
yahgwai Jan 16, 2024
1c6a328
Formatting
yahgwai Jan 16, 2024
0b7dc82
Moved calldata words inside
yahgwai Jan 16, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion deploy/BridgeStubCreator.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ module.exports = async hre => {
const { deploy } = deployments
const { deployer } = await getNamedAccounts()

await deploy('BridgeStub', { from: deployer, args: [] })
await deploy('BridgeStub', { from: deployer, args: [deployer] })
}

module.exports.tags = ['BridgeStub', 'test']
Expand Down
3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,8 @@
"test:storage": "./test/storage/test.bash",
"postinstall": "patch-package",
"deploy-factory": "hardhat run scripts/deployment.ts",
"deploy-rollup": "hardhat run scripts/rollupCreation.ts"
"deploy-rollup": "hardhat run scripts/rollupCreation.ts",
"test": "hardhat --network hardhat test test/contract/*.spec.ts"
},
"dependencies": {
"@openzeppelin/contracts": "4.5.0",
Expand Down
33 changes: 10 additions & 23 deletions src/bridge/ISequencerInbox.sol
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,11 @@ import "./IDelayedMessageProvider.sol";
import "./IBridge.sol";

interface ISequencerInbox is IDelayedMessageProvider {
/// @notice The maximum amount of time variatin between a message being posted on the L1 and being executed on the L2
/// @param delayBlocks The max amount of blocks in the past that a message can be received on L2
/// @param futureBlocks The max amount of blocks in the future that a message can be received on L2
/// @param delaySeconds The max amount of seconds in the past that a message can be received on L2
/// @param futureSeconds The max amount of seconds in the future that a message can be received on L2
struct MaxTimeVariation {
uint256 delayBlocks;
uint256 futureBlocks;
Expand Down Expand Up @@ -78,21 +83,11 @@ interface ISequencerInbox is IDelayedMessageProvider {
uint64 creationBlock;
}

function maxTimeVariation()
external
view
returns (
uint256,
uint256,
uint256,
uint256
);
/// @notice Returns the max time variation settings for this sequencer inbox
function maxTimeVariation() external view returns (ISequencerInbox.MaxTimeVariation memory);

function dasKeySetInfo(bytes32) external view returns (bool, uint64);

/// @notice Remove force inclusion delay after a L1 chainId fork
function removeDelayAfterFork() external;

/// @notice Force messages from the delayed inbox to be included in the chain
/// Callable by any address, but message can only be force-included after maxTimeVariation.delayBlocks and
/// maxTimeVariation.delaySeconds has elapsed. As part of normal behaviour the sequencer will include these
Expand Down Expand Up @@ -127,7 +122,9 @@ interface ISequencerInbox is IDelayedMessageProvider {
uint256 sequenceNumber,
bytes calldata data,
uint256 afterDelayedMessagesRead,
IGasRefunder gasRefunder
IGasRefunder gasRefunder,
uint256 prevMessageCount,
uint256 newMessageCount
) external;

function addSequencerL2Batch(
Expand All @@ -141,12 +138,6 @@ interface ISequencerInbox is IDelayedMessageProvider {

// ---------- onlyRollupOrOwner functions ----------

/**
* @notice Set max delay for sequencer inbox
* @param maxTimeVariation_ the maximum time variation parameters
*/
function setMaxTimeVariation(MaxTimeVariation memory maxTimeVariation_) external;

/**
* @notice Updates whether an address is authorized to be a batch poster at the sequencer inbox
* @param addr the address
Expand All @@ -173,8 +164,4 @@ interface ISequencerInbox is IDelayedMessageProvider {
* @param isSequencer_ if the specified address should be authorized as a sequencer
*/
function setIsSequencer(address addr, bool isSequencer_) external;

// ---------- initializer ----------

function initialize(IBridge bridge_, MaxTimeVariation calldata maxTimeVariation_) external;
}
131 changes: 53 additions & 78 deletions src/bridge/SequencerInbox.sol
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,8 @@ import {
DataNotAuthenticated,
AlreadyValidDASKeyset,
NoSuchKeyset,
NotForked
NotForked,
NotOwner
} from "../libraries/Error.sol";
import "./IBridge.sol";
import "./IInbox.sol";
Expand All @@ -32,7 +33,6 @@ import "../precompiles/ArbSys.sol";

import {L1MessageType_batchPostingReport} from "../libraries/MessageTypes.sol";
import {GasRefundEnabled, IGasRefunder} from "../libraries/IGasRefunder.sol";
import "../libraries/DelegateCallAware.sol";
import "../libraries/ArbitrumChecker.sol";

/**
Expand All @@ -42,20 +42,24 @@ import "../libraries/ArbitrumChecker.sol";
* in the delayed inbox (Bridge.sol). If items in the delayed inbox are not included by a
* sequencer within a time limit they can be force included into the rollup inbox by anyone.
*/
contract SequencerInbox is DelegateCallAware, GasRefundEnabled, ISequencerInbox {
contract SequencerInbox is GasRefundEnabled, ISequencerInbox {
uint256 public totalDelayedMessagesRead;

IBridge public bridge;
IBridge public immutable bridge;

/// @inheritdoc ISequencerInbox
uint256 public constant HEADER_LENGTH = 40;

/// @inheritdoc ISequencerInbox
bytes1 public constant DATA_AUTHENTICATED_FLAG = 0x40;

IOwnable public rollup;
IOwnable public immutable rollup;
mapping(address => bool) public isBatchPoster;
ISequencerInbox.MaxTimeVariation public maxTimeVariation;
// see ISequencerInbox.MaxTimeVariation
uint256 internal immutable delayBlocks;
uint256 internal immutable futureBlocks;
uint256 internal immutable delaySeconds;
uint256 internal immutable futureSeconds;

mapping(bytes32 => DasKeySetInfo) public dasKeySetInfo;

Expand All @@ -72,47 +76,57 @@ contract SequencerInbox is DelegateCallAware, GasRefundEnabled, ISequencerInbox
// If the chain this SequencerInbox is deployed on is an Arbitrum chain.
bool internal immutable hostChainIsArbitrum = ArbitrumChecker.runningOnArbitrum();

constructor(uint256 _maxDataSize) {
constructor(
IBridge bridge_,
ISequencerInbox.MaxTimeVariation memory maxTimeVariation_,
uint256 _maxDataSize
) {
if (bridge_ == IBridge(address(0))) revert HadZeroInit();
bridge = bridge_;
rollup = bridge_.rollup();
delayBlocks = maxTimeVariation_.delayBlocks;
futureBlocks = maxTimeVariation_.futureBlocks;
delaySeconds = maxTimeVariation_.delaySeconds;
futureSeconds = maxTimeVariation_.futureSeconds;
Comment on lines +87 to +90
Copy link
Member

@gzeoneth gzeoneth Nov 3, 2023

Choose a reason for hiding this comment

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

worth noting the uint64 cast below is not safe, this is an existing issue and will only be a problem with misconfiguration; tho we might also consider to have some sanity check here to make sure each value is within some bound

or we can just perform some check there, we already check for underflow but not overflow

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it's fine to have maxtimevariation as uint64s right? I've updated to that now

maxDataSize = _maxDataSize;
}

function _chainIdChanged() internal view returns (bool) {
return deployTimeChainId != block.chainid;
}

function initialize(
IBridge bridge_,
ISequencerInbox.MaxTimeVariation calldata maxTimeVariation_
) external onlyDelegated {
if (bridge != IBridge(address(0))) revert AlreadyInit();
if (bridge_ == IBridge(address(0))) revert HadZeroInit();
bridge = bridge_;
rollup = bridge_.rollup();
maxTimeVariation = maxTimeVariation_;
}

function getTimeBounds() internal view virtual returns (TimeBounds memory) {
TimeBounds memory bounds;
if (block.timestamp > maxTimeVariation.delaySeconds) {
bounds.minTimestamp = uint64(block.timestamp - maxTimeVariation.delaySeconds);
ISequencerInbox.MaxTimeVariation memory maxTimeVariation_ = maxTimeVariation();
if (block.timestamp > maxTimeVariation_.delaySeconds) {
bounds.minTimestamp = uint64(block.timestamp - maxTimeVariation_.delaySeconds);
}
bounds.maxTimestamp = uint64(block.timestamp + maxTimeVariation.futureSeconds);
if (block.number > maxTimeVariation.delayBlocks) {
bounds.minBlockNumber = uint64(block.number - maxTimeVariation.delayBlocks);
bounds.maxTimestamp = uint64(block.timestamp + maxTimeVariation_.futureSeconds);
if (block.number > maxTimeVariation_.delayBlocks) {
bounds.minBlockNumber = uint64(block.number - maxTimeVariation_.delayBlocks);
}
bounds.maxBlockNumber = uint64(block.number + maxTimeVariation.futureBlocks);
bounds.maxBlockNumber = uint64(block.number + maxTimeVariation_.futureBlocks);
return bounds;
}

/// @inheritdoc ISequencerInbox
function removeDelayAfterFork() external {
if (!_chainIdChanged()) revert NotForked();
maxTimeVariation = ISequencerInbox.MaxTimeVariation({
delayBlocks: 1,
futureBlocks: 1,
delaySeconds: 1,
futureSeconds: 1
});
function maxTimeVariation() public view returns (ISequencerInbox.MaxTimeVariation memory) {
if (_chainIdChanged()) {
return
ISequencerInbox.MaxTimeVariation({
delayBlocks: 1,
futureBlocks: 1,
delaySeconds: 1,
futureSeconds: 1
});
} else {
return
ISequencerInbox.MaxTimeVariation({
delayBlocks: delayBlocks,
futureBlocks: futureBlocks,
delaySeconds: delaySeconds,
futureSeconds: futureSeconds
});
}
}

/// @inheritdoc ISequencerInbox
Expand All @@ -134,10 +148,11 @@ contract SequencerInbox is DelegateCallAware, GasRefundEnabled, ISequencerInbox
baseFeeL1,
messageDataHash
);
ISequencerInbox.MaxTimeVariation memory maxTimeVariation_ = maxTimeVariation();
// Can only force-include after the Sequencer-only window has expired.
if (l1BlockAndTime[0] + maxTimeVariation.delayBlocks >= block.number)
if (l1BlockAndTime[0] + maxTimeVariation_.delayBlocks >= block.number)
revert ForceIncludeBlockTooSoon();
if (l1BlockAndTime[1] + maxTimeVariation.delaySeconds >= block.timestamp)
if (l1BlockAndTime[1] + maxTimeVariation_.delaySeconds >= block.timestamp)
revert ForceIncludeTimeTooSoon();

// Verify that message hash represents the last message sequence of delayed message to be included
Expand Down Expand Up @@ -181,40 +196,6 @@ contract SequencerInbox is DelegateCallAware, GasRefundEnabled, ISequencerInbox
);
}

/// @dev Deprecated in favor of the variant specifying message counts for consistency
function addSequencerL2BatchFromOrigin(
uint256 sequenceNumber,
bytes calldata data,
uint256 afterDelayedMessagesRead,
IGasRefunder gasRefunder
) external refundsGas(gasRefunder) {
// solhint-disable-next-line avoid-tx-origin
if (msg.sender != tx.origin) revert NotOrigin();
if (!isBatchPoster[msg.sender]) revert NotBatchPoster();

(bytes32 dataHash, TimeBounds memory timeBounds) = formDataHash(
data,
afterDelayedMessagesRead
);
(
uint256 seqMessageIndex,
bytes32 beforeAcc,
bytes32 delayedAcc,
bytes32 afterAcc
) = addSequencerL2BatchImpl(dataHash, afterDelayedMessagesRead, data.length, 0, 0);
if (seqMessageIndex != sequenceNumber)
revert BadSequencerNumber(seqMessageIndex, sequenceNumber);
emit SequencerBatchDelivered(
sequenceNumber,
beforeAcc,
afterAcc,
delayedAcc,
totalDelayedMessagesRead,
timeBounds,
BatchDataLocation.TxInput
);
}

function addSequencerL2BatchFromOrigin(
uint256 sequenceNumber,
bytes calldata data,
Expand Down Expand Up @@ -437,18 +418,12 @@ contract SequencerInbox is DelegateCallAware, GasRefundEnabled, ISequencerInbox
return bridge.sequencerMessageCount();
}

/// @inheritdoc ISequencerInbox
function setMaxTimeVariation(ISequencerInbox.MaxTimeVariation memory maxTimeVariation_)
external
onlyRollupOwner
{
maxTimeVariation = maxTimeVariation_;
emit OwnerFunctionCalled(0);
}

/// @inheritdoc ISequencerInbox
function setIsBatchPoster(address addr, bool isBatchPoster_) external onlyRollupOwner {
isBatchPoster[addr] = isBatchPoster_;
// we used to have OwnerFunctionCalled(0) for setting the maxTimeVariation
// so we dont use index = 0 here, even though this is the first owner function
// to stay compatible with legacy events
emit OwnerFunctionCalled(1);
}

Expand Down
9 changes: 5 additions & 4 deletions src/mocks/BridgeStub.sol
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,11 @@ contract BridgeStub is IBridge {

address public sequencerInbox;
uint256 public override sequencerReportedSubMessageCount;
IOwnable public rollup;

constructor(IOwnable rollup_) {
rollup = rollup_;
}

function setSequencerInbox(address _sequencerInbox) external override {
sequencerInbox = _sequencerInbox;
Expand Down Expand Up @@ -170,10 +175,6 @@ contract BridgeStub is IBridge {
return sequencerInboxAccs.length;
}

function rollup() external pure override returns (IOwnable) {
revert("NOT_IMPLEMENTED");
}

function acceptFundsFromOldBridge() external payable {}

function initialize(IOwnable) external pure {
Expand Down
5 changes: 1 addition & 4 deletions src/mocks/SequencerInboxStub.sol
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,7 @@ contract SequencerInboxStub is SequencerInbox {
address sequencer_,
ISequencerInbox.MaxTimeVariation memory maxTimeVariation_,
uint256 maxDataSize_
) SequencerInbox(maxDataSize_) {
bridge = bridge_;
rollup = IOwnable(msg.sender);
maxTimeVariation = maxTimeVariation_;
) SequencerInbox(bridge_, maxTimeVariation_, maxDataSize_) {
isBatchPoster[sequencer_] = true;
}

Expand Down
18 changes: 7 additions & 11 deletions src/rollup/BridgeCreator.sol
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ import "@openzeppelin/contracts/proxy/transparent/ProxyAdmin.sol";

contract BridgeCreator is Ownable {
Bridge public bridgeTemplate;
SequencerInbox public sequencerInboxTemplate;
Inbox public inboxTemplate;
RollupEventInbox public rollupEventInboxTemplate;
Outbox public outboxTemplate;
Expand All @@ -26,21 +25,18 @@ contract BridgeCreator is Ownable {

constructor(uint256 maxDataSize) Ownable() {
bridgeTemplate = new Bridge();
sequencerInboxTemplate = new SequencerInbox(maxDataSize);
inboxTemplate = new Inbox(maxDataSize);
rollupEventInboxTemplate = new RollupEventInbox();
outboxTemplate = new Outbox();
}

function updateTemplates(
address _bridgeTemplate,
address _sequencerInboxTemplate,
address _inboxTemplate,
address _rollupEventInboxTemplate,
address _outboxTemplate
) external onlyOwner {
bridgeTemplate = Bridge(_bridgeTemplate);
sequencerInboxTemplate = SequencerInbox(_sequencerInboxTemplate);
inboxTemplate = Inbox(_inboxTemplate);
rollupEventInboxTemplate = RollupEventInbox(_rollupEventInboxTemplate);
outboxTemplate = Outbox(_outboxTemplate);
Expand All @@ -60,7 +56,8 @@ contract BridgeCreator is Ownable {
function createBridge(
address adminProxy,
address rollup,
ISequencerInbox.MaxTimeVariation memory maxTimeVariation
ISequencerInbox.MaxTimeVariation memory maxTimeVariation,
uint256 maxDataSize
)
external
returns (
Expand All @@ -76,11 +73,6 @@ contract BridgeCreator is Ownable {
frame.bridge = Bridge(
address(new TransparentUpgradeableProxy(address(bridgeTemplate), adminProxy, ""))
);
frame.sequencerInbox = SequencerInbox(
address(
new TransparentUpgradeableProxy(address(sequencerInboxTemplate), adminProxy, "")
)
);
frame.inbox = Inbox(
address(new TransparentUpgradeableProxy(address(inboxTemplate), adminProxy, ""))
);
Expand All @@ -99,7 +91,11 @@ contract BridgeCreator is Ownable {
}

frame.bridge.initialize(IOwnable(rollup));
frame.sequencerInbox.initialize(IBridge(frame.bridge), maxTimeVariation);
frame.sequencerInbox = new SequencerInbox(
IBridge(frame.bridge),
maxTimeVariation,
maxDataSize
);
frame.inbox.initialize(IBridge(frame.bridge), ISequencerInbox(frame.sequencerInbox));
frame.rollupEventInbox.initialize(IBridge(frame.bridge));
frame.outbox.initialize(IBridge(frame.bridge));
Expand Down
Loading