Vote extensions implementation

[marcello33]

Description

This PR contains the implementation of the VEs functionality.
This requires some changes in cosmos-sdk and cometbfr. For reference, see this and this PRs

Changes

• Bugfix (non-breaking change that solves an issue)
• Hotfix (change that solves an urgent issue, and requires immediate attention)
• New feature (non-breaking change that adds functionality)
• Breaking change (change that is not backwards-compatible and/or changes current functionality)
• Changes only for a subset of nodes

Checklist

• I have added at least 2 reviewer or the whole pos-v1 team
• I have added sufficient documentation in code
• I will be resolving comments - if any - by pushing each fix in a separate commit and linking the commit hash in the comment reply

Testing

• I have added unit tests
• I have added tests to CI
• I have tested this code manually on local environment
• I have tested this code manually on remote devnet using express-cli
• I have tested this code manually on mumbai
• I have created new e2e tests into express-cli

[sergio-mena]

Hi @marcello33. These are my replies to this comment.

1. I compared the removed part, side by side, with the contents of ApplyAndReturnValidatorSetUpdates. LGTM. Only über-minor thing is that it is cleaner if you return nil as first return value every time you return a non-nil error (although I might be missing something).
2. In general, it LGTM, although I added a comment that we should not keep that info forever in the store, as it will make it grow a lot unnecessarily.
3. I saw what you did and LGTM. I suggested an alternative that may save you from having to make runTx public (one less change in SDK fork).
4. I saw that, TBH I'm not sure to understand what those checks do (in my view, blocks can have more that 2 TXs, because they may contain non-side TXs). As I suggested in my comment related to this, if you nevertheless think the check should be there, please proceed: if the check shouldn't be there in the end, we'll find out quite early (this is not one of those hard-to-debug potential problems that we better settle now).
5. Hmmm. I checked the commit. I think that might bring problems, as some TXs from (standard) SDK modules might need to include more than one message in a TX (I would revert that commit). If we simply check that, for a given TX, not more than one msg with SideTxHandler exists, I think that would suffice (this is what we do in PrepareProposal and ProcessProposal).

More on point 5 above:

• We are already doing the right check in PrepareProposal and ProcessProposal (since your last patch), so we'll never have to deal with this possibility in PreBlocker.
• However, it's true that we need to do something in the ante handlers, as the checks in PrepProp and ProcProp don't remove the offending TXs from the mempool (so they will keep on being proposed, and disseminated --> memory leak in the mempool).
• IIUC, all ante handlers configured by an app must succeed for CheckTx to return "success" to CometBFT. So, what I would do is add an new ante handler that repeats the checks we already have in PrepProp and ProcProp.
  • We could add this ante handler in auth (SDK fork), but if possible, we'd rather have HeimdallV2 app define it at the app level and pass it to the auth module (not sure it's possible, tho).
  • If we manage to introduce that new ante handler, then you might want to remove the corresponding checks in PrepProp and ProcProp, as runTx is supposed to run the ante handlers (although, to be on the safe side I'd leave them in PrepProp and ProcProp as sanity checks).

[sergio-mena]

Approving this because all major design-related comments have been addressed.
I left some non-trivial comments today, but I believe they shouldn't be blocking progress.
Please bring them back to me if you think it's necessary when I'm back.
Finally, thanks for your patience when addressing my (sometimes picky) comments 🙏.