x/epochs: AfterEpochEnd called with wrong epoch number

[ValarDragon]

What the logic should be:

Suppose epoch N ends at “time T”.
Suppose block 5 is at time T - 1. (The epoch shouldn’t have ended yet)
Suppose block 6 is at time T + 1. The epoch should have ended at time T. Since time T is after “block 5", and “before block 6”, we end epoch N as soon as we can within block 6 — namely at the beginning of the block before everything else.
We then start Epoch N+1 immediately, since all of the the txs in block 6 should be executed in epoch N+1.

Whats the bug

We are doing that, but if you look in the relevant code https://github.com/osmosis-labs/osmosis/blob/main/x/epochs/abci.go#L34-L43, we are modifying the EpochIdentifier before the AfterEpochEndCall!

epochInfo.CurrentEpoch += 1
epochInfo.CurrentEpochStartTime = epochInfo.CurrentEpochStartTime.Add(epochInfo.Duration)
k.AfterEpochEnd(ctx, epochInfo.Identifier, epochInfo.CurrentEpoch)

This results in the current epoch argument being wrong. However if you get the epoch from the Identifier, you would still get the correct result.

The fix is relatively simple, but the complex thing is making sure this:

• Doesn't break any data gathering workflows (The event emitted has the same error)
• We don't serialize anything by the CurrentEpoch number or else we'll have a migration error
• Ensure that the thirdening won't have an off by one error on epoch day

Superfluid component

This is tagged for superfluid, because we either need to:

• Fix the epochs bug for superfluid staking to work
• Change superfluid logic to get Epoch from reading from identifier (which will gracefully work across whenever the fix occurs)

[ValarDragon]

I suggest for the immediate problem of getting this release out, we just fix "Change superfluid logic to get Epoch from reading from identifier (which will gracefully work across whenever the fix occurs)"

[daniel-farina]

• We need to coordinate with integrators and explain the fix.
• We need an official message to send to integrators @czarcas7ic

Integrators to reach out to:

• Imperator
• Staketax
• Mintscan
• Citadel.one

Anything else?

[czarcas7ic]

I made the PR mentioned above and explained the change in the newly created Integrators discord channel. If I get sufficient engagement through that and no issues are heard, we can probably include this in the next release.

The only thing I have not tested is the potential for an off by one error during the thirdening. I can prioritize testing this on a local net if we feel this to be necessary.

[ValarDragon]

I think we should do thirdening testing separately and not block this on it!

[daniel-farina]

@czarcas7ic to merge to main.
backport to 7.x once tested

[czarcas7ic]

Merged and notified integrators. Will post here if any issues are heard