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

Open
wants to merge 21 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. (I don't know when we'd need that conversion -Mitch)
  • Constructor for AlgorithmUpdaterV1 from V1Metadata

Bonus:

  • Also fixed the tests that were testing initialize_algorithm to test the UninitializedTask instead--a public interface.

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

@MitchTurner MitchTurner Oct 4, 2024

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. (saved in Metadata)
    pub da_recorded_block_height: u32,
    /// Zero on start (saved in Metadata)
    pub latest_known_total_da_cost_excess: u128,
    /// Zero on start (saved in Metadata)
    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 (saved in Metadata)
    pub last_profit: i128,
    /// Zero on start (saved in Metadata)
    pub second_to_last_profit: i128,
    /// Zero on start (saved in Metadata)
    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 :)

Copy link
Member

Choose a reason for hiding this comment

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

I just realized that I put (saved in Metadata) on a few of them, but there are others that I missed.

Copy link
Member Author

Choose a reason for hiding this comment

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

what's the action here? should I change the metadata?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Sorry. Missed this. I just updated.

@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 3 times, most recently from bcb1fd3 to cfb7b1b Compare October 7, 2024 05:30
@rymnc rymnc force-pushed the chore/gas-price-service-v1-metadata branch from cfb7b1b to 298de5f Compare October 7, 2024 05:33
@MitchTurner MitchTurner marked this pull request as ready for review October 24, 2024 20:04
@MitchTurner
Copy link
Member

So. I wanted to remove the unneeded fields from the metadatas, this let me to modifying how the tests work... which led me to fixing some of the tests. I don't know if we want to keep that in this PR.

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