Implement worst case scenario for price algorithm v1 #2219

rafal-ch · 2024-09-18T14:22:44Z

Linked Issues/PRs

Closes #2203

Description

This PR adds the worst_case() calculation to the V1 price algorithm.

Checklist

New behavior is reflected in tests

Before requesting review

I have reviewed the code myself

MitchTurner · 2024-09-18T14:24:49Z

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

@@ -4,3 +4,24 @@

 pub mod v0;
 pub mod v1;
+
+#[allow(clippy::cast_possible_truncation)]
+pub(crate) fn cumulative_percentage(


I think we should name this "cumulative_percentage_change" or something like that instead

Updated in adffd99

Dentosal · 2024-09-18T16:58:53Z

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

+    // account for rounding errors and take a slightly higher value
+    const ARB_CUTOFF: f64 = 16948547188989277.0;
+    if approx > ARB_CUTOFF {
+        const ARB_ADDITION: f64 = 2000.0;
+        approx += ARB_ADDITION;
+    }


I don't understand what happens here. What's the meaning of ARB_CUTOFF?

It's a hack for dealing with really large floats. The rounding error will be on the order of 2000 and based on the prop tests will always result in a value that is larger than the actual worst case value.

So ARB_CUTOFF is where we start seeing the error and ARB_ADDITION is the the amount we add to account for that error. This is the cost of using floats to represent these values, maybe there is a cleaner way to represent it :).

It doesn't sound arbitrary then. Can we rename it to something like this?

Suggested change

// account for rounding errors and take a slightly higher value

const ARB_CUTOFF: f64 = 16948547188989277.0;

if approx > ARB_CUTOFF {

const ARB_ADDITION: f64 = 2000.0;

approx += ARB_ADDITION;

}

// account for rounding errors and take a slightly higher value

const ROUNDING_ERROR_CUTOFF: f64 = 16948547188989277.0;

if approx > ROUNDING_ERROR_CUTOFF {

const ROUNDING_ERROR_COMPENSATION: f64 = 2000.0;

approx += ROUNDING_ERROR_COMPENSATION;

}

Background:

#2025 (comment)

It doesn't sound arbitrary then. Can we rename it to something like this?

There isn't a "correct" value to choose here, that's why I called it ARB, but you're right that the reason we chose it isn't completely arbitrary and your suggestion is much clearer and helpful anyway.

Variable names updated in 4337b14

MitchTurner · 2024-09-19T09:29:55Z

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

+
+proptest! {
+    #[test]
+    fn worst_case__correctly_calculates_value(


We should add another test to check for overflows. See the V0 test and this convo:
#2025 (comment)

Test added in 6cb3161

netrome

Some suggestions, otherwise LGTM

netrome · 2024-09-19T09:26:13Z

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

+    // account for rounding errors and take a slightly higher value
+    const ARB_CUTOFF: f64 = 16948547188989277.0;
+    if approx > ARB_CUTOFF {
+        const ARB_ADDITION: f64 = 2000.0;
+        approx += ARB_ADDITION;
+    }


It doesn't sound arbitrary then. Can we rename it to something like this?

Suggested change

// account for rounding errors and take a slightly higher value

const ARB_CUTOFF: f64 = 16948547188989277.0;

if approx > ARB_CUTOFF {

const ARB_ADDITION: f64 = 2000.0;

approx += ARB_ADDITION;

}

// account for rounding errors and take a slightly higher value

const ROUNDING_ERROR_CUTOFF: f64 = 16948547188989277.0;

if approx > ROUNDING_ERROR_CUTOFF {

const ROUNDING_ERROR_COMPENSATION: f64 = 2000.0;

approx += ROUNDING_ERROR_COMPENSATION;

}

netrome · 2024-09-19T09:28:40Z

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

+#[allow(clippy::cast_possible_truncation)]
+pub(crate) fn cumulative_percentage_change(


It feels unclean to have this function on the crate root. How about putting it in a utils module or something similar?

Updated in d1672d3

Realized somethings we were missing.

…or_algorithm_v1

MitchTurner · 2024-09-20T08:11:45Z

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

+}
+
+#[test]
+fn worst_case__same_block_gives_new_exec_price() {


This name is wrong, it's misssing the da portion in the name. I think instead just saying it gives the same as calculate() would be best, for both the name and how it calculates the expected value.

Names updated in c55aa65

@rymnc

## Version v0.37.0 ### Added - [1609](#1609): Add DA compression support. Compressed blocks are stored in the offchain database when blocks are produced, and can be fetched using the GraphQL API. - [2290](#2290): Added a new CLI argument `--graphql-max-directives`. The default value is `10`. - [2195](#2195): Added enforcement of the limit on the size of the L2 transactions per block according to the `block_transaction_size_limit` parameter. - [2131](#2131): Add flow in TxPool in order to ask to newly connected peers to share their transaction pool - [2182](#2151): Limit number of transactions that can be fetched via TxSource::next - [2189](#2151): Select next DA height to never include more than u16::MAX -1 transactions from L1. - [2162](#2162): Pool structure with dependencies, etc.. for the next transaction pool module. Also adds insertion/verification process in PoolV2 and tests refactoring - [2265](#2265): Integrate Block Committer API for DA Block Costs. - [2280](#2280): Allow comma separated relayer addresses in cli - [2299](#2299): Support blobs in the predicates. - [2300](#2300): Added new function to `fuel-core-client` for checking whether a blob exists. ### Changed #### Breaking - [2299](#2299): Anyone who wants to participate in the transaction broadcasting via p2p must upgrade to support new predicates on the TxPool level. - [2299](#2299): Upgraded `fuel-vm` to `0.58.0`. More information in the [release](https://github.com/FuelLabs/fuel-vm/releases/tag/v0.58.0). - [2276](#2276): Changed how complexity for blocks is calculated. The default complexity now is 80_000. All queries that somehow touch the block header now are more expensive. - [2290](#2290): Added a new GraphQL limit on number of `directives`. The default value is `10`. - [2206](#2206): Use timestamp of last block when dry running transactions. - [2153](#2153): Updated default gas costs for the local testnet configuration to match `fuel-core 0.35.0`. ## What's Changed * fix: use core-test.fuellabs.net for dnsaddr resolution by @rymnc in #2214 * Removed state transition bytecode from the local testnet by @xgreenx in #2215 * Send whole transaction pool upon subscription to gossip by @AurelienFT in #2131 * Update default gas costs based on 0.35.0 benchmarks by @xgreenx in #2153 * feat: Use timestamp of last block when dry running transactions by @netrome in #2206 * fix(dnsaddr_resolution): use fqdn separator to prevent suffixing by dns resolvers by @rymnc in #2222 * TransactionSource: specify maximum number of transactions to be fetched by @acerone85 in #2182 * Implement worst case scenario for price algorithm v1 by @rafal-ch in #2219 * chore(gas_price_service): define port for L2 data by @rymnc in #2224 * Block producer selects da height to never exceed u64::MAX - 1 transactions from L1 by @acerone85 in #2189 * Weekly `cargo update` by @github-actions in #2236 * Use fees to calculate DA reward and avoid issues with Gwei/Wei conversions by @MitchTurner in #2229 * Protect against passing `i128::MIN` to `abs()` which causes overflow by @rafal-ch in #2241 * Acquire `da_finalization_period` from the command line by @rafal-ch in #2240 * Executor: test Tx_count limit with incorrect tx source by @acerone85 in #2242 * Minor updates to docs + a few typos fixed by @rafal-ch in #2250 * chore(gas_price_service): move algorithm_updater to fuel-core-gas-price-service by @rymnc in #2246 * Use single heavy input in the `transaction_throughput.rs` benchmarks by @xgreenx in #2205 * Enforce the block size limit by @rafal-ch in #2195 * feat: build ARM and AMD in parallel by @mchristopher in #2130 * Weekly `cargo update` by @github-actions in #2268 * chore(gas_price_service): split into v0 and v1 and squash FuelGasPriceUpdater type into GasPriceService by @rymnc in #2256 * feat(gas_price_service): update block committer da source with established contract by @rymnc in #2265 * Use bytes from `unrecorded_blocks` rather from the block from DA by @MitchTurner in #2252 * TxPool v2 General architecture by @AurelienFT in #2162 * Add value delimiter and tests args by @AurelienFT in #2280 * fix(da_block_costs): remove Arc<Mutex<>> on shared_state and expose channel by @rymnc in #2278 * fix(combined_database): syncing auxiliary databases on startup with custom behaviour by @rymnc in #2272 * fix: Manually encode Authorization header for eventsource_client by @Br1ght0ne in #2284 * Address `async-graphql` vulnerability by @MitchTurner in #2290 * Update the WASM compatibility tests for `0.36` release by @rafal-ch in #2271 * DA compression by @Dentosal in #1609 * Use different port for every version compatibility test by @rafal-ch in #2301 * Fix block query complexity by @xgreenx in #2297 * Support blobs in predicates by @Voxelot in #2299 **Full Changelog**: v0.36.0...v0.37.0

rafal-ch added 4 commits September 18, 2024 14:42

Extract 'cumulative_percentage()' from algorithm V0

dc3a670

Implement worst_case() for Algorithm V1

7693ecd

Use worst_case in impl of GasPriceAlgorithm for Algorithm

334bd3f

Satisfy Clippy

3db5157

rafal-ch added the no changelog Skip the CI check of the changelog modification label Sep 18, 2024

rafal-ch requested review from xgreenx, Dentosal and MitchTurner as code owners September 18, 2024 14:22

rafal-ch requested a review from a team September 18, 2024 14:22

MitchTurner reviewed Sep 18, 2024

View reviewed changes

rafal-ch and others added 2 commits September 18, 2024 16:35

Rename 'cumulative_percentage()' to 'cumulative_percentage_change()'

adffd99

Merge branch 'master' into 2203_worst_case_for_algorithm_v1

dd6bf4d

MitchTurner previously approved these changes Sep 18, 2024

View reviewed changes

Dentosal reviewed Sep 18, 2024

View reviewed changes

Merge branch 'master' into 2203_worst_case_for_algorithm_v1

512613a

MitchTurner reviewed Sep 19, 2024

View reviewed changes

netrome previously approved these changes Sep 19, 2024

View reviewed changes

rafal-ch added 2 commits September 19, 2024 12:10

Use more clear variable names in cumulative_percentage_change

4337b14

Add utils module and move cumulative_percentage_change inside

d1672d3

rafal-ch dismissed netrome’s stale review via d1672d3 September 19, 2024 10:17

rafal-ch added 2 commits September 19, 2024 12:59

Add additional tests to Algorithm V1

6cb3161

Merge remote-tracking branch 'upstream/master' into 2203_worst_case_f…

8875466

…or_algorithm_v1

MitchTurner reviewed Sep 20, 2024

View reviewed changes

rafal-ch and others added 3 commits September 20, 2024 11:45

Update price algorithm test names

c55aa65

Modify test to be a little more precise

94dc8b7

Merge branch 'master' into 2203_worst_case_for_algorithm_v1

e2346f1

MitchTurner approved these changes Sep 20, 2024

View reviewed changes

netrome approved these changes Sep 20, 2024

View reviewed changes

MitchTurner enabled auto-merge (squash) September 20, 2024 09:56

MitchTurner merged commit 43c1676 into master Sep 20, 2024
33 checks passed

MitchTurner deleted the 2203_worst_case_for_algorithm_v1 branch September 20, 2024 10:15

MitchTurner assigned rafal-ch Sep 23, 2024

xgreenx mentioned this pull request Oct 5, 2024

Release v0.37.0 #2302

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement worst case scenario for price algorithm v1 #2219

Implement worst case scenario for price algorithm v1 #2219

rafal-ch commented Sep 18, 2024

MitchTurner Sep 18, 2024

rafal-ch Sep 18, 2024

Dentosal Sep 18, 2024

MitchTurner Sep 19, 2024

netrome Sep 19, 2024

MitchTurner Sep 19, 2024

MitchTurner Sep 19, 2024

rafal-ch Sep 19, 2024

MitchTurner Sep 19, 2024

rafal-ch Sep 19, 2024

netrome left a comment

netrome Sep 19, 2024

netrome Sep 19, 2024

rafal-ch Sep 19, 2024

MitchTurner Sep 20, 2024

rafal-ch Sep 20, 2024

		#[allow(clippy::cast_possible_truncation)]
		pub(crate) fn cumulative_percentage_change(

Implement worst case scenario for price algorithm v1 #2219

Implement worst case scenario for price algorithm v1 #2219

Conversation

rafal-ch commented Sep 18, 2024

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

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

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

Choose a reason for hiding this comment