fix: invalid return value from keeper GetLastCompleteUpgrade

[daeMOn63]

Description

Add test case showing issue described in #11707

Expecting tests to pass, but currently fail with:

            	Error Trace:	keeper_test.go:313
            	Error:      	Not equal: 
            	            	expected: "test-v0.10"
            	            	actual  : "test-v0.9"

---

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

• included the correct type prefix in the PR title
• added ! to the type prefix if API or client breaking change
• targeted the correct branch (see PR Targeting)
• provided a link to the relevant issue or specification
• followed the guidelines for building modules
• included the necessary unit and integration tests
• added a changelog entry to CHANGELOG.md
• included comments for documenting Go code
• updated the relevant documentation or specification
• reviewed "Files changed" and left comments if necessary
• confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

• confirmed the correct type prefix in the PR title
• confirmed ! in the type prefix if API or client breaking change
• confirmed all author checklist items have been addressed
• reviewed state machine logic
• reviewed API design and naming
• reviewed documentation is accurate
• reviewed tests and test coverage
• manually tested (if applicable)

[alexanderbez]

LGTM. Let's be sure to add a changelog entry too 👍

[robert-zaremba]

Sorting is not a problem. The problem is with DB access. Now we query all upgrades. I think the better solution is to update the key and use block height (serialized as fixed len uint64) and name later.
So, we can keep this as a temporal fix, and do another (breaking) update to change the key scheme.

[alexanderbez]

Actually @robert-zaremba has a good point here in that his suggestion to change the index/key is a cleaner approach.

However, I don't agree with should go with this just to fix/change it again later (since we can't backport it anyway). I'd rather get this right in one go. So @daeMOn63 if you're open to it, would you be willing to take another stab at this? You'd essentially have to scrap this PR and change the key along with doing some migration stuff.

I can also tackle this if you think it's something you're not interested in and/or don't have the time for.

[daeMOn63]

@alexanderbez just opened #11800 actually with this alternative implementation :) First time playing with migration, looking forward to your review there!

If we directly go with #11800, we can just close this one.

[ValarDragon]

This should probably still be backported to 45

[alexanderbez]

@ValarDragon #11800 isn't state-machine compatible with v0.45 AFAICT

[ValarDragon]

Right, but this version is!

[alexanderbez]

Would GetLastCompletedUpgrade return something potentially different though, even with this (closed) version?

[daeMOn63]

Afaik, both versions of this PR would return similar results, the main difference is some potential performance impact with this one as it queries all upgrades to sort them by height in the code.

The v0.46 version should also apply nicely with or without this one in v0.45 as the migration will take care of reindexing things in the correct order.

[alexanderbez]

Well Im mainly concerned with determinism here. The PR that is in 0.46 can't be used because it introduces state changes. This PR doesn't but it does change the order of what GetLastCompletedUpgrade returns and as long as that's not used in state, then we're OK to add this to 0.45.x

[ValarDragon]

GetLastCompletedUpgrade was a function that was added in the v0.45 line to prevent accidental downgrades on bootup, it is safe to change its semantic! (Its not used in state machine)

[alexanderbez]

Ok, I will wrap this up for 0.45 then :)