-
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
feat(gas_price_service_v1): define RunnableTask for GasPriceServiceV1 #2416
Changes from 10 commits
6dc5ed7
b6149d1
8a912c4
c9d7ed7
7a65574
d576665
b4d8855
0a358f6
49a309a
c516ea9
53291a5
2291207
8ae983f
b2a94d8
db04ae1
1e3b3f5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
pub mod algorithm; | ||
pub mod da_source_service; | ||
pub mod metadata; | ||
pub mod service; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,12 +21,12 @@ trait BlockCommitterApi: Send + Sync { | |
/// Used to get the costs for a specific seqno | ||
async fn get_costs_by_seqno( | ||
&self, | ||
number: u64, | ||
number: u32, | ||
) -> DaBlockCostsResult<Option<RawDaBlockCosts>>; | ||
/// Used to get the costs for a range of blocks (inclusive) | ||
async fn get_cost_bundles_by_range( | ||
&self, | ||
range: core::ops::Range<u64>, | ||
range: core::ops::Range<u32>, | ||
) -> DaBlockCostsResult<Vec<Option<RawDaBlockCosts>>>; | ||
} | ||
|
||
|
@@ -40,9 +40,9 @@ pub struct BlockCommitterDaBlockCosts<BlockCommitter> { | |
#[derive(Debug, Deserialize, Serialize, Clone, Default, PartialEq)] | ||
pub struct RawDaBlockCosts { | ||
/// Sequence number (Monotonically increasing nonce) | ||
pub sequence_number: u64, | ||
pub sequence_number: u32, | ||
/// The range of blocks that the costs apply to | ||
pub blocks_range: core::ops::Range<u64>, | ||
pub blocks_range: core::ops::Range<u32>, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We'll need to change this to just be a vec with these changes: |
||
/// The DA block height of the last transaction for the range of blocks | ||
pub da_block_height: DaBlockHeight, | ||
/// Rolling sum cost of posting blobs (wei) | ||
|
@@ -143,7 +143,7 @@ impl BlockCommitterApi for BlockCommitterHttpApi { | |
|
||
async fn get_costs_by_seqno( | ||
&self, | ||
number: u64, | ||
number: u32, | ||
) -> DaBlockCostsResult<Option<RawDaBlockCosts>> { | ||
let response = self | ||
.client | ||
|
@@ -157,7 +157,7 @@ impl BlockCommitterApi for BlockCommitterHttpApi { | |
|
||
async fn get_cost_bundles_by_range( | ||
&self, | ||
range: core::ops::Range<u64>, | ||
range: core::ops::Range<u32>, | ||
) -> DaBlockCostsResult<Vec<Option<RawDaBlockCosts>>> { | ||
let response = self | ||
.client | ||
|
@@ -192,23 +192,24 @@ mod tests { | |
} | ||
async fn get_costs_by_seqno( | ||
&self, | ||
seq_no: u64, | ||
seq_no: u32, | ||
) -> DaBlockCostsResult<Option<RawDaBlockCosts>> { | ||
// arbitrary logic to generate a new value | ||
let mut value = self.value.clone(); | ||
if let Some(value) = &mut value { | ||
value.sequence_number = seq_no; | ||
value.blocks_range = | ||
value.blocks_range.end * seq_no..value.blocks_range.end * seq_no + 10; | ||
value.da_block_height = value.da_block_height + (seq_no + 1).into(); | ||
value.da_block_height = | ||
value.da_block_height + ((seq_no + 1) as u64).into(); | ||
value.total_cost += 1; | ||
value.total_size_bytes += 1; | ||
} | ||
Comment on lines
197
to
207
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is a lot of unchecked arithmetic and casting going on here. How do you feel about switching to checked and |
||
Ok(value) | ||
} | ||
async fn get_cost_bundles_by_range( | ||
&self, | ||
_: core::ops::Range<u64>, | ||
_: core::ops::Range<u32>, | ||
) -> DaBlockCostsResult<Vec<Option<RawDaBlockCosts>>> { | ||
Ok(vec![self.value.clone()]) | ||
} | ||
|
@@ -286,7 +287,7 @@ mod tests { | |
} | ||
async fn get_costs_by_seqno( | ||
&self, | ||
seq_no: u64, | ||
seq_no: u32, | ||
) -> DaBlockCostsResult<Option<RawDaBlockCosts>> { | ||
// arbitrary logic to generate a new value | ||
let mut value = self.value.clone(); | ||
|
@@ -301,7 +302,7 @@ mod tests { | |
} | ||
async fn get_cost_bundles_by_range( | ||
&self, | ||
_: core::ops::Range<u64>, | ||
_: core::ops::Range<u32>, | ||
) -> DaBlockCostsResult<Vec<Option<RawDaBlockCosts>>> { | ||
Ok(vec![self.value.clone()]) | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,6 +15,7 @@ use tokio::{ | |
|
||
use crate::v1::da_source_service::DaBlockCosts; | ||
pub use anyhow::Result; | ||
use fuel_core_services::stream::BoxFuture; | ||
|
||
#[derive(Clone)] | ||
pub struct SharedState(Sender<DaBlockCosts>); | ||
|
@@ -103,12 +104,14 @@ where | |
async fn run(&mut self, state_watcher: &mut StateWatcher) -> Result<bool> { | ||
let continue_running; | ||
|
||
let tick: BoxFuture<tokio::time::Instant> = Box::pin(self.poll_interval.tick()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you're right, I had it that way when i was testing something. removed in 53291a5 |
||
|
||
tokio::select! { | ||
biased; | ||
_ = state_watcher.while_started() => { | ||
continue_running = false; | ||
} | ||
_ = self.poll_interval.tick() => { | ||
_ = tick => { | ||
let da_block_costs = self.source.request_da_block_cost().await?; | ||
self.shared_state.0.send(da_block_costs)?; | ||
continue_running = true; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,6 @@ | ||
use crate::v0::metadata::V0Metadata; | ||
use fuel_gas_price_algorithm::v1::{ | ||
AlgorithmUpdaterV1, | ||
ClampedPercentage, | ||
L2ActivityTracker, | ||
}; | ||
use std::num::NonZeroU64; | ||
|
@@ -41,7 +40,7 @@ pub struct V1Metadata { | |
impl V1Metadata { | ||
pub fn construct_from_v0_metadata( | ||
v0_metadata: V0Metadata, | ||
config: V1AlgorithmConfig, | ||
config: &V1AlgorithmConfig, | ||
) -> anyhow::Result<Self> { | ||
let metadata = Self { | ||
new_scaled_exec_price: v0_metadata | ||
|
@@ -68,19 +67,56 @@ impl V1Metadata { | |
} | ||
|
||
pub struct V1AlgorithmConfig { | ||
new_exec_gas_price: u64, | ||
min_exec_gas_price: u64, | ||
exec_gas_price_change_percent: u16, | ||
l2_block_fullness_threshold_percent: u8, | ||
gas_price_factor: NonZeroU64, | ||
min_da_gas_price: u64, | ||
max_da_gas_price_change_percent: u16, | ||
da_p_component: i64, | ||
da_d_component: i64, | ||
normal_range_size: u16, | ||
capped_range_size: u16, | ||
decrease_range_size: u16, | ||
block_activity_threshold: u8, | ||
pub new_exec_gas_price: u64, | ||
pub min_exec_gas_price: u64, | ||
pub exec_gas_price_change_percent: u16, | ||
pub l2_block_fullness_threshold_percent: u8, | ||
pub gas_price_factor: NonZeroU64, | ||
pub min_da_gas_price: u64, | ||
pub max_da_gas_price_change_percent: u16, | ||
pub da_p_component: i64, | ||
pub da_d_component: i64, | ||
pub normal_range_size: u16, | ||
pub capped_range_size: u16, | ||
pub decrease_range_size: u16, | ||
pub block_activity_threshold: u8, | ||
pub unrecorded_blocks: Vec<(u32, u64)>, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm. I guess it doesn't hurt. I don't know why we'd need this if it wasn't already in the metadata. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. need this to bootstrap the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm. K. Technically you could provide a metadata with it, but that's expensive. |
||
} | ||
|
||
impl From<&V1AlgorithmConfig> for AlgorithmUpdaterV1 { | ||
fn from(value: &V1AlgorithmConfig) -> Self { | ||
let l2_activity = L2ActivityTracker::new_full( | ||
value.normal_range_size, | ||
value.capped_range_size, | ||
value.decrease_range_size, | ||
value.block_activity_threshold.into(), | ||
); | ||
let unrecorded_blocks = value.unrecorded_blocks.clone().into_iter().collect(); | ||
Self { | ||
new_scaled_exec_price: value.new_exec_gas_price, | ||
l2_block_height: 0, | ||
new_scaled_da_gas_price: value.min_da_gas_price, | ||
gas_price_factor: value.gas_price_factor, | ||
total_da_rewards_excess: 0, | ||
da_recorded_block_height: 0, | ||
latest_known_total_da_cost_excess: 0, | ||
projected_total_da_cost: 0, | ||
last_profit: 0, | ||
second_to_last_profit: 0, | ||
latest_da_cost_per_byte: 0, | ||
l2_activity, | ||
min_exec_gas_price: value.min_exec_gas_price, | ||
exec_gas_price_change_percent: value.exec_gas_price_change_percent, | ||
l2_block_fullness_threshold_percent: value | ||
.l2_block_fullness_threshold_percent | ||
.into(), | ||
min_da_gas_price: value.min_da_gas_price, | ||
max_da_gas_price_change_percent: value.max_da_gas_price_change_percent, | ||
da_p_component: value.da_p_component, | ||
da_d_component: value.da_d_component, | ||
unrecorded_blocks, | ||
} | ||
} | ||
} | ||
|
||
impl From<AlgorithmUpdaterV1> for V1Metadata { | ||
|
@@ -104,15 +140,19 @@ impl From<AlgorithmUpdaterV1> for V1Metadata { | |
|
||
pub fn v1_algorithm_from_metadata( | ||
metadata: V1Metadata, | ||
config: V1AlgorithmConfig, | ||
config: &V1AlgorithmConfig, | ||
) -> AlgorithmUpdaterV1 { | ||
let l2_activity = L2ActivityTracker::new_full( | ||
config.normal_range_size, | ||
config.capped_range_size, | ||
config.decrease_range_size, | ||
config.block_activity_threshold.into(), | ||
); | ||
let unrecorded_blocks = metadata.unrecorded_blocks.into_iter().collect(); | ||
let unrecorded_blocks = metadata | ||
.unrecorded_blocks | ||
.into_iter() | ||
.chain(config.unrecorded_blocks.clone()) | ||
.collect(); | ||
AlgorithmUpdaterV1 { | ||
new_scaled_exec_price: metadata.new_scaled_exec_price, | ||
l2_block_height: metadata.l2_block_height, | ||
|
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.
Ah. Haha. I get it now... I think it makes sense either way but this probably better.
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.
Probably should have some tests in the algo for this.
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.
addressed in 8ae983f