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.

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

chore: Update the default genesis params #285

Closed

Conversation

evan-forbes
Copy link
Member

@evan-forbes evan-forbes commented Nov 16, 2022

Description

During the onsite, we came to a few decisions around selecting a few genesis params. Given how easy they are to change, this PR changes the defaults to the values that we agreed upon. While this increases our diff slightly from upstream, it also will also help us keep params consistent across networks without having to configure them each time.

NOTE: we should not merge this PR, although we are keeping it open to continue the discussion around defualt params

closes celestiaorg/celestia-app#169

DefaultMaxMemoCharacters uint64 = 256
DefaultMaxMemoCharacters uint64 = 128
Copy link
Member Author

Choose a reason for hiding this comment

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

this cuts the max memo in half. The memo is now 1/4 the size of a single share, and costs 25% more per gas (10 gas/byte vs 8 gas/ blob byte)

Copy link
Member

Choose a reason for hiding this comment

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

I think we can explore how the memo is currently used. And limit the number to the minimum necessary. also, In celestia people can exchange messages directly in the blobs instead of the memo field.

Comment on lines 46 to 48
InflationRateChange: sdk.NewDecWithPrec(13, 2),
InflationMax: sdk.NewDecWithPrec(20, 2),
InflationMin: sdk.NewDecWithPrec(7, 2),
InflationRateChange: sdk.NewDecWithPrec(1, 2),
InflationMax: sdk.NewDecWithPrec(2, 2),
InflationMin: sdk.NewDecWithPrec(2, 2),
Copy link
Member Author

Choose a reason for hiding this comment

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

Here we are changing the inflation to a flat 2% across the board.

DefaultSignedBlocksWindow = int64(100)
DefaultSignedBlocksWindow = int64(5760)
Copy link
Member Author

@evan-forbes evan-forbes Nov 16, 2022

Choose a reason for hiding this comment

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

here we are changing the defaults window from 100 blocks to roughly the number of blocks per day assuming 4 blocks a minute. (24 * 60 * 4)

Copy link
Member

Choose a reason for hiding this comment

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

How is this parameter used by the Cosmos SDK?

Copy link
Member Author

Choose a reason for hiding this comment

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

its used when slashing for liveness. If a validator misses a certain percentage of signatures in this window, they will be jailed and slashed. Currently its 50%, so validators will be slashed if they miss roughly half the signatures in one day.

the parameter spec

docs

source

minHeight := signInfo.StartHeight + k.SignedBlocksWindow(ctx)
maxMissed := k.SignedBlocksWindow(ctx) - minSignedPerWindow
// if we are past the minimum height and the validator has missed too many blocks, punish them
if height > minHeight && signInfo.MissedBlocksCounter > maxMissed {
validator := k.sk.ValidatorByConsAddr(ctx, consAddr)
if validator != nil && !validator.IsJailed() {
// Downtime confirmed: slash and jail the validator
// We need to retrieve the stake distribution which signed the block, so we subtract ValidatorUpdateDelay from the evidence height,
// and subtract an additional 1 since this is the LastCommit.
// Note that this *can* result in a negative "distributionHeight" up to -ValidatorUpdateDelay-1,
// i.e. at the end of the pre-genesis block (none) = at the beginning of the genesis block.
// That's fine since this is just used to filter unbonding delegations & redelegations.
distributionHeight := height - sdk.ValidatorUpdateDelay - 1
coinsBurned := k.sk.Slash(ctx, consAddr, distributionHeight, power, k.SlashFractionDowntime(ctx))

var DefaultMinCommissionRate = sdk.ZeroDec()
var DefaultMinCommissionRate = sdk.NewDecWithPrec(5, 2)
Copy link
Member Author

Choose a reason for hiding this comment

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

All validators will have to have a minimum commission rate of 5%

Copy link
Collaborator

@rootulp rootulp left a comment

Choose a reason for hiding this comment

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

Only one blocking comment

x/staking/types/params.go Outdated Show resolved Hide resolved
x/slashing/types/params.go Outdated Show resolved Hide resolved
x/mint/types/params.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@rootulp rootulp left a comment

Choose a reason for hiding this comment

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

🚀

InflationMin: sdk.NewDecWithPrec(7, 2),
InflationRateChange: sdk.ZeroDec(),
InflationMax: sdk.NewDecWithPrec(2, 2),
InflationMin: sdk.NewDecWithPrec(2, 2),
Copy link
Member

Choose a reason for hiding this comment

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

We have not made any decision on the inflation parameters yet. It likely won't be 2% fixed though as that's probably too low.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, good to know.

Copy link
Member Author

Choose a reason for hiding this comment

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

this was initially changed because of our notes indicated heading in this direction (lower the percentage and keep it fixed)

Decided
we want a lower value for inflation
we want to set inflation max and inflation min to the same value
Undecided
the exact values for inflation are tbd

reverted here ea05762 and we can just rely on changing these params manually per network until we decide on values

Copy link
Member

Choose a reason for hiding this comment

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

We do want to reduce the values for inflation - but exact values are tbd as stated in notes

Copy link
Member

Choose a reason for hiding this comment

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

My thinking for inflation is a 3-5 % range (not flat). But we will decide this soonish.

InflationRateChange: sdk.NewDecWithPrec(13, 2),
InflationMax: sdk.NewDecWithPrec(20, 2),
InflationMin: sdk.NewDecWithPrec(7, 2),
InflationRateChange: sdk.ZeroDec(),
Copy link
Member

Choose a reason for hiding this comment

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

What does this do?

Copy link
Member Author

Choose a reason for hiding this comment

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

setting to zero doesn't allow for the inflation rate to change.

Copy link
Member

Choose a reason for hiding this comment

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

Note that this was set to 100% or 1 on the hub not too long ago. Zaki has some charts for rationale here.

GoalBonded: sdk.NewDecWithPrec(67, 2),
BlocksPerYear: uint64(60 * 60 * 8766 / 5), // assuming 5 second block times
BlocksPerYear: uint64(60 * 60 * 8766 / 15), // assuming 15 second block times
Copy link
Member

Choose a reason for hiding this comment

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

How is this used by the Cosmos SDK? Got a link to docs maybe?

Copy link
Member Author

Choose a reason for hiding this comment

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

blocks per year is used to determine the rate of change for inflation (we currently have the max inflation rate change set to 0, so this value shouldn't be important unless we ever change it)

https://docs.cosmos.network/main/modules/mint

Copy link
Member

@liamsi liamsi left a comment

Choose a reason for hiding this comment

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

Is it not possible to change all these in the app instead? Couldn't we wrap the default modules, change the defaults in the respective wrappers, and use these wrapped modules instead of the default ones? That way we do not need to modify the SDK itself.

That said, this PR is super useful to align on the actual params.

@evan-forbes
Copy link
Member Author

Couldn't we wrap the default modules, change the defaults in the respective wrappers, and use these wrapped modules instead of the default ones?

yeah we definitely can do that. initially I didn't do that as it will add even more boiler plate, but I think we'll have to to avoid breaking a bunch of tests

@evan-forbes
Copy link
Member Author

keeping this open for now, as I would like to continue the review process and discussions. but we shouldn't merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: app Changes related to the celestia-app branches C:x/auth C:x/slashing C:x/staking
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Define genesis params
5 participants