Skip to content

feat: add go/batch-submitter env var parsing#1537

Merged
tynes merged 2 commits intoethereum-optimism:developfrom
cfromknecht:bss-envvar
Oct 14, 2021
Merged

feat: add go/batch-submitter env var parsing#1537
tynes merged 2 commits intoethereum-optimism:developfrom
cfromknecht:bss-envvar

Conversation

@cfromknecht
Copy link
Copy Markdown
Contributor

@cfromknecht cfromknecht commented Oct 6, 2021

Description
This PR adds parsing for the environment variables to be used in the upcoming rewrite of the batch submitter in Go. The currently selected set is mostly the same as the existing ones, though with some historical or unneeded parameters removed. In addition, this PR sets up unit testing and linting for the go/batch-submitter directory in Github Actions.

Additional context
Example output of ./batch-submitter -h

NAME:
   batch-submitter - Batch Submitter Service

USAGE:
   batch-submitter [global options] command [command options] [arguments...]

VERSION:
   0.0.1-1.10.8-stable-4f0ef72c

DESCRIPTION:
   Service for generating and submitting batched transactions that synchronize L2 state to L1 contracts

COMMANDS:
   help, h  Shows a list of commands or help for one command

GLOBAL OPTIONS:
   --build-env value                   Build environment for which the binary is produced, e.g. production or development [$BATCH_SUBMITTER_BUILD_ENV]
   --eth-network-name value            Ethereum network name [$BATCH_SUBMITTER_ETH_NETWORK_NAME]
   --l1-node-web3-url value            HTTP provider URL for L1 [$BATCH_SUBMITTER_L1_NODE_WEB3_URL]
   --l2-node-web3-url value            HTTP provider URL for L2 [$BATCH_SUBMITTER_L2_NODE_WEB3_URL]
   --ctc-address value                 Address of the CTC contract [$BATCH_SUBMITTER_CTC_ADDRESS]
   --scc-address value                 Address of the SCC contract [$BATCH_SUBMITTER_SCC_ADDRESS]
   --max-l1-tx-size value              Maximum size in bytes of any L1 transaction that gets generated by the batch submitter (default: 0) [$BATCH_SUBMITTER_MAX_L1_TX_SIZE]
   --max-batch-submission-time value   Maximum amount of time (seconds) that we will wait before submitting an under-sized batch (default: 0) [$BATCH_SUBMITTER_MAX_BATCH_SUBMISSION_TIME]
   --poll-interval value               Delay in milliseconds between querying L2 for more transactions and creating a new batch (default: 0) [$BATCH_SUBMITTER_POLL_INTERVAL]
   --num-confirmations value           Number of confirmations which we will wait after appending a new batch (default: 0) [$BATCH_SUBMITTER_NUM_CONFIRMATIONS]
   --resubmission-timeout value        Number of seconds we will wait before resubmitting a transaction (default: 0) [$BATCH_SUBMITTER_RESUBMISSION_TIMEOUT]
   --finality-confirmations value      Number of confirmations that we should wait before submitting state roots for CTC elements (default: 0) [$BATCH_SUBMITTER_FINALITY_CONFIRMATIONS]
   --run-tx-batch-submitter            Determines whether or not to run the tx batch submitter [$BATCH_SUBMITTER_RUN_TX_BATCH_SUBMITTER]
   --run-state-batch-submitter         Determines whether or not to run the state batch submitter [$BATCH_SUBMITTER_RUN_STATE_BATCH_SUBMITTER]
   --safe-minimum-ether-balance value  Safe minimum amount of ether the batch submitter key should hold before it starts to log errors (default: 0) [$BATCH_SUBMITTER_SAFE_MINIMUM_ETHER_BALANCE]
   --clear-pending-txs                 Whether or not to clear pending transaction in the mempool on startup [$BATCH_SUBMITTER_CLEAR_PENDING_TXS]
   --log-level value                   The lowest log level that will be output (default: "info") [$BATCH_SUBMITTER_LOG_LEVEL]
   --use-sentry                        Whether or not to enable Sentry. If true, sentry-dsn must also be set [$BATCH_SUBMITTER_USE_SENTRY]
   --sentry-dsn value                  Sentry data source name [$BATCH_SUBMITTER_SENTRY_DSN]
   --sentry-trace-rate value           Sentry trace rate (default: 0.05) [$BATCH_SUBMITTER_SENTRY_TRACE_RATE]
   --block-offset value                The offset between the CTC contract start and the L2 geth blocks (default: 1) [$BATCH_SUBMITTER_BLOCK_OFFSET]
   --max-gas-price-in-gwei value       Maximum gas price the batch submitter can use for transactions (default: 100) [$BATCH_SUBMITTER_MAX_GAS_PRICE_IN_GWEI]
   --gas-retry-increment-flag value    Default step by which to increment gas price bumps (default: 5) [$BATCH_SUBMITTER_GAS_RETRY_INCREMENT_FLAG]
   --sequencer-private-key value       The private key to use for sending to the sequencer contract [$BATCH_SUBMITTER_SEQUENCER_PRIVATE_KEY]
   --proposer-private-key value        The private key to use for sending to the proposer contract [$BATCH_SUBMITTER_PROPOSER_PRIVATE_KEY]
   --mnemonic value                    The mnemonic used to derive the wallets for either the sequencer or the proposer [$BATCH_SUBMITTER_MNEMONIC]
   --sequencer-hd-path value           The HD path used to derive the sequencer wallet from the mnemonic. The mnemonic flag must also be set. [$BATCH_SUBMITTER_SEQUENCER_HD_PATH]
   --proposer-hd-path value            The HD path used to derive the proposer wallet from the mnemonic. The mnemonic flag must also be set. [$BATCH_SUBMITTER_PROPOSER_HD_PATH]
   --run-metrics-server                Whether or not to run the embedded metrics server [$BATCH_SUBMITTER_RUN_METRICS_SERVER]
   --metrics-hostname value            The hostname of the metrics server (default: "127.0.0.1") [$BATCH_SUBMITTER_METRICS_HOSTNAME]
   --metrics-port value                The port of the metrics server (default: 7300) [$BATCH_SUBMITTER_METRICS_PORT]
   --help, -h                          show help
   --version, -v                       print the version

Metadata

  • Fixes ENG-1479

@github-actions github-actions bot added 2-reviewers M-ci Meta: ci related work labels Oct 6, 2021
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Oct 6, 2021

Codecov Report

Merging #1537 (e5c2686) into develop (ad4b432) will not change coverage.
The diff coverage is n/a.

❗ Current head e5c2686 differs from pull request most recent head f18e676. Consider uploading reports for the commit f18e676 to get more accurate results
Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #1537   +/-   ##
========================================
  Coverage    76.71%   76.71%           
========================================
  Files           82       82           
  Lines         3032     3032           
  Branches       463      463           
========================================
  Hits          2326     2326           
  Misses         706      706           
Flag Coverage Δ
batch-submitter 61.74% <ø> (ø)
contracts 86.05% <ø> (ø)
core-utils 65.03% <ø> (ø)
data-transport-layer 37.86% <ø> (ø)
message-relayer 83.48% <ø> (ø)

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 ad4b432...f18e676. Read the comment docs.

@tynes
Copy link
Copy Markdown
Contributor

tynes commented Oct 7, 2021

Hey @optimisticben - any opinions on this new config scheme? Its very similar to the existing one

Copy link
Copy Markdown
Contributor

@tynes tynes left a comment

Choose a reason for hiding this comment

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

Will be good to merge with the removal of DisableQueueBatchAppend

@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Oct 7, 2021

⚠️ No Changeset found

Latest commit: f96f611

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@cfromknecht cfromknecht requested a review from tynes October 7, 2021 01:11
@optimisticben
Copy link
Copy Markdown
Contributor

Hey @optimisticben - any opinions on this new config scheme? Its very similar to the existing one

Opinions? Always.

We need this? I'm used to seeing a chain id

--eth-network-name

Flag names nits

--l1-node-web3-url - Strongly prefer --l1-eth-url, or --l1-eth-rpc
--l2-node-web3-url - Same
--use-sentry - Prefer --sentry-enable, keeping the same string at the beginning of the flag names helps mental grouping
--run-metrics-server - Same idea, --metrics-server-enable

ENV names for shared configs (maybe outside scope)

I would really like to see configurations that should be common across the whole system defined in a way that they can be easily shared.
For example BATCH_SUBMITTER_CTC_ADDRESS. Can this be just CTC_ADDRESS?
We can define multiple env lists to a deployment. There would be less changes, and less config over all, as we could reuse a common set of shared env across components in a namespace.
Practically, this means having a "mainnet-configmap" with values any deployment can use, then a batch-submitter-configmap that would contain settings specific to the batch-submitter (prefaced with BATCH_SUBMITTER_)

@tynes
Copy link
Copy Markdown
Contributor

tynes commented Oct 7, 2021

Hey @optimisticben - any opinions on this new config scheme? Its very similar to the existing one

Opinions? Always.

We need this? I'm used to seeing a chain id

--eth-network-name

Flag names nits

--l1-node-web3-url - Strongly prefer --l1-eth-url, or --l1-eth-rpc --l2-node-web3-url - Same --use-sentry - Prefer --sentry-enable, keeping the same string at the beginning of the flag names helps mental grouping --run-metrics-server - Same idea, --metrics-server-enable

ENV names for shared configs (maybe outside scope)

I would really like to see configurations that should be common across the whole system defined in a way that they can be easily shared. For example BATCH_SUBMITTER_CTC_ADDRESS. Can this be just CTC_ADDRESS? We can define multiple env lists to a deployment. There would be less changes, and less config over all, as we could reuse a common set of shared env across components in a namespace. Practically, this means having a "mainnet-configmap" with values any deployment can use, then a batch-submitter-configmap that would contain settings specific to the batch-submitter (prefaced with BATCH_SUBMITTER_)

If you have strong preference for removing the prefixes for the env vars then I'm fine with it. It was something that we planned on doing to prevent accidental configurations. Sorry for telling you to do that conner, it should be easy to remove that add prefix function in the flags

@cfromknecht
Copy link
Copy Markdown
Contributor Author

cfromknecht commented Oct 7, 2021

--eth-network-name

Currently this is only used for Sentry logs and Prometheus labels, is chain id still preferable there?

--l1-node-web3-url - Strongly prefer --l1-eth-url, or --l1-eth-rpc
--l2-node-web3-url - Same
--use-sentry - Prefer --sentry-enable, keeping the same string at the beginning of the flag names helps mental grouping
--run-metrics-server - Same idea, --metrics-server-enable

Done!

I would really like to see configurations that should be common across the whole system defined in a way that they can be easily shared.

I can definitely add this as well. Tried to comb through and ended up with this list:

  • build-env
  • eth-network-name/eth-chain-id
  • ctc-address
  • scc-address
  • l1-eth-rpc maybe?
  • l2-eth-rpc maybe?

I removed those prefixes, lmk what you think.

Sorry for telling you to do that conner, it should be easy to remove that add prefix function in the flags

No problem, easy change!

@optimisticben @tynes

Updated ./batch-submitter -h output
NAME:
   batch-submitter - Batch Submitter Service

USAGE:
   batch-submitter [global options] command [command options] [arguments...]

VERSION:
   0.0.1-1.10.8-stable-f18e6768

DESCRIPTION:
   Service for generating and submitting batched transactions that synchronize L2 state to L1 contracts

COMMANDS:
   help, h  Shows a list of commands or help for one command

GLOBAL OPTIONS:
   --build-env value                   Build environment for which the binary is produced, e.g. production or development [$BUILD_ENV]
   --eth-network-name value            Ethereum network name [$ETH_NETWORK_NAME]
   --l1-eth-rpc value                  HTTP provider URL for L1 [$L1_ETH_RPC]
   --l2-eth-rpc value                  HTTP provider URL for L2 [$L2_ETH_RPC]
   --ctc-address value                 Address of the CTC contract [$CTC_ADDRESS]
   --scc-address value                 Address of the SCC contract [$SCC_ADDRESS]
   --max-l1-tx-size value              Maximum size in bytes of any L1 transaction that gets generated by the batch submitter (default: 0) [$BATCH_SUBMITTER_MAX_L1_TX_SIZE]
   --max-batch-submission-time value   Maximum amount of time (seconds) that we will wait before submitting an under-sized batch (default: 0) [$BATCH_SUBMITTER_MAX_BATCH_SUBMISSION_TIME]
   --poll-interval value               Delay in milliseconds between querying L2 for more transactions and creating a new batch (default: 0) [$BATCH_SUBMITTER_POLL_INTERVAL]
   --num-confirmations value           Number of confirmations which we will wait after appending a new batch (default: 0) [$BATCH_SUBMITTER_NUM_CONFIRMATIONS]
   --resubmission-timeout value        Number of seconds we will wait before resubmitting a transaction (default: 0) [$BATCH_SUBMITTER_RESUBMISSION_TIMEOUT]
   --finality-confirmations value      Number of confirmations that we should wait before submitting state roots for CTC elements (default: 0) [$BATCH_SUBMITTER_FINALITY_CONFIRMATIONS]
   --run-tx-batch-submitter            Determines whether or not to run the tx batch submitter [$BATCH_SUBMITTER_RUN_TX_BATCH_SUBMITTER]
   --run-state-batch-submitter         Determines whether or not to run the state batch submitter [$BATCH_SUBMITTER_RUN_STATE_BATCH_SUBMITTER]
   --safe-minimum-ether-balance value  Safe minimum amount of ether the batch submitter key should hold before it starts to log errors (default: 0) [$BATCH_SUBMITTER_SAFE_MINIMUM_ETHER_BALANCE]
   --clear-pending-txs                 Whether or not to clear pending transaction in the mempool on startup [$BATCH_SUBMITTER_CLEAR_PENDING_TXS]
   --log-level value                   The lowest log level that will be output (default: "info") [$BATCH_SUBMITTER_LOG_LEVEL]
   --sentry-enable                     Whether or not to enable Sentry. If true, sentry-dsn must also be set [$BATCH_SUBMITTER_SENTRY_ENABLE]
   --sentry-dsn value                  Sentry data source name [$BATCH_SUBMITTER_SENTRY_DSN]
   --sentry-trace-rate value           Sentry trace rate (default: 0.05) [$BATCH_SUBMITTER_SENTRY_TRACE_RATE]
   --block-offset value                The offset between the CTC contract start and the L2 geth blocks (default: 1) [$BATCH_SUBMITTER_BLOCK_OFFSET]
   --max-gas-price-in-gwei value       Maximum gas price the batch submitter can use for transactions (default: 100) [$BATCH_SUBMITTER_MAX_GAS_PRICE_IN_GWEI]
   --gas-retry-increment value         Default step by which to increment gas price bumps (default: 5) [$BATCH_SUBMITTER_GAS_RETRY_INCREMENT_FLAG]
   --sequencer-private-key value       The private key to use for sending to the sequencer contract [$BATCH_SUBMITTER_SEQUENCER_PRIVATE_KEY]
   --proposer-private-key value        The private key to use for sending to the proposer contract [$BATCH_SUBMITTER_PROPOSER_PRIVATE_KEY]
   --mnemonic value                    The mnemonic used to derive the wallets for either the sequencer or the proposer [$BATCH_SUBMITTER_MNEMONIC]
   --sequencer-hd-path value           The HD path used to derive the sequencer wallet from the mnemonic. The mnemonic flag must also be set. [$BATCH_SUBMITTER_SEQUENCER_HD_PATH]
   --proposer-hd-path value            The HD path used to derive the proposer wallet from the mnemonic. The mnemonic flag must also be set. [$BATCH_SUBMITTER_PROPOSER_HD_PATH]
   --metrics-server-enable             Whether or not to run the embedded metrics server [$BATCH_SUBMITTER_METRICS_SERVER_ENABLE]
   --metrics-hostname value            The hostname of the metrics server (default: "127.0.0.1") [$BATCH_SUBMITTER_METRICS_HOSTNAME]
   --metrics-port value                The port of the metrics server (default: 7300) [$BATCH_SUBMITTER_METRICS_PORT]
   --help, -h                          show help
   --version, -v                       print the version

@optimisticben
Copy link
Copy Markdown
Contributor

LGTM, thanks!

@tynes
Copy link
Copy Markdown
Contributor

tynes commented Oct 12, 2021

I've noticed that you added in a changesets package.json file - you will also need to add the package here -

"go/gas-oracle"
.

This will allow us to run yarn changeset at the root of the monorepo and then add in changesets for automation around releases. https://github.com/atlassian/changesets

@tynes
Copy link
Copy Markdown
Contributor

tynes commented Oct 13, 2021

It looks like there is a problem due to name collisions. Perhaps we rename the new one to batch-submitter-service? Or go-batch-submitter. Not super opinionated on the name, it just has to be different

error There are more than one workspace with name "@eth-optimism/batch-submitter"

I believe it needs to be updated here:
ad91500#diff-854d23c079ac2a42728c569d5504b43408305a39f2a3b7adbdb0e15bfab026e6R2

Copy link
Copy Markdown
Contributor

@tynes tynes left a comment

Choose a reason for hiding this comment

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

Once the changeset name is updated, this should be good to go

@cfromknecht
Copy link
Copy Markdown
Contributor Author

@tynes updated to batch-submitter-service, should be good now!

@tynes
Copy link
Copy Markdown
Contributor

tynes commented Oct 13, 2021

Great work!

@tynes tynes merged commit 28a2e5d into ethereum-optimism:develop Oct 14, 2021
@cfromknecht cfromknecht deleted the bss-envvar branch October 14, 2021 00:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

M-ci Meta: ci related work

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants