BM-2408: Use consistent base fee and priority fee estimations#1612
BM-2408: Use consistent base fee and priority fee estimations#1612
Conversation
austinabell
requested review from
a team,
Wollac,
capossele,
ec2,
jonastheis,
nahoc,
unizarr,
willemolding and
zeroecco
as code owners
github-actions
bot
changed the title
| let gas_price: u128 = PriorityMode::default() | ||
| .estimate_max_fee_per_gas(&self.provider) | ||
| .await | ||
| .map_err(|err| anyhow::anyhow!("Failed to estimate gas price: {err:?}"))?; |
There was a problem hiding this comment.
@capossele this could increase the gas estimation (but will be closer to what provers will price in). Wondering if worth to reduce the gas cost estimate. Right now 2x seems like it might be a lot in gas spikes, where this value should be roughly the gas price slippage tolerance. Do you think we should reduce that factor and if so do you have an intuition of what it should be?
There was a problem hiding this comment.
Yeah setting that at 2x might indeed not be the right solution to account for potential future periods of higher gas prices. Maybe we should only add an additional 10 or 20% to that estimate.
And trying to include some extra buffer in the max_price instead. But this last comment should belong to a different PR
There was a problem hiding this comment.
Ah I thought #1579 had already come in. I think that strategy is reasonable, or we could have this use PriorityMode::High to be this estimation?
I'm quite unopinionated about this specific change, so let me know what you think an ideal strategy would be and I can change this PR to reflect that
There was a problem hiding this comment.
High would use the 50th percentile, yeah that's ok
There was a problem hiding this comment.
Inferring that you meant to use this high value and remove the multiplier, if that's not what you meant, let me know!
|
draft because I think gas buffer might need to be increased here for requestors to handle gas price fluctuations better |
Base fulfill cost and gas per journal byte pulled from transactions on mainnet and simulations on testnet. Builds on #1612
willpote
left a comment
willpote
left a comment
There was a problem hiding this comment.
We get gas price a lot in the broker, and now we're moving to a different endpoint for it which could have different performance.
I think is definitely one we should get this to staging + prod asap so we can leave running for a few days and see if it has any impact.
Also curious if this new gas price endpoint will change rpc costs?
| if order_gas_cost > U256::from(order.request.offer.maxPrice) && !lock_expired { | ||
| return Ok(Skip { | ||
| reason: format!( | ||
| "estimated gas cost to lock and fulfill order of {} exceeds max price of {}", |
There was a problem hiding this comment.
lets add "after accounting for journal costs" to distinguish the two cases
3 participants
removes all non-test usages of calls internally made to
eth_gasPriceand adds a test to ensure the estimate is >= the actual gas usage.