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

Test time-based inflation #1758

Closed
rootulp opened this issue May 12, 2023 · 4 comments
Closed

Test time-based inflation #1758

rootulp opened this issue May 12, 2023 · 4 comments
Assignees

Comments

@rootulp
Copy link
Collaborator

rootulp commented May 12, 2023

Context

#1578

Problem

The new x/mint module hasn't been used in production and may contain code errors. Additionally time-based inflation isn't battle tested and may introduce new attack vectors.

Proposal

@rootulp rootulp added the x/mint label May 12, 2023
@rootulp rootulp added this to the Mainnet milestone May 12, 2023
@rootulp rootulp self-assigned this May 12, 2023
@evan-forbes
Copy link
Member

evan-forbes commented May 15, 2023

Additionally time-based inflation isn't battle tested and may introduce new attack vectors.

this is a good point and testing this logic is definitely critical.

Is it correct to say that we are using this mechanism in a similar way to how osmosis is choosing the epoch block? Like, given a time in the header, we are measuring against another pre-agreed upon time to deterministically trigger some action? If not, can we document how it differs here for reviewers/auditors?

are there any other points that we think are important to note or watch out for? precision or format perhaps?

rootulp added a commit that referenced this issue May 17, 2023
### Description

Part of #1570
Implement the strict inflation schedule specified in
#1584

### Remaining tasks
- [x] implement inflation rate change based on block timestamp rather
than block height
- [x] set genesis time in x/mint state
- [x] move `MintDenom` from param to `x/mint` state
- [x] remove `TestRandomizedGenState`
- [x] investigate why `TestQueryGRPC` / other integration tests are
never run
- [x] write a test that 1. starts a new chain, 2. verifies state of
x/mint module for first few blocks, 3. skips ahead to a block one year
later, 4. verifies inflation rate went down a bit
- [x] remove `Params` entirely
- [x] annual provisions for a year should be based on total supply at
the beginning of that year
- [x] for time-based inflation, calculate block provision as current
block timestamp - previous block timestamp = seconds elapsed / seconds
in a year * annual provisions for a year
- [x] remove `BlocksPerYear`
- [x] elaborate in README.md

### FLUPs

- #1755
- #1757
- #1758
- #1756

---------

Co-authored-by: Rootul P <rootulp@gmail.com>
Co-authored-by: Evan Forbes <42654277+evan-forbes@users.noreply.github.com>
Co-authored-by: Callum Waters <cmwaters19@gmail.com>
@rootulp
Copy link
Collaborator Author

rootulp commented May 18, 2023

Is it correct to say that we are using this mechanism in a similar way to how osmosis is choosing the epoch block?

As far as I can tell, Osmosis mints new tokens due to inflation per epoch block that occurs once per day. The block is selected based on the x/epochs module. The x/epochs signals that an epoch has "ticked" when the block timestamp exceeds an individual time counter's start time + duration. It is generalized to handle chain downtime such that when the chain resumes, a ticker will catch-up by repeatedly ticking for successive blocks, even if a day hasn't elapsed per block.

Our x/mint module performs time based calculation strictly on a yearly basis here. It isn't generalized to support arbitrary time durations so the implementation is quite smaller (see Osmosis x/epoch BeginBlocker). Since each block's timestamp is compared against the genesis timestamp, I don't expect there to be issues with the calculation if there is chain downtime.

Like, given a time in the header, we are measuring against another pre-agreed upon time to deterministically trigger some action?

I think it's fair to say yes to this however it isn't exactly the same as how Osmosis chooses the epoch block.

are there any other points that we think are important to note or watch out for?

Yes auditors should pay attention to time precision. They should also explore time boundaries (i.e. the block timestamps near year boundaries). They can also explore edge cases like timestamps before genesis time or extremely far in the future or timestamps that aren't chronologically increasing (although CometBFT may already enforce this isn't possible).

@evan-forbes
Copy link
Member

can we close this?

@rootulp
Copy link
Collaborator Author

rootulp commented Jul 12, 2023

Sure, I'll create a separate issue to get x/mint audited by an external team and close this.

Update: created #2085

@rootulp rootulp closed this as not planned Won't fix, can't repro, duplicate, stale Jul 12, 2023
0xchainlover pushed a commit to celestia-org/celestia-app that referenced this issue Aug 1, 2024
### Description

Part of celestiaorg/celestia-app#1570
Implement the strict inflation schedule specified in
celestiaorg/celestia-app#1584

### Remaining tasks
- [x] implement inflation rate change based on block timestamp rather
than block height
- [x] set genesis time in x/mint state
- [x] move `MintDenom` from param to `x/mint` state
- [x] remove `TestRandomizedGenState`
- [x] investigate why `TestQueryGRPC` / other integration tests are
never run
- [x] write a test that 1. starts a new chain, 2. verifies state of
x/mint module for first few blocks, 3. skips ahead to a block one year
later, 4. verifies inflation rate went down a bit
- [x] remove `Params` entirely
- [x] annual provisions for a year should be based on total supply at
the beginning of that year
- [x] for time-based inflation, calculate block provision as current
block timestamp - previous block timestamp = seconds elapsed / seconds
in a year * annual provisions for a year
- [x] remove `BlocksPerYear`
- [x] elaborate in README.md

### FLUPs

- celestiaorg/celestia-app#1755
- celestiaorg/celestia-app#1757
- celestiaorg/celestia-app#1758
- celestiaorg/celestia-app#1756

---------

Co-authored-by: Rootul P <rootulp@gmail.com>
Co-authored-by: Evan Forbes <42654277+evan-forbes@users.noreply.github.com>
Co-authored-by: Callum Waters <cmwaters19@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants