-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Conversation
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.
x/upgrade's store wrt saving & retrieving module's ConsensusVersions looks good 👍
I'd like to see a test with applying an upgrade handler, and checking the store before and after the upgrade.
x/upgrade/module.go
Outdated
@@ -101,7 +101,8 @@ func (am AppModule) RegisterServices(cfg module.Configurator) { | |||
} | |||
|
|||
// InitGenesis is ignored, no sense in serializing future upgrades | |||
func (am AppModule) InitGenesis(_ sdk.Context, _ codec.JSONMarshaler, _ json.RawMessage) []abci.ValidatorUpdate { | |||
func (am AppModule) InitGenesis(ctx sdk.Context, _ codec.JSONMarshaler, _ json.RawMessage) []abci.ValidatorUpdate { | |||
InitChainer(am.keeper, ctx) |
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.
@tyler I think i might have been mistaken as to when we should call keeper.SetConsensusVersions
. We actually don't need to call it in InitChain, just leave it blank on a fresh chain, as per #8504 (comment), I believe it should be called AFTER the handler has run (so here), to say "ok I've ran the handler (e.g. migration scripts), now I'll update the store with the newest modules' ConsensusVersions".
I think the ultimate E2E test would be with 2 binaries, but since that's hard, let's try to simulate that in a test here:
- create a mock
module.VersionManager
, say withbank
set to 1, and everything else set to latest ConsensusVersion. upgradeKeeper.SetModuleManager(mockVersionMgr)
. This simulates saving the "old" modules version to state.- add an upgrade handler to keeper.
- call ApplyUpgrade on keeper
- after ApplyUpgrade, call
keeper.GetConsensusVersions
, test that the return value'sbank
key should be 2, not 1.
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.
test added and initchainer removed
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.
If it's not set in IntiChain then how do initial versions get set?? I don't see a good alternative and it's not a safe assumption that all chains start at module version 1
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 think we need to add this back
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.
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.
Looks like a great start here @technicallyty 👍. Would be nice to see some grpc query methods in a follow up PR.
…ng consensus version store in x/upgrade -cleaned up x/upgrade Keeper -handler in apply upgrade now handles errors and setting consensus versions -cleaned up migration map keys -removed init chainer method -simapp now implements GetConsensusVersions to assist with testing
Codecov Report
@@ Coverage Diff @@
## master #8743 +/- ##
==========================================
+ Coverage 59.23% 59.27% +0.04%
==========================================
Files 571 571
Lines 31800 31827 +27
==========================================
+ Hits 18837 18867 +30
+ Misses 10761 10757 -4
- Partials 2202 2203 +1
|
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.
Nice. ADR-041 has been updated with a bunch of comments from various people. Could you make sure the variable names etc match? 🙏
Two additional things which would be nice to have in this PR:
- a changelog (
UpgradeHandler
is API-breaking) - update x/upgrade/spec docs. Most importantly the state.md one. A link to ADR-041 from one of the other mds would be good too.
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.
getting close! Left some nits
…ons now returns an error
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.
lgtm, thanks @technicallyty 🎉
pinging @aaronc and @robert-zaremba for a review as you both had ideas on ADR-041.
Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com>
Changes should be aligned with @aaronc 's suggestions now. Additionally updated identifiers to be more go-like and changed the store get/set methods for |
if len(vm) > 0 { | ||
store := ctx.KVStore(k.storeKey) | ||
versionStore := prefix.NewStore(store, []byte{types.VersionMapByte}) | ||
for modName, ver := range vm { | ||
nameBytes := []byte(modName) | ||
verBytes := make([]byte, 8) | ||
binary.BigEndian.PutUint64(verBytes, ver) | ||
versionStore.Set(nameBytes, verBytes) | ||
} | ||
} |
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.
Why do these not happen in InitGenesis? I thought we agreed upon that.
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.
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.
this seemed like correct spot based on the suggestions, did you have somewhere else in mind?
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.
Maybe you're right. I guess I was thinking it could be in InitGenesis but maybe it doesn't matter
Mostly looks good now just one question and a couple suggested renamings. |
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.
ACK. But I would like to rename those functions -> Get/SetModuleVersions
…llyty/cosmos-sdk into ty-8514-module_tracking
Some tests are failing here @technicallyty |
Benchmark beginning. Status page: https://github.orijtech.com/benchmark/status?commit=5bd93bfe7bffef8d8012c97105c2df16c1fecdaf |
…#8743) * -added consensus version tracking to x/upgrade * -added interface to module manager -added e2e test for migrations using consensus version store in x/upgrade -cleaned up x/upgrade Keeper -handler in apply upgrade now handles errors and setting consensus versions -cleaned up migration map keys -removed init chainer method -simapp now implements GetConsensusVersions to assist with testing * Changed MigrationMap identifier to VersionMap removed module_test * updated docs * forgot this * added line to changelog for this PR * Change set consensus version function to match adr 041 spec * add documentation * remove newline from changelog unnecessary newline removed * updated example in simapp for RunMigrations, SetCurrentConsensusVersions now returns an error * switch TestMigrations to use Require instead of t.Fatal * Update CHANGELOG.md Co-authored-by: Aaron Craelius <aaron@regen.network> * docs for SetVersionManager * -init genesis method added -removed panics/fails from setting consensus versions * update identifiers to be more go-like * update docs and UpgradeHandler fnc sig * Upgrade Keeper now takes a VersionMap instead of a VersionManager interface * upgrade keeper transition to Version Map * cleanup, added versionmap return to RunMigrations * quick fix * Update docs/architecture/adr-041-in-place-store-migrations.md Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com> * remove support for versionmap field on upgrade keeper * cleanup * rename get/set version map keeper functions * update adr doc to match name changes * remove redudant line Co-authored-by: technicallyty <48813565+tytech3@users.noreply.github.com> Co-authored-by: Aaron Craelius <aaron@regen.network> Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Description
This PR is part of ADR 041 - In-Place Store Migrations
-Adds a new method to module.Manager to get a
VersionMap
-Adds a new store prefix
0x2
to x/upgrade to store a VersionMap-Adds new functionality to x/upgrade to get a
VersionMap
from state-Saves a VersionMap on InitGenesis in app.go
You may notice some things in simapp had to be added - this is to avoid circular dependancy. UpgradeKeeper needs a reference to Module Manager, so after the manager is initialized, we give the upgradekeeper a reference to it, and re-set the value of the upgrade keeper in the manager's module map.closes: #8514
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/
) or specification (x/<module>/spec/
)godoc
comments.Unreleased
section inCHANGELOG.md
Files changed
in the Github PR explorerCodecov Report
in the comment section below once CI passes