Skip to content

feat: add Min/MaxStateRootElements configurations#2340

Merged
mslipper merged 1 commit intoethereum-optimism:developfrom
cfromknecht:bss-min-state-root-count
Mar 18, 2022
Merged

feat: add Min/MaxStateRootElements configurations#2340
mslipper merged 1 commit intoethereum-optimism:developfrom
cfromknecht:bss-min-state-root-count

Conversation

@cfromknecht
Copy link
Copy Markdown
Contributor

Description
This PR adds configuration hooks to bound the number of state roots
that are permitted in state root batches. Specifically, the minimum
allows us to amortized the cost of adding state roots to the CTC
contract. Additionally, the maximum is used in place of the old limit
based on tx size, since this is more natural and mirrors the minimum
bound.

The maximum possible value should be less than 3750 to keep in line with
our conservative 120kb limit.

Metadata

  • Fixes ENG-2070

@cfromknecht cfromknecht requested a review from tynes as a code owner March 17, 2022 23:13
@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Mar 17, 2022

🦋 Changeset detected

Latest commit: 9678b35

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-service 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

@mslipper mslipper self-requested a review March 17, 2022 23:54
Copy link
Copy Markdown
Contributor

@mslipper mslipper left a comment

Choose a reason for hiding this comment

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

I think there need to be some changes to the docker compose file - tests are stalling.


// MaxStateBatchCount is the maximum number of L2 state roots that can ever
// be in a batch.
MaxStateBatchCount uint64
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If I understand correctly, this was unused previously

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yes it was, the member existed but it wasn't read/written anywhere

@tynes
Copy link
Copy Markdown
Contributor

tynes commented Mar 18, 2022

I think there need to be some changes to the docker compose file - tests are stalling.

Perhaps because the new config is marked as required

Name: "min-state-root-elements",
Usage: "Minimum number of elements required to submit a state " +
"root batch",
Required: true,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We could put a default of 1 here for simplicity

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ended up leaving it as optional, as this mirrors the min/max tx size

@tynes
Copy link
Copy Markdown
Contributor

tynes commented Mar 18, 2022

Generally looks good to me besides the need to update ops with the new config

This commit adds configuration hooks to bound the number of state roots
that are permitted in state root batches. Specifically, the minimum
allows us to amortized the cost of adding state roots to the CTC
contract. Additionally, the maximum is used in place of the old limit
based on tx size, since this is more natural and mirrors the minimum
bound.
@cfromknecht
Copy link
Copy Markdown
Contributor Author

Perhaps because the new config is marked as required

Indeed this was the problem, thanks!

@mslipper mslipper merged commit ad90e01 into ethereum-optimism:develop Mar 18, 2022
@cfromknecht cfromknecht deleted the bss-min-state-root-count branch March 18, 2022 22:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-ops Area: ops

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants