chore(gas_price_service): refactor gas price service #2192

rymnc · 2024-09-13T11:59:44Z

Linked Issues/PRs

chore(gas_price_service): refactor service initialisation in sub_services module #2184
feat(gas_price_service): create runnable task for expensive background polling for da metadata #2163

Description

This PR moves the L2 block source and DA block cost source out of the algorithm updater and into the GasPriceService for improved handling of lifecycle and reducing 1 layer of abstraction. There will be a follow up PR to sunset InitializeTask. I did not include it in this PR to reduce overhead since there were quite a few significant changes needed to be made to the l2 block source.

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]

Rust SDK
Sway compiler
Platform documentation (for out-of-organization contributors, the person merging the PR will do this)
Someone else?

xgreenx · 2024-09-17T08:36:02Z

crates/services/gas_price_service/src/lib.rs

+                    Ok(da_block_costs) => Some(da_block_costs),
+                    Err(err) => {
+                        tracing::error!("Failed to get da block costs: {:?}", err);
+                        None


If you have some value already, but the next request failed, you will set it to be None, while old value still is good=)

addressed in f0bbf49

xgreenx · 2024-09-17T08:37:58Z

crates/services/gas_price_service/src/lib.rs

+            }
+            l2_block = self.l2_block_source.get_l2_block() => {
+                let l2_block = l2_block?;
+                let da_block_costs = self.da_block_costs.clone();


Maybe you want to do take instead of clone=)

addressed in 2f33f7b

xgreenx · 2024-09-17T08:40:41Z

crates/services/gas_price_service/src/fuel_gas_price_updater/da_source_adapter/service.rs

+}
+
+#[async_trait::async_trait]
+pub trait DaBlockCostsProviderPort {


I don't see the reason of having trait for DaBlockCostsProvider. It looks like we can just implement methods directly on the DaBlockCostsProvider.

addressed in d68b02d

xgreenx · 2024-09-17T08:42:51Z

crates/services/gas_price_service/src/fuel_gas_price_updater/da_source_adapter/service.rs

    time::{
        interval,
        Interval,
    },
 };

 pub use anyhow::Result;
+use tokio::sync::broadcast::Receiver;
+
+pub struct DaBlockCostsProvider<Source: DaBlockCostsSource + 'static> {


Why do we want to wrap DaBlockCostsService into DaBlockCostsProvider? The gas price service already knows about DaBlockCostsService type. We can just work directly with this type.

The gas price service can subscribe in new_service from the shared state of the DaBlockCostsService

my initial impl did this, but it seemed cleaner to use the DaBlockCostsProvider to wrap over the service, and have only one field on the GasPriceService, which is responsible to get new block costs and the service handle :)

I would prefer if we maintained this structure instead :)

xgreenx · 2024-09-17T08:43:47Z

crates/services/gas_price_service/src/fuel_gas_price_updater/da_source_adapter/service.rs

+    fn send(&self, da_block_costs: DaBlockCosts) -> Result<()> {
+        self.sender.send(da_block_costs)?;
+        Ok(())
+    }


[nit] You have access to private fields of this structure, so maybe we don't need this helper function

addressed in 69651a5

xgreenx · 2024-09-17T08:47:39Z

crates/fuel-core/src/service/sub_services/algorithm_updater.rs

@@ -193,8 +188,6 @@ pub fn get_synced_gas_price_updater(
    settings: ConsensusParametersProvider,


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

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

xgreenx · 2024-09-17T08:48:27Z

crates/services/gas_price_service/src/lib.rs

@@ -60,18 +84,23 @@ pub trait UpdateAlgorithm {
    fn start(&self, for_block: BlockHeight) -> Self::Algorithm;

    /// Wait for the next algorithm to be available
-    async fn next(&mut self) -> anyhow::Result<Self::Algorithm>;
+    async fn next(


Do we still need async here?

addressed in 00e0d06

xgreenx · 2024-09-17T08:53:43Z

crates/services/gas_price_service/src/lib.rs

+        let l2_block = self.l2_block_source.get_l2_block().now_or_never();
+        if let Some(Ok(l2_block)) = l2_block {
+            let da_block_costs = self.da_block_costs.clone();
+            if let Some(new_algo) = self
+                .update_algorithm
+                .next(l2_block, da_block_costs)
+                .now_or_never()
+            {
+                let new_algo = new_algo?;
+                tracing::debug!("Updating gas price algorithm");
+                self.update(new_algo).await;
+            }
        }


It should be a loop, since we can have more blocks than one. Plus it would be nice to avoid logic duplication, could you move the same logic into the function and reuse it here and in run function?

avoided duplication in 911c06f

i didn't understand why this should be a loop since we're shutting down here :)

MitchTurner · 2024-09-18T09:40:48Z

crates/services/gas_price_service/src/fuel_gas_price_updater.rs

@@ -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> {


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.

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.

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?

(it also means we turn FuelGasPriceUpdater into a RunnableTask)

rymnc · 2024-09-19T11:04:48Z

closing in favor of another approach :)

…eUpdater type into GasPriceService (#2256) > [!WARNING] > 🚧🚧 This is PR 6/n in refactoring the gas price service. Now that the `algorithm_updater` is a part of `fuel-core-gas-price-service` we have squashed it into the `GasPriceService` using the `UninitializedTask` struct. We don't implement `RunnableService` *or* `RunnableTask` for the `UninitializedTask` struct, merely use it as a wrapper to generate the `GasPriceServiceV0` 🚧🚧 ## Linked Issues/PRs  - #2246 - #2245 - #2230 - #2226 - #2224 - #2192 ## Description  - created common module containing storage adapter, l2 block source and some updater metadata stuff (linked to storage) - created v0 module containing impl of GasPriceAlgorithm for AlgorithmV0, and V0metadata stuff - created v1 module containing impl of GasPriceAlgorithm for AlgorithmV1 & da block costs source (V1Metadata didn’t exist before so i didn’t create it) - fuel-gas-price-updater will be nuked, an UninitializedGasPriceServiceV(x) for each version that takes in all the args needed ```mermaid graph TD A["lib.rs (entry point)"] B[common] B1[utils] B2[storage] B3[l2_block_source] C[ports] D[v0] E[v1] F[v0/algorithm] G[v1/algorithm] H[v0/service] I[v1/da_source] J[v0/metadata] K[v1/service] L[v0/uninitialized] A --> B B --> B1 B --> B2 B --> B3 B --> C C --> D C --> E D --> F D --> H D --> J D --> L E --> G E --> I E --> K F --> H G --> I H --> J J --> L L --> M[SharedV0Algorithm] L --> N[GasPriceServiceV0] subgraph Common B B1 B2 B3 end subgraph Ports C end subgraph V0 D F H J L end subgraph V1 E G I K end ``` ## Checklist - [x] Breaking changes are clearly marked as such in the PR description and changelog - [x] New behavior is reflected in tests - [x] [The specification](https://github.com/FuelLabs/fuel-specs/) matches the implemented behavior (link update PR if changes are needed) ### Before requesting review - [x] 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] - [ ] [Rust SDK](https://github.com/FuelLabs/fuels-rs/) - [ ] [Sway compiler](https://github.com/FuelLabs/sway/) - [ ] [Platform documentation](https://github.com/FuelLabs/devrel-requests/issues/new?assignees=&labels=new+request&projects=&template=NEW-REQUEST.yml&title=%5BRequest%5D%3A+) (for out-of-organization contributors, the person merging the PR will do this) - [ ] Someone else? ---------

…hannel (#2278) ## Linked Issues/PRs  - follows up #2192 with some changes to clean up the da block costs source. ## Description  - Exposed a `broadcast::Sender` as the shared_state of the DaBlockCostsSource, which is to be subscribed to when a service needs this dependency. ## Checklist - [x] Breaking changes are clearly marked as such in the PR description and changelog - [x] New behavior is reflected in tests - [x] [The specification](https://github.com/FuelLabs/fuel-specs/) matches the implemented behavior (link update PR if changes are needed) ### Before requesting review - [x] 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] - [ ] [Rust SDK](https://github.com/FuelLabs/fuels-rs/) - [ ] [Sway compiler](https://github.com/FuelLabs/sway/) - [ ] [Platform documentation](https://github.com/FuelLabs/devrel-requests/issues/new?assignees=&labels=new+request&projects=&template=NEW-REQUEST.yml&title=%5BRequest%5D%3A+) (for out-of-organization contributors, the person merging the PR will do this) - [ ] Someone else? --------- Co-authored-by: Green Baneling <XgreenX9999@gmail.com>

rymnc self-assigned this Sep 13, 2024

rymnc added tech-debt The issue is to improve the current code and make it more clear/generic/reusable/pretty/avoidable. no changelog Skip the CI check of the changelog modification labels Sep 13, 2024

rymnc linked an issue Sep 13, 2024 that may be closed by this pull request

chore(gas_price_service): refactor service initialisation in sub_services module #2184

Closed

1 task

rymnc added 3 commits September 16, 2024 14:01

chore(gas_price_service): refactor gas price service

9ccc343

chore: hide impl details

6cf9248

fix: use buf size

f0ad83a

rymnc force-pushed the chore/refactor-gas-price-service branch from 18b84d5 to f0ad83a Compare September 16, 2024 08:31

rymnc marked this pull request as ready for review September 16, 2024 10:26

rymnc requested review from xgreenx, Dentosal and MitchTurner as code owners September 16, 2024 10:26

rymnc requested a review from a team September 16, 2024 10:27

xgreenx reviewed Sep 17, 2024

View reviewed changes

rymnc added 6 commits September 17, 2024 15:55

fix: dont set none if failed

f0bbf49

fix: use take instead of clone

2f33f7b

fix: make next sync

00e0d06

fix: remove trait port

d68b02d

fix: use priv fields

69651a5

chore: extract common logic out

911c06f

rymnc requested a review from xgreenx September 17, 2024 11:32

MitchTurner reviewed Sep 18, 2024

View reviewed changes

rymnc closed this Sep 19, 2024

rymnc mentioned this pull request Sep 27, 2024

chore(gas_price_service): split into v0 and v1 and squash FuelGasPriceUpdater type into GasPriceService #2256

Merged

9 tasks

rymnc deleted the chore/refactor-gas-price-service branch October 2, 2024 10:00

rymnc mentioned this pull request Oct 2, 2024

fix(da_block_costs): remove Arc<Mutex<>> on shared_state and expose channel #2278

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(gas_price_service): refactor gas price service #2192

chore(gas_price_service): refactor gas price service #2192

rymnc commented Sep 13, 2024 •

edited

Loading

xgreenx Sep 17, 2024

rymnc Sep 17, 2024

xgreenx Sep 17, 2024

rymnc Sep 17, 2024

xgreenx Sep 17, 2024

rymnc Sep 17, 2024 •

edited

Loading

xgreenx Sep 17, 2024

rymnc Sep 17, 2024 •

edited

Loading

rymnc Sep 17, 2024

xgreenx Sep 17, 2024

rymnc Sep 17, 2024

xgreenx Sep 17, 2024

rymnc Sep 17, 2024

xgreenx Sep 17, 2024

rymnc Sep 17, 2024

xgreenx Sep 17, 2024

rymnc Sep 17, 2024 •

edited

Loading

MitchTurner Sep 18, 2024

MitchTurner Sep 18, 2024 •

edited

Loading

rymnc Sep 18, 2024

rymnc Sep 18, 2024

rymnc commented Sep 19, 2024

		@@ -193,8 +188,6 @@ pub fn get_synced_gas_price_updater(
		settings: ConsensusParametersProvider,

chore(gas_price_service): refactor gas price service #2192

chore(gas_price_service): refactor gas price service #2192

Conversation

rymnc commented Sep 13, 2024 • edited Loading

Linked Issues/PRs

Description

Checklist

Before requesting review

After merging, notify other teams

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rymnc Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rymnc Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rymnc Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MitchTurner Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rymnc commented Sep 19, 2024

rymnc commented Sep 13, 2024 •

edited

Loading

rymnc Sep 17, 2024 •

edited

Loading

rymnc Sep 17, 2024 •

edited

Loading

rymnc Sep 17, 2024 •

edited

Loading

MitchTurner Sep 18, 2024 •

edited

Loading