Skip to content

feat(protocol): tract avgProverMarketFee and cap bids with the average #19260

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

Closed
wants to merge 13 commits into from

Conversation

dantaik
Copy link
Contributor

@dantaik dantaik commented Apr 13, 2025

No description provided.

@dantaik dantaik marked this pull request as ready for review April 13, 2025 04:03
@dantaik dantaik changed the title feat(protocol): restrict bid to a max fee feat(protocol): tract avgProverMarketFee and cap bids with the average Apr 13, 2025
Comment on lines +217 to +223
stats1 = state.stats1; // load from storage
stats1.avgProverMarketFee = uint64(
(
(proverMarketFeeChangeFactor - 1) * stats1.avgProverMarketFee
+ uint64(proverFee / (1 gwei))
) / proverMarketFeeChangeFactor
);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is a bit "on-chain heavy". I mean, this code is done when there is a proposal. While the fee only changes when there is an incoming bid. (at least with prover market).
I propose a very similar approach, but inside ProverMarket, and just the cycle of 4 fees can give a good, rough starting point to calculate the max threshold.

See this PR: #19254

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the moving average is updated by bids in the prover market, then the bidder can manipulate the average. In this PR, the moving average is only updated when the fee is actually used by a preconfer, avoiding such manipulation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe these two average can work together.

Choose a reason for hiding this comment

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

I agree that if we allow the average to naively change in the bid there's an option for the prover to manipulate that. You put the max bid possible, then submit a bunch of other bids undercutting you by the minimum possible in the same block effectively pushing the moving average higher, and then repeat the process with a higher number. Admittedly, this is a bit elaborated and would cost significant L1 gas fees.
I think if we want to eliminate the calculation in the proposeBatch function, one option we have is to only update the moving average once per block(or every X blocks). This way the average still changes slowly and gives enough time for other provers(or Taiko) to come in and bid a reasonable amount. The downside is that we would have to store an extra uint64, but significantly reduces the cost in the inbox(which is what should be called more often) since we don't have to do an SSTORE anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See #19264

@dantaik dantaik requested a review from adaki2004 April 13, 2025 09:07
Copy link

@ggonzalez94 ggonzalez94 left a comment

Choose a reason for hiding this comment

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

I like this approach. Left a few suggestions, the most important one being where the moving average fee is calculated and stored

Comment on lines +90 to +91
require(_fee % feeUnit == 0, FeeNotDivisibleByFeeUnit());
require(_fee / feeUnit <= type(uint64).max, FeeTooLarge());

Choose a reason for hiding this comment

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

Wouldn't it be simpler if the bid is already expressed in feeUnit(gwei) as a uint64. I think having different representations(what people use to bid vs how fees are represented internally) leads to more complexity and might even lead to more confusion

Choose a reason for hiding this comment

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

Alternatively if you represent everything with a uint96 should be enough to represent all Taiko supply, but I think you already had a reserved uint64 in the struct that you are reusing, so likely not possible

Comment on lines +217 to +223
stats1 = state.stats1; // load from storage
stats1.avgProverMarketFee = uint64(
(
(proverMarketFeeChangeFactor - 1) * stats1.avgProverMarketFee
+ uint64(proverFee / (1 gwei))
) / proverMarketFeeChangeFactor
);

Choose a reason for hiding this comment

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

I agree that if we allow the average to naively change in the bid there's an option for the prover to manipulate that. You put the max bid possible, then submit a bunch of other bids undercutting you by the minimum possible in the same block effectively pushing the moving average higher, and then repeat the process with a higher number. Admittedly, this is a bit elaborated and would cost significant L1 gas fees.
I think if we want to eliminate the calculation in the proposeBatch function, one option we have is to only update the moving average once per block(or every X blocks). This way the average still changes slowly and gives enough time for other provers(or Taiko) to come in and bid a reasonable amount. The downside is that we would have to store an extra uint64, but significantly reduces the cost in the inbox(which is what should be called more often) since we don't have to do an SSTORE anymore.

@dantaik
Copy link
Contributor Author

dantaik commented Apr 14, 2025

Now I prefer #19264 over this one.

@dantaik dantaik marked this pull request as draft April 14, 2025 02:23
@dantaik dantaik closed this Apr 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants