BeginBlock / EndBlock handlers

[UnitylChaos]

The primary purpose of this PR is to enable applications to update the Tendermint validator set (#123). This is done by requiring that modules specify handlers for the BeginBlock and Endblock ABCI messages. The code for turning these module handlers into an application handler follows the existing patterns for check/deliver/query, with an extension to the routing scheme(Router.hs), so that all begin/endblock handlers receive and process the message with their results merged.

This mostly follows the style of the Cosmos SDK, with two main exceptions. Both of these differences could be resolved, but I didn't feel it was worth the initial effort.

• In the Cosmos SDK, the application must specify an order in which the handlers for begin/endblock will be run, here it is determined by the order of the modulelist.
• The Cosmos SDK expects only one endblock handler to return any validator updates, and will error if multiple are returned (code), whereas I simply concatenated the update lists.

The EndBlock handler is enough for application makers to update the validator set, but I've also built a module "Validators" to simplify the process. This module tracks the current height, and the updates to the validator set at each height. On a beginblock message, it increments the stored height, and extends the array of updates. It has a keeper so that other modules can queue validator updates for the current height without having to directly interact with begin/endblock. And the EndBlock handler simply sends Tendermint the list of ValidatorUpdate items for the current block.

I've also modified the existing modules and tutorial so that they compile and all the existing tests pass. I have not included any tests of the begin/endblock functionality though, I wanted to get feedback on the proposed changes first.

Also just wanna say thanks for building kepler, It's nice to see that there's a viable Haskell SDK for building Tendermint/Cosmos chains.

[martyall]

First of all, it's amazing to see someone else pick this up!

Just a few initial thoughts:

1. I'm seeing that the begin/end block logic has a lot of copy/paste from the way we currently do things for transactions. For example, you have implemented a router in Tendermint.SDK.BaseApp.Block.Router, but in my mind, we don't need a router because there's only one "route" if you can even call it that. The reason we have a router for tx operations or query is because those messages have to get routed to the appropriate module, and even there they need to be routed to the appropriate handler. Here there are no choices but to go down some ordered list of handlers for a begin/end block message and execute every one of them in order. I guess one definite smell to me is that adding begin/end block changes the type of the module, which seems wrong.

2. I can definitely see a way that we can make use of the Eval typeclass (defined here) to translate a module's begin/end block handlers to TxEffs. Granted TxEffs is a larger effects list than we would allow for, but that's not a problem as I think we can make use of the constraint system to not allow those effects to be used during handler definition. I can then imagine using a sort of fold over the ModuleList using a different kind of Proxy list to specify the order the handlers are supposed to be run in. When we apply eval at each level of that fold in order to reduce all of the handlers of the same type, we can then combine them using >> or something, because I'm assuming they are all supposed to return ()

Does any of this make sense ?

[martyall]

honestly I would also propose breaking this PR up into stages since they are logically separate:

part 1: implement hooks for beginBlock/endBlock
part 2: implement a validator module that might use such hooks

It makes the review process easier. Let's make sure we also have issues for each so that we can evaluate that the PR is meeting the requirements. I'm not sure we even have a beginBlock/endBlock issue. For the validator module, i suppose we can go ahead and use #123

[UnitylChaos]

Yeah there was a lot of copying and pasting, and I agree that the usage of the Router system is unnecessary. Given that the routing is not necessary, and that much of Module.hs is concerned with merging the routers for modules into routers for the application, would you consider having the begin/endblock handlers skip the module/application interface entirely? My thought was that HandlersContext could be extended with something like

, beginBlockers :: [Req.BeginBlock -> Sem (BA.BaseAppEffs core) Resp.BeginBlock]
, endBlockers :: [Req.EndBlock -> Sem (BA.BaseAppEffs core) Resp.EndBlock]

And then the makeHandlers function would handle merging them into a single handler.

This way the individual handler functions could be written in modules, but wouldn't be part of the module type, and the execution order would be specified in the application just by listing them for the HandlersContext.

This still leaves open the question of where the evaluation from BlockEffs to BaseAppEffs core happens. And i'll need to do some more reading (or be given some direction) to know how best to resolve that cleanly.

I've opened a new issue for this as you suggested, #248. And I would be fine with taking out the Validators module and just focusing this on the handlers themselves.

[martyall]

I just deleted a previous comment realizing I misread what you are saying.

My thought was that HandlersContext could be extended with something like

Yes this is exactly what I'm suggesting

And then the makeHandlers function would handle merging them into a single handler.

This is what I'm suggesting by a "type level fold over the modules list"

This still leaves open the question of where the evaluation from BlockEffs to BaseAppEffs core happens.

I think it will have to happen in some way similar to how things get compiled down to TxEffs in this module now https://github.com/f-o-a-m/kepler/blob/733352cd49cda23770bd61348385b027af2e61ef/hs-abci-sdk/src/Tendermint/SDK/Application/Module.hs

[UnitylChaos]

Okay, so I tried to work through an example of this using the validators module. Managed to get it to compile and I like how it all flows.

beginBlockF
  :: Members B.BlockEffs r
  => Request.BeginBlock
  -> Sem r Response.BeginBlock
...

endBlockF
  :: Members B.BlockEffs r
  => Request.EndBlock
  -> Sem r Response.EndBlock
...

type ValidatorsModules =
  '[ Validators, A.Auth ]


hc :: HandlersContext Secp256k1 ValidatorsModules CoreEffs
hc = HandlersContext
  { signatureAlgP = Proxy @Secp256k1
  , modules = valModules
  , beginBlockers = [runBeginBlock . beginBlockF]
  , endBlockers = [runEndBlock . endBlockF]
  , compileToCore  = defaultCompileToCore
  , anteHandler = baseAppAnteHandler
  }
  where
    valModules = validatorsModule :+ A.authModule :+ NilModules

valApp :: App (Sem CoreEffs)
valApp = makeApp hc

in Handlers.hs:

data HandlersContext alg ms core = HandlersContext
  { signatureAlgP :: Proxy alg
  , modules       :: M.ModuleList ms (M.Effs ms core)
  , beginBlockers :: [Req.BeginBlock -> Sem (BA.BaseAppEffs core) Resp.BeginBlock]
  , endBlockers :: [Req.EndBlock -> Sem (BA.BaseAppEffs core) Resp.EndBlock]
  , anteHandler   :: BA.AnteHandler (M.Effs ms core)
  , compileToCore :: forall a. Sem (BA.BaseAppEffs core) a -> Sem core a
  }

makeHandlers (HandlersContext{..} :: HandlersContext alg ms core) =
...
      mergeBeginBlock (Resp.BeginBlock a) (Resp.BeginBlock b) = Resp.BeginBlock (a++b)
      beginBlock (RequestBeginBlock bb) = do
        res <- mapM (\f -> f bb) beginBlockers
        pure $ ResponseBeginBlock (foldl mergeBeginBlock (Resp.BeginBlock []) res)

      mergeEndBlock (Resp.EndBlock updatesA paramsA eventsA) (Resp.EndBlock updatesB paramsB eventsB) =
        Resp.EndBlock (updatesA ++ updatesB) (mergeParams paramsA paramsB) (eventsA ++ eventsB)
        where
          mergeParams Nothing y = y
          mergeParams x _       = x
      endBlock (RequestEndBlock eb) = do
        res <- mapM (\f -> f eb) endBlockers
        pure $ ResponseEndBlock (foldl mergeEndBlock (Resp.EndBlock [] Nothing []) res)
...

This isn't too bad, because the begin/endblock functions in Validators only use BlockEffs. If a user application had begin/endblock handlers which needed access to various module effects, they would have to run the eval methods themselves when building the HandlersContext, which could get somewhat messy. I can't really see a situation where you would want a begin/endblock handler which accessed external modules... I would think it would be cleaner to just write handlers which are constrained to each module, but I could be wrong.

If this seems like a reasonable approach, I'll go through and undo the changes I made to Module.hs, Router.hs, etc. and push a new commit.

[UnitylChaos]

I think I've gotten this to a reasonable place that could be reviewed / merged if appropriate.
Application developers can write begin/endblock handlers using BlockEffs (which is TxEffs without gas handling), and those handlers are automatically compiled to BaseAppEffs core by makeHandlers.
If developers want to do more complicated stuff involving multiple modules/keepers, they should be able to as long as they can compile it down to BlockEffs before passing to HandlersContext.

[martyall]

The way that this is written will make it pretty difficult to actually use. The problem is that if the developer is expected to write things at the level of ReadStore, WriteStore etc instead of BankEffs or ValidatorEffs, then it's pretty annoying.

I wrote a sketch here that i think could work:
2796ea3

in particular look at this
2796ea3#diff-7c547f66d847110dbec637c4acbda8efb22937293a2a31947077cc4764b30c3bR36

NOTE the sdk compiles in this sketch but the examples don't. I'm aware that the returned result is not () or undefined, but the hard part here is the effect system management and not those values so i leave them to you.

The way I'm thinking about it, the only difference in the effects for Tx vs BeginBlock or EndBlock is Gas (unless I'm really not seeing something). So then one solution would be to allow those handlers to run in TxEffs except gas is free and unlimited (we could actually change this as well to make sure such handlers can't run forever and instead crash the chain). The advantage to doing this is that we can hook into the Eval typeclass which controls how higher level effects that you actually write code in get translated down to lower level ones.

When you allow for the Gas effect, but interpret it as a no-op, then it allows you to reuse state changing code in your modules that does require gas in a transaction context. To be honest, I don't really know many uses of the before/after block hooks but this seems like something that you would want to have the option of.

Thoughts?

[UnitylChaos]

I take your point that developers might prefer to write handlers which use module effects. My use case is relatively simple, so I had been okay with the simple effects, but in a more complex system it would definitely be a hassle to not be able to have a begin/endblock handler in one module (say staking or slashing) be able to call the existing handlers in Bank to credit or debit an account.

I also hadn't thought about the value of keeping the gas effects, but just making them a NOP, and that doing it that way would allow you to use existing keepers/handlers written for TxEffs, that seems totally reasonable.

I think we could give developers a config option which allows a maximum gas for begin/end block handlers which would crash the chain if it went over, but I would be somewhat worried that this would just create an extra headache for them. Since now they would have to figure out a reasonable bound, and overshooting it would cause halting. For metrics purposes it might be worth recording the gas usage of the handlers though.

Thank for the sketch of your approach, it looks like a good way to use the existing TX handler stuff for blockeffs. I don't quite see how developer specification of an ordering would work, since this is just using >> to run all the handlers in the application in order of the module list.

I'll play around with this a bit, and try to get things like output types sorted out.

[martyall]

You’re right that I didn’t include the ordering. It’s slightly more complicated but totally doable by using some ordered heterogenous list indexed by module name or type or something, probably as an input to Application. There may be other ways...

[martyall]

On thinking about it more, it probably makes more sense to just define the handler at the level of the application rather than at the level of the individual module. What I mean is that the type here https://github.com/f-o-a-m/kepler/blob/master/hs-abci-sdk/src/Tendermint/SDK/Application/Module.hs#L57 should become something like

data Application check deliver query r s = Application
  { applicationTxChecker   :: T.RouteTx check r
  , applicationTxDeliverer :: T.RouteTx deliver r
  , applicationQuerier     :: Q.RouteQ query s
  , applicationBeginBlock :: Req.BeginBlock -> Sem r ()
  , ...
  }

That way we don't have to worry about the order, the user can just define the global handler directly like

appBeginBlock 
  :: Members BankEffs r
  => Members ValidatorEffs r
  => Req.BeginBlock
  -> Sem r ()
appBeginBlock bbMsg = do
  Bank.beginBlockH bbMsg
  Validators.beginBlockH bbMsg

where for example Bank.beginBlockH is just some handler defined in that module. You could alse get fancy and define some kind of newtype BeginBlocker with a monoid like structure defined by >> or something.

You would then need to pass this hander to the constructor here https://github.com/f-o-a-m/kepler/blob/master/hs-abci-sdk/src/Tendermint/SDK/Application/Module.hs#L131

Since the handler is in fact global, this seems to make the most sense, and it easily integrates into the types as they are already defined.

[UnitylChaos]

Yeah, I like this idea. Makes ordering trivial for the developer and also allows handlers which aren't constrained to a specific module. I also like that this doesn't force module developers to specify a null handler for every module which doesn't use them.

[UnitylChaos]

Also bonus of this method, App developers can decide how to handle what happens when multiple endblockers return validator updates or consensus parameter changes.

[martyall]

Awesome, we are getting really close. I like the way everything is structured in terms of where things are hooking in, but there are just a few minor improvements and then we'll get this PR merged!

[martyall]

Amazing, I think we got this one right in the end. Going to go ahead and merge, nice work!

There is one small issue that I'm going to correct after merging, but since you already did a lot and there was so much back and forth I will spare you. That way you can go ahead and move on to the validator module if you're still interested.

When you do something like

            return . ResponseException . Resp.Exception . pack $ "Fatal Error in handling of BeginBlock: " ++ show e

it becomes more difficult to do actual error handling downstream if you want to parse the thing. If you show a haskell type directly it's pretty ugly, and you have to remove the prefix ""Fatal Error in handling of BeginBlock: ". Since AppError has a nice structure anyway, it's best to just use the stringified JSON.

[UnitylChaos]

Awesome, glad to see this merged in. Thanks for guiding me through the process.
And good point re: json errors, will keep that in mind.

I'll open a new issue to spec out a validator set management module.