Skip to content
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

feat: add FMA for operator fee #186

Merged
merged 12 commits into from
Feb 13, 2025

Conversation

leruaa
Copy link
Contributor

@leruaa leruaa commented Jan 10, 2025

Description

This PR introduces the FMA for the operator fee feature, set to be included in Isthmus.

Additional context

- [Spec](https://github.com/ethereum-optimism/specs/pull/382)

## Failure Modes and Recovery Paths

Copy link
Contributor

Choose a reason for hiding this comment

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

Database growth increase results in need for 4444 faster or other history expiry solution

Copy link
Contributor

Choose a reason for hiding this comment

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

Added

Copy link
Contributor

Choose a reason for hiding this comment

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

What are the other potential history expiry solutions? Just state pruning?

Also, the operator fee won't result in significant database growth on it's own right?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a question for you to answer :)

Basically whatever the extra data to the L1 attributes txs is, that is stored to the db every 2s for OPM and Base, every 1s for Unichain/Ink/Soneium, so that adds up to be a lot over time. Its just a consideration and not a blocker. Eventually we need to use a diff based L1 attributes tx to not repeat data being sent over and over again

Copy link
Contributor

Choose a reason for hiding this comment

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

@leruaa has this been resolved?

@ratankaliani
Copy link
Contributor

Any other failure modes that come to mind @tynes? Are there other external parties beyond wallets that may fail to update their fee estimation logic that we should be aware of?

@tynes
Copy link
Contributor

tynes commented Feb 3, 2025

we have some considerations for generic hardforks here:

could be useful to look at this too

- **Mitigations:**
- Implement history expiry solutions like EIP-4444 when available.
- **Detection:**
- Monitor database growth rate compared to pre operator fee baseline.
Copy link
Contributor

Choose a reason for hiding this comment

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

A few of the detections are monitoring related. If we don't have monitors setup for these types of metrics already we will have to stand those up, which would be an action item to track

Copy link
Contributor Author

@leruaa leruaa Feb 11, 2025

Choose a reason for hiding this comment

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

I'll ask if OP Labs have monitoring in place related to database growth rate.

Copy link
Contributor

Choose a reason for hiding this comment

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

Before we merge, has this been resolved @leruaa?

Copy link
Contributor

@blmalone blmalone left a comment

Choose a reason for hiding this comment

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

first pass

@blmalone
Copy link
Contributor

blmalone commented Feb 12, 2025

I'm happy to move this PR into 'Implementing Actions' or 'Final' (as per states) if @leruaa doesn't have anything else to do.

@leruaa
Copy link
Contributor Author

leruaa commented Feb 12, 2025

@blmalone That's ok for me!

Comment on lines 91 to 94
- **Risk Assessment:**
Medium impact, high likelihood.
- **Mitigations:**
- Implement history expiry solutions like EIP-4444 when available.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is medium impact and high likelihood, but we have no mitigations others than waiting for EIP-4444, and no action items for this? It seems like we are just accepting this failure mode which doesn't seem good

Copy link
Contributor Author

@leruaa leruaa Feb 12, 2025

Choose a reason for hiding this comment

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

I added an action item

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks! So what happens if growth rate is too high and database size becomes a problem, what options do we have to mitigate? Is there any kind of benchmarking of anticipated growth rate we are going to do before shipping?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Operator fee adds 12 bytes per L2 block (4 for operator fee scalar, 8 for operator fee constant). It also add 12 bytes per transactions (in the transaction receipt) So, with the arbitrary number of 20s txs per block we have:

(12 bytes + 240 bytes / 2 seconds) x 365 days × 24 hours × 60 minutes × 60 seconds = 3,973,536,000 bytes in 1 year.

About 3,7 GB for 1 year

I aded this simulation in the FMA.

Copy link
Contributor

@blmalone blmalone left a comment

Choose a reason for hiding this comment

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

Approving as this is in 'Implementing Actions' state.

@blmalone blmalone merged commit 38b7cc0 into ethereum-optimism:main Feb 13, 2025
@BlocksOnAChain
Copy link
Contributor

@leruaa @ratankaliani - do you folks have it in your next steps/plans to create a new PR that will help us track your work on the action items from this Pr, since we merged it and are now in

Approving as this is in 'Implementing Actions' state

as mentioned by Blaine in his last comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants