Use gas prices from actual blocks to calculate estimate gas prices #2501
Conversation
| } | ||
|
|
||
| #[async_trait::async_trait] | ||
| impl GasPriceEstimate for ArcGasPriceEstimate<u32, u64> { |
| pub(crate) fn update_latest_gas_price(&self, block_info: &BlockInfo) { | ||
| match block_info { | ||
| BlockInfo::GenesisBlock => { | ||
| // do nothing |
There was a problem hiding this comment.
should we have a test to see what values are stored in the block after genesis? afaiu we test only post genesis transitions
| @@ -481,10 +506,12 @@ mod tests { | |||
| ), | |||
| None, | |||
| ); | |||
| let latest_gas_price = Arc::new(parking_lot::RwLock::new((0, 0))); | |||
There was a problem hiding this comment.
curious why we need this when perhaps the algo could contain this internally?
There was a problem hiding this comment.
Yeah... That works too. It's kinda a separate value so I thought to keep it separate but that would work.
| use super::*; | ||
| use proptest::proptest; | ||
|
|
||
| async fn _worst_case__correctly_calculates_value( |
There was a problem hiding this comment.
nit: this function is called later on
| async fn _worst_case__correctly_calculates_value( | |
| async fn worst_case__correctly_calculates_value( |
| } | ||
|
|
||
| #[allow(dead_code)] | ||
| pub struct ArcGasPriceEstimate<Height, GasPrice> { |
There was a problem hiding this comment.
would it be possible to have a bit of documentation here?
In particular, I'd like to understand what the "height" and "price" refer to here, since later they are named as "best_height" and "best_price"
|
|
||
| #[async_trait::async_trait] | ||
| impl GasPriceEstimate for ArcGasPriceEstimate<u32, u64> { | ||
| async fn worst_case_gas_price(&self, height: BlockHeight) -> Option<u64> { |
There was a problem hiding this comment.
so what I get here is that we assume that the price is going to raise by the specified percentage every block until height is reached, is that correct?
| best_gas_price: u64, | ||
| best_height: u32, | ||
| percentage: u64, | ||
| target_height: u32, |
There was a problem hiding this comment.
nit: you can pass directly the difference of target_height and best_height to this function and get rid of one parameter, but I don't have a strong opinion here
|
|
||
| #[allow(clippy::cast_possible_truncation)] | ||
| pub(crate) fn cumulative_percentage_change( | ||
| best_gas_price: u64, |
There was a problem hiding this comment.
nit: maybe start_gas_price is better?
| let multiple = (1.0f64 + percentage_as_decimal).powf(blocks); | ||
| let mut approx = best_gas_price as f64 * multiple; | ||
| // account for rounding errors and take a slightly higher value | ||
| const ROUNDING_ERROR_CUTOFF: f64 = 16948547188989277.0; |
There was a problem hiding this comment.
can you put a comment about how this value has been obtained?
| // account for rounding errors and take a slightly higher value | ||
| const ROUNDING_ERROR_CUTOFF: f64 = 16948547188989277.0; | ||
| if approx > ROUNDING_ERROR_CUTOFF { | ||
| const ROUNDING_ERROR_COMPENSATION: f64 = 2000.0; |
There was a problem hiding this comment.
| expected = expected.saturating_add(change_amount); | ||
| } | ||
|
|
||
| assert!(actual >= expected); |
There was a problem hiding this comment.
I think we can be more precise here, and place an upper bound on actual, e.g.
| assert!(actual >= expected); | |
| assert!(actual >= expected); | |
| assert!(actual <= expected.saturating_add(2000.0)) |
|
I'm not sure I understand what the role of |
Linked Issues/PRs
Description
Because some nodes won't be running the gas price algorithm, or might not be synced properly, we are going to calculate the price based on the latest block gas price. That is a reliable source of truth.
Checklist
Before requesting review
After merging, notify other teams
[Add or remove entries as needed]