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

Deal with negative feed back loop in DA gas price #2364

Merged

Conversation

MitchTurner
Copy link
Member

@MitchTurner MitchTurner commented Oct 16, 2024

Linked Issues/PRs

#2347

Description

/// Because the DA gas price can increase even when no-one is using the network, there is a potential
/// for a negative feedback loop to occur where the gas price increases, further decreasing activity
/// and increasing the gas price. The `L2ActivityTracker` is used to moderate changes to the DA
/// gas price based on the activity of the L2 chain.
///
/// For each L2 block, the activity is calculated as a percentage of the block capacity used. If the
/// activity is below a certain threshold, the activity is decreased. The activity exists on a scale
/// between 0 and the sum of the increase, hold, and decrease buffers.
///
/// e.g. if the decrease range is 50, the hold range is 50, and the increase range is 50:
///
/// 0<-- decrease range -->50<-- hold range -->100<-- increase range -->150
///
/// The current activity determines the behavior of the DA gas price.
///
/// For healthy behavior, the activity should be in the `increase` range.

Checklist

  • New behavior is reflected in tests

Before requesting review

  • I have reviewed the code myself

@MitchTurner MitchTurner changed the base branch from release/v0.40.0 to master October 17, 2024 14:53
@MitchTurner MitchTurner force-pushed the feature/do-not-increase-da-gas-price-if-no-activity branch from 6db49f4 to 6249d48 Compare October 17, 2024 15:06
@MitchTurner MitchTurner marked this pull request as ready for review October 17, 2024 15:32
@MitchTurner MitchTurner requested a review from a team October 17, 2024 15:33
netrome
netrome previously approved these changes Oct 17, 2024
Copy link
Contributor

@netrome netrome left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice stuff! A few suggestions from my end.

crates/fuel-gas-price-algorithm/src/v1.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.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.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.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.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.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@AurelienFT AurelienFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have an idea of the value you want to put in the threshold. You are saying that we should aim to be in normal range but right now on Ignition there is only 0 or 1 tx per block (excluding mint tx) and so the threshold seems hard to find given this granularity (0 or 1) and I'm having hard times to understand how we can maintain normal range in these conditions

crates/fuel-gas-price-algorithm/src/v1.rs Show resolved Hide resolved
@MitchTurner
Copy link
Member Author

Do you have an idea of the value you want to put in the threshold.

In a healthy network, I would want to keep it low, like 5% or 10%. We won't know for sure until we have a lot more data though.

You are saying that we should aim to be in normal range but right now on Ignition there is only 0 or 1 tx per block (excluding mint tx) and so the threshold seems hard to find given this granularity (0 or 1) and I'm having hard times to understand how we can maintain normal range in these conditions

This is a very good point. But safety mode only comes into play if we are trying to increase the gas price (we are unprofitable). Currently, the cost of posting blobs is so low that even if the DA gas price is set to the minimum, it will might still be profitable for us. Also, until the network gets more active, I think we should just swallow the costs of DA as a business. So, it's fine if most blocks are below the threshold.

AurelienFT
AurelienFT previously approved these changes Oct 21, 2024
Copy link
Contributor

@netrome netrome left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The L2ActivityTracker needs a small update to reflect the new parameters. Otherwise this looks good to me.

crates/fuel-gas-price-algorithm/src/v1.rs Outdated Show resolved Hide resolved
Co-authored-by: Mårten Blankfors <marten@blankfors.se>
AurelienFT
AurelienFT previously approved these changes Oct 21, 2024
netrome
netrome previously approved these changes Oct 21, 2024
@MitchTurner MitchTurner enabled auto-merge (squash) October 21, 2024 15:36
rafal-ch
rafal-ch previously approved these changes Oct 21, 2024
Copy link
Contributor

@rafal-ch rafal-ch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@MitchTurner MitchTurner self-assigned this Oct 22, 2024
Copy link
Contributor

@rafal-ch rafal-ch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-approving 👍

Copy link
Contributor

@AurelienFT AurelienFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even more test youhouuu

@MitchTurner MitchTurner merged commit a14922e into master Oct 24, 2024
32 of 33 checks passed
@MitchTurner MitchTurner deleted the feature/do-not-increase-da-gas-price-if-no-activity branch October 24, 2024 11:44
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

Successfully merging this pull request may close these issues.

6 participants