Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug: Gas Price Factor for V1 algorithm #2167

Closed
MitchTurner opened this issue Sep 5, 2024 · 1 comment
Closed

Bug: Gas Price Factor for V1 algorithm #2167

MitchTurner opened this issue Sep 5, 2024 · 1 comment
Assignees

Comments

@MitchTurner
Copy link
Member

Background

This is less relevant after we implement this: #2165

But assuming the cost is close to 1Wei and we want to increase the value by 10%, we need to use a gas price factor for the DA gas price to allow it to be 100 and then divide by the factor when we report it.

The problem is we are currently calculating the DA Gas Price outside of the Gas Price Updater, so we can only update the last_da_gas_price when we

            // implicitly deduce what our da gas price was for the l2 block
            self.last_da_gas_price = gas_price.saturating_sub(last_exec_price);

This erases the minute changes we can do thanks to the gas price factor!

Task

Currently, however, we aren't using the _block_bytes in the calculate() function. So we can move the calculation back into the algorithm updater, which allows us to know the DA gas price explicitly.

Essentially, V1 algorithm will just carry the total gas price now and we can move all the calculation back to the updater.

@MitchTurner
Copy link
Member Author

We should also take into account the min values and make sure the are scaled for bot exec and da

MitchTurner added a commit that referenced this issue Sep 6, 2024
## Linked Issues/PRs
#2138

## Description
This PR allows the analysis CLI tool to read data from a .csv file. 

Along with that work, I did significant refactoring and. cleanup to help
other engineers use the tool.

There are still some bugs that need to be sorted out as follow-up to
this issue:
#2167
#2164

### Before requesting review
- [x] I have reviewed the code myself
- [x] I have created follow-up issues caused by this PR and linked them
here

---------

Co-authored-by: green <xgreenx9999@gmail.com>
GoldenPath1109 added a commit to GoldenPath1109/fuel-core that referenced this issue Sep 7, 2024
## Linked Issues/PRs
FuelLabs/fuel-core#2138

## Description
This PR allows the analysis CLI tool to read data from a .csv file. 

Along with that work, I did significant refactoring and. cleanup to help
other engineers use the tool.

There are still some bugs that need to be sorted out as follow-up to
this issue:
FuelLabs/fuel-core#2167
FuelLabs/fuel-core#2164

### Before requesting review
- [x] I have reviewed the code myself
- [x] I have created follow-up issues caused by this PR and linked them
here

---------

Co-authored-by: green <xgreenx9999@gmail.com>
MitchTurner added a commit that referenced this issue Sep 17, 2024
## Linked Issues/PRs
#2167

## Description
Before, the final `da_gas_price` was calculated by the block producer,
but its chosen value wasn't communicated back to the updater
immediately. Instead the value was implicitly calculated by the updater
when it received the L2 block information. This was bad because it
destroyed information with respect to the gas price factor.

A value of 1 could actually have been 110 or 120 in the updater, but
then divided by the factor and rounded back to 1. On update, this would
be interpreted as just 100, and the incremental changes would be lost.

Since we are no long calculating the `da_gas_price` at the time of block
production, we could store the _actual_ `da_gas_price` value in the
updater and avoid the lost information.

Many tests existed to cover the previous behavior they were reconfigured
to match the newer, simpler behavior.

## Checklist
- [x] New behavior is reflected in tests

### Before requesting review
- [x] I have reviewed the code myself
@MitchTurner MitchTurner self-assigned this Sep 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant