From 7819c47eb235345a551ef1b94c33958716845886 Mon Sep 17 00:00:00 2001 From: itsasecret <87604944+tudorpintea999@users.noreply.github.com> Date: Sun, 15 Dec 2024 10:45:25 +0200 Subject: [PATCH 1/5] Update account_number.go MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Renamed variables for clarity (e.g., ak → authKeeper, num → currentAccountNum). • Added more descriptive comments for each step of the process. • Provided more informative error messages using sdk.ErrInternal and sdk.ErrInvalidRequest. • Added a type assertion with a fallback to handle potential incorrect types for AccountKeeper. • Highlighted the rationale for using a gas-free context when interacting with the account number. • Clearly defined the increment value (1_000_000) and its variation during simulation to make the logic easier to understand. • Retained flexibility to handle additional modes by centralizing the execution mode check. --- app/ante/accnum/account_number.go | 44 +++++++++++++++++++------------ 1 file changed, 27 insertions(+), 17 deletions(-) diff --git a/app/ante/accnum/account_number.go b/app/ante/accnum/account_number.go index 7f3935e6..2fc893cf 100644 --- a/app/ante/accnum/account_number.go +++ b/app/ante/accnum/account_number.go @@ -2,47 +2,57 @@ package accnum import ( storetypes "cosmossdk.io/store/types" - sdk "github.com/cosmos/cosmos-sdk/types" cosmosante "github.com/cosmos/cosmos-sdk/x/auth/ante" authkeeper "github.com/cosmos/cosmos-sdk/x/auth/keeper" ) -// AccountNumberDecorator is a custom ante handler that increments the account number depending on -// the execution mode (Simulate, CheckTx, Finalize). -// -// This is to avoid account number conflicts when running concurrent Simulate, CheckTx, and Finalize. +// AccountNumberDecorator is a custom AnteHandler that increments the account number +// to avoid conflicts when running concurrent Simulate, CheckTx, and Finalize operations. type AccountNumberDecorator struct { - ak cosmosante.AccountKeeper + accountKeeper cosmosante.AccountKeeper } -// NewAccountNumberDecorator creates a new instance of AccountNumberDecorator. -func NewAccountNumberDecorator(ak cosmosante.AccountKeeper) AccountNumberDecorator { - return AccountNumberDecorator{ak} +// NewAccountNumberDecorator creates a new AccountNumberDecorator. +func NewAccountNumberDecorator(accountKeeper cosmosante.AccountKeeper) AccountNumberDecorator { + return AccountNumberDecorator{accountKeeper: accountKeeper} } -// AnteHandle is the AnteHandler implementation for AccountNumberDecorator. +// AnteHandle implements the AnteHandler interface. +// It increments the account number depending on the execution mode (Simulate, CheckTx). func (and AccountNumberDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler) (sdk.Context, error) { + // Skip account number modification for FinalizeTx or DeliverTx. if !ctx.IsCheckTx() && !ctx.IsReCheckTx() && !simulate { return next(ctx, tx, simulate) } - ak := and.ak.(*authkeeper.AccountKeeper) + // Safely cast to the concrete implementation of AccountKeeper. + authKeeper, ok := and.accountKeeper.(*authkeeper.AccountKeeper) + if !ok { + return ctx, sdk.ErrInvalidRequest.Wrap("invalid AccountKeeper type") + } + // Create a gas-free context to interact with the account number storage. gasFreeCtx := ctx.WithGasMeter(storetypes.NewInfiniteGasMeter()) - num, err := ak.AccountNumber.Peek(gasFreeCtx) + + // Peek at the current account number. + currentAccountNum, err := authKeeper.AccountNumber.Peek(gasFreeCtx) if err != nil { - return ctx, err + return ctx, sdk.ErrInternal.Wrapf("failed to peek account number: %v", err) } - accountNumAddition := uint64(1_000_000) + // Determine the increment value based on the execution mode. + accountNumIncrement := uint64(1_000_000) if simulate { - accountNumAddition += 1_000_000 + accountNumIncrement += 1_000_000 } - if err := ak.AccountNumber.Set(gasFreeCtx, num+accountNumAddition); err != nil { - return ctx, err + // Increment and set the account number. + newAccountNum := currentAccountNum + accountNumIncrement + if err := authKeeper.AccountNumber.Set(gasFreeCtx, newAccountNum); err != nil { + return ctx, sdk.ErrInternal.Wrapf("failed to set account number: %v", err) } + // Proceed to the next AnteHandler. return next(ctx, tx, simulate) } From d1d292526807cc35089f0a8bf9c9b2585d2093ea Mon Sep 17 00:00:00 2001 From: itsasecret <87604944+tudorpintea999@users.noreply.github.com> Date: Sun, 15 Dec 2024 10:47:40 +0200 Subject: [PATCH 2/5] Update sigverify.go MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Broke down large functions (AnteHandle) into smaller, focused helper methods (verifySignature, performSignatureVerification, etc.). • Clearer variable names (sig → signature, acc → account). • Simplified nested logic by early returns and helpers. • Added context-specific error messages for better debugging. • Ensured consistency in gas calculation logic. • Centralized repeated operations into helper functions (e.g., performSignatureVerification). • Modularized logic for easier testing and extension. --- app/ante/sigverify/sigverify.go | 186 ++++++++++++++++---------------- 1 file changed, 90 insertions(+), 96 deletions(-) diff --git a/app/ante/sigverify/sigverify.go b/app/ante/sigverify/sigverify.go index 29835b14..8290fd45 100644 --- a/app/ante/sigverify/sigverify.go +++ b/app/ante/sigverify/sigverify.go @@ -25,133 +25,139 @@ import ( "github.com/initia-labs/initia/crypto/ethsecp256k1" ) +// Simulation signature values used to estimate gas consumption. var ( - // simulation signature values used to estimate gas consumption key = make([]byte, secp256k1.PubKeySize) simSecp256k1Pubkey = &secp256k1.PubKey{Key: key} ) func init() { - // This decodes a valid hex string into a sepc256k1Pubkey for use in transaction simulation + // Decode a valid hex string into a secp256k1 public key for transaction simulation. bz, _ := hex.DecodeString("035AD6810A47F073553FF30D2FCC7E0D3B1C0B74B61A1AAA2582344037151E143A") copy(key, bz) simSecp256k1Pubkey.Key = key } -// SigVerificationDecorator verifies all signatures for a tx and return an error if any are invalid. Note, -// the SigVerificationDecorator will not check signatures on ReCheck. -// -// CONTRACT: Pubkeys are set in context for all signers before this decorator runs -// CONTRACT: Tx must implement SigVerifiableTx interface +// SigVerificationDecorator verifies all signatures in a transaction and ensures their validity. +// Note: This decorator skips signature verification during ReCheckTx. type SigVerificationDecorator struct { - ak authante.AccountKeeper + accountKeeper authante.AccountKeeper signModeHandler *txsigning.HandlerMap } -func NewSigVerificationDecorator(ak authante.AccountKeeper, signModeHandler *txsigning.HandlerMap) SigVerificationDecorator { +// NewSigVerificationDecorator creates a new SigVerificationDecorator. +func NewSigVerificationDecorator(accountKeeper authante.AccountKeeper, signModeHandler *txsigning.HandlerMap) SigVerificationDecorator { return SigVerificationDecorator{ - ak: ak, + accountKeeper: accountKeeper, signModeHandler: signModeHandler, } } -func (svd SigVerificationDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler) (newCtx sdk.Context, err error) { +// AnteHandle verifies signatures and enforces sequence numbers in transactions. +func (svd SigVerificationDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler) (sdk.Context, error) { sigTx, ok := tx.(authsigning.Tx) if !ok { return ctx, errorsmod.Wrap(sdkerrors.ErrTxDecode, "invalid transaction type") } - // stdSigs contains the sequence number, account number, and signatures. - // When simulating, this would just be a 0-length slice. - sigs, err := sigTx.GetSignaturesV2() + // Get signatures and signers from the transaction. + signatures, err := sigTx.GetSignaturesV2() if err != nil { return ctx, err } - signers, err := sigTx.GetSigners() if err != nil { return ctx, err } - // check that signer length and signature length are the same - if len(sigs) != len(signers) { - return ctx, errorsmod.Wrapf(sdkerrors.ErrUnauthorized, "invalid number of signer; expected: %d, got %d", len(signers), len(sigs)) + // Ensure the number of signatures matches the number of signers. + if len(signatures) != len(signers) { + return ctx, errorsmod.Wrapf(sdkerrors.ErrUnauthorized, "mismatch in number of signers and signatures: expected %d, got %d", len(signers), len(signatures)) } - for i, sig := range sigs { - acc, err := authante.GetSignerAcc(ctx, svd.ak, signers[i]) - if err != nil { + for i, signature := range signatures { + if err := svd.verifySignature(ctx, signers[i], signature, tx, simulate); err != nil { return ctx, err } + } - // retrieve pubkey - pubKey := acc.GetPubKey() - if !simulate && pubKey == nil { - return ctx, errorsmod.Wrap(sdkerrors.ErrInvalidPubKey, "pubkey on account is not set") - } + return next(ctx, tx, simulate) +} - // Check account sequence number. - if sig.Sequence != acc.GetSequence() { - return ctx, errorsmod.Wrapf( - sdkerrors.ErrWrongSequence, - "account sequence mismatch, expected %d, got %d", acc.GetSequence(), sig.Sequence, - ) - } +// verifySignature performs validation on a single signature. +func (svd SigVerificationDecorator) verifySignature(ctx sdk.Context, signer sdk.AccAddress, signature signing.SignatureV2, tx sdk.Tx, simulate bool) error { + // Get the signer account. + account, err := authante.GetSignerAcc(ctx, svd.accountKeeper, signer) + if err != nil { + return err + } - // retrieve signer data - genesis := ctx.BlockHeight() == 0 - chainID := ctx.ChainID() - var accNum uint64 - if !genesis { - accNum = acc.GetAccountNumber() - } + pubKey := account.GetPubKey() + if !simulate && pubKey == nil { + return errorsmod.Wrap(sdkerrors.ErrInvalidPubKey, "account pubkey is not set") + } - // no need to verify signatures on recheck tx - if !simulate && !ctx.IsReCheckTx() { - anyPk, _ := codectypes.NewAnyWithValue(pubKey) - - signerData := txsigning.SignerData{ - Address: acc.GetAddress().String(), - ChainID: chainID, - AccountNumber: accNum, - Sequence: acc.GetSequence(), - PubKey: &anypb.Any{ - TypeUrl: anyPk.TypeUrl, - Value: anyPk.Value, - }, - } - adaptableTx, ok := tx.(authsigning.V2AdaptableTx) - if !ok { - return ctx, fmt.Errorf("expected tx to implement V2AdaptableTx, got %T", tx) - } - txData := adaptableTx.GetSigningTxData() - err = verifySignature(ctx, pubKey, signerData, sig.Data, svd.signModeHandler, txData) - if err != nil { - var errMsg string - if authante.OnlyLegacyAminoSigners(sig.Data) { - // If all signers are using SIGN_MODE_LEGACY_AMINO, we rely on VerifySignature to check account sequence number, - // and therefore communicate sequence number as a potential cause of error. - errMsg = fmt.Sprintf("signature verification failed; please verify account number (%d), sequence (%d) and chain-id (%s)", accNum, acc.GetSequence(), chainID) - } else { - errMsg = fmt.Sprintf("signature verification failed; please verify account number (%d) and chain-id (%s): (%s)", accNum, chainID, err.Error()) - } - return ctx, errorsmod.Wrap(sdkerrors.ErrUnauthorized, errMsg) - - } - } + // Check sequence number. + if signature.Sequence != account.GetSequence() { + return errorsmod.Wrapf(sdkerrors.ErrWrongSequence, "sequence mismatch: expected %d, got %d", account.GetSequence(), signature.Sequence) } - return next(ctx, tx, simulate) + // Skip signature verification for ReCheckTx. + if !simulate && !ctx.IsReCheckTx() { + return svd.performSignatureVerification(ctx, account, pubKey, signature, tx) + } + + return nil +} + +// performSignatureVerification verifies the signature using the provided pubkey and signer data. +func (svd SigVerificationDecorator) performSignatureVerification(ctx sdk.Context, account types.AccountI, pubKey sdk.PubKey, signature signing.SignatureV2, tx sdk.Tx) error { + genesis := ctx.BlockHeight() == 0 + chainID := ctx.ChainID() + accountNumber := account.GetAccountNumber() + if genesis { + accountNumber = 0 + } + + anyPk, _ := codectypes.NewAnyWithValue(pubKey) + signerData := txsigning.SignerData{ + Address: account.GetAddress().String(), + ChainID: chainID, + AccountNumber: accountNumber, + Sequence: account.GetSequence(), + PubKey: &anypb.Any{ + TypeUrl: anyPk.TypeUrl, + Value: anyPk.Value, + }, + } + + adaptableTx, ok := tx.(authsigning.V2AdaptableTx) + if !ok { + return fmt.Errorf("expected tx to implement V2AdaptableTx, got %T", tx) + } + + txData := adaptableTx.GetSigningTxData() + err := verifySignature(ctx, pubKey, signerData, signature.Data, svd.signModeHandler, txData) + if err != nil { + return wrapSignatureError(err, signerData, account.GetSequence()) + } + + return nil +} + +// wrapSignatureError provides a detailed error message for signature verification failures. +func wrapSignatureError(err error, signerData txsigning.SignerData, sequence uint64) error { + return errorsmod.Wrap(sdkerrors.ErrUnauthorized, fmt.Sprintf( + "signature verification failed: verify account number (%d), sequence (%d), and chain-id (%s): %s", + signerData.AccountNumber, sequence, signerData.ChainID, err.Error(), + )) } -// defaultSigVerificationGasConsumer is the default implementation of SignatureVerificationGasConsumer. It consumes gas -// for signature verification based upon the public key type. The cost is fetched from the given params and is matched -// by the concrete type. +// DefaultSigVerificationGasConsumer calculates gas consumption based on the public key type. func DefaultSigVerificationGasConsumer( meter storetypes.GasMeter, sig signing.SignatureV2, params types.Params, ) error { - pubkey := sig.PubKey - switch pubkey := pubkey.(type) { + switch pubkey := sig.PubKey.(type) { case *ed25519.PubKey: meter.ConsumeGas(params.SigVerifyCostED25519, "ante verify: ed25519") return errorsmod.Wrap(sdkerrors.ErrInvalidPubKey, "ED25519 public keys are unsupported") @@ -165,44 +171,32 @@ func DefaultSigVerificationGasConsumer( return nil case multisig.PubKey: - multisignature, ok := sig.Data.(*signing.MultiSignatureData) - if !ok { - return fmt.Errorf("expected %T, got, %T", &signing.MultiSignatureData{}, sig.Data) - } - err := consumeMultisignatureVerificationGas(meter, multisignature, pubkey, params, sig.Sequence) - if err != nil { - return err - } - return nil + return consumeMultisignatureVerificationGas(meter, sig.Data.(*signing.MultiSignatureData), pubkey, params, sig.Sequence) default: return errorsmod.Wrapf(sdkerrors.ErrInvalidPubKey, "unrecognized public key type: %T", pubkey) } } -// consumeMultisignatureVerificationGas consumes gas from a GasMeter for verifying a multisig pubkey signature +// consumeMultisignatureVerificationGas calculates gas for multisignature verification. func consumeMultisignatureVerificationGas( meter storetypes.GasMeter, sig *signing.MultiSignatureData, pubkey multisig.PubKey, params types.Params, accSeq uint64, ) error { size := sig.BitArray.Count() - sigIndex := 0 - - for i := 0; i < size; i++ { + for i, sigIndex := 0, 0; i < size; i++ { if !sig.BitArray.GetIndex(i) { continue } - sigV2 := signing.SignatureV2{ + subSig := signing.SignatureV2{ PubKey: pubkey.GetPubKeys()[i], Data: sig.Signatures[sigIndex], Sequence: accSeq, } - err := DefaultSigVerificationGasConsumer(meter, sigV2, params) - if err != nil { + if err := DefaultSigVerificationGasConsumer(meter, subSig, params); err != nil { return err } sigIndex++ } - return nil } From 6d07c410cd3ccfbed8aa5300c81558f7228fa341 Mon Sep 17 00:00:00 2001 From: itsasecret <87604944+tudorpintea999@users.noreply.github.com> Date: Sun, 15 Dec 2024 10:49:47 +0200 Subject: [PATCH 3/5] Update ante.go MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit validateHandlerOptions: Validates that all required dependencies are provided. • getTxFeeChecker: Retrieves the appropriate fee checker. • buildAnteDecorators: Constructs the AnteDecorator chain. • Grouped related operations together for easier understanding. • Enhanced the readability of the free lane fee checker by clarifying its logic. • Kept the design flexible by accommodating custom fee checkers (TxFeeChecker) and defaulting to a predefined one if not provided. --- app/ante/ante.go | 78 ++++++++++++++++++++++++++---------------------- 1 file changed, 43 insertions(+), 35 deletions(-) diff --git a/app/ante/ante.go b/app/ante/ante.go index c1574209..9ddaa9c4 100644 --- a/app/ante/ante.go +++ b/app/ante/ante.go @@ -20,8 +20,7 @@ import ( auctionkeeper "github.com/skip-mev/block-sdk/v2/x/auction/keeper" ) -// HandlerOptions extends the SDK's AnteHandler options by requiring the IBC -// channel keeper. +// HandlerOptions extends the SDK's AnteHandler options by including IBC channel keeper and custom handlers. type HandlerOptions struct { ante.HandlerOptions Codec codec.BinaryCodec @@ -33,50 +32,62 @@ type HandlerOptions struct { FreeLane block.Lane } -// NewAnteHandler returns an AnteHandler that checks and increments sequence -// numbers, checks signatures & account numbers, and deducts fees from the first -// signer. +// NewAnteHandler creates a custom AnteHandler pipeline for transaction processing. func NewAnteHandler(options HandlerOptions) (sdk.AnteHandler, error) { - if options.AccountKeeper == nil { - return nil, errors.Wrap(sdkerrors.ErrLogic, "account keeper is required for ante builder") - } - - if options.BankKeeper == nil { - return nil, errors.Wrap(sdkerrors.ErrLogic, "bank keeper is required for ante builder") + // Validate mandatory dependencies. + if err := validateHandlerOptions(options); err != nil { + return nil, err } - if options.SignModeHandler == nil { - return nil, errors.Wrap(sdkerrors.ErrLogic, "sign mode handler is required for ante builder") - } - - sigGasConsumer := options.SigGasConsumer - if sigGasConsumer == nil { - sigGasConsumer = sigverify.DefaultSigVerificationGasConsumer - } - - txFeeChecker := options.TxFeeChecker - if txFeeChecker == nil { - txFeeChecker = moveante.NewMempoolFeeChecker(options.MoveKeeper).CheckTxFeeWithMinGasPrices - } + // Default to provided or custom fee checker. + txFeeChecker := getTxFeeChecker(options) + // Define a custom free lane fee checker. freeLaneFeeChecker := func(ctx sdk.Context, tx sdk.Tx) (sdk.Coins, int64, error) { - // skip fee checker if the tx is free lane tx. if !options.FreeLane.Match(ctx, tx) { return txFeeChecker(ctx, tx) } - - // return fee without fee check feeTx, ok := tx.(sdk.FeeTx) if !ok { return nil, 0, errors.Wrap(sdkerrors.ErrTxDecode, "Tx must be a FeeTx") } - return feeTx.GetFee(), 1 /* FIFO */, nil } - anteDecorators := []sdk.AnteDecorator{ + // Create the AnteDecorators sequence. + anteDecorators := buildAnteDecorators(options, freeLaneFeeChecker) + + // Chain the AnteDecorators to construct the AnteHandler. + return sdk.ChainAnteDecorators(anteDecorators...), nil +} + +// validateHandlerOptions ensures all mandatory dependencies are provided. +func validateHandlerOptions(options HandlerOptions) error { + if options.AccountKeeper == nil { + return errors.Wrap(sdkerrors.ErrLogic, "account keeper is required for ante builder") + } + if options.BankKeeper == nil { + return errors.Wrap(sdkerrors.ErrLogic, "bank keeper is required for ante builder") + } + if options.SignModeHandler == nil { + return errors.Wrap(sdkerrors.ErrLogic, "sign mode handler is required for ante builder") + } + return nil +} + +// getTxFeeChecker returns the appropriate TxFeeChecker function. +func getTxFeeChecker(options HandlerOptions) func(sdk.Context, sdk.Tx) (sdk.Coins, int64, error) { + if options.TxFeeChecker != nil { + return options.TxFeeChecker + } + return moveante.NewMempoolFeeChecker(options.MoveKeeper).CheckTxFeeWithMinGasPrices +} + +// buildAnteDecorators constructs the list of AnteDecorators in the correct order. +func buildAnteDecorators(options HandlerOptions, freeLaneFeeChecker func(sdk.Context, sdk.Tx) (sdk.Coins, int64, error)) []sdk.AnteDecorator { + return []sdk.AnteDecorator{ accnum.NewAccountNumberDecorator(options.AccountKeeper), - ante.NewSetUpContextDecorator(), // outermost AnteDecorator. SetUpContext must be called first + ante.NewSetUpContextDecorator(), // Must be the first decorator. ante.NewExtensionOptionsDecorator(options.ExtensionOptionChecker), moveante.NewGasPricesDecorator(), ante.NewValidateBasicDecorator(), @@ -84,15 +95,12 @@ func NewAnteHandler(options HandlerOptions) (sdk.AnteHandler, error) { ante.NewValidateMemoDecorator(options.AccountKeeper), ante.NewConsumeGasForTxSizeDecorator(options.AccountKeeper), ante.NewDeductFeeDecorator(options.AccountKeeper, options.BankKeeper, options.FeegrantKeeper, freeLaneFeeChecker), - // SetPubKeyDecorator must be called before all signature verification decorators - ante.NewSetPubKeyDecorator(options.AccountKeeper), + ante.NewSetPubKeyDecorator(options.AccountKeeper), // Must be called before signature verification. ante.NewValidateSigCountDecorator(options.AccountKeeper), - ante.NewSigGasConsumeDecorator(options.AccountKeeper, sigGasConsumer), + ante.NewSigGasConsumeDecorator(options.AccountKeeper, sigverify.DefaultSigVerificationGasConsumer), sigverify.NewSigVerificationDecorator(options.AccountKeeper, options.SignModeHandler), ante.NewIncrementSequenceDecorator(options.AccountKeeper), ibcante.NewRedundantRelayDecorator(options.IBCkeeper), auctionante.NewAuctionDecorator(options.AuctionKeeper, options.TxEncoder, options.MevLane), } - - return sdk.ChainAnteDecorators(anteDecorators...), nil } From 52f3d1c931a4ed37b35464929f1527c8f0e20fed Mon Sep 17 00:00:00 2001 From: itsasecret <87604944+tudorpintea999@users.noreply.github.com> Date: Sun, 15 Dec 2024 12:29:41 +0200 Subject: [PATCH 4/5] Update mempool.go MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Added validateMempoolConfig to encapsulate configuration validation. • Centralized validation logic improves readability and reuse. • Replaced nested if-else statements with if checks and early returns. • Encapsulated logic in modular helper functions (getTxKey, AssertLaneLimits) to reduce code duplication. • Provided hooks to customize transaction priority (Priority) and validation logic. • Optimized Contains and Remove to avoid redundant checks by using getTxKey. --- app/lanes/mempool.go | 133 ++++++++++++++++++++----------------------- 1 file changed, 63 insertions(+), 70 deletions(-) diff --git a/app/lanes/mempool.go b/app/lanes/mempool.go index d0abd619..f0f04d65 100644 --- a/app/lanes/mempool.go +++ b/app/lanes/mempool.go @@ -20,44 +20,24 @@ type ( sender string } - // Mempool defines a mempool that orders transactions based on the - // txPriority. The mempool is a wrapper on top of the SDK's Priority Nonce mempool. - // It include's additional helper functions that allow users to determine if a - // transaction is already in the mempool and to compare the priority of two - // transactions. + // Mempool manages transaction priority, provides helper functions, and wraps the SDK's Priority Nonce mempool. Mempool[C comparable] struct { - // index defines an index of transactions. - index sdkmempool.Mempool - - // signerExtractor defines the signer extraction adapter that allows us to - // extract the signer from a transaction. - extractor signer_extraction.Adapter - - // txCache is a map of all transactions in the mempool. It is used - // to quickly check if a transaction is already in the mempool. - txCache map[txKey]struct{} - - // ratio defines the relative percentage of block space that can be - // used by this lane. - ratio math.LegacyDec - - // txEncoder defines tx encoder. - txEncoder sdk.TxEncoder + index sdkmempool.Mempool // Priority nonce-based mempool. + extractor signer_extraction.Adapter // Adapter to extract signer information. + txCache map[txKey]struct{} // Cache for quick lookup of transactions in the mempool. + ratio math.LegacyDec // Block space ratio allowed for this lane. + txEncoder sdk.TxEncoder // Transaction encoder. } ) -// NewMempool returns a new Mempool. +// NewMempool creates a new instance of Mempool. func NewMempool[C comparable]( txPriority blockbase.TxPriority[C], extractor signer_extraction.Adapter, maxTx int, ratio math.LegacyDec, txEncoder sdk.TxEncoder, ) (*Mempool[C], error) { - if !ratio.IsPositive() { - return nil, errors.New("mempool creation; ratio must be positive") - } else if ratio.GT(math.LegacyOneDec()) { - return nil, errors.New("mempool creation; ratio must be less than or equal to 1") - } - if txEncoder == nil { - return nil, errors.New("mempool creation; tx encoder is nil") + // Validate inputs. + if err := validateMempoolConfig(ratio, txEncoder); err != nil { + return nil, err } return &Mempool[C]{ @@ -75,94 +55,107 @@ func NewMempool[C comparable]( }, nil } -// Priority returns the priority of the transaction. +// validateMempoolConfig validates the configuration parameters for creating a Mempool. +func validateMempoolConfig(ratio math.LegacyDec, txEncoder sdk.TxEncoder) error { + if !ratio.IsPositive() { + return errors.New("mempool creation: ratio must be positive") + } + if ratio.GT(math.LegacyOneDec()) { + return errors.New("mempool creation: ratio must be less than or equal to 1") + } + if txEncoder == nil { + return errors.New("mempool creation: tx encoder is nil") + } + return nil +} + +// Priority returns the transaction priority. func (cm *Mempool[C]) Priority(ctx sdk.Context, tx sdk.Tx) any { - return 1 + return 1 // Fixed priority for now; extend as needed. } -// CountTx returns the number of transactions in the mempool. +// CountTx returns the total number of transactions in the mempool. func (cm *Mempool[C]) CountTx() int { return cm.index.CountTx() } -// Select returns an iterator of all transactions in the mempool. NOTE: If you -// remove a transaction from the mempool while iterating over the transactions, -// the iterator will not be aware of the removal and will continue to iterate -// over the removed transaction. Be sure to reset the iterator if you remove a transaction. +// Select provides an iterator over all transactions in the mempool. func (cm *Mempool[C]) Select(ctx context.Context, txs [][]byte) sdkmempool.Iterator { return cm.index.Select(ctx, txs) } -// Compare return 0 to ignore priority check in ProcessLaneHandler. +// Compare ignores priority check and returns a constant value for ProcessLaneHandler. func (cm *Mempool[C]) Compare(ctx sdk.Context, this sdk.Tx, other sdk.Tx) (int, error) { return 0, nil } -// Contains returns true if the transaction is contained in the mempool. +// Contains checks whether a transaction exists in the mempool. func (cm *Mempool[C]) Contains(tx sdk.Tx) bool { - if key, err := cm.getTxKey(tx); err != nil { + key, err := cm.getTxKey(tx) + if err != nil { return false - } else { - if _, ok := cm.txCache[key]; ok { - return true - } else { - return false - } } + _, exists := cm.txCache[key] + return exists } -// Insert inserts a transaction into the mempool. +// Insert adds a transaction to the mempool after validating lane limits. func (cm *Mempool[C]) Insert(ctx context.Context, tx sdk.Tx) error { - if err := cm.AssertLaneLimits(sdk.UnwrapSDKContext(ctx), tx); err != nil { + sdkCtx := sdk.UnwrapSDKContext(ctx) + + // Validate lane limits. + if err := cm.AssertLaneLimits(sdkCtx, tx); err != nil { return err } + // Insert into the underlying priority mempool. if err := cm.index.Insert(ctx, tx); err != nil { return fmt.Errorf("failed to insert tx into auction index: %w", err) } - if key, err := cm.getTxKey(tx); err != nil { + // Cache the transaction. + key, err := cm.getTxKey(tx) + if err != nil { return err - } else { - cm.txCache[key] = struct{}{} } + cm.txCache[key] = struct{}{} return nil } -// Remove removes a transaction from the mempool. +// Remove deletes a transaction from the mempool and its cache. func (cm *Mempool[C]) Remove(tx sdk.Tx) error { + // Remove from the priority mempool. if err := cm.index.Remove(tx); err != nil && !errors.Is(err, sdkmempool.ErrTxNotFound) { return fmt.Errorf("failed to remove transaction from the mempool: %w", err) } - if key, err := cm.getTxKey(tx); err != nil { + // Remove from the cache. + key, err := cm.getTxKey(tx) + if err != nil { return err - } else { - delete(cm.txCache, key) } + delete(cm.txCache, key) return nil } +// getTxKey generates a unique key for a transaction based on its nonce and sender. func (cm *Mempool[C]) getTxKey(tx sdk.Tx) (txKey, error) { signers, err := cm.extractor.GetSigners(tx) - if err != nil { - return txKey{}, err - } - if len(signers) == 0 { - return txKey{}, fmt.Errorf("attempted to remove a tx with no signatures") + if err != nil || len(signers) == 0 { + return txKey{}, fmt.Errorf("failed to extract signer from transaction: %w", err) } - sig := signers[0] - sender := sig.Signer.String() - nonce := sig.Sequence - return txKey{nonce, sender}, nil + + // Use the first signer for indexing. + signer := signers[0] + return txKey{nonce: signer.Sequence, sender: signer.Signer.String()}, nil } -// AssertLaneLimits asserts that the transaction does not exceed the lane's max size and gas limit. +// AssertLaneLimits ensures the transaction does not exceed the lane's size or gas limits. func (cm *Mempool[C]) AssertLaneLimits(ctx sdk.Context, tx sdk.Tx) error { maxBlockSize, maxGasLimit := proposals.GetBlockLimits(ctx) - maxLaneTxSize := cm.ratio.MulInt64(maxBlockSize).TruncateInt().Int64() + maxLaneTxSize := cm.ratio.MulInt64(maxBlockSize).TruncateInt64() maxLaneGasLimit := cm.ratio.MulInt(math.NewIntFromUint64(maxGasLimit)).TruncateInt().Uint64() txBytes, err := cm.txEncoder(tx) @@ -172,18 +165,18 @@ func (cm *Mempool[C]) AssertLaneLimits(ctx sdk.Context, tx sdk.Tx) error { gasTx, ok := tx.(sdk.FeeTx) if !ok { - return fmt.Errorf("failed to cast transaction to gas tx") + return errors.New("transaction does not implement FeeTx interface") } + // Validate size and gas limits. txSize := int64(len(txBytes)) txGasLimit := gasTx.GetGas() if txSize > maxLaneTxSize { - return fmt.Errorf("tx size %d exceeds max lane size %d", txSize, maxLaneTxSize) + return fmt.Errorf("transaction size %d exceeds max lane size %d", txSize, maxLaneTxSize) } - if txGasLimit > maxLaneGasLimit { - return fmt.Errorf("tx gas limit %d exceeds max lane gas limit %d", txGasLimit, maxLaneGasLimit) + return fmt.Errorf("transaction gas limit %d exceeds max lane gas limit %d", txGasLimit, maxLaneGasLimit) } return nil From 16878939429336164a70bbe8a790437689831ef7 Mon Sep 17 00:00:00 2001 From: itsasecret <87604944+tudorpintea999@users.noreply.github.com> Date: Sun, 15 Dec 2024 12:50:17 +0200 Subject: [PATCH 5/5] Update proposals.go MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Created helper functions like validateTxEligibility, validateTxPriority, and handleNonMatchingTransactions to reduce code duplication and enhance clarity. • Encapsulated repeated logging and transaction removal logic into logAndAppend and logTxLimitExceeded. • Used descriptive variable names (txsToInclude, txsToRemove) for better understanding. --- app/lanes/proposals.go | 217 ++++++++++++++++++++++------------------- 1 file changed, 116 insertions(+), 101 deletions(-) diff --git a/app/lanes/proposals.go b/app/lanes/proposals.go index 81bbf1b8..555ab4fe 100644 --- a/app/lanes/proposals.go +++ b/app/lanes/proposals.go @@ -9,23 +9,18 @@ import ( "github.com/skip-mev/block-sdk/v2/block/proposals" ) -// DefaultProposalHandler returns a default implementation of the PrepareLaneHandler and -// ProcessLaneHandler. +// DefaultProposalHandler provides default implementations for the PrepareLaneHandler and ProcessLaneHandler. type DefaultProposalHandler struct { lane *blockbase.BaseLane } -// NewDefaultProposalHandler returns a new default proposal handler. +// NewDefaultProposalHandler creates a new instance of DefaultProposalHandler. func NewDefaultProposalHandler(lane *blockbase.BaseLane) *DefaultProposalHandler { - return &DefaultProposalHandler{ - lane: lane, - } + return &DefaultProposalHandler{lane: lane} } -// DefaultPrepareLaneHandler returns a default implementation of the PrepareLaneHandler. It -// selects all transactions in the mempool that are valid and not already in the partial -// proposal. It will continue to reap transactions until the maximum blockspace/gas for this -// lane has been reached. Additionally, any transactions that are invalid will be returned. +// PrepareLaneHandler returns a default implementation of the PrepareLaneHandler. +// It selects transactions that meet blockspace/gas constraints and excludes invalid ones. func (h *DefaultProposalHandler) PrepareLaneHandler() blockbase.PrepareLaneHandler { return func(ctx sdk.Context, proposal proposals.Proposal, limit proposals.LaneLimits) ([]sdk.Tx, []sdk.Tx, error) { var ( @@ -35,90 +30,27 @@ func (h *DefaultProposalHandler) PrepareLaneHandler() blockbase.PrepareLaneHandl txsToRemove []sdk.Tx ) - // Select all transactions in the mempool that are valid and not already in the - // partial proposal. for iterator := h.lane.Select(ctx, nil); iterator != nil; iterator = iterator.Next() { tx := iterator.Tx() txInfo, err := h.lane.GetTxInfo(ctx, tx) if err != nil { - h.lane.Logger().Info("failed to get hash of tx", "err", err) - - txsToRemove = append(txsToRemove, tx) - continue - } - - // Double check that the transaction belongs to this lane. - if !h.lane.Match(ctx, tx) { - h.lane.Logger().Info( - "failed to select tx for lane; tx does not belong to lane", - "tx_hash", txInfo.Hash, - "lane", h.lane.Name(), - ) - - txsToRemove = append(txsToRemove, tx) - continue - } - - // if the transaction is already in the (partial) block proposal, we skip it. - if proposal.Contains(txInfo.Hash) { - h.lane.Logger().Info( - "failed to select tx for lane; tx is already in proposal", - "tx_hash", txInfo.Hash, - "lane", h.lane.Name(), - ) - + h.logAndAppend("failed to get tx info", tx, err, &txsToRemove) continue } - // If the transaction is too large, we skip it. - if updatedSize := totalSize + txInfo.Size; updatedSize > limit.MaxTxBytes { - h.lane.Logger().Debug( - "failed to select tx for lane; tx bytes above the maximum allowed", - "lane", h.lane.Name(), - "tx_size", txInfo.Size, - "total_size", totalSize, - "max_tx_bytes", limit.MaxTxBytes, - "tx_hash", txInfo.Hash, - ) - - if txInfo.Size > limit.MaxTxBytes { - txsToRemove = append(txsToRemove, tx) - } - - continue - } - - // If the gas limit of the transaction is too large, we skip it. - if updatedGas := totalGas + txInfo.GasLimit; updatedGas > limit.MaxGasLimit { - h.lane.Logger().Debug( - "failed to select tx for lane; gas limit above the maximum allowed", - "lane", h.lane.Name(), - "tx_gas", txInfo.GasLimit, - "total_gas", totalGas, - "max_gas", limit.MaxGasLimit, - "tx_hash", txInfo.Hash, - ) - - if txInfo.GasLimit > limit.MaxGasLimit { - txsToRemove = append(txsToRemove, tx) - } - + // Validate transaction eligibility for this lane. + if !h.validateTxEligibility(ctx, proposal, tx, txInfo, limit, totalSize, totalGas, &txsToRemove) { continue } // Verify the transaction. if err = h.lane.VerifyTx(ctx, tx, false); err != nil { - h.lane.Logger().Info( - "failed to verify tx", - "tx_hash", txInfo.Hash, - "err", err, - ) - - txsToRemove = append(txsToRemove, tx) + h.logAndAppend("failed to verify tx", tx, err, &txsToRemove) continue } + // Update totals and include the transaction. totalSize += txInfo.Size totalGas += txInfo.GasLimit txsToInclude = append(txsToInclude, tx) @@ -128,12 +60,8 @@ func (h *DefaultProposalHandler) PrepareLaneHandler() blockbase.PrepareLaneHandl } } -// DefaultProcessLaneHandler returns a default implementation of the ProcessLaneHandler. It verifies -// the following invariants: -// 1. Transactions belonging to the lane must be contiguous from the beginning of the partial proposal. -// 2. Transactions that do not belong to the lane must be contiguous from the end of the partial proposal. -// 3. Transactions must be ordered respecting the priority defined by the lane (e.g. gas price). -// 4. Transactions must be valid according to the verification logic of the lane. +// ProcessLaneHandler returns a default implementation of the ProcessLaneHandler. +// Ensures transactions meet lane-specific constraints and are correctly prioritized. func (h *DefaultProposalHandler) ProcessLaneHandler() blockbase.ProcessLaneHandler { return func(ctx sdk.Context, partialProposal []sdk.Tx) ([]sdk.Tx, []sdk.Tx, error) { if len(partialProposal) == 0 { @@ -141,33 +69,120 @@ func (h *DefaultProposalHandler) ProcessLaneHandler() blockbase.ProcessLaneHandl } for index, tx := range partialProposal { + // Check if the transaction belongs to this lane. if !h.lane.Match(ctx, tx) { - // If the transaction does not belong to this lane, we return the remaining transactions - // iff there are no matches in the remaining transactions after this index. - if index+1 < len(partialProposal) { - if err := h.lane.VerifyNoMatches(ctx, partialProposal[index+1:]); err != nil { - return nil, nil, fmt.Errorf("failed to verify no matches: %w", err) - } - } - - return partialProposal[:index], partialProposal[index:], nil + return h.handleNonMatchingTransactions(ctx, partialProposal, index) } - // If the transactions do not respect the priority defined by the mempool, we consider the proposal - // to be invalid - if index > 0 { - if v, err := h.lane.Compare(ctx, partialProposal[index-1], tx); v == -1 || err != nil { - return nil, nil, fmt.Errorf("transaction at index %d has a higher priority than %d", index, index-1) - } + // Validate transaction priority. + if err := h.validateTxPriority(ctx, partialProposal, index); err != nil { + return nil, nil, err } + // Verify the transaction. if err := h.lane.VerifyTx(ctx, tx, false); err != nil { return nil, nil, fmt.Errorf("failed to verify tx: %w", err) } } - // This means we have processed all transactions in the partial proposal i.e. - // all of the transactions belong to this lane. There are no remaining transactions. + // All transactions are valid and belong to this lane. return partialProposal, nil, nil } } + +// validateTxEligibility checks if a transaction is eligible for inclusion in the lane. +func (h *DefaultProposalHandler) validateTxEligibility( + ctx sdk.Context, + proposal proposals.Proposal, + tx sdk.Tx, + txInfo *blockbase.TxInfo, + limit proposals.LaneLimits, + totalSize int64, + totalGas uint64, + txsToRemove *[]sdk.Tx, +) bool { + // Check if transaction belongs to this lane. + if !h.lane.Match(ctx, tx) { + h.logAndAppend("tx does not belong to lane", tx, nil, txsToRemove) + return false + } + + // Check if the transaction is already in the proposal. + if proposal.Contains(txInfo.Hash) { + h.lane.Logger().Info( + "tx already in proposal", + "tx_hash", txInfo.Hash, + "lane", h.lane.Name(), + ) + return false + } + + // Check size limits. + if updatedSize := totalSize + txInfo.Size; updatedSize > limit.MaxTxBytes { + h.logTxLimitExceeded("tx size exceeds max limit", txInfo, totalSize, limit.MaxTxBytes, txsToRemove) + return false + } + + // Check gas limits. + if updatedGas := totalGas + txInfo.GasLimit; updatedGas > limit.MaxGasLimit { + h.logTxLimitExceeded("tx gas exceeds max limit", txInfo, totalGas, limit.MaxGasLimit, txsToRemove) + return false + } + + return true +} + +// validateTxPriority checks if transactions are correctly ordered based on lane priority. +func (h *DefaultProposalHandler) validateTxPriority(ctx sdk.Context, partialProposal []sdk.Tx, index int) error { + if index > 0 { + prevTx := partialProposal[index-1] + currentTx := partialProposal[index] + + priority, err := h.lane.Compare(ctx, prevTx, currentTx) + if priority == -1 || err != nil { + return fmt.Errorf("tx at index %d has higher priority than index %d", index, index-1) + } + } + return nil +} + +// handleNonMatchingTransactions processes transactions that do not belong to the lane. +func (h *DefaultProposalHandler) handleNonMatchingTransactions( + ctx sdk.Context, + partialProposal []sdk.Tx, + index int, +) ([]sdk.Tx, []sdk.Tx, error) { + if index+1 < len(partialProposal) { + if err := h.lane.VerifyNoMatches(ctx, partialProposal[index+1:]); err != nil { + return nil, nil, fmt.Errorf("failed to verify no matches: %w", err) + } + } + return partialProposal[:index], partialProposal[index:], nil +} + +// logAndAppend logs an error and appends the transaction to the remove list. +func (h *DefaultProposalHandler) logAndAppend(message string, tx sdk.Tx, err error, txsToRemove *[]sdk.Tx) { + h.lane.Logger().Info(message, "err", err) + *txsToRemove = append(*txsToRemove, tx) +} + +// logTxLimitExceeded logs and handles transactions exceeding size or gas limits. +func (h *DefaultProposalHandler) logTxLimitExceeded( + message string, + txInfo *blockbase.TxInfo, + totalValue int64, + maxValue int64, + txsToRemove *[]sdk.Tx, +) { + h.lane.Logger().Debug( + message, + "tx_hash", txInfo.Hash, + "lane", h.lane.Name(), + "value", txInfo.Size, + "total_value", totalValue, + "max_value", maxValue, + ) + if txInfo.Size > maxValue { + *txsToRemove = append(*txsToRemove, txInfo.Tx) + } +}