From 4753d16ef4b2afc6a393e4f4000cdf63df2cca7c Mon Sep 17 00:00:00 2001 From: testinginprod <98415576+testinginprod@users.noreply.github.com> Date: Wed, 20 Dec 2023 19:58:09 +0100 Subject: [PATCH] refactor(auth): group sig verification ante handlers (SigGasConsume, SigVerify, IncreaseSequence) (#18817) Co-authored-by: unknown unknown Co-authored-by: Aleksandr Bezobchuk --- UPGRADING.md | 5 + baseapp/block_gas_test.go | 2 +- simapp/ante.go | 4 +- x/auth/CHANGELOG.md | 4 + x/auth/ante/ante.go | 4 +- x/auth/ante/sigverify.go | 247 +++++++++++++--------------------- x/auth/ante/sigverify_test.go | 77 +++-------- 7 files changed, 123 insertions(+), 220 deletions(-) diff --git a/UPGRADING.md b/UPGRADING.md index c78ec35604..9d97ceed29 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -14,6 +14,11 @@ Note, always read the **SimApp** section for more information on application wir In this section we describe the changes made in Cosmos SDK' SimApp. **These changes are directly applicable to your application wiring.** +#### AnteHandlers + +The GasConsumptionDecorator and IncreaseSequenceDecorator have been merged with the SigVerificationDecorator, so you'll +need to remove them both from your app.go code, they will yield to unresolvable symbols when compiling. + #### Client (`root.go`) The `client` package has been refactored to make use of the address codecs (address, validator address, consensus address, etc.). diff --git a/baseapp/block_gas_test.go b/baseapp/block_gas_test.go index e41bd212a4..bef8ef4d75 100644 --- a/baseapp/block_gas_test.go +++ b/baseapp/block_gas_test.go @@ -174,7 +174,7 @@ func TestBaseApp_BlockGas(t *testing.T) { require.Equal(t, []byte("ok"), okValue) } // check block gas is always consumed - baseGas := uint64(57504) // baseGas is the gas consumed before tx msg + baseGas := uint64(54436) // baseGas is the gas consumed before tx msg expGasConsumed := addUint64Saturating(tc.gasToConsume, baseGas) if expGasConsumed > uint64(simtestutil.DefaultConsensusParams.Block.MaxGas) { // capped by gasLimit diff --git a/simapp/ante.go b/simapp/ante.go index 918244a814..451ab03648 100644 --- a/simapp/ante.go +++ b/simapp/ante.go @@ -42,9 +42,7 @@ func NewAnteHandler(options HandlerOptions) (sdk.AnteHandler, error) { ante.NewDeductFeeDecorator(options.AccountKeeper, options.BankKeeper, options.FeegrantKeeper, options.TxFeeChecker), ante.NewSetPubKeyDecorator(options.AccountKeeper), // SetPubKeyDecorator must be called before all signature verification decorators ante.NewValidateSigCountDecorator(options.AccountKeeper), - ante.NewSigGasConsumeDecorator(options.AccountKeeper, options.SigGasConsumer), - ante.NewSigVerificationDecorator(options.AccountKeeper, options.SignModeHandler), - ante.NewIncrementSequenceDecorator(options.AccountKeeper), + ante.NewSigVerificationDecorator(options.AccountKeeper, options.SignModeHandler, options.SigGasConsumer), } return sdk.ChainAnteDecorators(anteDecorators...), nil diff --git a/x/auth/CHANGELOG.md b/x/auth/CHANGELOG.md index 6afba13523..7990c15356 100644 --- a/x/auth/CHANGELOG.md +++ b/x/auth/CHANGELOG.md @@ -35,4 +35,8 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### API Breaking Changes +### Consensus Breaking Changes + +* [#18817](https://github.com/cosmos/cosmos-sdk/pull/18817) SigVerification, GasConsumption, IncreaseSequence ante decorators have all been joined into one SigVerification decorator. Gas consumption during TX validation flow has reduced. + ### Bug Fixes diff --git a/x/auth/ante/ante.go b/x/auth/ante/ante.go index 9c1371a67b..acfc980147 100644 --- a/x/auth/ante/ante.go +++ b/x/auth/ante/ante.go @@ -48,9 +48,7 @@ func NewAnteHandler(options HandlerOptions) (sdk.AnteHandler, error) { NewDeductFeeDecorator(options.AccountKeeper, options.BankKeeper, options.FeegrantKeeper, options.TxFeeChecker), NewSetPubKeyDecorator(options.AccountKeeper), // SetPubKeyDecorator must be called before all signature verification decorators NewValidateSigCountDecorator(options.AccountKeeper), - NewSigGasConsumeDecorator(options.AccountKeeper, options.SigGasConsumer), - NewSigVerificationDecorator(options.AccountKeeper, options.SignModeHandler), - NewIncrementSequenceDecorator(options.AccountKeeper), + NewSigVerificationDecorator(options.AccountKeeper, options.SignModeHandler, options.SigGasConsumer), } return sdk.ChainAnteDecorators(anteDecorators...), nil diff --git a/x/auth/ante/sigverify.go b/x/auth/ante/sigverify.go index c681a12dcf..0b08d1876a 100644 --- a/x/auth/ante/sigverify.go +++ b/x/auth/ante/sigverify.go @@ -146,97 +146,23 @@ func (spkd SetPubKeyDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate b return next(ctx, tx, simulate) } -// Consume parameter-defined amount of gas for each signature according to the passed-in SignatureVerificationGasConsumer function -// before calling the next AnteHandler -// CONTRACT: Pubkeys are set in context for all signers before this decorator runs -// CONTRACT: Tx must implement SigVerifiableTx interface -type SigGasConsumeDecorator struct { - ak AccountKeeper - sigGasConsumer SignatureVerificationGasConsumer -} - -func NewSigGasConsumeDecorator(ak AccountKeeper, sigGasConsumer SignatureVerificationGasConsumer) SigGasConsumeDecorator { - if sigGasConsumer == nil { - sigGasConsumer = DefaultSigVerificationGasConsumer - } - - return SigGasConsumeDecorator{ - ak: ak, - sigGasConsumer: sigGasConsumer, - } -} - -func (sgcd SigGasConsumeDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler) (newCtx sdk.Context, err error) { - sigTx, ok := tx.(authsigning.SigVerifiableTx) - if !ok { - return ctx, errorsmod.Wrap(sdkerrors.ErrTxDecode, "invalid transaction type") - } - - params := sgcd.ak.GetParams(ctx) - sigs, err := sigTx.GetSignaturesV2() - if err != nil { - return ctx, err - } - - // stdSigs contains the sequence number, account number, and signatures. - // When simulating, this would just be a 0-length slice. - signers, err := sigTx.GetSigners() - if err != nil { - return ctx, err - } - - for i, sig := range sigs { - signerAcc, err := GetSignerAcc(ctx, sgcd.ak, signers[i]) - if err != nil { - return ctx, err - } - - pubKey := signerAcc.GetPubKey() - if !simulate && pubKey == nil { - return ctx, errorsmod.Wrap(sdkerrors.ErrInvalidPubKey, "pubkey on account is not set") - } - if err := verifyIsOnCurve(pubKey); err != nil { - return ctx, err - } - - // In simulate mode the transaction comes with no signatures, thus if the - // account's pubkey is nil, both signature verification and gasKVStore.Set() - // shall consume the largest amount, i.e. it takes more gas to verify - // secp256k1 keys than ed25519 ones. - if simulate && pubKey == nil { - pubKey = simSecp256k1Pubkey - } - - // make a SignatureV2 with PubKey filled in from above - sig = signing.SignatureV2{ - PubKey: pubKey, - Data: sig.Data, - Sequence: sig.Sequence, - } - - err = sgcd.sigGasConsumer(ctx.GasMeter(), sig, params) - if err != nil { - return ctx, err - } - } - - return next(ctx, tx, simulate) -} - // SigVerificationDecorator verifies all signatures for a tx and return an error if any are invalid. Note, // the SigVerificationDecorator will not check signatures on ReCheck. +// It will also increase the sequence number, and consume gas for signature verification. // // CONTRACT: Pubkeys are set in context for all signers before this decorator runs // CONTRACT: Tx must implement SigVerifiableTx interface type SigVerificationDecorator struct { ak AccountKeeper signModeHandler *txsigning.HandlerMap + sigGasConsumer SignatureVerificationGasConsumer } -func NewSigVerificationDecorator(ak AccountKeeper, signModeHandler *txsigning.HandlerMap) SigVerificationDecorator { +func NewSigVerificationDecorator(ak AccountKeeper, signModeHandler *txsigning.HandlerMap, sigGasConsumer SignatureVerificationGasConsumer) SigVerificationDecorator { return SigVerificationDecorator{ ak: ak, signModeHandler: signModeHandler, + sigGasConsumer: sigGasConsumer, } } @@ -341,6 +267,53 @@ func (svd SigVerificationDecorator) authenticate(ctx sdk.Context, tx sdk.Tx, sim return err } + err = svd.consumeSignatureGas(ctx, simulate, acc.GetPubKey(), sig) + if err != nil { + return err + } + + err = svd.verifySig(ctx, simulate, tx, acc, sig) + if err != nil { + return err + } + + err = svd.increaseSequence(ctx, acc) + if err != nil { + return err + } + return nil +} + +// consumeSignatureGas will consume gas according to the pub-key being verified. +func (svd SigVerificationDecorator) consumeSignatureGas( + ctx sdk.Context, + simulate bool, + pubKey cryptotypes.PubKey, + signature signing.SignatureV2, +) error { + if simulate && pubKey == nil { + pubKey = simSecp256k1Pubkey + } + + // make a SignatureV2 with PubKey filled in from above + signature = signing.SignatureV2{ + PubKey: pubKey, + Data: signature.Data, + Sequence: signature.Sequence, + } + + err := svd.sigGasConsumer(ctx.GasMeter(), signature, svd.ak.GetParams(ctx)) + if err != nil { + return err + } + return nil +} + +// verifySig will verify the signature of the provided signer account. +// it will assess: +// - the pub key is on the curve. +// - verify sig +func (svd SigVerificationDecorator) verifySig(ctx sdk.Context, simulate bool, tx sdk.Tx, acc sdk.AccountI, sig signing.SignatureV2) error { // retrieve pubkey pubKey := acc.GetPubKey() if !simulate && pubKey == nil { @@ -358,6 +331,13 @@ func (svd SigVerificationDecorator) authenticate(ctx sdk.Context, tx sdk.Tx, sim ) } + // we're in simulation mode, or in ReCheckTx, or context is not + // on sig verify tx, then we do not need to verify the signatures + // in the tx. + if simulate || ctx.IsReCheckTx() || !ctx.IsSigverifyTx() { + return nil + } + // retrieve signer data genesis := ctx.BlockHeight() == 0 chainID := ctx.ChainID() @@ -366,90 +346,47 @@ func (svd SigVerificationDecorator) authenticate(ctx sdk.Context, tx sdk.Tx, sim accNum = acc.GetAccountNumber() } - // no need to verify signatures on recheck tx - if !simulate && !ctx.IsReCheckTx() && ctx.IsSigverifyTx() { - anyPk, _ := codectypes.NewAnyWithValue(pubKey) + 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 fmt.Errorf("expected tx to implement V2AdaptableTx, got %T", tx) - } - txData := adaptableTx.GetSigningTxData() - err = authsigning.VerifySignature(ctx, pubKey, signerData, sig.Data, svd.signModeHandler, txData) - if err != nil { - var errMsg string - if 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 errorsmod.Wrap(sdkerrors.ErrUnauthorized, errMsg) - } + 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 fmt.Errorf("expected tx to implement V2AdaptableTx, got %T", tx) + } + txData := adaptableTx.GetSigningTxData() + err := authsigning.VerifySignature(ctx, pubKey, signerData, sig.Data, svd.signModeHandler, txData) + if err != nil { + var errMsg string + if 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 errorsmod.Wrap(sdkerrors.ErrUnauthorized, errMsg) + } + return nil } -// IncrementSequenceDecorator handles incrementing sequences of all signers. -// Use the IncrementSequenceDecorator decorator to prevent replay attacks. Note, -// there is need to execute IncrementSequenceDecorator on RecheckTx since -// BaseApp.Commit() will set the check state based on the latest header. -// -// NOTE: Since CheckTx and DeliverTx state are managed separately, subsequent and -// sequential txs originating from the same account cannot be handled correctly in -// a reliable way unless sequence numbers are managed and tracked manually by a -// client. It is recommended to instead use multiple messages in a tx. -type IncrementSequenceDecorator struct { - ak AccountKeeper -} - -func NewIncrementSequenceDecorator(ak AccountKeeper) IncrementSequenceDecorator { - return IncrementSequenceDecorator{ - ak: ak, - } -} - -func (isd IncrementSequenceDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler) (sdk.Context, error) { - sigTx, ok := tx.(authsigning.SigVerifiableTx) - if !ok { - return ctx, errorsmod.Wrap(sdkerrors.ErrTxDecode, "invalid transaction type") +// increaseSequence will increase the sequence number of the account. +func (svd SigVerificationDecorator) increaseSequence(ctx sdk.Context, acc sdk.AccountI) error { + if err := acc.SetSequence(acc.GetSequence() + 1); err != nil { + return err } - // increment sequence of all signers - signers, err := sigTx.GetSigners() - if err != nil { - return sdk.Context{}, err - } - - for _, signer := range signers { - acc := isd.ak.GetAccount(ctx, signer) - if err := acc.SetSequence(acc.GetSequence() + 1); err != nil { - panic(err) - } - - pubKey := acc.GetPubKey() - if !simulate && pubKey == nil { - return ctx, errorsmod.Wrap(sdkerrors.ErrInvalidPubKey, "pubkey on account is not set") - } - if err := verifyIsOnCurve(pubKey); err != nil { - return ctx, err - } - - isd.ak.SetAccount(ctx, acc) - } - - return next(ctx, tx, simulate) + svd.ak.SetAccount(ctx, acc) + return nil } // ValidateSigCountDecorator takes in Params and returns errors if there are too many signatures in the tx for the given params diff --git a/x/auth/ante/sigverify_test.go b/x/auth/ante/sigverify_test.go index ab6e406991..27824131bc 100644 --- a/x/auth/ante/sigverify_test.go +++ b/x/auth/ante/sigverify_test.go @@ -178,7 +178,8 @@ func TestSigVerification(t *testing.T) { txConfigOpts, ) require.NoError(t, err) - svd := ante.NewSigVerificationDecorator(suite.accountKeeper, anteTxConfig.SignModeHandler()) + noOpGasConsume := func(_ storetypes.GasMeter, _ signing.SignatureV2, _ types.Params) error { return nil } + svd := ante.NewSigVerificationDecorator(suite.accountKeeper, anteTxConfig.SignModeHandler(), noOpGasConsume) antehandler := sdk.ChainAnteDecorators(spkd, svd) defaultSignMode, err := authsign.APISignModeToInternal(anteTxConfig.SignModeHandler().DefaultMode()) require.NoError(t, err) @@ -209,14 +210,15 @@ func TestSigVerification(t *testing.T) { for i, tc := range testCases { for _, signMode := range enabledSignModes { t.Run(fmt.Sprintf("%s with %s", tc.name, signMode), func(t *testing.T) { - suite.ctx = suite.ctx.WithIsReCheckTx(tc.recheck).WithIsSigverifyTx(tc.sigverify) + ctx, _ := suite.ctx.CacheContext() + ctx = ctx.WithIsReCheckTx(tc.recheck).WithIsSigverifyTx(tc.sigverify) suite.txBuilder = suite.clientCtx.TxConfig.NewTxBuilder() // Create new txBuilder for each test require.NoError(t, suite.txBuilder.SetMsgs(msgs...)) suite.txBuilder.SetFeeAmount(feeAmount) suite.txBuilder.SetGasLimit(gasLimit) - tx, err := suite.CreateTestTx(suite.ctx, tc.privs, tc.accNums, tc.accSeqs, suite.ctx.ChainID(), signMode) + tx, err := suite.CreateTestTx(ctx, tc.privs, tc.accNums, tc.accSeqs, ctx.ChainID(), signMode) require.NoError(t, err) if tc.invalidSigs { txSigs, _ := tx.GetSignaturesV2() @@ -237,12 +239,21 @@ func TestSigVerification(t *testing.T) { txBytes, err := suite.clientCtx.TxConfig.TxEncoder()(tx) require.NoError(t, err) - byteCtx := suite.ctx.WithTxBytes(txBytes) + byteCtx := ctx.WithTxBytes(txBytes) _, err = antehandler(byteCtx, tx, false) if tc.shouldErr { require.NotNil(t, err, "TestCase %d: %s did not error as expected", i, tc.name) } else { require.Nil(t, err, "TestCase %d: %s errored unexpectedly. Err: %v", i, tc.name, err) + // check account sequence + signers, err := tx.GetSigners() + require.NoError(t, err) + for i, signer := range signers { + wantSeq := tc.accSeqs[i] + 1 + acc, err := suite.accountKeeper.Accounts.Get(ctx, signer) + require.NoError(t, err) + require.Equal(t, int(wantSeq), int(acc.GetSequence())) + } } }) } @@ -303,9 +314,8 @@ func runSigDecorators(t *testing.T, params types.Params, _ bool, privs ...crypto require.NoError(t, err) spkd := ante.NewSetPubKeyDecorator(suite.accountKeeper) - svgc := ante.NewSigGasConsumeDecorator(suite.accountKeeper, ante.DefaultSigVerificationGasConsumer) - svd := ante.NewSigVerificationDecorator(suite.accountKeeper, suite.clientCtx.TxConfig.SignModeHandler()) - antehandler := sdk.ChainAnteDecorators(spkd, svgc, svd) + svd := ante.NewSigVerificationDecorator(suite.accountKeeper, suite.clientCtx.TxConfig.SignModeHandler(), ante.DefaultSigVerificationGasConsumer) + antehandler := sdk.ChainAnteDecorators(spkd, svd) txBytes, err := suite.clientCtx.TxConfig.TxEncoder()(tx) require.NoError(t, err) @@ -319,53 +329,6 @@ func runSigDecorators(t *testing.T, params types.Params, _ bool, privs ...crypto return after - before, err } -func TestIncrementSequenceDecorator(t *testing.T) { - suite := SetupTestSuite(t, true) - suite.txBuilder = suite.clientCtx.TxConfig.NewTxBuilder() - - priv, _, addr := testdata.KeyTestPubAddr() - acc := suite.accountKeeper.NewAccountWithAddress(suite.ctx, addr) - require.NoError(t, acc.SetAccountNumber(uint64(50))) - suite.accountKeeper.SetAccount(suite.ctx, acc) - - msgs := []sdk.Msg{testdata.NewTestMsg(addr)} - require.NoError(t, suite.txBuilder.SetMsgs(msgs...)) - privs := []cryptotypes.PrivKey{priv} - accNums := []uint64{suite.accountKeeper.GetAccount(suite.ctx, addr).GetAccountNumber()} - accSeqs := []uint64{suite.accountKeeper.GetAccount(suite.ctx, addr).GetSequence()} - feeAmount := testdata.NewTestFeeAmount() - gasLimit := testdata.NewTestGasLimit() - suite.txBuilder.SetFeeAmount(feeAmount) - suite.txBuilder.SetGasLimit(gasLimit) - - tx, err := suite.CreateTestTx(suite.ctx, privs, accNums, accSeqs, suite.ctx.ChainID(), signing.SignMode_SIGN_MODE_DIRECT) - require.NoError(t, err) - - pubKeyDecorator := ante.NewSetPubKeyDecorator(suite.accountKeeper) - IncrementSequenceDecorator := ante.NewIncrementSequenceDecorator(suite.accountKeeper) - antehandler := sdk.ChainAnteDecorators(pubKeyDecorator, IncrementSequenceDecorator) - - testCases := []struct { - ctx sdk.Context - simulate bool - expectedSeq uint64 - }{ - {suite.ctx.WithIsReCheckTx(true), false, 1}, - {suite.ctx.WithIsCheckTx(true).WithIsReCheckTx(false), false, 2}, - {suite.ctx.WithIsReCheckTx(true), false, 3}, - {suite.ctx.WithIsReCheckTx(true), false, 4}, - {suite.ctx.WithIsReCheckTx(true), true, 5}, - } - - for i, tc := range testCases { - t.Run(fmt.Sprintf("%d test", i), func(t *testing.T) { - _, err = antehandler(tc.ctx, tx, tc.simulate) - require.NoError(t, err, "unexpected error; tc #%d, %v", i, tc) - require.Equal(t, tc.expectedSeq, suite.accountKeeper.GetAccount(suite.ctx, addr).GetSequence()) - }) - } -} - func TestAnteHandlerChecks(t *testing.T) { suite := SetupTestSuite(t, true) suite.txBankKeeper.EXPECT().DenomMetadataV2(gomock.Any(), gomock.Any()).Return(&bankv1beta1.QueryDenomMetadataResponse{}, nil).AnyTimes() @@ -408,11 +371,9 @@ func TestAnteHandlerChecks(t *testing.T) { } setPubKeyDecorator := ante.NewSetPubKeyDecorator(suite.accountKeeper) - sigGasConsumeDecorator := ante.NewSigGasConsumeDecorator(suite.accountKeeper, ante.DefaultSigVerificationGasConsumer) - sigVerificationDecorator := ante.NewSigVerificationDecorator(suite.accountKeeper, anteTxConfig.SignModeHandler()) - IncrementSequenceDecorator := ante.NewIncrementSequenceDecorator(suite.accountKeeper) + sigVerificationDecorator := ante.NewSigVerificationDecorator(suite.accountKeeper, anteTxConfig.SignModeHandler(), ante.DefaultSigVerificationGasConsumer) - anteHandler := sdk.ChainAnteDecorators(setPubKeyDecorator, sigGasConsumeDecorator, sigVerificationDecorator, IncrementSequenceDecorator) + anteHandler := sdk.ChainAnteDecorators(setPubKeyDecorator, sigVerificationDecorator) type testCase struct { name string