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

feat(bolt-sidecar): implement pricing models for preconfs #593

Merged
merged 7 commits into from
Dec 18, 2024
Merged

Conversation

estensen
Copy link
Contributor

@estensen estensen commented Dec 18, 2024

Implement pricing model from https://research.lido.fi/t/a-pricing-model-for-inclusion-preconfirmations/9136

Some limitations that we need to consider before using this model, is that the price of a preconf is both dependent on how much gas has already been preconfirmed for a slot and how much gas a preconf tx will consume:

Price race
If preconfirmations P1 and P2 race the, latter will use an outdated price because the price depends on how much gas has been preconfirmed. We don't want P2 to be rejected, as this would result in poor UX. It will also limit how many tx we can preconfirm based on latency.

We can do either or both slippage on the preconf tip or add some slack in the validation, so P2..Px would still get accepted with P1 price.

To avoid that the price that users gets is too outdated the entity that provides the price to users (or the users) needs to be aware of how much gas has been preconfirmed.

Preconfirmations that use a lot of gas could underpay
A preconf that consumes 170k gas should pay more than one that consumes a 17k gas. Currently, I'm not sure how this aligns with the existing wallet flow, put for a preconf req we need to know the gas that will be consumed.

Since price is calulated in steps, not as an integral, the 210k gas preconf would also pay less than 10x21k gas preconfs. While this discrepancy might not be significant, we need to be mindful of the oportunity cost for proposers.

Alternatively, users can just pay the worst preconf price and alway be included.

After this I plan to:

  • Avoid hardcoded gas limit as it might change soon
  • Start using pricing model in validation

impl PreconfPricing {
/// Initializes a new PreconfPricing with default parameters.
pub fn new() -> Self {
Self { block_gas_limit: 30_000_000, base_multiplier: 0.019, gas_scalar: 1.02e-6 }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Follow-up to remove hardcoded gas limit as it will likely change to 36M soon

@estensen estensen marked this pull request as ready for review December 18, 2024 14:21
let denominator = self.gas_scalar * (after_gas as f64) + 1.0;

// Calculate gas used
let expected_val = self.base_multiplier * (numerator / denominator).ln();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Consider using U256 to avoid using floats since it support ln

@estensen estensen merged commit fb00734 into unstable Dec 18, 2024
3 checks passed
@estensen estensen deleted the price branch December 18, 2024 15:14

impl Default for PreconfPricing {
fn default() -> Self {
Self::new(30_000_000)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you gate this to a constant?

impl PreconfPricing {
/// Initializes a new PreconfPricing with default parameters.
pub fn new(block_gas_limit: u64) -> Self {
Self { block_gas_limit, base_multiplier: 0.019, gas_scalar: 1.02e-6 }
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, can you add them in constants with a reference comment on top of each one?

bolt-sidecar/src/state/pricing.rs Show resolved Hide resolved
});
}

// T(IG,UG) = 0.019 * ln(1.02⋅10^-6(30M-UG)+1 / 1.02⋅10^-6(30M-UG-IG)+1) / IG
Copy link
Contributor

Choose a reason for hiding this comment

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

Reminder that these calculations can be performed directly using wei as unit of measure and not ether by multiplying the base_muliplier of 0.019 to 10**18. Then, you directly work with either u128 or U256. There is the U256::log function that should perform the natural logarithm of two 256 bit unsigned integers.

Comment on lines +115 to +121
// Pricing model article expects fee of 0.61 Gwei
assert!(
(min_fee_wei as f64 - 613_499_092.0).abs() < 1_000.0,
"Expected ~613,499,092 Wei, got {} Wei",
min_fee_wei
);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We should find out why we're getting a 0.02 gwei difference between this code and their model. Perhaps working in wei instead of ether can help

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Won't look into it now, but created an issue to address it later
#596

Comment on lines +171 to +173
// This will likely never happen, since you want to reserve some gas
// on top of the block for MEV, but enforcing this is not the responsibility
// of the pricing model.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd drop these types of comments since this might not be the intention of every proposer.

Comment on lines +174 to +179
assert!(
(min_fee_wei as f64 - 19_175_357_339.0).abs() < 1_000.0,
"Expected ~19,175,357,339 Wei, got {} Wei",
min_fee_wei
);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd add a disclaimer that these checks are mainly for tracking down whether some changes in the algo are highly different compared to previous result, but these target values are not to be considered "correct" since they have been produced with this function in the beginning.

@thedevbirb
Copy link
Contributor

Added some comments!

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.

3 participants