-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Changes from all commits
3226ae0
aa930ae
36d8171
01e7a1b
4ccffdc
5bfe2aa
3fd6c44
e075562
7baa787
a8ad1b5
2446ff3
30e934d
b7ab3aa
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,6 +18,7 @@ contract ProverMarket is EssentialContract, IProverMarket { | |
error InvalidThresholds(); | ||
error NotCurrentProver(); | ||
error FeeNotDivisibleByFeeUnit(); | ||
error FeeBiggerThanMax(); | ||
error FeeTooLarge(); | ||
error TooEarly(); | ||
|
||
|
@@ -34,6 +35,8 @@ contract ProverMarket is EssentialContract, IProverMarket { | |
uint256 public immutable provingThreshold; | ||
/// @dev The minimum delay required before a prover can exit the prover market. | ||
uint256 public immutable minExitDelay; | ||
/// @dev The unit of fee to make sure fee can fit into uint64 | ||
uint256 public immutable feeUnit; | ||
|
||
/// @dev Slot 1 | ||
address internal prover; | ||
|
@@ -84,9 +87,12 @@ contract ProverMarket is EssentialContract, IProverMarket { | |
external | ||
validExitTimestamp(_exitTimestamp) | ||
{ | ||
require(_fee % (1 gwei) == 0, FeeNotDivisibleByFeeUnit()); | ||
require(_fee / (1 gwei) <= type(uint64).max, FeeTooLarge()); | ||
uint64 fee_ = uint64(_fee / (1 gwei)); | ||
require(_fee % feeUnit == 0, FeeNotDivisibleByFeeUnit()); | ||
require(_fee / feeUnit <= type(uint64).max, FeeTooLarge()); | ||
Comment on lines
+90
to
+91
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Alternatively if you represent everything with a |
||
|
||
uint256 maxFee = getMaxFee(); | ||
require(maxFee == 0 || _fee <= maxFee, FeeBiggerThanMax()); | ||
uint64 fee_ = uint64(_fee / feeUnit); | ||
|
||
require(inbox.bondBalanceOf(msg.sender) >= biddingThreshold, InsufficientBondBalance()); | ||
|
||
|
@@ -113,7 +119,7 @@ contract ProverMarket is EssentialContract, IProverMarket { | |
onlyCurrentProver | ||
{ | ||
exitTimestamps[msg.sender] = _exitTimestamp; | ||
emit ProverChanged(msg.sender, 1 gwei * fee, _exitTimestamp); | ||
emit ProverChanged(msg.sender, feeUnit * fee, _exitTimestamp); | ||
} | ||
|
||
/// @inheritdoc IProverMarket | ||
|
@@ -122,10 +128,16 @@ contract ProverMarket is EssentialContract, IProverMarket { | |
_getCurrentProver(); | ||
return currentProverBalance < provingThreshold | ||
? (address(0), 0) | ||
: (currentProver, 1 gwei * currentFee); | ||
: (currentProver, feeUnit * currentFee); | ||
} | ||
|
||
/// @dev The maximum fee that can be used by provers to bid. | ||
/// The current implementation returns a 5 times of the moving average. | ||
function getMaxFee() public view returns (uint256) { | ||
return 5 gwei * inbox.getStats1().avgProverMarketFee; | ||
} | ||
|
||
function _getCurrentProver() public view returns (address, uint64, uint256) { | ||
function _getCurrentProver() internal view returns (address, uint64, uint256) { | ||
address currentProver = prover; | ||
if ( | ||
currentProver == address(0) // no bidding | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,5 @@ | ||
See `test_inbox_measure_gas_used` in InboxTest_ProposeAndProve.t.sol | ||
|
||
Gas per proposeBatches: 680644 | ||
Gas per proveBatches: 51267 | ||
Total: 731911 | ||
Gas per proposeBatches: 680858 | ||
Gas per proveBatches: 50934 | ||
Total: 731792 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 extrauint64
, but significantly reduces the cost in the inbox(which is what should be called more often) since we don't have to do anSSTORE
anymore.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #19264