Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
x/upgrade: added consensus version tracking (part of ADR-041) #8743
x/upgrade: added consensus version tracking (part of ADR-041) #8743
Changes from 13 commits
e900fb1
036db4f
b640766
8c7b105
5c1ae59
4c48f35
6db21e3
c9b435a
1c5b132
304abfa
d06bbbd
5c75ead
e53423e
c36086f
758c67d
0dac9c2
8f526ef
027740d
18eaed2
fc38e5a
28d5c04
1fca959
d912f4f
a99c538
e6424ce
9b40fe7
799b646
0f9447c
a157c48
b839a59
75f8ae5
22ab55e
47ce533
7f5b6aa
8547426
1504a2d
c3072ab
1f82fff
cf1343f
3f6bea8
d987555
5d6e4ab
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought the initial plan was to make this:
Is the reason for not doing this because we want to support setting versions in
InitChain
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm failing to see how the return
module.VersionMap
is used. Right now, inApplyUpgrade
we do:so I don't see the utility of returning the (I suppose new) versionMap from the handler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess my original idea was that we don't pass a reference to
ModuleManager
intox/upgrade
but instead communicate the new versions with the return type... However, to avoid bike-shedding let's just go with this approach for now as I think they're basically equivalent.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah... actually I think this is an issue. So the code in this PR fails if the module manager is not passed in. We should either:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the Aarons approach. Limiting dependencies (especially for something as big as
ModuleManager
) sounds as a good approach.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK sgtm, we can do like this 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if we just avoid the extra field altogether, export the
setConsensusVersions
function and update it to take aVersionMap
? TheversionMap
field on the keeper only ever gets used once anyways, which is from theInitGenesis
call where we set the initial consensus versions to state. If we go with my proposed change, we don't need the extra field on upgrade's keeper, nor the InitGenesis code. Are there any advantages to committing the versions to state through InitGenesis rather than outright at the end of simapp?We add this line to the bottom of simapp
ctx := app.BaseApp.NewUncachedContext(true, tmproto.Header{}) app.CapabilityKeeper.InitializeAndSeal(ctx) + app.UpgradeKeeper.SetConsensusVersions(ctx, app.mm.GetVersionMap())
This is in conjunction with the changes proposed above btw!
thoughts @AmauryM, @robert-zaremba, @aaronc ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
InitGenesis is run once per chain, while "end of simapp" is run once per binary startup.
End of simapp would not work in the following scenario:
Does that make sense?
We need to call
UpgradeKeeper.SetConsensusVersions
twice:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely true what you're saying about simapp @AmauryM.
Here's what I suggest:
UpgradeKeeper.SetConsensusVersions(VersionMap)
method - this sets current versions which get written duringInitChain
. So inapp.go
, users will write:VersionMap
return parameter toRunMigrations
and aVersionMap
return parameter toUpgrade Handler
, so that most upgrade handlers will simply be implemented as:Unless there is a strong objection, I suggest we go with this. And if we need to go into any more details, I suggest we get on a short call to reach consensus.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, changes implemented