Validators module

[UnitylChaos]

Initial draft of a module for apps to manage validator sets (#252).

Changes from what was described in that issue:

• Addition of store Var to keep a set of the current validator pubkeys, since store Map doesn't have a method to get all keys/values
• Used List type for QueuedUpdates, so that adding an update would be fast, and Map like behavior can be constructed from a foldl
• Used Word64 instead of Natural. Still non-negative, but bounded since Tendermint has a 64bit bound on validator power anyway.
• Didn't implement Queries for QueuedUpdates, since it is always reset in EndBlock, and query is for committed state (it would always be [])

I'm not very familiar with the Query system, so that is in particular need of review. Also Weeder is going to fail until this has some tests, and I imagine that is going to take a while because I'm not familiar with the Kepler testing system, and proper testing will require multiple Tendermint docker instances.

[martyall]

regarding a proper tests setup with multiple docker images, I am not really worried about that in this PR but feel free to file an issue. It seems simple enough that things should just work 🤞

[UnitylChaos]

To your comment points:

I think the "Set of Map keys" pattern could be added as a feature in Map. Since IAVL has the IterateRange methods and the gRPC layer has the List method, it should be possible to get all the key/value pairs for a Map substore. I can write up an issue if you think this is worth pursuing.

ValidatorUpdate is basically just a redefinition of the ValidatorUpdate data type from ABCI, but using PubKey_ instead of Maybe ABCI.PubKey (Not clear to me why ABCI.ValidatorUpdate has a Maybe on it's PubKey) so that HasCodec could easily implemented on both (using Generic and JSON). I would generally prefer to just reuse the ABCI types directly, but we can't really put HasCodec and RawKey implementations in ABCI without creating a circular dependency mess between ABCI and the SDK.

I can probably do this more cleanly with newtype and by reusing the proto stuff from ABCI to implement HasCodec / RawKey in types.hs.

Also ValidatorUpdate is not externally accessible partly because it's only being used as a way to queue updates from different deliverTx into the endblock, it's never actually stored in committed state. Which is somewhat weird, but I can't really see a better way to do this other than creating some entirely new system for local block processing storage.

I agree with the larger point about cross platform compatibility and standardization, so I'll switch all of this over to proto.

[martyall]

Regarding new typing to avoid circular dependency, or what would usually be orphan instances , it’s a very common method. If you need to do it then go for it.

i think you should write up this issue for map with keys because it would be really useful. I remember it being a big annoyance in ethereum contracts. Regarding the grpc stuff, maybe you’ve seen but I have a branch to upgrade the grpc protobuf file from iavl. I just haven’t finished it yet...

[UnitylChaos]

Okay. Did some stuff:

• Used StateT as recommended in EndBlock.
• Removed Keeper methods
• Switched ValidatorUpdate and PubKey to be newtypes around ABCI types which reuse existing proto serializations

Two things to note:

• I didn't switch KeySet to protobuf, because I expect this will end up being removed eventually anyway if #254 happens. (and didn't want to clutter things up with a new proto file for one temporary type, which isn't externally accessible)
• I also removed the Maybe from PubKey in the ABCI ValidatorUpdate definition. The actual proto has this as not nullable, and the Tendermint side expects it to not be null, so it seemed valid to expect the data type to always have a pubkey value.

[UnitylChaos]

So this is an interesting test failure:

Failures:

  test/Network/ABCI/Test/Types/MessagesSpec.hs:46:5: 
  1) Network.ABCI.Test.Types.Messages.Request.initChain Unwrapped -> Wrapped -> Unwrapped == identity
       Falsified (after 4 tests and 5 shrinks):
         ArbitraryMessage {unArbitraryMessage = {validators { }}}                                                

  To rerun use: --match "/Network.ABCI.Test.Types.Messages/Request/initChain/Unwrapped -> Wrapped -> Unwrapped == identity/"

  test/Network/ABCI/Test/Types/MessagesSpec.hs:46:5: 
  2) Network.ABCI.Test.Types.Messages.Response.initChain Unwrapped -> Wrapped -> Unwrapped == identity
       Falsified (after 2 tests and 2 shrinks):
         ArbitraryMessage {unArbitraryMessage = {validators { }}}

  To rerun use: --match "/Network.ABCI.Test.Types.Messages/Response/initChain/Unwrapped -> Wrapped -> Unwrapped == identity/"

  test/Network/ABCI/Test/Types/MessagesSpec.hs:46:5: 
  3) Network.ABCI.Test.Types.Messages.Response.endBlock Unwrapped -> Wrapped -> Unwrapped == identity
       Falsified (after 3 tests and 3 shrinks):
         ArbitraryMessage {unArbitraryMessage = {validator_updates { }}}

  To rerun use: --match "/Network.ABCI.Test.Types.Messages/Response/endBlock/Unwrapped -> Wrapped -> Unwrapped == identity/"

  test/Network/ABCI/Test/Types/MessagesSpec.hs:46:5: 
  4) Network.ABCI.Test.Types.Messages.FieldTypes.ValidatorUpdate Unwrapped -> Wrapped -> Unwrapped == identity
       Falsified (after 3 tests and 1 shrink):
         ArbitraryMessage {unArbitraryMessage = {}}

This is being caused by my changing the ValidatorUpdate type from: Maybe PubKey to PubKey. Interesting that it's only on the "Unwrapped -> Wrapped -> Unwrapped" side of the tests.

My reading of this is that it can start with an "Unwrapped" protobuf value which doesn't have the key, and then when it tries to turn it into a ValidatorUpdate object it fails...

Is this basically why the value was Maybe PubKey in the first place? I sorta figured there was some way to cleanly tell it that a null PubKey could not be used to form a valid ValidatorUpdate, but I don't know how to tell the tests that.

I can also just switch the type back to Maybe PubKey if this is too much of a mess to fix.

[martyall]

Yeah to be honest I’m not really sure, but try putting the Maybe back and if it passes we can get this in without worrying too much about it for now