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]

Overall looks good! Let's go ahead and get these changes in and then I will do a second pass.

Two kind of interesting things are happening here that haven't come up before:

1. You are using a Set to keep track of the keys that belong to a certain store map.
2. There is a state object (ValidatorUpdate) that is not accessible to the outside world.

In both cases you opted to use JSON to implement the HasCodec instance. In general I am not a fan of this because it makes it more difficult to operate on these values across languages and applications. There are a few reasons why, you can see this discussion if you're interested: cosmos/cosmos-rust#14 (comment)

I'm not even sure I care about addressing this now, but:
(2) above would easily solved by just implementing a protobuf definition, but I have a feeling (1) is going to come up over and over again because it does with solidity.

[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 Iteration in Store Map #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.

[martyall]

OK looks good to me! Just a few small review comments, only one small change. Also need to Otherwise the majority of the work will be in figuring out #254 at some point. Great work!

Regarding weeder, I think you need to put the tope level Tendermint.SDK.Modules.Validators and generated protobuf modules in the package.yaml, similar to what you see here

[martyall]

What does it mean if two ValidatorUpdate_s have been queued for the same PubKey_? It seems like here we just take the one that happened last as the update and disregard whatever came before it?

[UnitylChaos]

Yes that was the intended behavior here. I figured there were roughly 3 options of how to handle multiple updates to the same validator within a single block:

1. Don't allow it, which would require turning the updates array into a map to check if the pubkey already existed
2. Allow it, and apply only the first update
3. Allow it, and apply only the last update

Applying the last update made the most sense to me, as all the updates are replacements not relative changes. So the result of composing those replacements in order would be to just take the last one.

[martyall]

This is fine, but as a note I'm sort of remembering that there is actually no instance where the from method of the iso is being used, we're almost always using it as a value of type :: Lens' a ByteString. This is why the PubKey "" "" seems awkward, I will get around to fixing this soon.

[martyall]

See #255

[UnitylChaos]

re PubKey "" "":
I figured this would be okay, since the only time it would be deserializing would be if it was reading the bytes from the db. And they must have been serialized from the pubkey to get put in the DB, so it should never fail. Definitely is awkward thought...

[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