Skip to content
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

refactor(baseapp): create checktx handler #21979

Merged
merged 13 commits into from
Oct 2, 2024
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ Every module contains its own CHANGELOG.md. Please refer to the module you are i
* (client/keys) [#21829](https://github.com/cosmos/cosmos-sdk/pull/21829) Add support for importing hex key using standard input.
* (x/validate) [#21822](https://github.com/cosmos/cosmos-sdk/pull/21822) New module solely responsible for providing ante/post handlers and tx validators for v2. It can be extended by the app developer to provide extra tx validators.
* In comparison to x/auth/tx/config, there is no app config to skip ante/post handlers, as overwriting them in baseapp or not injecting the x/validate module has the same effect.
* (baeapp) [#21979](https://github.com/cosmos/cosmos-sdk/pull/21979) Create CheckTxHandler to allow extending the logic of CheckTx.

### Improvements

Expand Down
29 changes: 19 additions & 10 deletions baseapp/abci.go
Original file line number Diff line number Diff line change
Expand Up @@ -367,18 +367,27 @@ func (app *BaseApp) CheckTx(req *abci.CheckTxRequest) (*abci.CheckTxResponse, er
return nil, fmt.Errorf("unknown RequestCheckTx type: %s", req.Type)
}

gInfo, result, anteEvents, err := app.runTx(mode, req.Tx)
if err != nil {
return responseCheckTxWithEvents(err, gInfo.GasWanted, gInfo.GasUsed, anteEvents, app.trace), nil
if app.checkTxHandler == nil {
gInfo, result, anteEvents, err := app.runTx(mode, req.Tx, nil)
if err != nil {
return responseCheckTxWithEvents(err, gInfo.GasWanted, gInfo.GasUsed, anteEvents, app.trace), nil
}

return &abci.CheckTxResponse{
GasWanted: int64(gInfo.GasWanted), // TODO: Should type accept unsigned ints?
GasUsed: int64(gInfo.GasUsed), // TODO: Should type accept unsigned ints?
Log: result.Log,
Data: result.Data,
Events: sdk.MarkEventsToIndex(result.Events, app.indexEvents),
}, nil
Comment on lines +370 to +382
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Resolve TODO comments regarding gas value types

The code includes TODO comments questioning whether GasWanted and GasUsed should accept unsigned integers. Since gas values are inherently non-negative, consider using uint64 instead of int64 to accurately represent these values and prevent potential issues with negative values.

}

return &abci.CheckTxResponse{
GasWanted: int64(gInfo.GasWanted), // TODO: Should type accept unsigned ints?
GasUsed: int64(gInfo.GasUsed), // TODO: Should type accept unsigned ints?
Log: result.Log,
Data: result.Data,
Events: sdk.MarkEventsToIndex(result.Events, app.indexEvents),
}, nil
// Create wrapper to avoid users overriding the execution mode
runTx := func(txBytes []byte, tx sdk.Tx) (gInfo sdk.GasInfo, result *sdk.Result, anteEvents []abci.Event, err error) {
return app.runTx(mode, txBytes, tx)
}

return app.checkTxHandler(runTx, req)
}

// PrepareProposal implements the PrepareProposal ABCI method and returns a
Expand Down
21 changes: 12 additions & 9 deletions baseapp/baseapp.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ import (
)

type (
execMode uint8
execMode = uint8
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Avoid using type aliases; prefer type definitions

According to the Uber Go Style Guide, type aliases should be avoided. Consider defining execMode as a new type instead of an alias for better type safety.

-type (
-	execMode = uint8
+type (
+	execMode uint8
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
execMode = uint8
execMode uint8

tac0turtle marked this conversation as resolved.
Show resolved Hide resolved

// StoreLoader defines a customizable function to control how we load the
// CommitMultiStore from disk. This is useful for state migration, when
Expand Down Expand Up @@ -91,6 +91,7 @@ type BaseApp struct {
prepareCheckStater sdk.PrepareCheckStater // logic to run during commit using the checkState
precommiter sdk.Precommiter // logic to run during commit using the deliverState
versionModifier server.VersionModifier // interface to get and set the app version
checkTxHandler sdk.CheckTxHandler
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Group checkTxHandler with related handler fields for consistency

Per the Uber Go Style Guide, related struct fields should be grouped together. Consider moving checkTxHandler next to other handler fields to improve readability.

     postHandler        sdk.PostHandler                // post handler, optional
+    checkTxHandler     sdk.CheckTxHandler             // ABCI CheckTx handler
     initChainer        sdk.InitChainer                // ABCI InitChain handler
     preBlocker         sdk.PreBlocker                 // logic to run before BeginBlocker
     beginBlocker       sdk.BeginBlocker               // (legacy ABCI) BeginBlock handler
     endBlocker         sdk.EndBlocker                 // (legacy ABCI) EndBlock handler
-    checkTxHandler     sdk.CheckTxHandler
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
checkTxHandler sdk.CheckTxHandler
postHandler sdk.PostHandler // post handler, optional
checkTxHandler sdk.CheckTxHandler // ABCI CheckTx handler
initChainer sdk.InitChainer // ABCI InitChain handler
preBlocker sdk.PreBlocker // logic to run before BeginBlocker
beginBlocker sdk.BeginBlocker // (legacy ABCI) BeginBlock handler
endBlocker sdk.EndBlocker // (legacy ABCI) EndBlock handler

Copy link
Member

@julienrbrt julienrbrt Sep 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for adding a comment and moving it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@julienrbrt, understood! Adding a comment to checkTxHandler will improve clarity. Here's the updated code:

	checkTxHandler     sdk.CheckTxHandler             // ABCI CheckTx handler

😊🐰


addrPeerFilter sdk.PeerFilter // filter peers by address and port
idPeerFilter sdk.PeerFilter // filter peers by node ID
Expand Down Expand Up @@ -688,7 +689,6 @@ func (app *BaseApp) getContextForTx(mode execMode, txBytes []byte) sdk.Context {
// a branched multi-store.
func (app *BaseApp) cacheTxContext(ctx sdk.Context, txBytes []byte) (sdk.Context, storetypes.CacheMultiStore) {
ms := ctx.MultiStore()
// TODO: https://github.com/cosmos/cosmos-sdk/issues/2824
msCache := ms.CacheMultiStore()
if msCache.TracingEnabled() {
msCache = msCache.SetTracingContext(
Expand Down Expand Up @@ -761,7 +761,7 @@ func (app *BaseApp) deliverTx(tx []byte) *abci.ExecTxResult {
telemetry.SetGauge(float32(gInfo.GasWanted), "tx", "gas", "wanted")
}()

gInfo, result, anteEvents, err := app.runTx(execModeFinalize, tx)
gInfo, result, anteEvents, err := app.runTx(execModeFinalize, tx, nil)
if err != nil {
resultStr = "failed"
resp = responseExecTxResultWithEvents(
Expand Down Expand Up @@ -822,7 +822,7 @@ type HasNestedMsgs interface {
// Note, gas execution info is always returned. A reference to a Result is
// returned if the tx does not run out of gas and if all the messages are valid
// and execute successfully. An error is returned otherwise.
func (app *BaseApp) runTx(mode execMode, txBytes []byte) (gInfo sdk.GasInfo, result *sdk.Result, anteEvents []abci.Event, err error) {
func (app *BaseApp) runTx(mode execMode, txBytes []byte, tx sdk.Tx) (gInfo sdk.GasInfo, result *sdk.Result, anteEvents []abci.Event, err error) {
julienrbrt marked this conversation as resolved.
Show resolved Hide resolved
// NOTE: GasWanted should be returned by the AnteHandler. GasUsed is
// determined by the GasMeter. We need access to the context to get the gas
// meter, so we initialize upfront.
Expand Down Expand Up @@ -870,9 +870,12 @@ func (app *BaseApp) runTx(mode execMode, txBytes []byte) (gInfo sdk.GasInfo, res
defer consumeBlockGas()
}

tx, err := app.txDecoder(txBytes)
if err != nil {
return sdk.GasInfo{GasUsed: 0, GasWanted: 0}, nil, nil, sdkerrors.ErrTxDecode.Wrap(err.Error())
// if the transaction is not decoded, decode it here
if tx == nil {
tx, err = app.txDecoder(txBytes)
if err != nil {
return sdk.GasInfo{GasUsed: 0, GasWanted: 0}, nil, nil, sdkerrors.ErrTxDecode.Wrap(err.Error())
}
}

msgs := tx.GetMsgs()
Expand Down Expand Up @@ -1160,7 +1163,7 @@ func (app *BaseApp) PrepareProposalVerifyTx(tx sdk.Tx) ([]byte, error) {
return nil, err
}

_, _, _, err = app.runTx(execModePrepareProposal, bz)
_, _, _, err = app.runTx(execModePrepareProposal, bz, tx)
if err != nil {
return nil, err
Comment on lines +1168 to 1170
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider refactoring to eliminate code duplication

Both PrepareProposalVerifyTx and ProcessProposalVerifyTx perform similar transaction verification by calling runTx and handling the error in the same way. Refactoring this common logic into a shared helper function could improve maintainability.

Also applies to: 1185-1187

}
Expand All @@ -1179,7 +1182,7 @@ func (app *BaseApp) ProcessProposalVerifyTx(txBz []byte) (sdk.Tx, error) {
return nil, err
}

_, _, _, err = app.runTx(execModeProcessProposal, txBz)
_, _, _, err = app.runTx(execModeProcessProposal, txBz, tx)
if err != nil {
return nil, err
}
Expand Down
9 changes: 9 additions & 0 deletions baseapp/options.go
Original file line number Diff line number Diff line change
Expand Up @@ -367,6 +367,15 @@ func (app *BaseApp) SetPrepareProposal(handler sdk.PrepareProposalHandler) {
app.prepareProposal = handler
}

// SetCheckTx sets the checkTx function for the BaseApp.
func (app *BaseApp) SetCheckTxHandler(handler sdk.CheckTxHandler) {
if app.sealed {
panic("SetCheckTx() on sealed BaseApp")
}

app.checkTxHandler = handler
}

func (app *BaseApp) SetExtendVoteHandler(handler sdk.ExtendVoteHandler) {
if app.sealed {
panic("SetExtendVoteHandler() on sealed BaseApp")
Expand Down
6 changes: 3 additions & 3 deletions baseapp/test_helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,13 @@ func (app *BaseApp) SimCheck(txEncoder sdk.TxEncoder, tx sdk.Tx) (sdk.GasInfo, *
return sdk.GasInfo{}, nil, errorsmod.Wrapf(sdkerrors.ErrInvalidRequest, "%s", err)
}

gasInfo, result, _, err := app.runTx(execModeCheck, bz)
gasInfo, result, _, err := app.runTx(execModeCheck, bz, tx)
return gasInfo, result, err
}

// Simulate executes a tx in simulate mode to get result and gas info.
func (app *BaseApp) Simulate(txBytes []byte) (sdk.GasInfo, *sdk.Result, error) {
gasInfo, result, _, err := app.runTx(execModeSimulate, txBytes)
gasInfo, result, _, err := app.runTx(execModeSimulate, txBytes, nil)
return gasInfo, result, err
}

Expand All @@ -36,7 +36,7 @@ func (app *BaseApp) SimDeliver(txEncoder sdk.TxEncoder, tx sdk.Tx) (sdk.GasInfo,
return sdk.GasInfo{}, nil, errorsmod.Wrapf(sdkerrors.ErrInvalidRequest, "%s", err)
}

gasInfo, result, _, err := app.runTx(execModeFinalize, bz)
gasInfo, result, _, err := app.runTx(execModeFinalize, bz, tx)
return gasInfo, result, err
}

Expand Down
50 changes: 50 additions & 0 deletions docs/build/abci/04-checktx.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
# CheckTx

CheckTx is called by the `BaseApp` when comet receives a transaction from a client, over the p2p network or RPC. The CheckTx method is responsible for validating the transaction and returning an error if the transaction is invalid.

```mermaid
graph TD
subgraph SDK[Cosmos SDK]
B[Baseapp]
A[AnteHandlers]
B <-->|Validate TX| A
end
C[CometBFT] <-->|CheckTx|SDK
U((User)) -->|Submit TX| C
N[P2P] -->|Receive TX| C
```

```go reference
https://github.com/cosmos/cosmos-sdk/blob/31c604762a434c7b676b6a89897ecbd7c4653a23/baseapp/abci.go#L350-L390
```

## CheckTx Handler

`CheckTxHandler` allows users to extend the logic of `CheckTx`. `CheckTxHandler` is called by pasding context and the transaction bytes received through ABCI. It is required that the handler returns deterministic results given the same transaction bytes.

:::note
we return the raw decoded transaction here to avoid decoding it twice.
:::

```go
type CheckTxHandler func(ctx sdk.Context, tx []byte) (Tx, error)
```

Setting a custom `CheckTxHandler` is optional. It can be done from your app.go file:

```go
func NewSimApp(
logger log.Logger,
db corestore.KVStoreWithBatch,
traceStore io.Writer,
loadLatest bool,
appOpts servertypes.AppOptions,
baseAppOptions ...func(*baseapp.BaseApp),
) *SimApp {
...
// Create ChecktxHandler
checktxHandler := abci.NewCustomCheckTxHandler(...)
app.SetCheckTxHandler(checktxHandler)
...
}
```
2 changes: 1 addition & 1 deletion docs/build/building-apps/02-app-mempool.md
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ Notably it introduces the `PrepareProposal` and `ProcessProposal` steps of ABCI+

## Mempool

+ Before we delve into `PrepareProposal` and `ProcessProposal`, let's first walk through the mempool concepts.
* Before we delve into `PrepareProposal` and `ProcessProposal`, let's first walk through the mempool concepts.

There are countless designs that an application developer can write for a mempool, the SDK opted to provide only simple mempool implementations.
Namely, the SDK provides the following mempools:
Expand Down
7 changes: 7 additions & 0 deletions types/abci.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,13 @@ type ProcessProposalHandler func(Context, *abci.ProcessProposalRequest) (*abci.P
// PrepareProposalHandler defines a function type alias for preparing a proposal
type PrepareProposalHandler func(Context, *abci.PrepareProposalRequest) (*abci.PrepareProposalResponse, error)

// CheckTxHandler defines a function type alias for executing logic before transactions are executed.
// `RunTx` is a function type alias for executing logic before transactions are executed.
testinginprod marked this conversation as resolved.
Show resolved Hide resolved
type CheckTxHandler func(RunTx, *abci.CheckTxRequest) (*abci.CheckTxResponse, error)

// RunTx defines a function type alias for executing logic before transactions are executed.
tac0turtle marked this conversation as resolved.
Show resolved Hide resolved
type RunTx func(txBytes []byte, tx Tx) (gInfo GasInfo, result *Result, anteEvents []abci.Event, err error)

// ExtendVoteHandler defines a function type alias for extending a pre-commit vote.
type ExtendVoteHandler func(Context, *abci.ExtendVoteRequest) (*abci.ExtendVoteResponse, error)

Expand Down
Loading