-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
BeginBlock / EndBlock handlers #247
Conversation
First of all, it's amazing to see someone else pick this up! Just a few initial thoughts:
Does any of this make sense ? |
honestly I would also propose breaking this PR up into stages since they are logically separate: part 1: implement hooks for beginBlock/endBlock 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 |
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
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. |
I just deleted a previous comment realizing I misread what you are saying.
Yes this is exactly what I'm suggesting
This is what I'm suggesting by a "type level fold over the modules list"
I think it will have to happen in some way similar to how things get compiled down to |
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.
in Handlers.hs:
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. |
34182fe
to
ae45193
Compare
I think I've gotten this to a reasonable place that could be reviewed / merged if appropriate. |
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 I wrote a sketch here that i think could work: in particular look at this NOTE the sdk compiles in this sketch but the examples don't. I'm aware that the returned result is not 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 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? |
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 I'll play around with this a bit, and try to get things like output types sorted out. |
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... |
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 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. |
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. |
Also bonus of this method, App developers can decide how to handle what happens when multiple endblockers return validator updates or consensus parameter changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Awesome, glad to see this merged in. Thanks for guiding me through the process. I'll open a new issue to spec out a validator set management module. |
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.
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.