Allow minor releases before the block height #320
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #320 +/- ##
==========================================
+ Coverage 55.38% 55.40% +0.02%
==========================================
Files 620 620
Lines 51639 51669 +30
==========================================
+ Hits 28598 28626 +28
- Misses 20959 20960 +1
- Partials 2082 2083 +1
|
stevenlanders
changed the title
| if !k.HasHandler(plan.Name) { | ||
| panicUpgradeNeeded(k, ctx, plan) | ||
| } | ||
| applyUpgrade(k, ctx, plan) |
There was a problem hiding this comment.
based on the other discussion we were doing, if the upgrade plan gets cleared after applying upgrade, wouldnt this cause issue for generating apphash since it does affect the stored state?
There was a problem hiding this comment.
My initial thought was that the upgrade action was stateless since registered plans stay registered, but if we specifically clear stuff out upon running upgrades, this may cause issue.
There was a problem hiding this comment.
Got it - I updated with the new logic (see next commit)
| panicUpgradeNeeded(k, ctx, plan) | ||
| } | ||
| applyUpgrade(k, ctx, plan) | ||
| return |
There was a problem hiding this comment.
If a node is behind, and it sees the current scheduled plan being a minor version (e.g. 1.3.0->1.3.1) but there is a "future" (future only to that node; it is already the current scheduled plan for up-to-date nodes) major version (e.g. 1.3.1->2.0.0), and it uses the 2.0.0 binary, then it would only panic once it passes the 1.3.1 upgrade height, which might result in bad data being written because of changes in 2.0.0
There was a problem hiding this comment.
We discussed in thread, but for other's purposes, we determined that this scenario and logic exists today and are electing to focus on the minimum change to allow the minor releases at the moment.
| skipUpgrade(k, ctx, plan) | ||
| return |
There was a problem hiding this comment.
Nit: combine into
return skipUpgrade(k, ctx, plan)
There was a problem hiding this comment.
It'd be cool, but this isn't yet legal golang :)
| func (p Plan) UpgradeDetails() UpgradeDetails { | ||
| var details UpgradeDetails | ||
| if err := json.Unmarshal([]byte(p.Info), &details); err != nil { | ||
| // invalid json, assume no upgrade details |
There was a problem hiding this comment.
Can we add an error log here for failed to unmarshal upgrade info so that we know something bad is happening??
|
FYI - leaving this open until integration-tests work on sei-chain |
Describe your changes and provide context
One can pass an upgradeType to info as follows
Testing performed to validate your change