Store migrations take place two blocks after a new version, rather than at the (preferred) start of the next block.
Currently, the version querier reports a block's 'current' version according to the node state at the end of the block. However, (I believe) the code behaviour for the new version in fact starts from the beginning of the next block, when AppModule-referring BeginBlock
runs am.mgr.BeginBlock
, which checks and sets the current version in the Manager, in turn checked during the rest of the block for which code to run (That managers.go BeginBlock
also sets the rest of the Manager fields and I believe is not affected by StoreMgr).
Creating a new StoreMgr and KVStore migration (iterating through all applicable store migrations according to version distance) is handled in the same AppModule-referring BeginBlock
as the Manager setting, but before the Manager setting. More specifically, newStoreMgr(am.mgr)
sets mgr as a field of the store manager, and smgr.Iterator
gets the current version from that manager.
Notably, before version 1.90.0 the version was checked before the current location of the localVer check and the store manager preparation.
(v1.89.0 version)
More precisely, in v1.89.0 the StoreMgr-referring Iterator
checked the version itself rather than getting it from the Manager.
An example of what this means in practice:
In Block 6634138, the last "set_version" event occurs necessary to update the version from 1.93.0 to 1.94.0 .
https://thornode.ninerealms.com/thorchain/version?height=6634137
current "1.93.0"
https://thornode.ninerealms.com/thorchain/version?height=6634138
current "1.94.0"
Block 6634139 is the first block with version 1.94.0 code, but the localVer check and store migration check at the beginning of the block are still checking in reference to version 1.93.0 .
The TERRA.LUNA pool's migrateStoreV94
RUNE depth migration and pool closure take place in Block 6634140, one block late.
https://thornode.ninerealms.com/thorchain/pool/TERRA.LUNA?height=6634139
balance_rune "77243905859"
https://thornode.ninerealms.com/thorchain/pool/TERRA.LUNA?height=6634140
error
I currently propose moving the below code in some way (see notes below) from above to directly below the three lines of the am.mgr.BeginBlock
's if
.
ctx.Logger().Debug("Begin Block", "height", req.Header.Height)
version := am.mgr.GetVersion()
if version.LT(semver.MustParse("1.90.0")) {
_ = am.mgr.Keeper().GetLowestActiveVersion(ctx) // TODO: remove me on fork
}
localVer := semver.MustParse(constants.SWVersion.String())
if version.Major > localVer.Major || version.Minor > localVer.Minor {
panic(fmt.Sprintf("Unsupported Version: update your binary (your version: %s, network consensus version: %s)", constants.SWVersion.String(), version.String()))
}
// Does a kvstore migration
smgr := newStoreMgr(am.mgr)
if err := smgr.Iterator(ctx); err != nil {
os.Exit(10) // halt the chain if unsuccessful
}
Considerations:
To maintain consensus for previous blocks, a if version.LT(semver.MustParse
-like qualifier should be necessary, with two instances of the moved code.
However, if the first instance gets the version from the last block's version and the second instance from the current block's version, then a single block might try to run both instances twice.
To resolve this, it could be appropriate to enclose a larger section of code (including the am.mgr.BeginBlock
portion) within an if{}else{}
in reference to the previous block's version, such that any store migration for its version change would also happen a block later, but store migrations for later versions would take place on schedule.
Edit: For further context, !2476 (merged) (by allowing the querier to check the version set at the beginning of the block) should allow the querier to with 'current' correctly indicate which code version was used within a block, in which case (together with a merge request for this Issue) store migration should take place in the first block that the querier reports as having a new version.