Create uninitialized task for v1 gas price service #2442

MitchTurner · 2024-11-18T04:05:24Z

Linked Issues/PRs

part of #2140

Description

Adding the UninitializedTask with minimum test coverage.

Because of how we are organizing #2415, I don't believe we need to do any syncing of the DA Cost service before startup. If it's "behind" somehow, then the old values it provides will just be ignored. It might result in a little error in the profit value early on, but it will get washed out pretty quickly.

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?

…to feature/init-task-for-v1-gas-price-service

rymnc · 2024-11-20T09:13:24Z

crates/services/gas_price_service/src/v1/uninitialized_task.rs

+    DA: DaBlockCostsSource,
+    SettingsProvider: GasPriceSettingsProvider,
+{
+    const NAME: &'static str = "GasPriceServiceV0";


Suggested change

const NAME: &'static str = "GasPriceServiceV0";

const NAME: &'static str = "GasPriceServiceV1";

rymnc · 2024-11-20T09:14:10Z

crates/services/gas_price_service/src/v1/uninitialized_task.rs

+pub fn initialize_algorithm<Metadata>(
+    config: &V1AlgorithmConfig,
+    latest_block_height: u32,
+    metadata_storage: &Metadata,
+) -> GasPriceResult<(AlgorithmUpdaterV1, SharedV1Algorithm)>
+where
+    Metadata: MetadataStorage,
+{
+    let algorithm_updater;
+    if let Some(updater_metadata) = metadata_storage
+        .get_metadata(&latest_block_height.into())
+        .map_err(|err| GasPriceError::CouldNotInitUpdater(anyhow::anyhow!(err)))?
+    {
+        let previous_metadata: V1Metadata = updater_metadata.try_into()?;
+        algorithm_updater = v1_algorithm_from_metadata(previous_metadata, config);
+    } else {
+        algorithm_updater = config.into();
+    }
+
+    let shared_algo =
+        SharedGasPriceAlgo::new_with_algorithm(algorithm_updater.algorithm());
+
+    Ok((algorithm_updater, shared_algo))
+}


i think this is a dupe of initialize_algorithm from the v1/service.rs file, right?

https://github.com/FuelLabs/fuel-core/pull/2442/files#diff-ddae082e091f4688309a88faa0e368835781b2edd78044fd50a06d0c7b7dd428L291

MitchTurner · 2024-11-20T09:26:20Z

There is also the case where we have unrecorded blocks in the metadata that the committer has already reported. In that case we might want to recursively ask for batches of our smallest heights. The problem is we can't be sure we've got all the correct batches since the lowest height might be skipped.

netrome · 2024-11-20T09:18:09Z

crates/services/gas_price_service/src/v1/uninitialized_task.rs

+        config: V1AlgorithmConfig,
+        genesis_block_height: BlockHeight,
+        settings: SettingsProvider,
+        block_stream: BoxStream<SharedImportResult>,


Do we need to box this stream? Can't we just be generic over the stream here to be more flexible for e.g. tests?

netrome · 2024-11-20T09:36:18Z

crates/services/gas_price_service/src/v1/uninitialized_task.rs

+};
+use fuel_gas_price_algorithm::v1::AlgorithmUpdaterV1;
+
+pub struct UninitializedTask<


I'm not following why we need a separate UninitializedTask. As I read this, the purpose of this type is to separate task creation (UninitializedTask::new) and initialization UninitializedTask::init. I don't see any extra information being provided during initialization, so what's the benefit of doing this over having a single function that returns GasPriceServiceV1 directly?

Resolved offline.

Summary: The initialization logic depends on database concepts (getting blocks at different heights etc.) that we don't want the gas price algo to be aware of, to maintain a good separation between domain and integration logic. Ideally this initialization logic shouldn't even be here but rather closer to the fuel-core code that starts the services, but for the time being it's better to keep it isolated to this particular task.

…to feature/init-task-for-v1-gas-price-service

netrome

Looks good to me 👍

rymnc · 2024-11-20T19:16:13Z

crates/services/gas_price_service/src/v0/tests.rs

    let start = read_algo.next_gas_price();
+    let mut watcher = StateWatcher::started();


is there a reason this isn't StateWatcher::default()?

There were some places that process_l2_block_res wasn't getting called because of get_l2_block yielding a result, but because the service was shutting down (and we always check if there is an l2 block available before shutting down).

I added the started in a few places that it was shutting down early.

That being said, this isn't doing what the test says it's doing next__new_l2_block_saves_old_metadata. It's checking the gas price:

// then let new = read_algo.next_gas_price(); assert_ne!(start, new);

So I'll fix that.

rafal-ch · 2024-11-26T14:35:01Z

crates/services/gas_price_service/src/v0/service.rs

@@ -141,13 +141,15 @@ where
    Metadata: MetadataStorage,
 {
    async fn run(&mut self, watcher: &mut StateWatcher) -> TaskNextAction {
+        tracing::debug!("Starting gas price service");


We might not want to log a "sunny day" events on the debug! level. Please consider using trace!.

Reference: this comment (and a couple of other comments in the linked PR).

WIP

1836656

MitchTurner changed the base branch from master to feature/allow-DA-blocks-to-be-received-out-of-order November 19, 2024 02:45

MitchTurner added 11 commits November 19, 2024 09:46

Merge branch 'feature/allow-DA-blocks-to-be-received-out-of-order' in…

18fefb7

…to feature/init-task-for-v1-gas-price-service

Get compiling

b15a93f

Start adding tests

279191c

Get existing tests passing

63b0266

WIP

477c731

Merge branch 'feature/allow-DA-blocks-to-be-received-out-of-order' in…

4e07415

…to feature/init-task-for-v1-gas-price-service

Merge branch 'feature/allow-DA-blocks-to-be-received-out-of-order' in…

11df7ae

…to feature/init-task-for-v1-gas-price-service

Merge branch 'feature/allow-DA-blocks-to-be-received-out-of-order' in…

ca6e1e5

…to feature/init-task-for-v1-gas-price-service

Update CHANGELOG

2592ad0

Remove some TODOs

eb233ab

Remove other todo

d32b347

MitchTurner marked this pull request as ready for review November 20, 2024 08:54

MitchTurner requested review from rafal-ch, xgreenx and Dentosal as code owners November 20, 2024 08:54

MitchTurner self-assigned this Nov 20, 2024

rymnc reviewed Nov 20, 2024

View reviewed changes

netrome reviewed Nov 20, 2024

View reviewed changes

MitchTurner added 3 commits November 20, 2024 18:00

Merge branch 'feature/allow-DA-blocks-to-be-received-out-of-order' in…

6b9969c

…to feature/init-task-for-v1-gas-price-service

Fix copy-paste errors

fc8c427

Fix broken tests

87dc547

netrome approved these changes Nov 20, 2024

View reviewed changes

rymnc reviewed Nov 20, 2024

View reviewed changes

netrome mentioned this pull request Nov 25, 2024

Shared sequencer integration for fuel-core 0.40.0 #2451

Merged

1 task

rafal-ch reviewed Nov 26, 2024

View reviewed changes

Correct test, use trace instead of debug

008f5fb

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create uninitialized task for v1 gas price service #2442

Create uninitialized task for v1 gas price service #2442

MitchTurner commented Nov 18, 2024 •

edited

Loading

rymnc Nov 20, 2024

rymnc Nov 20, 2024

rymnc Nov 20, 2024

MitchTurner commented Nov 20, 2024

netrome Nov 20, 2024

netrome Nov 20, 2024

MitchTurner Nov 20, 2024

netrome Nov 20, 2024

netrome left a comment

rymnc Nov 20, 2024

MitchTurner Nov 27, 2024

MitchTurner Nov 27, 2024

rafal-ch Nov 26, 2024

	const NAME: &'static str = "GasPriceServiceV0";
	const NAME: &'static str = "GasPriceServiceV1";

		let start = read_algo.next_gas_price();
		let mut watcher = StateWatcher::started();

Create uninitialized task for v1 gas price service #2442

Are you sure you want to change the base?

Create uninitialized task for v1 gas price service #2442

Conversation

MitchTurner commented Nov 18, 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

MitchTurner commented Nov 20, 2024

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

netrome left a 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

MitchTurner commented Nov 18, 2024 •

edited

Loading