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

feat: CheckTX Priority #10507

Merged
merged 8 commits into from
Nov 23, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions x/auth/middleware/fee.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,12 @@ import (
"context"
"fmt"

abci "github.com/tendermint/tendermint/abci/types"

sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"

"github.com/cosmos/cosmos-sdk/types/tx"
"github.com/cosmos/cosmos-sdk/x/auth/types"
abci "github.com/tendermint/tendermint/abci/types"
)

var _ tx.Handler = mempoolFeeTxHandler{}
Expand All @@ -30,7 +30,12 @@ func MempoolFeeMiddleware(txh tx.Handler) tx.Handler {
}
}

// CheckTx implements tx.Handler.CheckTx.
// CheckTx implements tx.Handler.CheckTx. It is responsible for determining if a
// transaction's fees meet the required minimum of the processing node. Note, a
// node can have zero fees set as the minimum. If non-zero minimum fees are set
// and the transaction does not meet the minimum, the transaction is rejected.
//
// Recall, a transaction's fee is determined by ceil(minGasPrice * gasLimit).
func (txh mempoolFeeTxHandler) CheckTx(ctx context.Context, tx sdk.Tx, req abci.RequestCheckTx) (abci.ResponseCheckTx, error) {
sdkCtx := sdk.UnwrapSDKContext(ctx)

Expand Down
1 change: 1 addition & 0 deletions x/auth/middleware/middleware.go
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ func NewDefaultTxHandler(options TxHandlerOptions) (tx.Handler, error) {
ValidateMemoMiddleware(options.AccountKeeper),
ConsumeTxSizeGasMiddleware(options.AccountKeeper),
DeductFeeMiddleware(options.AccountKeeper, options.BankKeeper, options.FeegrantKeeper),
TxPriorityHandler,
SetPubKeyMiddleware(options.AccountKeeper),
ValidateSigCountMiddleware(options.AccountKeeper),
SigGasConsumeMiddleware(options.AccountKeeper, sigGasConsumer),
Expand Down
62 changes: 62 additions & 0 deletions x/auth/middleware/tx_priority.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
package middleware

import (
"context"

abci "github.com/tendermint/tendermint/abci/types"

sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
"github.com/cosmos/cosmos-sdk/types/tx"
)

var _ tx.Handler = txPriorityHandler{}

type txPriorityHandler struct {
next tx.Handler
}

// TxPriorityHandler implements tx handling middleware that determines a
// transaction's priority via a naive mechanism -- the total sum of fees provided.
// It sets the Priority in ResponseCheckTx only.
func TxPriorityHandler(txh tx.Handler) tx.Handler {
return txPriorityHandler{next: txh}
}

// CheckTx implements tx.Handler.CheckTx. We set the Priority of the transaction
// to be ordered in the Tendermint mempool based naively on the total sum of all
// fees included. Applications that need more sophisticated mempool ordering
// should look to implement their own fee handling middleware instead of using
// TxPriorityHandler.
func (h txPriorityHandler) CheckTx(ctx context.Context, tx sdk.Tx, req abci.RequestCheckTx) (abci.ResponseCheckTx, error) {
feeTx, ok := tx.(sdk.FeeTx)
if !ok {
return abci.ResponseCheckTx{}, sdkerrors.Wrap(sdkerrors.ErrTxDecode, "Tx must be a FeeTx")
}

feeCoins := feeTx.GetFee()

res, err := h.next.CheckTx(ctx, tx, req)
res.Priority = GetTxPriority(feeCoins)

return res, err
}

func (h txPriorityHandler) DeliverTx(ctx context.Context, tx sdk.Tx, req abci.RequestDeliverTx) (abci.ResponseDeliverTx, error) {
return h.next.DeliverTx(ctx, tx, req)
}

func (h txPriorityHandler) SimulateTx(ctx context.Context, sdkTx sdk.Tx, req tx.RequestSimulateTx) (tx.ResponseSimulateTx, error) {
return h.next.SimulateTx(ctx, sdkTx, req)
}

// GetTxPriority returns a naive tx priority based on the total sum of all fees
// provided in a transaction.
func GetTxPriority(fee sdk.Coins) int64 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

how about renaming to ComputeTxPriority?

var priority int64
for _, c := range fee {
priority += c.Amount.Int64()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
priority += c.Amount.Int64()
p := c.Amount.Int64()
if priority == 0 || p < priority {
priority = p
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know you're just taking the minimum here. What does this really buy you? If all fees provided are valid denominations, what is the issue exactly?

Copy link
Member

Choose a reason for hiding this comment

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

but the denominations have different prices and settling on the minimum makes it hard to use a cheaper denom alongside the primary denom to boost a tx. the primary denom will take precedence with the minimum

Copy link
Member

Choose a reason for hiding this comment

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

I would say this is out of scope of a naive implementation. Its either we spend time designing a new way to do this, which we have pre open for but no reviews, or we document teams how do this and say they have to do it based on their use case. We don't have the man power to design yet another thing right now

Maybe adding a warning godoc could benefit this.

Copy link
Member

Choose a reason for hiding this comment

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

But why is there a lot of design needed? It seems like just taking the min solves a lot to potential issues and is a very simple code change

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree with Aaron, what if one denom has exponent = 2, and another one 6? Adding 2 = 0.2 (fee from first denom) to 500 = 0.0005 (fee from the secon denom) doesn't really make sense. So at least we should document / add a comment, that his is a naive implementation.

}

return priority
}