[STAB-31] Allow AnteHandler to control locking semantics. #37
Conversation
|
@lcwik your pull request is missing a changelog! |
lcwik
force-pushed
the
lukestab31
branch
2 times, most recently
from
3becbc2 to
6a70523
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| WithTxBytes(txBytes) | ||
| // WithVoteInfos(app.voteInfos) // TODO: identify if this is needed | ||
|
|
||
| // Use a new gas meter since loading the consensus params below consumes gas and causes a race condition. | ||
| // | ||
| // TODO(STAB-35): See if this can be removed. | ||
| ctx = ctx.WithGasMeter(storetypes.NewInfiniteGasMeter()) |
There was a problem hiding this comment.
Change potentially affects state.
Call sequence:
(*github.com/cosmos/cosmos-sdk/baseapp.BaseApp).getContextForTx (baseapp/baseapp.go:647)
(*github.com/cosmos/cosmos-sdk/baseapp.BaseApp).runTx (baseapp/baseapp.go:943)
(*github.com/cosmos/cosmos-sdk/baseapp.BaseApp).deliverTx (baseapp/baseapp.go:734)
(*github.com/cosmos/cosmos-sdk/baseapp.BaseApp).internalFinalizeBlock (baseapp/baseapp.go:725)
(*github.com/cosmos/cosmos-sdk/baseapp.BaseApp).FinalizeBlock (baseapp/baseapp.go:886)
Updated existing usages of AnteHandler in cosmos to use the instance which always locks restoring the existing contract.
| // We also guarantee that the passed in context is held with a read lock allowing for concurrent | ||
| // execution. | ||
| anteCtx := ctx.WithEventManager(sdk.NewEventManager()) | ||
| newCtx, err = app.anteHandler(anteCtx, tx, false /* mode == execModeSimulate */) |
There was a problem hiding this comment.
Understand that all callsites never make this end up as execModeSimulate, but why did you make this explicitly false?
There was a problem hiding this comment.
super minor perf optimization to reduce branch complexity
| @@ -842,8 +847,8 @@ func (app *BaseApp) runCheckTxConcurrently(mode execMode, txBytes []byte) (gInfo | |||
| // embedded here to ensure that the lifetime of the mutex is limited to only this function allowing | |||
| // for the return values to be computed without holding the lock. | |||
| func() { | |||
| app.mtx.Lock() | |||
| defer app.mtx.Unlock() | |||
| app.mtx.RLock() | |||
There was a problem hiding this comment.
Why are we confident that mutation after cacheTxContext is either safe or not done?
There was a problem hiding this comment.
We are not. This PR changes the contract so that the AnteHandler must ensure concurrency correctness and why I added a mutex lock AnteDecorator as the first one so that the existing concurrency model was enforced (i.e. a single global lock).
Description
[STAB-31] Allow AnteHandler to control locking semantics.
Updated existing usages of AnteHandler in cosmos to use the instance which always locks restoring the existing contract.
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.mdmake lintandmake testReviewers 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