feat(gas_price_service): create runnable task for expensive background polling for da metadata #2163

rymnc · 2024-09-05T07:36:02Z

Warning

🏗️ This PR is 2/3 of including block committer "da"ta in v1 of the gas price algo. We have a dependency on the api of the block committer, which will warrant tweaking of the DaMetadataResponse type. The next PR will hook it up with cli args 🚧

Linked Issues/PRs

follow up to: feat(gas_price_service): include trait declaration for block committer data #2155
Create committer DA block provider for V1 gas price updater #2139

Description

specified a Service that polls an ingestor at specified intervals to fetch the da metadata in the background
specified a "sync"/"non-expensive" api for the data, which marks it as stale/None after fetching it

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?

…leanly

…to where its needed

MitchTurner

Looking good. I've made some general suggestions wrt architecture and nomenclature that I hope will help.

crates/services/gas_price_service/src/fuel_gas_price_updater/fuel_da_source_adapter.rs

crates/services/gas_price_service/src/fuel_gas_price_updater.rs

...services/gas_price_service/src/fuel_gas_price_updater/fuel_da_source_adapter/dummy_source.rs

crates/services/gas_price_service/src/fuel_gas_price_updater/fuel_da_source_adapter.rs

...as_price_service/src/fuel_gas_price_updater/fuel_da_source_adapter/block_committer_source.rs

crates/services/gas_price_service/src/fuel_gas_price_updater.rs

rymnc · 2024-09-11T11:09:32Z

please note that all changes to the block committer api will be handled in a follow up PR :)

CHANGELOG.md

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

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

MitchTurner · 2024-09-11T13:32:15Z

crates/services/gas_price_service/src/lib.rs

+    // The da block cost sub service
+    da_block_costs_task_handle: ServiceRunner<DaBlockCostsService<DummyDaBlockCosts>>,


I think this handle can just be on the adapter.

#[derive(Clone)] pub struct DaBlockCostsProvider { service_handle: ServiceRunner<DaBlockCostsService<DummyDaBlockCosts>>, receiver: Arc<Mutex<mpsc::Receiver<DaBlockCosts>>>, }

Why not?

The primary reason not to place the service_handle directly on the adapter is that it violates separation of concerns and single responsibility principle. The adapter’s role should be strictly focused on fetching and providing the da block costs. wdyt?

I don't think it's a violation of either of those.

It would be violation of SoC if we had the client also making calls for the L2block info. Those things can vary independently, and it would be wrong to couple them. The concern here is getting info from the committer. Our implementation is having a service that runs in another thread that eagerly buffers the data for us. If we were doing it lazily--without the service thread--everything would be in the adapter as well, and I assume you wouldn't have any issue with that.

The responsibility here is getting the costs from DA. It's not a misdirection at all for something called DaBlockCostsProvider to have a handle to task that is providing us with the latest DaBlockCosts.

I definitely think the GasPriceService is the wrong place for it. It has no concept of L2 or DA at that level. That's the job of our specific update_algorithm (not the best name, mb)

The only question I have about if this is the "right" place to hold the handle is how we want to handle errors from the service.

The only question I have about if this is the "right" place to hold the handle is how we want to handle errors from the service.

On second thought. The adapter is the only place that should be concerned with the errors from its service. So, actually, I think it's a great place for it.

MitchTurner · 2024-09-11T13:40:56Z

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

+{
+    const NAME: &'static str = "DaBlockCostsService";
+
+    type SharedData = DaBlockCostsProvider;


Hmm. This doesn't seem right, especially if you add the handle to this service to this struct.

I think this can just be (). I'd say you could have it return the receiver, but even that is wrong since this should be cloneable and you used a sc receiver.

why not? we have to create the channel internally and if we have no SharedData we would have to create the channel externally

Oh. It's Clone.

Okay. You can have it return the Arc<Mutex<Receiver>> and then the DaBlockCostsProvider can be constructed from that :)

MitchTurner

LGTM!

xgreenx

Based on our discussions we need to change a lot of stuff, but we can do that in follow up PRs

Let's merge it and start work on next changes:)

xgreenx · 2024-09-13T09:28:27Z

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

+    poll_interval: Interval,
+    source: Source,
+    sender: Sender<DaBlockCosts>,
+    cache: HashSet<DaBlockCosts>,


It would be nice to clean it up periodically:) Or even not to use a cache. We can just track the last range/da height of the bundle

this will be omitted anyway in the next pr when we restructure the gas price service

@rymnc

## Version v0.36.0 ### Added - [2135](#2135): Added metrics logging for number of blocks served over the p2p req/res protocol. - [2151](#2151): Added limitations on gas used during dry_run in API. - [2188](#2188): Added the new variant `V2` for the `ConsensusParameters` which contains the new `block_transaction_size_limit` parameter. - [2163](#2163): Added runnable task for fetching block committer data. - [2204](#2204): Added `dnsaddr` resolution for TLD without suffixes. ### Changed #### Breaking - [2199](#2199): Applying several breaking changes to the WASM interface from backlog: - Get the module to execute WASM byte code from the storage first, an fallback to the built-in version in the case of the `FUEL_ALWAYS_USE_WASM`. - Added `host_v1` with a new `peek_next_txs_size` method, that accepts `tx_number_limit` and `size_limit`. - Added new variant of the return type to pass the validation result. It removes block serialization and deserialization and should improve performance. - Added a V1 execution result type that uses `JSONError` instead of postcard serialized error. It adds flexibility of how variants of the error can be managed. More information about it in FuelLabs/fuel-vm#797. The change also moves `TooManyOutputs` error to the top. It shows that `JSONError` works as expected. - [2145](#2145): feat: Introduce time port in PoA service. - [2155](#2155): Added trait declaration for block committer data - [2142](#2142): Added benchmarks for varied forms of db lookups to assist in optimizations. - [2158](#2158): Log the public address of the signing key, if it is specified - [2188](#2188): Upgraded the `fuel-vm` to `0.57.0`. More information in the [release](https://github.com/FuelLabs/fuel-vm/releases/tag/v0.57.0). ## What's Changed * chore(p2p_service): add metrics for number of blocks requested over p2p req/res protocol by @rymnc in #2135 * Weekly `cargo update` by @github-actions in #2149 * Debug V1 algorightm and use more realistic values in gas price analysis by @MitchTurner in #2129 * feat(gas_price_service): include trait declaration for block committer data by @rymnc in #2155 * Convert gas price analysis tool to CLI by @MitchTurner in #2156 * chore: add benchmarks for varied forms of lookups by @rymnc in #2142 * Add label nochangelog on weekly cargo update by @AurelienFT in #2152 * Log consensus-key signer address if specified by @acerone85 in #2158 * chore(rocks_db): move ShallowTempDir to benches crate by @rymnc in #2168 * chore(benches): conditional dropping of databases in benchmarks by @rymnc in #2170 * feat: Introduce time port in PoA service by @netrome in #2145 * Get DA costs from predefined data by @MitchTurner in #2157 * chore(shallow_temp_dir): panic if not panicking by @rymnc in #2172 * chore: Add initial CODEOWNERS file by @netrome in #2179 * Weekly `cargo update` by @github-actions in #2177 * fix(db_lookup_times): rework core logic of benchmark by @rymnc in #2159 * Add verification on transaction dry_run that they don't spend more than block gas limit by @AurelienFT in #2151 * bug: fix algorithm overflow issues by @MitchTurner in #2173 * feat(gas_price_service): create runnable task for expensive background polling for da metadata by @rymnc in #2163 * Weekly `cargo update` by @github-actions in #2197 * Fix bug with gas price factor in V1 algorithm by @MitchTurner in #2201 * Applying several breaking changes to the WASM interface from backlog by @xgreenx in #2199 * chore(p2p): dnsaddr recursive resolution by @rymnc in #2204 ## New Contributors * @acerone85 made their first contribution in #2158 **Full Changelog**: v0.35.0...v0.36.0

rymnc self-assigned this Sep 5, 2024

rymnc force-pushed the feat/da-source-implementation branch from 52a079e to be66fb4 Compare September 5, 2024 07:56

feat: runnable tasks for background polling of block committer data

204a12e

rymnc force-pushed the feat/da-source-implementation branch from d48d8c3 to 204a12e Compare September 5, 2024 12:24

fix: make the duration optional

8e204bf

rymnc force-pushed the feat/da-source-implementation branch from c753c86 to 8e204bf Compare September 5, 2024 12:33

rymnc added 6 commits September 6, 2024 16:11

chore: add subservice, mnt before gaspriceservice, pass shared data c…

a70b7a8

…leanly

Merge branch 'master' into feat/da-source-implementation

1ac1af7

fix: inc expected_services

66cb107

fix: race condition on tests

139c0ee

fix: stupid error

927099b

fix: start clean up

487330a

rymnc force-pushed the feat/da-source-implementation branch from 7054a11 to 487330a Compare September 7, 2024 18:14

rymnc added 2 commits September 8, 2024 05:48

chore: verbose names, nice re-exports and source/sink usage

6d91500

fix: more renaming and refactoring

6c0b578

rymnc force-pushed the feat/da-source-implementation branch from 17960b6 to 6c0b578 Compare September 8, 2024 00:38

rymnc added 5 commits September 8, 2024 06:15

chore: more cleanup

55e85c3

fix: split gas price sink getter and setter trait, only allow access …

a50a150

…to where its needed

fix: clearer naming

3539a9c

fix: decouple single gas price source response

fc300a5

fix: var naming

57ef4d2

rymnc marked this pull request as ready for review September 9, 2024 06:35

Merge branch 'master' into feat/da-source-implementation

d1ebbcf

rymnc requested review from MitchTurner and acerone85 September 9, 2024 06:35

Merge branch 'master' into feat/da-source-implementation

5643599

rymnc requested review from xgreenx and Dentosal as code owners September 9, 2024 08:49

MitchTurner reviewed Sep 9, 2024

View reviewed changes

chore: refactor naming and tests

74fc9bf

rymnc added 8 commits September 9, 2024 20:08

Merge branch 'master' into feat/da-source-implementation

d368574

fix: blocking lock on setting block cost

e566385

fix: stream with channel instead

ce2b413

unique stream

8ff0038

test: internal sub service

2f803f9

Merge branch 'master' into feat/da-source-implementation

daae44e

fix: test

fc8b66e

fix: number of sub services

73e31e1

rymnc mentioned this pull request Sep 11, 2024

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

Closed

1 task

rymnc requested review from MitchTurner and xgreenx September 11, 2024 11:08

MitchTurner reviewed Sep 11, 2024

View reviewed changes

fix: changelog

cbab355

MitchTurner reviewed Sep 11, 2024

View reviewed changes

rymnc added 3 commits September 11, 2024 19:36

chore: dummy takes in a value

fee8fd1

fix: chan buf

0240ba7

test: smoke

c34c7bb

rymnc force-pushed the feat/da-source-implementation branch from 96af5d0 to c34c7bb Compare September 11, 2024 15:47

MitchTurner previously approved these changes Sep 12, 2024

View reviewed changes

chore: remove mutable reference in getter

42ffd08

rymnc dismissed MitchTurner’s stale review via 42ffd08 September 12, 2024 11:01

rymnc requested a review from MitchTurner September 12, 2024 11:01

MitchTurner approved these changes Sep 12, 2024

View reviewed changes

xgreenx approved these changes Sep 13, 2024

View reviewed changes

rymnc merged commit 8c67681 into master Sep 13, 2024
35 checks passed

rymnc deleted the feat/da-source-implementation branch September 13, 2024 09:35

rymnc mentioned this pull request Sep 16, 2024

chore(gas_price_service): refactor gas price service #2192

Closed

9 tasks

xgreenx mentioned this pull request Sep 17, 2024

Release v0.36.0 #2213

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(gas_price_service): create runnable task for expensive background polling for da metadata #2163

feat(gas_price_service): create runnable task for expensive background polling for da metadata #2163

rymnc commented Sep 5, 2024 •

edited

Loading

MitchTurner left a comment

rymnc commented Sep 11, 2024

MitchTurner Sep 11, 2024

rymnc Sep 11, 2024

MitchTurner Sep 11, 2024 •

edited

Loading

MitchTurner Sep 11, 2024 •

edited

Loading

MitchTurner Sep 11, 2024

rymnc Sep 11, 2024

MitchTurner Sep 11, 2024

MitchTurner left a comment

xgreenx left a comment

xgreenx Sep 13, 2024

rymnc Sep 13, 2024

		// The da block cost sub service
		da_block_costs_task_handle: ServiceRunner<DaBlockCostsService<DummyDaBlockCosts>>,

feat(gas_price_service): create runnable task for expensive background polling for da metadata #2163

feat(gas_price_service): create runnable task for expensive background polling for da metadata #2163

Conversation

rymnc commented Sep 5, 2024 • edited Loading

Linked Issues/PRs

Description

Checklist

Before requesting review

After merging, notify other teams

MitchTurner left a comment

Choose a reason for hiding this comment

rymnc commented Sep 11, 2024

MitchTurner Sep 11, 2024

Choose a reason for hiding this comment

rymnc Sep 11, 2024

Choose a reason for hiding this comment

MitchTurner Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

MitchTurner Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

MitchTurner Sep 11, 2024

Choose a reason for hiding this comment

rymnc Sep 11, 2024

Choose a reason for hiding this comment

MitchTurner Sep 11, 2024

Choose a reason for hiding this comment

MitchTurner left a comment

Choose a reason for hiding this comment

xgreenx left a comment

Choose a reason for hiding this comment

xgreenx Sep 13, 2024

Choose a reason for hiding this comment

rymnc Sep 13, 2024

Choose a reason for hiding this comment

rymnc commented Sep 5, 2024 •

edited

Loading

MitchTurner Sep 11, 2024 •

edited

Loading

MitchTurner Sep 11, 2024 •

edited

Loading