Implement ACP-77 fee calculations #3367

StephenButtolph · 2024-09-06T05:49:11Z

Why this should be merged

The fees logic is factored out of #3287.

How this works

ACP-77 charges validators fees over time. While it may be possible to solve for these fees directly using integrals, the approximation function makes performing this optimization difficult.

Therefore, this PR does not attempt to calculate the fees in constant time (w.r.t. the duration). Rather, it permits falling back to a linear iteration (based on the duration of the fee charge). This is fine for our performance needs, as the duration is limited to the number of seconds between blocks on the chain and calculating weeks of fees takes milliseconds.

There are some special cases that are further optimized.

There is also a derivation of the excessConversionConstant based on the capacity, target, and minimumTimeToDouble.

How this was tested

New unit tests
New benchmarks
Both CalculateCost and CalculateDuration have "naive" implementations that were used to fuzz the optimized implementations.

Signed-off-by: Dhruba Basu <7675102+dhrubabasu@users.noreply.github.com>

vms/platformvm/validators/fee/fee_test.go

vms/platformvm/validators/fee/fee.go

StephenButtolph · 2024-09-08T01:38:39Z

TODO: Add dhruba as co-author.

vms/platformvm/validators/fee/fee_test.go

I'm not qualified to review this.

ARR4N

I need to concentrate on the tests and there's currently construction outside my window so I'm sending the rest of the review early.

ARR4N · 2024-09-10T16:55:27Z

vms/platformvm/validators/fee/fee.go

+
+// CalculateExcess updates the excess value based on the target and current
+// usage over the provided duration.
+func CalculateExcess(


(style) You can collapse the parameter types instead of splitting over many lines.

func CalculateExcess(target, current, excess gas.Gas, duration uint64) {

(not just style) That said, I worry when there are too many of the same type in a row that the call site can become ambiguous and bugs slip in. Since these params are common to all three functions, should you maybe create a struct and make them methods on it?

type Context struct { Target, Current, Excess gas.Gas } func (c Context) CalculateExcess(duration uint64) gas.Gas {

Presumably you're going to pass the excess back in every time so the struct might actually work nicely because you can go one step further:

func (c *Context) UpdateExcess(duration uint64) { // note nothing returned

The name is fairly arbitrary but fee.Context felt accurate and descriptive.

The thing that is a bit weird is: target, minPrice, and excessConversionConstant are all essentially constants. current and excess are dynamic (and persisted to state). duration, maxDuration, and targetCost are the "real" arguments for these functions.

I had considered having:

type Config struct { Target gas.Gas MinPrice gas.Price ExcessConversionConstant gas.Gas } type State struct { Current gas.Gas Excess gas.Gas }

But then we'd need to be passing both of these into CalculateExcess... I'll try to play around with some things to see if I can find something I don't dislike

ARR4N · 2024-09-10T17:01:28Z

vms/platformvm/validators/fee/fee.go

+	target gas.Gas,
+	current gas.Gas,
+	excess gas.Gas,
+	duration uint64,


The unit of duration is only known to be seconds once you look inside the code, so I'd just call it seconds everywhere.

ARR4N · 2024-09-10T17:07:22Z

vms/platformvm/validators/fee/fee.go

+
+// CalculateCost calculates how much to charge based on the dynamic fee
+// mechanism for [duration].
+func CalculateCost(


(optional) The Calculate prefix feels a bit redundant and reminds me of this recommendation re getters. If you change to the GasContext struct then you could call the method CostOf()?

ARR4N · 2024-09-10T17:09:31Z

vms/platformvm/validators/fee/fee.go

+		price := gas.CalculatePrice(minPrice, excess, excessConversionConstant)
+		cost, err := safemath.Mul(duration, uint64(price))
+		if err != nil {
+			return math.MaxUint64


I'll see your error and raise you an exorbitant transaction cost!

This is actually a discount! (because this is just capping the cost at MaxUint64 if it would have overflowed)

ARR4N · 2024-09-11T11:07:07Z

vms/platformvm/validators/fee/fee.go

+	)
+	for i := uint64(0); i < duration; i++ {
+		excess = CalculateExcess(target, current, excess, 1)
+		// If the excess is 0, the price will remain constant for all future


This took a long time for me to grok (including asking you in a DM) so I think it's ok to be verbose in the explanation. Here's a first draft of my understanding that you can copy and tweak:

The change in excess is positively correlated with the difference between current and target. It is also strictly bounded in [0,2^64) with (under/over)flows being clipped. As we know that current != target, excess would be strictly increasing or strictly decreasing if it weren't for clipping. However, when coupled with clipping, this guarantees that a zero excess will remain zero for all future iterations.

ARR4N · 2024-09-11T11:15:53Z

vms/platformvm/validators/fee/fee.go

+			return duration
+		}
+	}
+	return duration


This will always be maxDuration so I think it should be explicit.

This is not always maxDuration. (The tests will fail if this is changed to maxDuration). The loop exits if duration == maxDuration or if cost >= targetCost

ARR4N · 2024-09-11T11:17:49Z

vms/platformvm/validators/fee/fee.go

+			return duration
+		}
+
+		duration++


I think this should be in the loop. As far as I can tell, the only branch it would affect would be the error on safemath.Add(cost...).

for ; cost < targetCost && duration < maxDuration; duration++ {

Having:

if err != nil { return duration + 1 }

feels pretty weird to me... Personally I find the code as-is more readable... but if you have conviction then I'm fine moving the increment around.

ARR4N · 2024-09-11T11:17:52Z

vms/platformvm/validators/fee/fee.go

+		price := gas.CalculatePrice(minPrice, excess, excessConversionConstant)
+		cost, err = safemath.Add(cost, uint64(price))
+		if err != nil {
+			return duration


Why duration and not maxDuration?

We are looking for the duration where cost >= targetCost. cost would be pinned to MaxUint64 here - so we know that cost is >= any possible targetCost at this duration.

ARR4N · 2024-09-11T11:18:57Z

vms/platformvm/validators/fee/fee_test.go

+	hour   = 60 * minute
+	day    = 24 * hour
+	week   = 7 * day
+	year   = 365 * day


Not 525,600 minutes?

I'm not sure if this comment is implying that year should be set to 52 * week rather than 365 * day or not haha... But the rest of the P-chain uses this definition of years.

The chain doesn't really need to care about any leap seconds or leap years... We can set our own time cadence... We use this definition of a year for our maximum staking duration (among other things)

vms/platformvm/validators/fee/fee_test.go

ARR4N · 2024-09-11T13:18:04Z

vms/platformvm/validators/fee/fee_test.go

+		name                string
+		target              gas.Gas
+		current             gas.Gas
+		excess              gas.Gas


excessAtStart?

Been folded into state.

ARR4N · 2024-09-11T13:18:50Z

vms/platformvm/validators/fee/fee_test.go

+			excess:              0,
+			duration:            minute,
+			cost:                122_880,
+			excessAfterDuration: 0,


Because of no underflow? If so, please add that to the test name.

I think the test name is accurate... I added a comment on expectedExcess

ARR4N · 2024-09-11T13:22:57Z

vms/platformvm/validators/fee/fee_test.go

+		excess              gas.Gas
+		duration            uint64
+		cost                uint64
+		excessAfterDuration gas.Gas


(nit) want or expect prefix to signal early on to the reader. I had to go to the test implementation and then come back here to be sure that I understood.

The same probably applies to cost even though it's plugged into CalculateDuration because that function is effectively just an inversion.

I had avoided expected because they are used as both inputs and outputs... I switched to use expected and commented that they are used for both.

vms/platformvm/validators/fee/fee_test.go

ARR4N · 2024-09-11T14:17:02Z

vms/platformvm/validators/fee/fee_test.go

+			current:             10,
+			excess:              0,
+			duration:            minute,
+			cost:                122_880,


(no action required) I'm working on the assumption that you've correctly calculated these from a canonical source.

The "canonical source" is:

def cost_over_time(current:int, target:int, excess:int, Δt: int) -> int: cost = 0 for _ in range(Δt): excess = max(excess + current - target, 0) cost += fake_exponential(minPrice, excess, conversionConstant) return cost

See: avalanche-foundation/ACPs#119

ARR4N

LGTM!

ARR4N · 2024-09-12T14:54:45Z

vms/platformvm/validators/fee/fee.go

+	} else if s.Current > target {
+		excess = excess.AddPerSecond(s.Current-target, seconds)
+	}
+	return State{


Is this because we were talking about functional languages? 🤣

It's actually because we might be passing these through different block states... And we definitely don't want them to be mutable there

ARR4N · 2024-09-12T14:56:13Z

vms/platformvm/validators/fee/fee.go

 }

-// CalculateDuration calculates the duration that it would take to charge at
+// SecondsUntil calculates the number of seconds that it would take to charge at


Nice naming! I love code that reads like prose.

dhrubabasu added 30 commits June 18, 2024 11:20

[vms/platformvm] Add tracking of a Subnet manager

1857cb9

Merge branch 'master' into store-warp-addressed-call-pair

4d11a29

Update vms/platformvm/state/diff.go

90a38ff

Signed-off-by: Dhruba Basu <7675102+dhrubabasu@users.noreply.github.com>

nit

f60e4ea

nit

1ca0caf

nit

1cd2428

nit

3587c32

[vms/platformvm] Cleanup execution config tests

ec16c11

Merge branch 'master' into cleanup-execution-config-test

f52cbbe

nit

076b1f0

nit

df5ca64

Merge branch 'master' into cleanup-execution-config-test

dcc056b

nit

3279764

mark as helper

911a167

Merge branch 'master' into cleanup-execution-config-test

ddf3392

merged

25802a8

reduce diff

01b854a

add write

f79e0c7

merged

1eda29c

nits

3fa1306

nit

16ffe7b

nits

3035064

Merge branch 'master' into store-warp-addressed-call-pair

62ece94

fix UT

333a41e

add UTs

520ac91

lint

db5f197

nit

574a120

nit

501beb1

add test for cache

ab5c23b

Merge branch 'master' into store-warp-addressed-call-pair

e0d0a00

StephenButtolph added this to the v1.11.12 milestone Sep 6, 2024

StephenButtolph self-assigned this Sep 6, 2024

StephenButtolph added 2 commits September 6, 2024 01:54

fix test

2bc3f86

fix comment

1a8e5f2

aaronbuchwald approved these changes Sep 6, 2024

View reviewed changes

vms/platformvm/validators/fee/fee_test.go Outdated Show resolved Hide resolved

vms/platformvm/validators/fee/fee.go Outdated Show resolved Hide resolved

StephenButtolph added 7 commits September 6, 2024 10:48

comment grammer

29af8da

Implement function

77a9820

Merge branch 'implement-acp-77-fees' into sov-data-struct

471871f

merge fee implementation

a490641

Add CalculateExcess tests

62a2ae1

Merge branch 'implement-acp-77-fees' into sov-data-struct

c780c34

Remove validator state struct

5af7744

marun previously approved these changes Sep 9, 2024

View reviewed changes

vms/platformvm/validators/fee/fee_test.go Outdated Show resolved Hide resolved

Add dhruba as co-author

471b03e

ARR4N reviewed Sep 11, 2024

View reviewed changes

👀

8e468ab

ARR4N requested changes Sep 11, 2024

View reviewed changes

StephenButtolph added 3 commits September 11, 2024 10:52

Address most suggestions

7940741

add additional tests

d642d1d

Make exported structs pretty

605aa61

StephenButtolph requested a review from ARR4N September 11, 2024 15:48

ARR4N approved these changes Sep 12, 2024

View reviewed changes

add comment

e5ef65d

StephenButtolph enabled auto-merge September 12, 2024 15:32

StephenButtolph added this pull request to the merge queue Sep 12, 2024

Merged via the queue into master with commit df3b4fa Sep 12, 2024
20 of 21 checks passed

StephenButtolph deleted the implement-acp-77-fees branch September 12, 2024 16:02

Implement ACP-77 fee calculations #3367

Implement ACP-77 fee calculations #3367

Uh oh!

Conversation

StephenButtolph commented Sep 6, 2024

Why this should be merged

How this works

How this was tested

Uh oh!

Uh oh!

Uh oh!

StephenButtolph commented Sep 8, 2024

Uh oh!

Uh oh!

ARR4N left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

StephenButtolph Sep 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ARR4N left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

StephenButtolph Sep 11, 2024 •

edited

Loading