made all Application.CheckTx calls recover panics (CON-259)#3334
Conversation
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
pompon0
changed the title
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3334 +/- ##
==========================================
+ Coverage 59.18% 59.20% +0.02%
==========================================
Files 2097 2098 +1
Lines 172517 172554 +37
==========================================
+ Hits 102107 102166 +59
+ Misses 61558 61541 -17
+ Partials 8852 8847 -5
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
| if err != nil { | ||
| res := sdkerrors.ResponseCheckTx(err, 0, 0, false) | ||
| return &abci.ResponseCheckTxV2{ResponseCheckTx: &res}, err | ||
| return nil, err |
There was a problem hiding this comment.
Why are we returning a nil result here now? Would this break any downstream tools?
There was a problem hiding this comment.
CheckTx is only called by tendermint
| res, err := txmp.app.CheckTxSafe(ctx, &abci.RequestCheckTxV2{Tx: tx}) | ||
| if err != nil { | ||
| removeHandler(true) | ||
| res.Log = txmp.AppendCheckTxErr(res.Log, err.Error()) |
There was a problem hiding this comment.
Do we not need the error on log any more?
There was a problem hiding this comment.
there is no res any more, so there is no Log to add it to.
| defer addTimeSample(app.metrics.MethodTiming.With("method", "check_tx", "type", "sync"))() | ||
| defer func() { | ||
| if r := recover(); r != nil { | ||
| err = fmt.Errorf("panic recovered in CheckTxSafe: %v", r) |
There was a problem hiding this comment.
Should we attach debug.Stack() here for debugging?
Maybe we should log this as well?
There was a problem hiding this comment.
it will be logged by the caller. I will add the stack.
| // sm.SaveState(stateDB,state) //height 1's validatorsInfo already saved in LoadStateFromDBOrGenesisDoc above | ||
|
|
||
| css[i] = newStateWithConfig(t, thisConfig, state, privVal, app) | ||
| proxyApp := proxy.New(app, proxy.NopMetrics()) |
There was a problem hiding this comment.
nit: if it's always proxy.NopMetrics() maybe we can use a constructor to hide this.
There was a problem hiding this comment.
in prod we use actual metrics. In tests we usually use kvstore.NewProxy() to hide this, but in those cases where app needs to be adjusted before wrapping we call proxy.New() directly
…ol#3386) sei-protocol#3334 made TxMempool.CheckTx and Application.CheckTx return an error instead of ResponseCheckTx with an error code. This PR reverts to the previous behavior. Since there is no case in which Application.CheckTx returns unwrapped error, the signature has been tightened to always return ResponseCheckTxV2. Proxy.CheckTxSafe() keeps returning (*ResponseCheckTxV2,error) though, so that in case of panic the stack trace is not send as part of the RPC response.
3 participants
This has been achieved by:
Additionally removed some remains of stateful leader election tests, unused PersistentKVStore.