feat[contracts]: Add block/allow and pausing to the L1 Messenger by maurelian · Pull Request #579 · ethereum-optimism/optimism

maurelian · 2021-04-23T03:20:45Z

Description

This PR adds safety functionality to the L1 Messenger:

Pausable ~~except with the 'Context._msgSender()` thing removed.~~
Ownable
blockMessage
allowMessage

Additional context

I think the main design decision worth discussing here is the additional gas cost of needing to SLOAD from the blockedMessages mapping on every relayMessage() call. The alternative would be to use the successfulMessages mapping to block messages. The very large downside of that is it gives the owner the ability to allow a message to be relayed twice (or more).

IMO the cost is probably acceptable consider the frequency of use.

changeset-bot · 2021-04-23T03:20:47Z

🦋 Changeset detected

Latest commit: 78246e1

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@eth-optimism/contracts	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

.changeset/wild-cycles-care.md

maurelian · 2021-04-23T03:29:31Z

.../contracts/contracts/optimistic-ethereum/OVM/bridge/messaging/OVM_L1CrossDomainMessenger.sol

     ***************/

    /**
-     * Pass a default zero address to the address resolver. This will be updated when initialized.


I felt like this comment was slightly misleading, as the constructor is setting nothing in the proxy.

maurelian · 2021-04-23T03:31:13Z

.../contracts/contracts/optimistic-ethereum/OVM/bridge/messaging/OVM_L1CrossDomainMessenger.sol

        override
        public
        nonReentrant
-        onlyRelayer()


I looked elsewhere: using brackets on modifiers with no args was inconsistent with other parts of the code.

maurelian · 2021-04-23T03:31:31Z

.../contracts/contracts/optimistic-ethereum/OVM/bridge/messaging/OVM_L1CrossDomainMessenger.sol

    /**
     * Relays a cross domain message to a contract.
-     * @inheritdoc iOVM_L1CrossDomainMessenger
+     * This function executes a transaction on L1, which was initiated on L2.


I brought these doc strings in here from the interface.

The docs should live in the interface, so let's revert this change.

.../contracts/contracts/optimistic-ethereum/OVM/bridge/messaging/OVM_L1CrossDomainMessenger.sol

packages/contracts/contracts/optimistic-ethereum/libraries/utils/Lib_Pausable.sol

packages/contracts/test/contracts/OVM/bridge/base/OVM_L1CrossDomainMessenger.spec.ts

.../contracts/contracts/optimistic-ethereum/OVM/bridge/messaging/OVM_L1CrossDomainMessenger.sol

ben-chain

Looking good, requested a quick additional test.

@K-Ho this code doesn't feel super meaningful to me without removing the onlyRelayer. I feel like we should:

Make this PR be against the 0.3.0 RC
Remove the onlyRelayer and MultiMessageRelayer.

Maybe there's an argument for having 2. in a separate step though?

.../contracts/contracts/optimistic-ethereum/OVM/bridge/messaging/OVM_L1CrossDomainMessenger.sol

packages/contracts/contracts/optimistic-ethereum/libraries/utils/Lib_Pausable.sol

packages/contracts/test/contracts/OVM/bridge/base/OVM_L1CrossDomainMessenger.spec.ts

tynes

So, downgrade to patch?

Yup please downgrade this to patch, we will bump the minor version once for the RLP regenesis

tynes · 2021-04-23T19:58:52Z

Remove the onlyRelayer and MultiMessageRelayer

@ben-chain Isn't the MultiMessageRelayer still useful if somebody wants to do multiple withdrawals when it is not permissioned?

ben-chain · 2021-04-23T20:10:12Z

@ben-chain Isn't the MultiMessageRelayer still useful if somebody wants to do multiple withdrawals when it is not permissioned?

This is true, although we have had the idea in the past of using the msg.sender to pay a fee to the relayer, which would break if the msg.sender is always the MultiMessageRelayer. I tend to think that just adding a batchRelayMessages into the L1CrossDomainMessenger itself would be the preferable way to have multiple withdrawals for this reason. That could be a good argument to do that in a separate PR, though.

tynes · 2021-04-23T20:24:44Z

I tend to think that just adding a batchRelayMessages into the L1CrossDomainMessenger itself would be the preferable way to have multiple withdrawals for this reason. That could be a good argument to do that in a separate PR, though.

I agree that this is a better solution

.changeset/wild-cycles-care.md

maurelian · 2021-04-24T18:54:00Z

That could be a good argument to do that in a separate PR, though.

I agree that this is a better solution

#623

gakonst · 2021-04-25T09:12:29Z

I've rebased and squashed the PR, while resolving merge conflicts. Let's try to avoid git merge master too many times, and favor rebasing and keeping things clean. Also let's make sure that we have passing tests before we dive into reviews!

ben-chain

This is looking good to me. I have a tiny request to leave a test in, but then I think we'll be good to merge.

packages/contracts/test/contracts/OVM/bridge/base/OVM_L1CrossDomainMessenger.spec.ts

tynes · 2021-04-26T20:00:49Z

Looks like this needs another rebase/squash

tynes · 2021-04-28T22:32:46Z

.../contracts/contracts/optimistic-ethereum/OVM/bridge/messaging/OVM_L1CrossDomainMessenger.sol

Have you thought about not adding a blockedMessages mapping and instead just writing to successfulMessages? This would save a SLOAD in relayMessage. The name successfulMessages doesn't really make sense anymore if that was the case, it could be something like messages or relayedMessages

Good question! Yes, it's mentioned in the PR description, and called out inline here.

An important downside IMO is that it changes the trust assumptions significantly, because if we retain the allowMessage() functionality, the owner could delete successfulMessages entries, making messages replayable.

Agreed that this is not a permission we want to enable.

tynes · 2021-04-28T22:35:11Z

This PR is on master, I could rebase the v0.3.0-rc branch on top of master again for the next rc release but then our git history would get a little out of whack. I will need to rebase again anyways due to other PRs going into master that need to go into the rc branch before the next release

maurelian · 2021-04-28T23:43:32Z

This PR is on master, I could rebase the v0.3.0-rc branch on top of master again for the next rc release but then our git history would get a little out of whack. I will need to rebase again anyways due to other PRs going into master that need to go into the rc branch before the next release

@tynes Is there any action you would like me to take? :)

The change requested that I not remove a test. The test was not removed, it was simply relocated to the end of the file: https://github.com/ethereum-optimism/optimism/pull/579/files#diff-b54fbcb912d89d7f34df9c51f5476fa991feafec083c879b83a56b50fc52dd40R446

This is identical to Pausable, except for replacing Context._msgSender() with just msg.sender

This reverts commit 4bbfe60.

gakonst

LGTM - nice work.

gakonst · 2021-04-29T09:54:35Z

.../contracts/contracts/optimistic-ethereum/OVM/bridge/messaging/OVM_L1CrossDomainMessenger.sol

Agreed that this is not a permission we want to enable.

* feat(contracts): add pausable.sol This is identical to Pausable, except for replacing Context._msgSender() with just msg.sender * feat: add pause, block and allow to L1 messenger * test(l1-messenger): access control, block, allow * chore: add changeset * chore: yarn lint * test[contracts]: maintain previous test order * fix[contracts]: add upgradable OZ contracts * Revert "test[contracts]: maintain previous test order" This reverts commit 4bbfe60. * test[contracts]: add initializer modifier * fix(contracts): delete unused file Co-authored-by: Georgios Konstantopoulos <me@gakonst.com>

* feat(derive): holocene frame queue changes * feat(derive): holocene frame queue * Update crates/derive/src/stages/frame_queue.rs Co-authored-by: clabby <ben@clab.by> * fix(derive): fq nits --------- Co-authored-by: clabby <ben@clab.by>

Part of #441

maurelian requested review from ben-chain and smartcontracts as code owners April 23, 2021 03:20

maurelian commented Apr 23, 2021

View reviewed changes

maurelian requested a review from K-Ho April 23, 2021 14:26

maurelian commented Apr 23, 2021

View reviewed changes

.../contracts/contracts/optimistic-ethereum/OVM/bridge/messaging/OVM_L1CrossDomainMessenger.sol Show resolved Hide resolved

.../contracts/contracts/optimistic-ethereum/OVM/bridge/messaging/OVM_L1CrossDomainMessenger.sol Show resolved Hide resolved

tynes reviewed Apr 23, 2021

View reviewed changes

.../contracts/contracts/optimistic-ethereum/OVM/bridge/messaging/OVM_L1CrossDomainMessenger.sol Show resolved Hide resolved

ben-chain requested changes Apr 23, 2021

View reviewed changes

tynes requested changes Apr 23, 2021

View reviewed changes

maurelian commented Apr 24, 2021

View reviewed changes

.changeset/wild-cycles-care.md Outdated Show resolved Hide resolved

maurelian mentioned this pull request Apr 24, 2021

Remove onlyRelayer and MultiMessageRelayer from L1 messenger #623

Closed

maurelian requested review from annieke and karlfloersch as code owners April 24, 2021 19:04

gakonst force-pushed the feat/pauseBlockAllowL1Msgr branch from 7192f59 to 88bfc22 Compare April 25, 2021 09:11

gakonst force-pushed the feat/pauseBlockAllowL1Msgr branch from 88bfc22 to 36909c9 Compare April 25, 2021 09:13

maurelian changed the title ~~Add block/allow and pausing to the L1 Messenger~~ Feat[contracts]: Add block/allow and pausing to the L1 Messenger Apr 25, 2021

maurelian changed the title ~~Feat[contracts]: Add block/allow and pausing to the L1 Messenger~~ feat[contracts]: Add block/allow and pausing to the L1 Messenger Apr 25, 2021

maurelian requested review from ben-chain and tynes and removed request for K-Ho and annieke April 26, 2021 17:08

ben-chain previously requested changes Apr 26, 2021

View reviewed changes

packages/contracts/test/contracts/OVM/bridge/base/OVM_L1CrossDomainMessenger.spec.ts Outdated Show resolved Hide resolved

maurelian requested a review from ben-chain April 27, 2021 01:38

maurelian marked this pull request as draft April 27, 2021 02:33

maurelian mentioned this pull request Apr 28, 2021

Downloaded Solidity by hardhat does not have kall, causing compilation errors #675

Closed

gakonst self-assigned this Apr 28, 2021

maurelian force-pushed the feat/pauseBlockAllowL1Msgr branch 2 times, most recently from ec46324 to 554e5eb Compare April 28, 2021 20:47

maurelian marked this pull request as ready for review April 28, 2021 21:03

tynes reviewed Apr 28, 2021

View reviewed changes

maurelian requested a review from tynes April 29, 2021 00:19

maurelian and others added 10 commits April 29, 2021 12:35

feat(contracts): add pausable.sol

eeeb65f

This is identical to Pausable, except for replacing Context._msgSender() with just msg.sender

feat: add pause, block and allow to L1 messenger

5f995e6

test(l1-messenger): access control, block, allow

c5a6097

chore: add changeset

a95094f

chore: yarn lint

90299b5

test[contracts]: maintain previous test order

bb7e87a

fix[contracts]: add upgradable OZ contracts

e9e29cc

Revert "test[contracts]: maintain previous test order"

b167e62

This reverts commit 4bbfe60.

test[contracts]: add initializer modifier

6084267

fix(contracts): delete unused file

78246e1

gakonst force-pushed the feat/pauseBlockAllowL1Msgr branch from fe287a6 to 78246e1 Compare April 29, 2021 09:36

gakonst approved these changes Apr 29, 2021

View reviewed changes

gakonst merged commit 0ef3069 into master Apr 29, 2021

gakonst deleted the feat/pauseBlockAllowL1Msgr branch April 29, 2021 09:56

ben-chain mentioned this pull request May 10, 2021

feat[contracts]: have hardhat-deploy register L1MultiMessageRelayer on mainnet #828

Merged

agusduha pushed a commit that referenced this pull request Sep 25, 2025

Merge pull request #579 from defi-wonderland/chore/last-splitter-sync

2157a49

theochap pushed a commit that referenced this pull request Jan 15, 2026

chore: bump msrv 1.86 (#579)

753b2d9

emhane pushed a commit that referenced this pull request Feb 4, 2026

chore: hardware requirement section added (#579)

5704d59

Part of #441

Conversation

maurelian commented Apr 23, 2021 • edited by gakonst Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

changeset-bot bot commented Apr 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

Uh oh!

maurelian Apr 23, 2021

Choose a reason for hiding this comment

Uh oh!

maurelian Apr 23, 2021

Choose a reason for hiding this comment

Uh oh!

maurelian Apr 23, 2021

Choose a reason for hiding this comment

Uh oh!

gakonst Apr 25, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ben-chain left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tynes left a comment

Choose a reason for hiding this comment

Uh oh!

tynes commented Apr 23, 2021

Uh oh!

ben-chain commented Apr 23, 2021

Uh oh!

tynes commented Apr 23, 2021

Uh oh!

Uh oh!

maurelian commented Apr 24, 2021

Uh oh!

gakonst commented Apr 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ben-chain left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tynes commented Apr 26, 2021

Uh oh!

tynes Apr 28, 2021

Choose a reason for hiding this comment

Uh oh!

maurelian Apr 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gakonst Apr 29, 2021

Choose a reason for hiding this comment

Uh oh!

tynes commented Apr 28, 2021

Uh oh!

maurelian commented Apr 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gakonst left a comment

Choose a reason for hiding this comment

Uh oh!

gakonst Apr 29, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

maurelian commented Apr 23, 2021 •

edited by gakonst

Loading

changeset-bot bot commented Apr 23, 2021 •

edited

Loading

gakonst commented Apr 25, 2021 •

edited

Loading

maurelian Apr 28, 2021 •

edited

Loading

maurelian commented Apr 28, 2021 •

edited

Loading