-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
chore(gas_price_service): initialize v1 metadata #2288
base: master
Are you sure you want to change the base?
Conversation
pub struct V1Metadata { | ||
// Execution | ||
/// The gas price (scaled by the `gas_price_factor`) to cover the execution of the next block | ||
pub new_scaled_exec_price: u64, | ||
/// The lowest the algorithm allows the exec gas price to go | ||
pub min_exec_gas_price: u64, | ||
/// The Percentage the execution gas price will change in a single block, either increase or decrease | ||
/// based on the fullness of the last L2 block. Using `u16` because it can go above 100% and | ||
/// possibly over 255% | ||
pub exec_gas_price_change_percent: u16, | ||
/// The height of the next L2 block | ||
pub l2_block_height: u32, | ||
/// The threshold of gas usage above and below which the gas price will increase or decrease | ||
/// This is a percentage of the total capacity of the L2 block | ||
pub l2_block_fullness_threshold_percent: ClampedPercentage, | ||
// DA | ||
/// The gas price (scaled by the `gas_price_factor`) to cover the DA commitment of the next block | ||
pub new_scaled_da_gas_price: u64, | ||
|
||
/// Scale factor for the gas price. | ||
pub gas_price_factor: NonZeroU64, | ||
/// The lowest the algorithm allows the da gas price to go | ||
pub min_da_gas_price: u64, | ||
/// The maximum percentage that the DA portion of the gas price can change in a single block | ||
/// Using `u16` because it can go above 100% and possibly over 255% | ||
pub max_da_gas_price_change_percent: u16, | ||
/// The cumulative reward from the DA portion of the gas price | ||
pub total_da_rewards_excess: u128, | ||
/// The height of the last L2 block recorded on the DA chain | ||
pub da_recorded_block_height: u32, | ||
/// The cumulative cost of recording L2 blocks on the DA chain as of the last recorded block | ||
pub latest_known_total_da_cost_excess: u128, | ||
/// The predicted cost of recording L2 blocks on the DA chain as of the last L2 block | ||
/// (This value is added on top of the `latest_known_total_da_cost` if the L2 height is higher) | ||
pub projected_total_da_cost: u128, | ||
/// The P component of the PID control for the DA gas price | ||
pub da_p_component: i64, | ||
/// The D component of the PID control for the DA gas price | ||
pub da_d_component: i64, | ||
/// The last profit | ||
pub last_profit: i128, | ||
/// The profit before last | ||
pub second_to_last_profit: i128, | ||
/// The latest known cost per byte for recording blocks on the DA chain | ||
pub latest_da_cost_per_byte: u128, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc: @MitchTurner, if you can describe which ones we need and which ones are just being used/mutated internally by the AlgorithmUpdater
please
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// Same as with V0, but we need to multiply it by the `gas_price_factor`. A constructor would help a lot here :)
pub new_scaled_exec_price: u64,
/// Same as with V0 (this isn't scaled, I double checked.
pub min_exec_gas_price: u64,
/// Same as V0
pub exec_gas_price_change_percent: u16,
/// Should be confirmed with metadata sync
pub l2_block_height: u32,
/// Same as V0
pub l2_block_fullness_threshold_percent: ClampedPercentage,
/// Should be defined in config. Should be multiplied by `gas_price_factor`. Again, constructor would be good.
pub new_scaled_da_gas_price: u64,
/// `100` should be good and we can hardcode it if we want.
pub gas_price_factor: NonZeroU64,
/// Should be set with config
pub min_da_gas_price: u64,
/// Should be set with config
pub max_da_gas_price_change_percent: u16,
/// Zero on start (saved in Metadata)
pub total_da_rewards_excess: u128,
/// Currently the correct value is required here on startup, i.e. we should know what we expect next from the committer and do the block before that. BUT I think we should allow it to start with None.
pub da_recorded_block_height: u32,
/// Zero on start (saved in Metadata)
pub latest_known_total_da_cost_excess: u128,
/// Zero on start
pub projected_total_da_cost: u128,
/// Set with Config
pub da_p_component: i64,
/// Set with Config
pub da_d_component: i64,
/// Zero on start
pub last_profit: i128,
/// Zero on start
pub second_to_last_profit: i128,
/// Zero on start
pub latest_da_cost_per_byte: u128,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is super useful, making the required changes!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lmk if bcb1fd3 makes sense, then i will open the PR for review from others :)
I have the rest of the code for |
673b049
to
87f39be
Compare
87f39be
to
bcb1fd3
Compare
Note
This is PR 1/n in introducing
GasPriceServiceV1
which links tov1
of thegas-price-algorithm
. This PR is mainly used to start the discussion around v1 metadata and the fields we need.Linked Issues/PRs
fixes #2286
Description
V1Metadata
that will be stored alongV0Metadata
wrapped within theUpdaterMetadata
struct.v1
=>v0
should be possible, but I hope we can discuss and decide about default values for thev0
=>v1
metadata migration.AlgorithmUpdaterV1
fromV1Metadata
Checklist
Before requesting review
After merging, notify other teams
[Add or remove entries as needed]