bug: fix algorithm overflow issues #2173

MitchTurner · 2024-09-06T11:47:10Z

Linked Issues/PRs

Closes #2164
Closes #2147

Description

The main change with this code is "normalizaing" the costs and rewards instead of keeping a total over all time. i.e. every time we receive a DA block, we see if the reward is greater than the costs, or vice versa. If the reward is higher, we set the reward to the difference and set the the last known cost to 0 and adjust the projected cost accordingly.

In addition, we were using a random set of types for the algorithm and also used casts in many places. This PR should fix a lot of those problems.

Bonus: This fix prompted me to run the optimization again. Since the set is much bigger now, I decided to enable running the simulation in parallel tasks to speed up the code.

Checklist

New behavior is reflected in tests

Before requesting review

I have reviewed the code myself

This reverts commit 1836a24.

…p-2164

crates/fuel-gas-price-algorithm/gas-price-analysis/src/simulation.rs

crates/fuel-gas-price-algorithm/src/v1.rs

crates/fuel-gas-price-algorithm/src/v1/tests/update_da_record_data_tests.rs

rymnc · 2024-09-10T15:45:02Z

crates/fuel-gas-price-algorithm/src/v1.rs

    da_p_factor: i64,
    /// The D component of the PID control for the DA gas price
    da_d_factor: i64,


is my understanding correct that these values are i64's because we need them to be for float calculation?

I'm not sure what you mean by "float calculation". We don't have any floats in this code.

These actually probably don't need to be i and could be u. When I added them originally, I was open to the idea of using negative values. Just haven't reevaluated.

ah okay. as far as i remember there were a bunch of floats used so i might be wrong

MitchTurner · 2024-09-10T15:45:03Z

crates/fuel-gas-price-algorithm/src/v1/tests/update_da_record_data_tests.rs

@@ -283,3 +290,127 @@ fn update__da_block_updates_projected_total_cost_with_known_and_guesses_on_top()
    let expected = new_known_total_cost + guessed_part;
    assert_eq!(actual, expected as u128);
 }
+
+prop_compose! {
+    fn arb_vec_of_da_blocks()(last_da_block: u32, count in 1..255usize, rng_seed: u64) -> Vec<RecordedBlock> {


Maybe should use a different value than 255. That makes it seem non-arbitrary.

I often use numbers like 123, 1234 or 0x1234, if there's no good reason to do otherwise. They contain no secondary meaning unlike 100 or 255, and are good nothing-up-my-sleeve numbers

crates/fuel-gas-price-algorithm/src/v1.rs

rymnc

LGTM in general, left some suggestions/questions

crates/fuel-gas-price-algorithm/src/v1.rs

Co-authored-by: Aaryamann Challani <43716372+rymnc@users.noreply.github.com>

Dentosal · 2024-09-10T16:02:43Z

crates/fuel-gas-price-algorithm/src/v1.rs

@@ -153,11 +161,11 @@ pub struct AlgorithmUpdaterV1 {
    /// The maximum percentage that the DA portion of the gas price can change in a single block
    pub max_da_gas_price_change_percent: u8,


nit: I feel like introducing a newtype to enforce the percentage invariant (in range 0..=100, or even nonzero here) might be a good idea. Up to you.

I'm open to it! I'd just have to introduce fallibility into the constructor.

Also. In theory, this could be set to something larger than 100%. So even u8 is an arbitrary ceiling to how high it could be set to. I'm realizing that's why exec_gas_price_change_percent wasn't u8 before. Both could be higher in theory.

The l2_block_fullness_threshold_percent on the otherhand would be a better candidate for a newtype since there is no such thing as a 110% full block hopefully.

The arbitrary u8 ceiling feels weird, if there's ever any reason to set it to 200%, surely there are also reasons to set it to 300% as well? In any case, it wasn't clear to me from the doc comment, so maybe explicitly writing it there would be a good first step, and the newtype can be introduced later when the implementation has settled down.

Changed both to u16 and added explanation in comments. Will look into newtype.

Added a newtype for the threshold percentage. I called it ClampedPercent to communicate that it's infallible and just clamps the value.

Dentosal

LGTM; although I was mostly looking at the changeset and not the whole picture

xgreenx

I hope @rymnc tried to dive deeper in the formulas and checked them=D

xgreenx · 2024-09-10T16:30:05Z

crates/fuel-gas-price-algorithm/gas-price-analysis/Cargo.toml

 plotters = "0.3.5"
 rand = "0.8.5"
 rand_distr = "0.4.3"
 serde = { version = "1.0.209", features = ["derive"] }
+tokio = { version = "1.40.0", features = ["full"] }


It is better to not use full if you only need something specific=)

xgreenx · 2024-09-10T16:41:05Z

crates/fuel-gas-price-algorithm/gas-price-analysis/src/simulation/da_cost_per_byte.rs

@@ -25,6 +25,7 @@ pub fn get_da_cost_per_byte_from_source(
    }
 }

+#[allow(dead_code)]


Where do we use it?

It's used in CSV reader.

crates/fuel-gas-price-algorithm/gas-price-analysis/src/simulation.rs

@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

MitchTurner and others added 19 commits September 3, 2024 16:41

Add abstraction for da costs

889618c

Remove generated chart

3911903

Refactor

a300b02

WIP add csv reader

af9a072

Modify units to accomodate bigger values

3601cd1

Refactor

6d90d66

Create directory if doesn't exist

0037ca5

Merge branch 'master' into feature/allow-predefined-blob-tx-costs

d75a0d2

Move da cost calc stuff to its own mod

17582b8

Appease Clippy-sama

a92e090

Add results, fix gitignore

6a53157

Rename function, fix iterators, add other PR review suggestions

f8573e2

Merge branch 'master' into feature/allow-predefined-blob-tx-costs

6452ae1

empty commit to bump CI

0b6fcd0

Add one passing test

e541435

Improve documentation for simulation code

1836a24

Revert "Improve documentation for simulation code"

2286da4

This reverts commit 1836a24.

Fix dumb bug in simulation

af7391b

Parallelize optimization a bit

2912ea9

MitchTurner mentioned this pull request Sep 9, 2024

Use more advanced optimisation for determining P and D value in V1 gas price algorithm #2181

Open

2 tasks

MitchTurner added 3 commits September 10, 2024 11:50

Fix a bunch of casts and conversions

b709b5a

Fix warnings

3141914

Appease Clippy-sama

ab5b9dd

MitchTurner self-assigned this Sep 10, 2024

Merge remote-tracking branch 'origin' into bug/fix-algorithm-giving-u…

412bc95

…p-2164

MitchTurner commented Sep 10, 2024

View reviewed changes

crates/fuel-gas-price-algorithm/gas-price-analysis/src/simulation.rs Outdated Show resolved Hide resolved

crates/fuel-gas-price-algorithm/src/v1.rs Outdated Show resolved Hide resolved

crates/fuel-gas-price-algorithm/src/v1/tests/update_da_record_data_tests.rs Outdated Show resolved Hide resolved

Improve documentation for simulation code

1857fe2

MitchTurner added the no changelog Skip the CI check of the changelog modification label Sep 10, 2024

MitchTurner added 2 commits September 10, 2024 17:33

Add better testing with proptests :)

06d37e8

Lint toml

fcfd024

MitchTurner changed the title ~~Bug/fix algorithm giving up 2164~~ bug: fix algorithm overflow issues Sep 10, 2024

MitchTurner marked this pull request as ready for review September 10, 2024 15:41

MitchTurner requested review from xgreenx and Dentosal as code owners September 10, 2024 15:41

MitchTurner requested a review from a team September 10, 2024 15:41

rymnc reviewed Sep 10, 2024

View reviewed changes

MitchTurner commented Sep 10, 2024

View reviewed changes

rymnc reviewed Sep 10, 2024

View reviewed changes

crates/fuel-gas-price-algorithm/src/v1.rs Outdated Show resolved Hide resolved

rymnc reviewed Sep 10, 2024

View reviewed changes

crates/fuel-gas-price-algorithm/src/v1.rs Outdated Show resolved Hide resolved

rymnc reviewed Sep 10, 2024

View reviewed changes

crates/fuel-gas-price-algorithm/src/v1.rs Outdated Show resolved Hide resolved

MitchTurner and others added 3 commits September 10, 2024 18:01

Update crates/fuel-gas-price-algorithm/src/v1.rs

ce9c0ba

Co-authored-by: Aaryamann Challani <43716372+rymnc@users.noreply.github.com>

Update crates/fuel-gas-price-algorithm/src/v1.rs

88af11d

Co-authored-by: Aaryamann Challani <43716372+rymnc@users.noreply.github.com>

Update crates/fuel-gas-price-algorithm/src/v1.rs

c2ce485

Co-authored-by: Aaryamann Challani <43716372+rymnc@users.noreply.github.com>

Dentosal reviewed Sep 10, 2024

View reviewed changes

MitchTurner added 2 commits September 10, 2024 18:21

Clarify percentage types

7f4f785

Add newtype for threshold percentage

13693ef

Dentosal previously approved these changes Sep 10, 2024

View reviewed changes

xgreenx reviewed Sep 10, 2024

View reviewed changes

Use more arbitrary number, reduce feature deps for tokio

604279b

MitchTurner dismissed Dentosal’s stale review via 604279b September 10, 2024 21:02

rymnc reviewed Sep 10, 2024

View reviewed changes

crates/fuel-gas-price-algorithm/gas-price-analysis/src/simulation.rs Show resolved Hide resolved

Merge branch 'master' into bug/fix-algorithm-giving-up-2164

a4eff83

rymnc approved these changes Sep 11, 2024

View reviewed changes

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

xgreenx approved these changes Sep 11, 2024

View reviewed changes

MitchTurner merged commit f308bae into master Sep 11, 2024
35 checks passed

MitchTurner deleted the bug/fix-algorithm-giving-up-2164 branch September 11, 2024 08:53

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

bug: fix algorithm overflow issues #2173

bug: fix algorithm overflow issues #2173

MitchTurner commented Sep 6, 2024 •

edited

Loading

rymnc Sep 10, 2024

MitchTurner Sep 10, 2024

rymnc Sep 10, 2024

MitchTurner Sep 10, 2024

Dentosal Sep 10, 2024 •

edited

Loading

rymnc left a comment

Dentosal Sep 10, 2024 •

edited

Loading

MitchTurner Sep 10, 2024

Dentosal Sep 10, 2024 •

edited

Loading

MitchTurner Sep 10, 2024 •

edited

Loading

MitchTurner Sep 10, 2024

Dentosal left a comment

xgreenx left a comment

xgreenx Sep 10, 2024

xgreenx Sep 10, 2024

MitchTurner Sep 10, 2024

		@@ -153,11 +161,11 @@ pub struct AlgorithmUpdaterV1 {
		/// The maximum percentage that the DA portion of the gas price can change in a single block
		pub max_da_gas_price_change_percent: u8,

bug: fix algorithm overflow issues #2173

bug: fix algorithm overflow issues #2173

Conversation

MitchTurner commented Sep 6, 2024 • edited Loading

Linked Issues/PRs

Description

Checklist

Before requesting review

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

Dentosal Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

rymnc left a comment

Choose a reason for hiding this comment

Dentosal Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dentosal Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

MitchTurner Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dentosal left a comment

Choose a reason for hiding this comment

xgreenx 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

MitchTurner commented Sep 6, 2024 •

edited

Loading

Dentosal Sep 10, 2024 •

edited

Loading

Dentosal Sep 10, 2024 •

edited

Loading

Dentosal Sep 10, 2024 •

edited

Loading

MitchTurner Sep 10, 2024 •

edited

Loading