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 additional fee scalars #81

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

puma314
Copy link

@puma314 puma314 commented Sep 17, 2024

A design doc for additional fee scalars to add to the fee formula that allows for more flexibility for chains leverage alt-DA, ZK proving, or custom gas tokens.

<!-- What is the resource usage of the proposed solution?
Does it consume a large amount of computational resources or time? -->

There are basically no additional resources used by this proposal.
Copy link
Contributor

Choose a reason for hiding this comment

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

One consideration here is that it will add 2 additional new values to the L1 attributes tx calldata. Depending on the type, if we go with uint256 (i think we only need u64 tbh) then it would be an extra 64 bytes of calldata being written to disk every 2 seconds. I think this is fine, if we wanted to fix this we could turn other values that are u256 into u64 as part of this

Copy link

Choose a reason for hiding this comment

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

In the spec, I proposed that these get packed together with the EIP1559 parameters.

These scalars should be `u256`, so as to be on the same order of magnitude as the other terms in the fee calculation. These 2 new config values can be added to the [`L1 attributes`](https://github.com/ethereum-optimism/specs/blob/main/specs/protocol/ecotone/l1-attributes.md) transaction, with a very small diff to the proof.

The chain operator will have control over these values (similar to `baseFeeScalar` and `blobBaseFeeScalar`) and these values will be set and updated in the exact same way as the existing `scalar` attributes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Another part of scope: we will need to update the GasPriceOracle contract on L2 to include the new values. There is branching logic depending on which hardfork is activated, this means we will need a setHolocene function and branching logic based on that. The derivation pipeline will need to call setHolocene on the hardfork block

Copy link
Contributor

Choose a reason for hiding this comment

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

For context, this is where you will need to make a diff to compute the new fee formula.

You will need to update the L1Block contract with a new function that parses the new values from storage and add new storage variables to the L1Block contract

* **Alt-DA**: For chains using alt-DA, the `constant_scalar` is useful if there is a relatively constant overhead to posting to another DA layer.
* **ZK Proving**: For chains with ZK proving, ZKP generation costs are usually proportional to `gas_used` with a fixed overhead per transaction, making use of both the `constant_scalar` and the `gas_used_scalar`.
* **Custom gas token**: For chains with a custom gas token, both scalars can be useful as a ratio between the cost of the gas token and ETH to balance costs vs. the fee computation.

Copy link
Contributor

Choose a reason for hiding this comment

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

One consideration is how the RPC Receipt JSON will be presented, we generally want to include all of the information required to compute the L1 portion of the fee, could be good to double check here and note if any changes are requried

Copy link
Contributor

Choose a reason for hiding this comment

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

Another part of the RPC receipt call path is here

@tynes
Copy link
Contributor

tynes commented Sep 18, 2024

I am generally on board with this change, although we have scoped holocene already that doesn't mean that this cannot be included if it is code frozen by mid/late October


We propose the addition of 2 new rollup operator configured scalars (that can be updated via the `SystemConfig`) named `gas_used_scalar` and `constant_scalar` that factor in the fee calculation as follows:
```
fee = gas_used * (base_fee + priority_fee) + l1Fee + gas_used_scalar * gas_used + constant_scalar
Copy link

@ratankaliani ratankaliani Sep 18, 2024

Choose a reason for hiding this comment

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

Currently, fee = gas_used * (base_fee + priority_fee) + l1Fee.

Now, fee = gas_used * (base_fee + priority_fee) + l1Fee + gas_used_scalar * gas_used + constant_scalar.


We propose the addition of 2 new rollup operator configured scalars (that can be updated via the `SystemConfig`) named `gas_used_scalar` and `constant_scalar` that factor in the fee calculation as follows:
```
fee = constant_scalar + gas_used * (base_fee + priority_fee + gas_used_scalar) + l1Fee
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 out of date given the spec, can we update this here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mind updating the doc so there is a single source of truth for the formula so its easier to reason about?

@tynes
Copy link
Contributor

tynes commented Oct 1, 2024

Some open considerations:

Roles. We need a section on the chain operator UX for modifying this value and who can do it. It should be really explicit about the SystemConfig being the place that this is modified. Who can do it? There are 2 options - the chain operator (the SystemConfig.owner() or we start moving towards an RBAC system in the system config. We generally want to move towards a more RBAC based system. It could be useful for this now and adding a new role specific to who can modify this value. Operationally this can be useful, if you want to have a less cold key be able to modify this value more regularly

Standardness. We have a definition of standard config here. If we do add a new role to be able to modify this value, then we would want to also define the standard config value. I would recommend making it the L1 Proxy Admin for simplicity

@tynes
Copy link
Contributor

tynes commented Oct 1, 2024

Fee Vault: While this went into the spec originally and then removed and was not mentioned in the design doc, I think that its a good idea to introduce a new fee vault for this change. Each fee vault captures revenue from a different resource, it will ultimately be more forwards compatible if the operator fees went into an OperatorFeeVault

@tynes
Copy link
Contributor

tynes commented Oct 1, 2024

Upgrading a live chain: The consideration here is for the ConfigUpdate event. Right now, the derivation pipeline will error on unknown events. Which is why we previously modified the existing events. We can do this again - first fork, then upgrade the L1 contracts that now emit the new serialization.

edit: context in #81 (comment)

@yuwen01
Copy link

yuwen01 commented Oct 1, 2024

Upgrading a live chain: The consideration here is for the ConfigUpdate event. Right now, the derivation pipeline will error on unknown events. Which is why we previously modified the existing events. We can do this again - first fork, then upgrade the L1 contracts that now emit the new serialization.

I think we can update the existing GAS_CONFIG UpdateType (as of holocene, renamed to FEE_SCALARS). Right now, the GAS_CONFIG packs the blobbasefeeScalar and the basefeeScalar into a single uint256. There's still room for the other operatorFeeScalar and operatorFeeConstant in the higher bits of this uint256. This behavior is specified here.

@yuwen01
Copy link

yuwen01 commented Oct 1, 2024

Some open considerations:

Roles. We need a section on the chain operator UX for modifying this value and who can do it. It should be really explicit about the SystemConfig being the place that this is modified. Who can do it? There are 2 options - the chain operator (the SystemConfig.owner() or we start moving towards an RBAC system in the system config. We generally want to move towards a more RBAC based system. It could be useful for this now and adding a new role specific to who can modify this value. Operationally this can be useful, if you want to have a less cold key be able to modify this value more regularly

Standardness. We have a definition of standard config here. If we do add a new role to be able to modify this value, then we would want to also define the standard config value. I would recommend making it the L1 Proxy Admin for simplicity

For making a new role, maybe we could make a new role called FeeManager or something that manages both the basefeeScalar, blobbasefeeScalar, and also the operator fee scalars? And we'd have setGasConfig only modifiable by the FeeManager? I mostly just feel like the entity who's managing the l1 fee scalars should also manage the operator fee scalars -- so I think the best alternative to this new role would be the System Config Owner.

Also, when you refer to "Standardness", you're referring to the "Requirements", right? I'm a bit confused if I need to set standard values for the operator fee scalars, since the basefeeScalar and blobbasefeeScalar don't have requirements.

@tynes
Copy link
Contributor

tynes commented Oct 2, 2024

Upgrading a live chain: The consideration here is for the ConfigUpdate event. Right now, the derivation pipeline will error on unknown events. Which is why we previously modified the existing events. We can do this again - first fork, then upgrade the L1 contracts that now emit the new serialization.

I think we can update the existing GAS_CONFIG UpdateType (as of holocene, renamed to FEE_SCALARS). Right now, the GAS_CONFIG packs the blobbasefeeScalar and the basefeeScalar into a single uint256. There's still room for the other operatorFeeScalar and operatorFeeConstant in the higher bits of this uint256. This behavior is specified here.

I would prefer if we add a new enum for ConfigUpdate specific for these values rather than modifying the existing one. The specific reason why this is preferred is because it will reduce need for us to update tooling that calls out to the system config. If they are all part of the same ConfigUpdate, then we will need a high level ABI change to the SystemConfig to a function that accepts 4 fee config fee params rather than 2 fee params. This also allows us to skip needing to pass in the values into SystemConfig.initialize and it becomes an opt in change - the only way to set them would be to explicitly call the new function on the SystemConfig that accepts these values and then emits the new enum in the ConfigUpdate event

@tynes
Copy link
Contributor

tynes commented Oct 2, 2024

Some open considerations:
Roles. We need a section on the chain operator UX for modifying this value and who can do it. It should be really explicit about the SystemConfig being the place that this is modified. Who can do it? There are 2 options - the chain operator (the SystemConfig.owner() or we start moving towards an RBAC system in the system config. We generally want to move towards a more RBAC based system. It could be useful for this now and adding a new role specific to who can modify this value. Operationally this can be useful, if you want to have a less cold key be able to modify this value more regularly
Standardness. We have a definition of standard config here. If we do add a new role to be able to modify this value, then we would want to also define the standard config value. I would recommend making it the L1 Proxy Admin for simplicity

For making a new role, maybe we could make a new role called FeeManager or something that manages both the basefeeScalar, blobbasefeeScalar, and also the operator fee scalars? And we'd have setGasConfig only modifiable by the FeeManager? I mostly just feel like the entity who's managing the l1 fee scalars should also manage the operator fee scalars -- so I think the best alternative to this new role would be the System Config Owner.

Also, when you refer to "Standardness", you're referring to the "Requirements", right? I'm a bit confused if I need to set standard values for the operator fee scalars, since the basefeeScalar and blobbasefeeScalar don't have requirements.

Given #81 (comment), I think we should make the new role be called Operator Fee Manager and wrap function setOperatorFees(uint32,uint64) with onlyOperatorFeeManager

We would need to add a new section to the requirements that you linked with a new section that includes the operator fee

@yuwen01
Copy link

yuwen01 commented Oct 2, 2024

I understand your point about separating these new parameters logically, and added the new role and updatetype as suggested. I've also started thinking about standard values for the new scalars, and will update you when those are finalized.

Regarding the new OperatorFeeManager role -- I was thinking that this role should only be responsible for tuning the operator fee scalars, and not be involved with fee vault collection at all. The job of directing fees to different vaults is already managed entirely by the chain governor.

For setting standard values for the operatorFeeScalar, I think setting a bound based on marginal gains might make the most sense, similar to this (perhaps outdated) parameter. Just wanted to check that something like that would be appropriate here.

edit: added standard values for the new scalars

@tynes
Copy link
Contributor

tynes commented Oct 2, 2024

Regarding the new OperatorFeeManager role -- I was thinking that this role should only be responsible for tuning the operator fee scalars, and not be involved with fee vault collection at all. The job of directing fees to different vaults is already managed entirely by the chain governor.

Agreed with this 100%, there is a FeeVault recipient that should be configurable by the chain operator


These 2 new config values can be added to the [`L1 attributes`](https://github.com/ethereum-optimism/specs/blob/main/specs/protocol/ecotone/l1-attributes.md) transaction, with a very small diff to the proof.

The chain governor (aka [System Config Owner](https://specs.optimism.io/protocol/configurability.html#system-config-owner)) will be responsible for updating these values. We expose a new function in the `SystemConfig` contract for updating the `operatorFeeScalar` and `operatorFeeConstant`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Line 76 conflicts with line 83 about the roles

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.

4 participants