Skip to content

chore(gas_price_service): refactor gas price service #2192

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.

Sign up for GitHub

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

Closed
wants to merge 9 commits into from
Closed

chore(gas_price_service): refactor gas price service #2192

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
9ccc343
chore(gas_price_service): refactor gas price service
rymnc Sep 13, 2024
6cf9248
chore: hide impl details
rymnc Sep 13, 2024
f0ad83a
fix: use buf size
rymnc Sep 13, 2024
f0bbf49
fix: dont set none if failed
rymnc Sep 17, 2024
2f33f7b
fix: use take instead of clone
rymnc Sep 17, 2024
00e0d06
fix: make next sync
rymnc Sep 17, 2024
d68b02d
fix: remove trait port
rymnc Sep 17, 2024
69651a5
fix: use priv fields
rymnc Sep 17, 2024
911c06f
chore: extract common logic out
rymnc Sep 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 25 additions & 41 deletions crates/fuel-core/src/service/sub_services/algorithm_updater.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,7 @@ use crate::{

use fuel_core_gas_price_service::{
fuel_gas_price_updater::{
da_source_adapter::{
dummy_costs::DummyDaBlockCosts,
DaBlockCostsProvider,
DaBlockCostsSharedState,
},
da_source_adapter::dummy_costs::DummyDaBlockCosts,
fuel_core_storage_adapter::{
storage::GasPriceMetadata,
FuelL2BlockSource,
Expand All @@ -39,7 +35,6 @@ use fuel_core_gas_price_service::{
use fuel_core_services::{
stream::BoxStream,
RunnableService,
Service,
StateWatcher,
};
use fuel_core_storage::{
Expand All @@ -60,11 +55,7 @@ use fuel_core_types::{
services::block_importer::SharedImportResult,
};

type Updater = FuelGasPriceUpdater<
FuelL2BlockSource<ConsensusParametersProvider>,
MetadataStorageAdapter,
DaBlockCostsSharedState,
>;
type Updater = FuelGasPriceUpdater<MetadataStorageAdapter>;

pub struct InitializeTask {
pub config: Config,
Expand All @@ -74,13 +65,17 @@ pub struct InitializeTask {
pub on_chain_db: Database<OnChain, RegularStage<OnChain>>,
pub block_stream: BoxStream<SharedImportResult>,
pub shared_algo: SharedGasPriceAlgo<Algorithm>,
pub da_block_costs_provider: DaBlockCostsProvider<DummyDaBlockCosts>,
}

type MetadataStorageAdapter =
StructuredStorage<Database<GasPriceDatabase, RegularStage<GasPriceDatabase>>>;

type Task = GasPriceService<Algorithm, Updater>;
type Task = GasPriceService<
Algorithm,
Updater,
FuelL2BlockSource<ConsensusParametersProvider>,
DummyDaBlockCosts,
>;

impl InitializeTask {
pub fn new(
Expand All @@ -98,12 +93,6 @@ impl InitializeTask {
let default_metadata = get_default_metadata(&config, latest_block_height);
let algo = get_best_algo(&gas_price_db, default_metadata)?;
let shared_algo = SharedGasPriceAlgo::new_with_algorithm(algo);
// there's no use of this source yet, so we can safely return an error
let da_block_costs_source =
DummyDaBlockCosts::new(Err(anyhow::anyhow!("Not used")));
let da_block_costs_provider =
DaBlockCostsProvider::new(da_block_costs_source, None);

let task = Self {
config,
genesis_block_height,
Expand All @@ -112,7 +101,6 @@ impl InitializeTask {
on_chain_db,
block_stream,
shared_algo,
da_block_costs_provider,
};
Ok(task)
}
Expand Down Expand Up @@ -170,19 +158,26 @@ impl RunnableService for InitializeTask {
let updater = get_synced_gas_price_updater(
self.config,
self.genesis_block_height,
self.settings,
self.settings.clone(),
self.gas_price_db,
self.on_chain_db,
self.block_stream,
self.da_block_costs_provider.shared_state,
)?;

self.da_block_costs_provider
.service
.start_and_await()
.await?;
let inner_service =
GasPriceService::new(starting_height, updater, self.shared_algo).await;
let l2_block_source = FuelL2BlockSource::new(
self.genesis_block_height,
self.settings,
self.block_stream,
);
let da_block_cost_source = DummyDaBlockCosts::new(Err(anyhow::anyhow!("unused")));

let inner_service = GasPriceService::new(
starting_height,
updater,
self.shared_algo,
l2_block_source,
da_block_cost_source,
)
.await;
Ok(inner_service)
}
}
Expand All @@ -193,8 +188,6 @@ pub fn get_synced_gas_price_updater(
settings: ConsensusParametersProvider,
Copy link
Collaborator

@xgreenx xgreenx Sep 17, 2024

Choose a reason for hiding this comment

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

I think that most of the logic of this function should live inside the service itself. Only a small part of the sync_metadata_storage_with_on_chain_storage logic related to the on-chain database should live here

Copy link
Member Author

@rymnc rymnc Sep 17, 2024

Choose a reason for hiding this comment

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

I agree ~ this is part of the follow up PR though, which sunsets InitializeTask appropriately.

mut gas_price_db: Database<GasPriceDatabase, RegularStage<GasPriceDatabase>>,
on_chain_db: Database<OnChain, RegularStage<OnChain>>,
block_stream: BoxStream<SharedImportResult>,
da_block_costs: DaBlockCostsSharedState,
) -> anyhow::Result<Updater> {
let mut first_run = false;
let latest_block_height: u32 = on_chain_db
Expand Down Expand Up @@ -222,16 +215,9 @@ pub fn get_synced_gas_price_updater(
}

let mut metadata_storage = StructuredStorage::new(gas_price_db);
let l2_block_source =
FuelL2BlockSource::new(genesis_block_height, settings.clone(), block_stream);

if BlockHeight::from(latest_block_height) == genesis_block_height || first_run {
let updater = FuelGasPriceUpdater::new(
default_metadata.into(),
l2_block_source,
metadata_storage,
da_block_costs,
);
let updater = FuelGasPriceUpdater::new(default_metadata.into(), metadata_storage);
Ok(updater)
} else {
if latest_block_height > metadata_height {
Expand All @@ -246,9 +232,7 @@ pub fn get_synced_gas_price_updater(

FuelGasPriceUpdater::init(
latest_block_height.into(),
l2_block_source,
metadata_storage,
da_block_costs,
config.min_gas_price,
config.gas_price_change_percent,
config.gas_price_threshold_percent,
Expand Down
72 changes: 28 additions & 44 deletions crates/services/gas_price_service/src/fuel_gas_price_updater.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,9 @@ pub mod fuel_core_storage_adapter;

pub mod da_source_adapter;

pub struct FuelGasPriceUpdater<L2, Metadata, DaBlockCosts> {
pub struct FuelGasPriceUpdater<Metadata> {
Copy link
Member

@MitchTurner MitchTurner Sep 18, 2024

Choose a reason for hiding this comment

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

Hmm. I'm not sure how much we are actually gaining from this. I was kinda thinking that we could get rid of the UpdateAlgorithm abstraction completely, including removing this struct.

But I don't think the gas-price-algorithm should depend directly on some algorithm, in which case we want to keep the UpdateAlgorithm trait.

This approach commits us to a specific trait interface with

        l2_block: BlockInfo,
        da_block_costs: Option<DaBlockCosts>,

whereas before the gas-price-service didn't know anything about the dependencies of the updater.

Copy link
Member

@MitchTurner MitchTurner Sep 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We are going to want to add another value here soon with this issue:
#2166

Should the trait change or the internal implementation. I think maybe the implementation.

Copy link
Member Author

@rymnc rymnc Sep 18, 2024

Choose a reason for hiding this comment

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

okay. then I would propose to move the L2 source and DA back into the FuelGasPriceUpdater and delegate sub-service startup and shutdown to it. does that sound okay?

Copy link
Member Author

@rymnc rymnc Sep 18, 2024

Choose a reason for hiding this comment

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

(it also means we turn FuelGasPriceUpdater into a RunnableTask)

inner: AlgorithmUpdater,
l2_block_source: L2,
metadata_storage: Metadata,
#[allow(dead_code)]
da_block_costs: DaBlockCosts,
}

#[derive(Debug, Clone, PartialEq)]
Expand All @@ -54,29 +51,23 @@ impl AlgorithmUpdater {
}
}

impl<L2, Metadata, DaBlockCosts> FuelGasPriceUpdater<L2, Metadata, DaBlockCosts> {
pub fn new(
inner: AlgorithmUpdater,
l2_block_source: L2,
metadata_storage: Metadata,
da_block_costs: DaBlockCosts,
) -> Self {
impl<Metadata> FuelGasPriceUpdater<Metadata> {
pub fn new(inner: AlgorithmUpdater, metadata_storage: Metadata) -> Self {
Self {
inner,
l2_block_source,
metadata_storage,
da_block_costs,
}
}
}

#[derive(Debug, thiserror::Error)]
pub enum Error {
#[error("Failed to find L2 block at height {block_height:?}: {source_error:?}")]
CouldNotFetchL2Block {
block_height: BlockHeight,
source_error: anyhow::Error,
},
#[error("Failed to find L2 block: {source_error:?}")]
CouldNotFetchL2Block { source_error: anyhow::Error },
#[error("Failed to compute: {source_error:?}")]
MathError { source_error: anyhow::Error },
#[error("No Mint transaction in block")]
NoMintTx,
#[error("Failed to find DA records: {0:?}")]
CouldNotFetchDARecord(anyhow::Error),
#[error("Failed to retrieve updater metadata: {source_error:?}")]
Expand Down Expand Up @@ -111,7 +102,7 @@ pub enum BlockInfo {
}
#[async_trait::async_trait]
pub trait L2BlockSource: Send + Sync {
async fn get_l2_block(&mut self, height: BlockHeight) -> Result<BlockInfo>;
async fn get_l2_block(&mut self) -> Result<BlockInfo>;
}

#[derive(Debug, Default, Clone, Eq, Hash, PartialEq)]
Expand All @@ -121,10 +112,6 @@ pub struct DaBlockCosts {
pub blob_cost_wei: u128,
}

pub trait GetDaBlockCosts: Send + Sync {
fn get(&self) -> Result<Option<DaBlockCosts>>;
}

#[derive(serde::Serialize, serde::Deserialize, Debug, Clone, PartialEq)]
pub enum UpdaterMetadata {
V0(V0Metadata),
Expand Down Expand Up @@ -206,16 +193,13 @@ pub trait MetadataStorage: Send + Sync {
fn set_metadata(&mut self, metadata: UpdaterMetadata) -> Result<()>;
}

impl<L2, Metadata, DaBlockCosts> FuelGasPriceUpdater<L2, Metadata, DaBlockCosts>
impl<Metadata> FuelGasPriceUpdater<Metadata>
where
Metadata: MetadataStorage,
DaBlockCosts: GetDaBlockCosts,
{
pub fn init(
target_block_height: BlockHeight,
l2_block_source: L2,
metadata_storage: Metadata,
da_block_costs: DaBlockCosts,
min_exec_gas_price: u64,
exec_gas_price_change_percent: u64,
l2_block_fullness_threshold_percent: u64,
Expand All @@ -240,9 +224,7 @@ where
};
let updater = Self {
inner,
l2_block_source,
metadata_storage,
da_block_costs,
};
Ok(updater)
}
Expand All @@ -266,6 +248,7 @@ where
height: u32,
gas_used: u64,
block_gas_capacity: u64,
_da_block_costs: Option<DaBlockCosts>,
) -> anyhow::Result<()> {
let capacity = self.validate_block_gas_capacity(block_gas_capacity)?;

Expand All @@ -287,6 +270,7 @@ where
async fn apply_block_info_to_gas_algorithm(
&mut self,
l2_block: BlockInfo,
da_block_costs: Option<DaBlockCosts>,
) -> anyhow::Result<()> {
match l2_block {
BlockInfo::GenesisBlock => {
Expand All @@ -297,37 +281,37 @@ where
gas_used,
block_gas_capacity,
} => {
self.handle_normal_block(height, gas_used, block_gas_capacity)
.await?;
self.handle_normal_block(
height,
gas_used,
block_gas_capacity,
da_block_costs,
)
.await?;
}
}
Ok(())
}
}

#[async_trait::async_trait]
impl<L2, Metadata, DaBlockCosts> UpdateAlgorithm
for FuelGasPriceUpdater<L2, Metadata, DaBlockCosts>
impl<Metadata> UpdateAlgorithm for FuelGasPriceUpdater<Metadata>
where
L2: L2BlockSource,
Metadata: MetadataStorage + Send + Sync,
DaBlockCosts: GetDaBlockCosts,
{
type Algorithm = Algorithm;

fn start(&self, _for_block: BlockHeight) -> Self::Algorithm {
self.inner.algorithm()
}

async fn next(&mut self) -> anyhow::Result<Self::Algorithm> {
let l2_block_res = self
.l2_block_source
.get_l2_block(self.inner.l2_block_height())
.await;
tracing::info!("Received L2 block result: {:?}", l2_block_res);
let l2_block = l2_block_res?;

self.apply_block_info_to_gas_algorithm(l2_block).await?;
async fn next(
&mut self,
l2_block: BlockInfo,
da_block_costs: Option<DaBlockCosts>,
) -> anyhow::Result<Self::Algorithm> {
self.apply_block_info_to_gas_algorithm(l2_block, da_block_costs)
.await?;

Ok(self.inner.algorithm())
}
Expand Down
Loading