feat(inflation): Implement Akash custom inflation function according to the whitepaper. by arijitAD · Pull Request #1352 · akash-network/node

arijitAD · 2021-08-23T11:15:10Z

Description

Motivation and Context

I have raised an issue to propose this change (required)
My issue has received approval from the maintainers or lead with the design/approved label

How Has This Been Tested?

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I've read the CONTRIBUTION guide
I have signed-off my commits with git commit -s
I have added tests to cover my changes.
All new and existing tests passed.

codecov · 2021-08-23T11:23:17Z

Codecov Report

Merging #1352 (50ac384) into master (9dbc53f) will increase coverage by 4.71%.
The diff coverage is 72.41%.

❗ Current head 50ac384 differs from pull request most recent head ededc6f. Consider uploading reports for the commit ededc6f to get more accurate results

@@            Coverage Diff             @@
##           master    #1352      +/-   ##
==========================================
+ Coverage   38.23%   42.94%   +4.71%     
==========================================
  Files         347      276      -71     
  Lines       20047    15528    -4519     
==========================================
- Hits         7664     6668     -996     
+ Misses      11552     8115    -3437     
+ Partials      831      745      -86

boz · 2021-08-27T00:42:17Z

app/app.go

+		panic(err)
+	}
+	minInflation := minter.Inflation.Sub(minter.Inflation.Mul(r))
+	maxInflation := minter.Inflation.Add(minter.Inflation.Mul(r))


There are more parameters to consider. In the default cosmos inflation calculation , there is:

current bonded ratio

target bonded ratio

max inflation rate change

We should continue to use these. My thought was to have:

I(t) is the "ideal" inflation: ideal_inflation.

If bonded_ratio=desired_ratio then target_inflation=ideal_inflation.

If bonded_ratio<desired_ratio then target_inflation=ideal_inflation + adjustment.

If bonded_ratio>desired_ratio then target_inflation=ideal_inflation - adjustment.

max_inflation=I(t)*(1 + 0.05), min_inflation=I(t)*(1 - 0.05)

Change current_inflation to target_inflation, bound by both max_inflation_change and {min,max}_inflation.

Is minter.Inflation eventually set to the return value of this function?

We should continue to use these. My thought was to have:

I(t) is the "ideal" inflation: ideal_inflation.

If bonded_ratio=desired_ratio then target_inflation=ideal_inflation.

If bonded_ratio<desired_ratio then target_inflation=ideal_inflation + adjustment.

If bonded_ratio>desired_ratio then target_inflation=ideal_inflation - adjustment.

By adjustment did you mean inflationRateChange as in the new commit?

Is minter.Inflation eventually set to the return value of this function?

Yes, that's why I am calculating {min, max}_inflation based on minter.Inflation instead of ideal_inflation. Otherwise, there seems no point of applying {min, max} logic using ideal_inflation as the base for calculating {min, max}.

I guess the issue I have is: does using idealInflation + inflationRateChange overly restrict the possible range? If so then the entire range of acceptable inflation rates isn't available.

I think that we should use idealInflation to get [max,min], and calculate currentInflation = minter.Inflation + inflationRateChange. This way it will continue moving until an acceptable bonded ratio is found (in theory).

We also need to make a minimum minInflation - it can be 0 for now.

I think that we should use idealInflation to get [max,min], and calculate currentInflation = minter.Inflation + inflationRateChange. This way it will continue moving until an acceptable bonded ratio is found (in theory).

We also need to make a minimum minInflation - it can be 0 for now.

Done

boz · 2021-08-27T00:56:22Z

app/app.go


+func init() {
+	var err error
+	genesisTimeStr := "2020-09-25T14:00:00Z"


Might want to make this a var so that it can be set at build time. Would prefer to not have a global genesisTime, but it's okay for now.

github-actions · 2021-09-07T00:01:57Z

Marked as stale; will be closed in five days.

Cut bait or go fishing!

arijitAD · 2021-10-27T16:24:24Z

go.mod


 replace (
-	github.com/cosmos/cosmos-sdk => github.com/ovrclk/cosmos-sdk v0.43.0-patches
+	github.com/cosmos/cosmos-sdk => github.com/ovrclk/cosmos-sdk v0.42.0-alpha1.0.20210909100325-11e54c8fc931


The latest master uses v0.44.1-patches branch. Should we wait for cosmos/cosmos-sdk#10441 to get released or should we just make the changes required for custom inflation function in our v0.44.1-patches branch so that we can go ahead with merging this PR?

Hmm, let's carry the patch over into v0.44.4-patches for now.

boz

Looking good. Some comments inline.

boz · 2021-10-27T18:22:45Z

app/app.go

+func init() {
+	var err error
+
+	genesisTime, err = time.Parse(time.RFC3339, GenesisTimeStr)


This should be passed in; it's defined in the genesis and will be different for different chains (testnet, etc...)

Done. Reading from genesis file now.

boz · 2021-10-27T18:26:23Z

app/app.go


+func inflationCalculator(ctx sdk.Context, minter minttypes.Minter, params minttypes.Params, bondedRatio sdk.Dec) sdk.Dec {
+	i0 := 100.0
+	t := ctx.BlockTime().Sub(genesisTime).Seconds() / (60 * 60 * 8766) // years passed


Should t be an integer number of years since genesis?

It's hard to understand where the 60*60*8766 comes from.

I think we should use days since genesis.

Both t and tHalf have been defined in years in the whitepaper. It doesn't matter whether we use days or years to represent both of them, we are finally calculating t/tHalf which is a float value with no units.

Added an explanation for 60*60*8766

But t will be a severe step function at each year passing, instead of a more incremental daily increase.

No, t is a float value. It isn't an int, so it won't be a step function. Float value will increase continuously, unlike int.
i.e., t can represent 0.5 years. It won't go to 1 year directly from 0 years.

boz · 2021-10-27T18:30:23Z

app/app.go

 const (
 	AppName = "akash"
+
+	tHalf = 2 // years


This should be an on-chain parameter.

Made it a parameter of deployment module. There is no way to extend the parameters of the mint module.

It is not a clean way of handling this parameter, but the other way would mean creating a new module just to store this parameter.

boz · 2021-10-27T18:33:05Z

app/app.go

 }

+func inflationCalculator(ctx sdk.Context, minter minttypes.Minter, params minttypes.Params, bondedRatio sdk.Dec) sdk.Dec {
+	i0 := 100.0


this is unnecessary; use a comment with 100.0 when it's used.

boz

Looking great.

Let's not pollute x/deployment - make a new module for it (x/inflation), and put the actual inflation calculation in there as well.

I commented inline re a couple more parameters that we should make driven by governance.

boz · 2021-10-30T04:56:06Z

go.mod


 replace (
-	github.com/cosmos/cosmos-sdk => github.com/ovrclk/cosmos-sdk v0.43.0-patches
+	github.com/cosmos/cosmos-sdk => github.com/ovrclk/cosmos-sdk v0.42.0-alpha1.0.20210909100325-11e54c8fc931


Hmm, let's carry the patch over into v0.44.4-patches for now.

boz · 2021-10-30T04:56:36Z

app/app.go

+	// Number of seconds in a minute = 60
+	// => 60 * 60 * 8766 = Number of seconds per year
+	t := ctx.BlockTime().Sub(genesisTime).Seconds() / (60 * 60 * 8766) // years passed
+	i := 100.0 * math.Pow(2, -t/tHalf)                                 // 100 = initial inflation (i0)


Initial inflation was not 100%. Let's make this a parameter as well.

boz · 2021-10-30T04:56:55Z

app/app.go

+
+	// min, max currentInflation based on a defined range parameter 'r'
+	// currentInflation range = [I(t) - I(t) * R, I(t) + I(t) * R]
+	r, err := sdk.NewDecFromStr("0.05") // let's say we allow 5% variance


Let's make the variance a parameter too.

app/app.go

arijitAD · 2021-11-02T14:16:32Z

app/export.go:232:7: G404: Use of weak random number generator (math/rand instead of crypto/rand) (gosec)
	r := rand.New(rand.NewSource(seed))

This lint failure can't be fixed. The function from cosmos-sdk that we need to use, accepts only math/rand and not crypto/rand. So, we have no choice. Also, it is used only in tests. Doesn't affect security in any sensible way.

boz · 2021-11-04T14:55:08Z

app/export.go:232:7: G404: Use of weak random number generator (math/rand instead of crypto/rand) (gosec)
	r := rand.New(rand.NewSource(seed))
This lint failure can't be fixed. The function from cosmos-sdk that we need to use, accepts only math/rand and not crypto/rand. So, we have no choice. Also, it is used only in tests. Doesn't affect security in any sensible way.

you can disable the check for that line with // nolint: gosec like here

boz · 2021-11-04T14:55:26Z

Please rebase against master.

arijitAD · 2021-11-05T07:17:11Z

you can disable the check for that line with // nolint: gosec like here

Thanks, Done.

boz · 2021-11-10T05:28:36Z

app/app.go

+	if v := appOpts.Get("GenesisTime"); v != nil {
+		// in tests, GenesisTime is supplied using appOpts
+		genTime, ok := v.(time.Time)
+		if !ok {
+			panic("expected GenesisTime to be a Time value")
+		}
+		inflationtypes.GenesisTime = genTime
+	} else if genDoc, err := tmtypes.GenesisDocFromFile(filepath.Join(homePath, "config/genesis.json")); err != nil {
+		panic(err)
+	} else {
+		inflationtypes.GenesisTime = genDoc.GenesisTime
+	}


Put this in a helper function that returns the genesis time.

Please don't use global variables for state. Find another way to pass the parameters into the inflation calculation.

x/inflation/alias.go

boz · 2021-11-10T05:30:50Z

x/inflation/types/v1beta2/inflation_calculator.go

+	InflationParamSubspace paramstypes.Subspace
+)
+
+func InflationCalculator(ctx sdk.Context, minter minttypes.Minter, params minttypes.Params, bondedRatio sdk.Dec) sdk.Dec {


agreed. in particular, rounding issues will likely cause different nodes to produce different values.

boz · 2021-11-10T05:35:21Z

x/inflation/types/v1beta2/inflation_calculator.go

+	var inflationParams Params
+	InflationParamSubspace.GetParamSet(ctx, &inflationParams)
+	tHalf := float64(inflationParams.InflationDecayFactor)
+	initialInflation, err := strconv.ParseFloat(inflationParams.InitialInflation, 32)


Params should be represented by the appropriate type here. See how x/deploy does it here.

I agree with @hydrogen18 - we should use a decimal or rational here.

boz · 2021-11-10T05:37:41Z

x/inflation/types/v1beta2/inflation_calculator.go

+	// Number of minutes in an hour = 60
+	// Number of seconds in a minute = 60
+	// => 60 * 60 * 8766 = Number of seconds per year
+	t := ctx.BlockTime().Sub(GenesisTime).Seconds() / (60 * 60 * 8766) // years passed


Yes, I believe it's equivalent. I can double check.

I still think that the above calculation should not use integer division - it will be 0 for a whole year, then 1 for a year, then 2, etc..., causing unnecessary and unwanted discontinuities on the anniversary of genesis.

boz · 2021-11-10T05:39:54Z

x/inflation/types/v1beta2/inflation_calculator.go

+	// => 60 * 60 * 8766 = Number of seconds per year
+	t := ctx.BlockTime().Sub(GenesisTime).Seconds() / (60 * 60 * 8766) // years passed
+	i := initialInflation * math.Pow(2, -t/tHalf)
+	idealInflation := sdk.NewDec(int64(i))


I don't think that there is any loss of precision from int to decimal.

I do think that we should use a consistent type from the outset of this whole thing - rational and decimal for arbitrary precision, then truncate as necessary at the end (or continuously via some large fixed precision).

boz · 2021-11-10T05:43:56Z

x/inflation/types/v1beta2/params.go

+	DefaultInitialInflation     string = "100.0"
+	DefaultVarince              string = "0.05"


Make these sdk.Decimal or even integers that are used to populate a decimal later. Maybe look at how some other parameters in cosmos-sdk are implemented.

boz · 2021-11-10T16:28:05Z

x/inflation/types/v1beta2/params.go

+	DefaultInitialInflation = sdk.NewDec(100)
+	DefaultVarince          = sdk.MustNewDecFromStr("0.05")
+
+	MaxInitialInflation = sdk.NewDec(100)
+	MinInitialInflation = sdk.ZeroDec()
+
+	MaxVariance = sdk.NewDec(1)
+	MinVariance = sdk.ZeroDec()


Make these functions so that they're constant-like.

boz · 2021-11-10T16:31:29Z

x/inflation/types/v1beta2/inflation_calculator.go

+		t := ctx.BlockTime().Sub(genesisTime).Seconds() / 31557600 // years passed (can be a fraction, eg: 0.5)
+		idealInflation := inflationParams.InitialInflation.Mul(sdk.MustNewDecFromStr(
+			strconv.FormatFloat(math.Pow(2, -t/tHalf), 'f', -1, 64),
+		))


Use sdk.Dec or standard big.*.

There's no Pow() equivalent on sdk.Dec or big.* that accepts a fraction. That's why using this way of formatting the Pow() float value to a string and parsing a sdk.Dec from that.

I see. How annoying.

How about using this from this, or something similar?

Done. Using: https://github.com/ericlagergren/decimal

arijitAD · 2021-11-10T17:25:37Z

This seems to have genesis time as a configurable option? Wouldn't this create all sorts of issues if one validator was using a different time?

The option is used to to supply the genesis time, only to make unit tests pass. Actual validators would have the genesis time in the config file, and that should be same for all validators, I think.

hydrogen18 · 2021-11-10T19:52:30Z

This seems to have genesis time as a configurable option? Wouldn't this create all sorts of issues if one validator was using a different time?

The option is used to to supply the genesis time, only to make unit tests pass. Actual validators would have the genesis time in the config file, and that should be same for all validators, I think.

Can't the tests just assume some random arbitrary genesis time then? Just pick a time that makes sense, i.e. sometime in 2018 or something?

boz · 2021-11-11T01:39:32Z

@arijitAD, this looks promising.

boz · 2021-11-11T03:50:04Z

There's also this, which looks like it's more actively maintained: https://github.com/ericlagergren/decimal

arijitAD · 2021-11-11T20:23:00Z

Can't the tests just assume some random arbitrary genesis time then? Just pick a time that makes sense, i.e. sometime in 2018 or something?

No, having a random genesis time doesn't work with tests. They start failing in that case. Seems, there are some parts in tests which depend on the genesis time being correctly set as per the test seed.

boz · 2021-11-12T04:05:08Z

x/inflation/types/v1beta2/inflation_calculator.go

+							),
+					),
+			)
+		idealInflation := inflationParams.InitialInflation.Mul(sdk.MustNewDecFromStr(pow.String()))


Use Int() or Float().

Multiply then divide by precision if needed.

inflationParams.InitialInflation is a sdk.Dec. sdk.Dec has 18 precision digits.
pow is a decimal.Big. I am using the same precision of 18 digits for decimal.Big everywhere.

So, converting pow to string and parsing it into sdk.Dec won't lose any precision.

Could you please elaborate why and where should we use Int()/Float()? Also, why and where we need to multiply and divide by precision?

We don't need that much precision, especially for the final result.

If the result is less than one and we use 6 units of precision then you can make an sdk.Decimal by with something like:

sdk.Decimal(big.Int(val * 10^6)) / 10^6

boz · 2021-11-12T17:35:37Z

proto/akash/inflation/v1beta2/params.proto

+    (gogoproto.moretags)   = "yaml:\"initial_inflation\""
+  ];
+
+  // Variance defines the fraction by which inflation can vary from its previous value in a block.


Isn't it how much it can vary from the ideal inflation curve?

Yes, updated comment.

boz · 2021-11-12T17:40:30Z

x/inflation/types/v1beta2/params.go

+}
+
+func validateInitialInflation(i interface{}) error {
+	v, ok := i.(sdk.Dec)


Will this work if the param type is a string? https://github.com/ovrclk/akash/pull/1352/files#diff-bbf326520f4fa8aa2e6f40542d4dfb257fe6bc3233f0d5b382b7021416619d3bR18

That is a string only in proto definition. In Go code, it maps to sdk.Dec because of this configured option in the proto definition:

(gogoproto.customtype) = "github.com/cosmos/cosmos-sdk/types.Dec",

This is how other modules in cosmos-sdk are mapping their parameters to sdk.Dec.

hydrogen18 · 2021-11-12T21:38:39Z

Can't the tests just assume some random arbitrary genesis time then? Just pick a time that makes sense, i.e. sometime in 2018 or something?

No, having a random genesis time doesn't work with tests. They start failing in that case. Seems, there are some parts in tests which depend on the genesis time being correctly set as per the test seed.

If that is the case then can OptsWithGenesisTime be moved into a _test.go file and in the regular codepath we continue to use simapp.EmptyAppOptions{} ?

review comments review comments rebase cosmos-sdk patches review comment address comments

arijitAD · 2021-11-22T10:05:13Z

If that is the case then can OptsWithGenesisTime be moved into a _test.go file and in the regular codepath we continue to use simapp.EmptyAppOptions{} ?

No, it doesn't seem feasible. There are existing test utility functions in non _test.go files, which have to use OptsWithGenesisTime.

boz

Looks great!

We'll have to adjust some of the defaults to make sure it matches mainnet, but this is good to go for now.

hydrogen18 · 2021-11-23T23:11:38Z

The e2e tests are failing with

E[2021-11-22|13:45:28.281] applying statefulSet                         module=provider-cluster-kube err="StatefulSet.apps \"web\" is invalid: spec: Forbidden: updates to statefulset spec for fields other than 'replicas', 'template', and 'updateStrategy' are forbidden" lease=akash1v7u8mq7npa6g7pcncnqgshc5dw5cf5m40gww26/100/1/1/akash1ra7cz7jsljstqk3230426949s79j079h8ptjg7 service=web
I[2021-11-22|13:45:28.284] declaring hostname

@troian is this something added with storage?

troian · 2021-11-23T23:37:08Z

@hydrogen18 correct, i’ll give it a check why it updated multiple times

arijitAD requested a review from boz August 23, 2021 11:16

boz reviewed Aug 27, 2021

View reviewed changes

github-actions bot added the stale label Sep 7, 2021

arijitAD force-pushed the test-inflation branch from 2e40623 to 539b9a9 Compare September 9, 2021 08:47

github-actions bot closed this Sep 15, 2021

arijitAD reopened this Sep 15, 2021

github-actions bot closed this Sep 21, 2021

troian deleted the test-inflation branch September 25, 2021 21:30

arijitAD reopened this Oct 26, 2021

arijitAD commented Oct 27, 2021

View reviewed changes

arijitAD mentioned this pull request Oct 27, 2021

feat(mint): Implement custom inflation function. cosmos/cosmos-sdk#10441

Merged

19 tasks

boz added keepalive Exempt these when managing stale PRs and removed stale labels Oct 27, 2021

boz reviewed Oct 27, 2021

View reviewed changes

boz requested changes Oct 30, 2021

View reviewed changes

arijitAD force-pushed the test-inflation branch from 1d68f92 to c65f9fe Compare November 1, 2021 11:37

arijitAD marked this pull request as ready for review November 1, 2021 12:39

arijitAD requested review from hydrogen18 and troian as code owners November 1, 2021 12:39

arijitAD commented Nov 1, 2021

View reviewed changes

app/app.go Outdated Show resolved Hide resolved

arijitAD force-pushed the test-inflation branch from de7ebee to 349cdbc Compare November 2, 2021 14:13

arijitAD requested a review from boz November 2, 2021 16:03

arijitAD force-pushed the test-inflation branch from 349cdbc to be8972a Compare November 5, 2021 07:15

boz requested changes Nov 10, 2021

View reviewed changes

arijitAD force-pushed the test-inflation branch from 20f6a63 to e37f306 Compare November 10, 2021 13:25

boz requested changes Nov 10, 2021

View reviewed changes

arijitAD requested a review from boz November 11, 2021 20:23

boz requested changes Nov 12, 2021

View reviewed changes

boz reviewed Nov 12, 2021

View reviewed changes

arijitAD added 10 commits November 22, 2021 15:27

test custom inflation func

b6e5332

review comments review comments rebase cosmos-sdk patches review comment address comments

fix tests

92dc94e

fix simulation tests

5acac9e

fix panic on missing genesis file in tests

db4be6d

fix k8s integration tests

2b34683

fix lint + add upgrade

38e78b2

fix gosec lint

077b7b8

address comments

07bd6ee

fix precision

62d140e

address comments

93b2b94

arijitAD force-pushed the test-inflation branch from 1cfa4ad to 93b2b94 Compare November 22, 2021 09:57

arijitAD requested review from boz and hydrogen18 November 22, 2021 17:05

boz approved these changes Nov 23, 2021

View reviewed changes

arijitAD merged commit c89507d into master Nov 24, 2021

		DefaultInitialInflation string = "100.0"
		DefaultVarince string = "0.05"

Conversation

arijitAD commented Aug 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

Uh oh!

codecov bot commented Aug 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

github-actions bot commented Sep 7, 2021

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!

boz 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!

arijitAD Oct 29, 2021 • 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!

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!

boz 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!

Uh oh!

arijitAD commented Nov 2, 2021

Uh oh!

boz commented Nov 4, 2021

Uh oh!

boz commented Nov 4, 2021

Uh oh!

arijitAD commented Nov 5, 2021

Uh oh!

arijitAD commented Aug 23, 2021 •

edited

Loading

codecov bot commented Aug 23, 2021 •

edited

Loading

arijitAD Oct 29, 2021 •

edited

Loading