Skip to content

fix[batch-submitter]: add same-address error#1708

Merged
smartcontracts merged 5 commits intoethereum-optimism:developfrom
0xYYY:fix-batch-submitter-warn-same-address
Nov 13, 2021
Merged

fix[batch-submitter]: add same-address error#1708
smartcontracts merged 5 commits intoethereum-optimism:developfrom
0xYYY:fix-batch-submitter-warn-same-address

Conversation

@0xYYY
Copy link
Copy Markdown
Contributor

@0xYYY 0xYYY commented Nov 5, 2021

Description
Add a warning when proposer and sequencer have the same address.

Metadata

@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Nov 5, 2021 •
edited
Loading

🦋 Changeset detected

Latest commit: d370b07

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/batch-submitter 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

@github-actions github-actions bot added 2-reviewers A-op-batcher Area: op-batcher labels Nov 5, 2021
@smartcontracts
Copy link
Copy Markdown
Contributor

smartcontracts commented Nov 5, 2021 •
edited
Loading

Actually, could you make this throw an error instead of a warning (throw new Error instead of logger.warn)? Fantastic work btw, thank you!

@tynes
Copy link
Copy Markdown
Contributor

tynes commented Nov 5, 2021

Be sure to squash the commits into a single commit

@0xYYY
Copy link
Copy Markdown
Contributor Author

0xYYY commented Nov 6, 2021

No problem, I'll change it to throw an Error. And also squash the commits.

@0xYYY 0xYYY force-pushed the fix-batch-submitter-warn-same-address branch from 02a4ed4 to c40d86e Compare November 6, 2021 04:52
@0xYYY
Copy link
Copy Markdown
Contributor Author

0xYYY commented Nov 9, 2021 •
edited
Loading

It seems like the failed tests are those doing L2 => L1 actions.

I'm guessing the reason is that the batch_submitter service failed to start because of our modification, plus that PROPOSER_PRIVATE_KEY isn't specified in ops/docker-compose.yml.

optimism/ops/docker-compose.yml

Lines 116 to 132 in c17a73b

batch_submitter:
depends_on:
- l1_chain
- deployer
- l2geth
image: ethereumoptimism/batch-submitter
build:
context: ..
dockerfile: ./ops/docker/Dockerfile.batch-submitter
entrypoint: ./batches.sh
env_file:
- ./envs/batches.env
environment:
L1_NODE_WEB3_URL: http://l1_chain:8545
L2_NODE_WEB3_URL: http://l2geth:8545
URL: http://deployer:8081/addresses.json
SEQUENCER_PRIVATE_KEY: "0x59c6995e998f97a5a0044966f0945389dc9e86dae88c7a8412f4603b6b78690d"

I can modify the docker files so that sequencer and proposer use different addresses. Sounds reasonable?

(I ran the integration tests locally and all the tests passed if we specify another address for the proposer.)

@0xYYY 0xYYY force-pushed the fix-batch-submitter-warn-same-address branch from c40d86e to e84c502 Compare November 9, 2021 08:49
@smartcontracts
Copy link
Copy Markdown
Contributor

smartcontracts commented Nov 9, 2021

Great catch @0xYYY. Running CI again...

@smartcontracts
Copy link
Copy Markdown
Contributor

smartcontracts commented Nov 9, 2021

@0xYYY Could you just rebase this on top of the latest develop? Should be good to go once that's done. Thank you!

@0xYYY 0xYYY force-pushed the fix-batch-submitter-warn-same-address branch from e84c502 to 3e6ad45 Compare November 11, 2021 01:45
@0xYYY 0xYYY force-pushed the fix-batch-submitter-warn-same-address branch from 3e6ad45 to 526d7e5 Compare November 11, 2021 01:48
@0xYYY
Copy link
Copy Markdown
Contributor Author

0xYYY commented Nov 11, 2021

@smartcontracts I have rebased this to the latest develop.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Nov 11, 2021 •
edited
Loading

Codecov Report

Merging #1708 (d370b07) into develop (fb54425) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #1708   +/-   ##
========================================
  Coverage    71.81%   71.81%           
========================================
  Files           69       69           
  Lines         2303     2303           
  Branches       344      344           
========================================
  Hits          1654     1654           
  Misses         649      649
Flag Coverage Δ
batch-submitter 61.56% <ø> (ø)
contracts 87.96% <ø> (ø)
core-utils 56.53% <ø> (ø)
data-transport-layer 38.23% <ø> (ø)
message-relayer 70.86% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fb54425...d370b07. Read the comment docs.

@0xYYY
Copy link
Copy Markdown
Contributor Author

0xYYY commented Nov 12, 2021 •
edited
Loading

@smartcontracts rebased to the latest develop again. Should be good to go after this CI?

@0xYYY 0xYYY changed the title fix[batch-submitter]: add same-address warning fix[batch-submitter]: add same-address error Nov 12, 2021
@smartcontracts smartcontracts merged commit 1caa26e into ethereum-optimism:develop Nov 13, 2021
@smartcontracts
Copy link
Copy Markdown
Contributor

smartcontracts commented Nov 13, 2021

Thank you!

@smartcontracts
Copy link
Copy Markdown
Contributor

smartcontracts commented Nov 13, 2021

Send me an ETH address and I'll send you a hand-drawn contributor NFT :-)

@0xYYY
Copy link
Copy Markdown
Contributor Author

0xYYY commented Nov 13, 2021

Thanks for the guidance on my first PR to Optimism!
I'll send you an email to claim the NFT. :)

theochap pushed a commit that referenced this pull request Dec 10, 2025
@refcell
fix(node/engine): Derivation Sync Start (#1708)
8a8c808
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@smartcontracts smartcontracts smartcontracts approved these changes

Assignees

No one assigned

Labels

A-op-batcher Area: op-batcher A-ops Area: ops

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Warn or throw an error when sequencer and proposer addresses are the same

4 participants

@0xYYY @smartcontracts @tynes @codecov-commenter