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

chore(gas_price_service): initialize v1 metadata #2288

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

rymnc
Copy link
Member

@rymnc rymnc commented Oct 4, 2024

Note

This is PR 1/n in introducing GasPriceServiceV1 which links to v1 of the gas-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

  • Specifies a V1Metadata that will be stored along V0Metadata wrapped within the UpdaterMetadata struct.
  • We use fallible conversions between the two, v1 => v0 should be possible, but I hope we can discuss and decide about default values for the v0 => v1 metadata migration.
  • Constructor for AlgorithmUpdaterV1 from V1Metadata

Checklist

  • Breaking changes are clearly marked as such in the PR description and changelog
  • New behavior is reflected in tests
  • The specification matches the implemented behavior (link update PR if changes are needed)

Before requesting review

  • I have reviewed the code myself
  • I have created follow-up issues caused by this PR and linked them here

After merging, notify other teams

[Add or remove entries as needed]

@rymnc rymnc self-assigned this Oct 4, 2024
Comment on lines +8 to +53
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,
}
Copy link
Member Author

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

Copy link
Member

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,

Copy link
Member Author

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!

Copy link
Member Author

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 :)

@rymnc
Copy link
Member Author

rymnc commented Oct 4, 2024

I have the rest of the code for GasPriceServiceV1, getting them out in chunks though

@rymnc rymnc force-pushed the chore/gas-price-service-v1-metadata branch from 673b049 to 87f39be Compare October 4, 2024 11:43
@rymnc rymnc force-pushed the chore/gas-price-service-v1-metadata branch from 87f39be to bcb1fd3 Compare October 4, 2024 11:45
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.

2 participants