fix!: Branch antehandlers and commit to state before runMsgs #11942
Conversation
| beginAnteBranch, endAnteBranch := middleware.WithBranchAnte() | ||
| beginRunMsgsBranch, endRunMsgsBranch := middleware.WithBranchRunMsgs() |
There was a problem hiding this comment.
not sure if this is the best API, but couldn't think of anything else for now
|
I generally think cache wrap and branch aren't the clearest names for what this does outside of the cosmos space. In other db terminology people would say transaction isolation. So maybe |
|
R4R. @aaronc I remember having this conversation already: #8102 (comment). At that time it was decided to use "branching", and docs have been updated to reflect that. I can create an issue to rename that wording, but I think this PR should still use "branching" to be consistent. |
x/auth/middleware/branch_store.go
Outdated
| } | ||
|
|
||
| // CheckTx implements tx.Handler.CheckTx method. | ||
| // Do nothing during CheckTx. | ||
| func (sh branchStoreHandler) CheckTx(ctx context.Context, req tx.Request, checkReq tx.RequestCheckTx) (tx.Response, tx.ResponseCheckTx, error) { | ||
| func (sh anteBranchBegin) CheckTx(ctx context.Context, req tx.Request, checkReq tx.RequestCheckTx) (tx.Response, tx.ResponseCheckTx, error) { | ||
| return sh.next.CheckTx(ctx, req, checkReq) |
There was a problem hiding this comment.
why do we just call next here? I recall that in previous versions, we also write state (to checkTxState)
There was a problem hiding this comment.
oh yeah, you're right, just had another look at 0.45's code.
fixed in 062ddd4
Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>
| WithBranchedStore, | ||
| // - Storage writes below this middleware should ALWAYS be discarded | ||
| // together when middleware fails, but those above can have state writes. | ||
| WithRunMsgsBranch, | ||
| // Consume block gas. All middlewares whose gas consumption after their `next` handler | ||
| // should be accounted for, should go below this middleware. | ||
| ConsumeBlockGasMiddleware, |
There was a problem hiding this comment.
looks alright, the WithRunMsgsBranch is just renamed WithBranchedStore right?
There was a problem hiding this comment.
would it be clearer if we keep two separate middleware stacks for ante handlers and runMsgs, and they are run under different branched stores (db transaction)?
Personally, I'm more familiar with the old runTx code structure, the panic recovery logic is also clearer in the old code structure.
| sdkCtx = sdkCtx.WithMultiStore(originalStore) | ||
| } | ||
|
|
||
| branchedStore.Write() |
There was a problem hiding this comment.
I'm confused...where do we actually check for errors if say there is an error in any of the middleware ante-handlers?
There was a problem hiding this comment.
If any of the antehandlers fails, then this anteBranchWrite one is never called. Take a look at the middleware stack in middleware.go, endAnteBranch is below all the antehandlers.
Co-authored-by: Levi Schoen <levismschoen@gmail.com>
Co-authored-by: Levi Schoen <levismschoen@gmail.com>
…smos-sdk into am/fix-branch-middleware
|
superseded by #11979 |
Description
This PR is to make sure v0.46 middleware logic matches v0.45's baseapp runTx logic
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!to the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!in the type prefix if API or client breaking change