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.

Sign up for GitHub

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

Jump to bottom

Fix unit fees dynamics #732

Closed
wants to merge 2 commits into from
Closed

Fix unit fees dynamics #732

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
657d722
fixed unit fees change
abi87 Feb 14, 2024
bca0f15
nits
abi87 Feb 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 29 additions & 34 deletions fees/manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,10 +213,10 @@ func (f *Manager) UnitsConsumed() Dimensions {
func computeNextPriceWindow(
previous window.Window,
previousConsumed uint64,
previousPrice uint64,
target uint64, /* per window */
previousUnitFee uint64,
target uint64, /* per window, must be non-zero */
changeDenom uint64,
minPrice uint64,
minUnitFee uint64,
since int, /* seconds */
) (uint64, window.Window, error) {
newRollupWindow, err := window.Roll(previous, since)
Expand All @@ -231,53 +231,48 @@ func computeNextPriceWindow(
start := slot * consts.Uint64Len
window.Update(&newRollupWindow, start, previousConsumed)
}
total := window.Sum(newRollupWindow)

nextPrice := previousPrice
if total > target {
var (
totalUnitsConsumed = window.Sum(newRollupWindow)
nextUnitFee = previousUnitFee
)
switch {
case totalUnitsConsumed > target:
// If the parent block used more units than its target, the baseFee should increase.
delta := total - target
x := previousPrice * delta
y := x / target
baseDelta := y / changeDenom
if baseDelta < 1 {
baseDelta = 1
}
n, over := math.Add64(nextPrice, baseDelta)
rawDelta := previousUnitFee * (totalUnitsConsumed - target) / target
delta := max(rawDelta/changeDenom, 1) * changeDenom // price must change in increments on changeDenom
Copy link
Author

Choose a reason for hiding this comment

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

This is the main point: delta here is ensured to be a multiple of changeDenom. Moverover, from a dimensional analysis standpoint, delta is a "token amont" quantity, which can be added to unit fees.
In the previous version baseDelta is a dimensionless quantity (ration of two fees) which cannot be added to a dimensioned quantity like next price

Copy link
Author

@abi87 abi87 Feb 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This is actually wrong: this code comes from coreth (see coreth code) which in turns comes from EIP1559 proposal (see for instance ethereum/EIPs#1559 for the original meaning of changeDenom and https://eips.ethereum.org/EIPS/eip-1559 for how it was evolved from there).

The goal of changeDenom is not to make sure that delta happens in multiples of the changeDenom.


var over error
nextUnitFee, over = math.Add64(nextUnitFee, delta)
if over != nil {
nextPrice = consts.MaxUint64
} else {
nextPrice = n
nextUnitFee = consts.MaxUint64
}
} else if total < target {

case totalUnitsConsumed < target:
// Otherwise if the parent block used less units than its target, the baseFee should decrease.
delta := target - total
x := previousPrice * delta
y := x / target
baseDelta := y / changeDenom
if baseDelta < 1 {
baseDelta = 1
}
rawDelta := previousUnitFee * (target - totalUnitsConsumed) / target
delta := max(rawDelta/changeDenom, 1) * changeDenom // price must change in increments on changeDenom

// If [roll] is greater than [rollupWindow], apply the state transition to the base fee to account
// for the interval during which no blocks were produced.
// We use roll/rollupWindow, so that the transition is applied for every [rollupWindow] seconds
// that has elapsed between the parent and this block.
if since > window.WindowSize {
// Note: roll/rollupWindow must be greater than 1 since we've checked that roll > rollupWindow
baseDelta *= uint64(since / window.WindowSize)
delta *= uint64(since / window.WindowSize)
}
n, under := math.Sub(nextPrice, baseDelta)

var under error
nextUnitFee, under = math.Sub(nextUnitFee, delta)
if under != nil {
nextPrice = 0
} else {
nextPrice = n
nextUnitFee = 0
}
case totalUnitsConsumed == target:
return nextUnitFee, newRollupWindow, nil
}
if nextPrice < minPrice {
nextPrice = minPrice
}
return nextPrice, newRollupWindow, nil

nextUnitFee = max(nextUnitFee, minUnitFee)
return nextUnitFee, newRollupWindow, nil
}

func Add(a, b Dimensions) (Dimensions, error) {
Expand Down
Loading