bug: fix algorithm overflow issues #2173
Conversation
This reverts commit 1836a24.
crates/fuel-gas-price-algorithm/gas-price-analysis/src/simulation.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
MitchTurner
added
the
no changelog
MitchTurner
changed the title
| da_p_factor: i64, | ||
| /// The D component of the PID control for the DA gas price | ||
| da_d_factor: i64, |
There was a problem hiding this comment.
is my understanding correct that these values are i64's because we need them to be for float calculation?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
ah okay. as far as i remember there were a bunch of floats used so i might be wrong
| @@ -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> { | |||
There was a problem hiding this comment.
Maybe should use a different value than 255. That makes it seem non-arbitrary.
There was a problem hiding this comment.
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
Co-authored-by: Aaryamann Challani <[email protected]>
Co-authored-by: Aaryamann Challani <[email protected]>
Co-authored-by: Aaryamann Challani <[email protected]>
| @@ -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, | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Changed both to u16 and added explanation in comments. Will look into newtype.
There was a problem hiding this comment.
Added a newtype for the threshold percentage. I called it ClampedPercent to communicate that it's infallible and just clamps the value.
| 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"] } |
There was a problem hiding this comment.
It is better to not use full if you only need something specific=)
| @@ -25,6 +25,7 @@ pub fn get_da_cost_per_byte_from_source( | |||
| } | |||
| } | |||
|
|
|||
| #[allow(dead_code)] | |||
There was a problem hiding this comment.
It's used in CSV reader.
## 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
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
0and 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
Before requesting review