diff --git a/x/auth/ante/ante_test.go b/x/auth/ante/ante_test.go index 1d55b4d320..ddbe2ea63f 100644 --- a/x/auth/ante/ante_test.go +++ b/x/auth/ante/ante_test.go @@ -737,7 +737,10 @@ func TestAnteHandlerReCheck(t *testing.T) { app, ctx := createTestApp(true) // set blockheight and recheck=true ctx = ctx.WithBlockHeight(1) - ctx = ctx.WithIsReCheckTx(true) + + checkCtx, _ := ctx.CacheContext() + recheckCtx, _ := ctx.CacheContext() + recheckCtx = recheckCtx.WithIsReCheckTx(true) // keys and addresses priv1, _, addr1 := types.KeyTestPubAddr() @@ -760,18 +763,16 @@ func TestAnteHandlerReCheck(t *testing.T) { privs, accnums, seqs := []crypto.PrivKey{priv1}, []uint64{0}, []uint64{0} tx := types.NewTestTxWithMemo(ctx, msgs, privs, accnums, seqs, fee, "thisisatestmemo") - // make signature array empty which would normally cause ValidateBasicDecorator and SigVerificationDecorator fail - // since these decorators don't run on recheck, the tx should pass the antehandler - stdTx := tx.(types.StdTx) - stdTx.Signatures = []types.StdSignature{} + _, err := antehandler(checkCtx, tx, false) + require.Nil(t, err, "AnteHandler errored on check unexpectedly: %v", err) - _, err := antehandler(ctx, stdTx, false) + _, err = antehandler(recheckCtx, tx, false) require.Nil(t, err, "AnteHandler errored on recheck unexpectedly: %v", err) - tx = types.NewTestTxWithMemo(ctx, msgs, privs, accnums, seqs, fee, "thisisatestmemo") + tx = types.NewTestTxWithMemo(recheckCtx, msgs, privs, accnums, seqs, fee, "thisisatestmemo") txBytes, err := json.Marshal(tx) require.Nil(t, err, "Error marshalling tx: %v", err) - ctx = ctx.WithTxBytes(txBytes) + recheckCtx = recheckCtx.WithTxBytes(txBytes) // require that state machine param-dependent checking is still run on recheck since parameters can change between check and recheck testCases := []struct { @@ -784,31 +785,32 @@ func TestAnteHandlerReCheck(t *testing.T) { } for _, tc := range testCases { // set testcase parameters - app.AccountKeeper.SetParams(ctx, tc.params) + app.AccountKeeper.SetParams(recheckCtx, tc.params) - _, err := antehandler(ctx, tx, false) + _, err := antehandler(recheckCtx, tx, false) require.NotNil(t, err, "tx does not fail on recheck with updated params in test case: %s", tc.name) // reset parameters to default values - app.AccountKeeper.SetParams(ctx, types.DefaultParams()) + app.AccountKeeper.SetParams(recheckCtx, types.DefaultParams()) } // require that local mempool fee check is still run on recheck since validator may change minFee between check and recheck // create new minimum gas price so antehandler fails on recheck - ctx = ctx.WithMinGasPrices([]sdk.DecCoin{{ + recheckCtx = recheckCtx.WithMinGasPrices([]sdk.DecCoin{{ Denom: "dnecoin", // fee does not have this denom Amount: sdk.NewDec(5), }}) - _, err = antehandler(ctx, tx, false) + + _, err = antehandler(recheckCtx, tx, false) require.NotNil(t, err, "antehandler on recheck did not fail when mingasPrice was changed") // reset min gasprice - ctx = ctx.WithMinGasPrices(sdk.DecCoins{}) + recheckCtx = recheckCtx.WithMinGasPrices(sdk.DecCoins{}) // remove funds for account so antehandler fails on recheck acc1.SetCoins(sdk.Coins{}) - app.AccountKeeper.SetAccount(ctx, acc1) + app.AccountKeeper.SetAccount(recheckCtx, acc1) - _, err = antehandler(ctx, tx, false) + _, err = antehandler(recheckCtx, tx, false) require.NotNil(t, err, "antehandler on recheck did not fail once feePayer no longer has sufficient funds") } diff --git a/x/auth/ante/sigverify.go b/x/auth/ante/sigverify.go index 9e04c8feda..7c4238aa00 100644 --- a/x/auth/ante/sigverify.go +++ b/x/auth/ante/sigverify.go @@ -2,7 +2,10 @@ package ante import ( "bytes" + "crypto/sha256" "encoding/hex" + "fmt" + "sync" "github.com/tendermint/tendermint/crypto" "github.com/tendermint/tendermint/crypto/ed25519" @@ -159,16 +162,19 @@ func (sgcd SigGasConsumeDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simula // CONTRACT: Pubkeys are set in context for all signers before this decorator runs // CONTRACT: Tx must implement SigVerifiableTx interface type SigVerificationDecorator struct { - ak keeper.AccountKeeper + ak keeper.AccountKeeper + txHashCache sync.Map } -func NewSigVerificationDecorator(ak keeper.AccountKeeper) SigVerificationDecorator { - return SigVerificationDecorator{ - ak: ak, +func NewSigVerificationDecorator(ak keeper.AccountKeeper) *SigVerificationDecorator { + return &SigVerificationDecorator{ + ak: ak, + txHashCache: sync.Map{}, } } -func (svd SigVerificationDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler) (newCtx sdk.Context, err error) { +func (svd *SigVerificationDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler) (newCtx sdk.Context, err error) { + // TODO https://github.com/line/link/issues/1136 // no need to verify signatures on recheck tx if ctx.IsReCheckTx() { return next(ctx, tx, simulate) @@ -181,43 +187,108 @@ func (svd SigVerificationDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simul // stdSigs contains the sequence number, account number, and signatures. // When simulating, this would just be a 0-length slice. sigs := sigTx.GetSignatures() - - // stdSigs contains the sequence number, account number, and signatures. - // When simulating, this would just be a 0-length slice. signerAddrs := sigTx.GetSigners() - signerAccs := make([]exported.Account, len(signerAddrs)) // check that signer length and signature length are the same if len(sigs) != len(signerAddrs) { return ctx, sdkerrors.Wrapf(sdkerrors.ErrUnauthorized, "invalid number of signer; expected: %d, got %d", len(signerAddrs), len(sigs)) } + newSigKeys := make([]string, 0, len(sigs)) + defer func() { + // remove txHashCash if got an error + if err != nil { + for _, sigKey := range newSigKeys { + svd.txHashCache.Delete(sigKey) + } + } + }() + for i, sig := range sigs { - signerAccs[i], err = GetSignerAcc(ctx, svd.ak, signerAddrs[i]) + signerAcc, err := GetSignerAcc(ctx, svd.ak, signerAddrs[i]) if err != nil { return ctx, err } - // retrieve signBytes of tx - signBytes := sigTx.GetSignBytes(ctx, signerAccs[i]) + if simulate { + continue + } // retrieve pubkey - pubKey := signerAccs[i].GetPubKey() - if !simulate && pubKey == nil { + pubKey := signerAcc.GetPubKey() + if pubKey == nil { return ctx, sdkerrors.Wrap(sdkerrors.ErrInvalidPubKey, "pubkey on account is not set") } - // verify signature - if !simulate && !pubKey.VerifyBytes(signBytes, sig) { + txHash := sha256.Sum256(ctx.TxBytes()) + sigKey := fmt.Sprintf("%d:%d", signerAcc.GetAccountNumber(), signerAcc.GetSequence()) + + verified, stored := svd.verifySignatureWithCache(ctx, sigTx, sigKey, txHash[:], signerAcc, pubKey, sig) + + if stored { + newSigKeys = append(newSigKeys, sigKey) + } + + if !verified { return ctx, sdkerrors.Wrapf( sdkerrors.ErrUnauthorized, - "signature verification failed; verify correct account sequence (%d) and chain-id (%s)", signerAccs[i].GetSequence(), ctx.ChainID()) + "signature verification failed; verify correct account sequence (%d) and chain-id (%s)", signerAcc.GetSequence(), ctx.ChainID()) } } return next(ctx, tx, simulate) } +func (svd *SigVerificationDecorator) verifySignatureWithCache( + ctx sdk.Context, + sigTx SigVerifiableTx, + sigKey string, + txHash []byte, + signerAcc exported.Account, + pubKey crypto.PubKey, + sig []byte, +) (verified, stored bool) { + // #NOTE `genesis` transactions should not use `cache` + if ctx.BlockHeight() == 0 { + verified = pubKey.VerifyBytes(sigTx.GetSignBytes(ctx, signerAcc), sig) + return verified, stored + } + + exist := false + + switch { + case ctx.IsCheckTx() && !ctx.IsReCheckTx(): // CheckTx + verified = pubKey.VerifyBytes(sigTx.GetSignBytes(ctx, signerAcc), sig) + if verified { + svd.txHashCache.Store(sigKey, txHash) + stored = true + } + + case ctx.IsReCheckTx(): // ReCheckTx + verified, exist = svd.checkCache(sigKey, txHash) + if !verified && exist { + svd.txHashCache.Delete(sigKey) + } + + default: // DeliverTx + verified, exist = svd.checkCache(sigKey, txHash) + if exist { + svd.txHashCache.Delete(sigKey) + } + if !verified { + verified = pubKey.VerifyBytes(sigTx.GetSignBytes(ctx, signerAcc), sig) + } + } + + return verified, stored +} + +func (svd *SigVerificationDecorator) checkCache(sigKey string, txHash []byte) (verified, exist bool) { + cached, exist := svd.txHashCache.Load(sigKey) + verified = exist && bytes.Equal(cached.([]byte), txHash) + return verified, exist +} + // IncrementSequenceDecorator handles incrementing sequences of all signers. // Use the IncrementSequenceDecorator decorator to prevent replay attacks. Note, // there is no need to execute IncrementSequenceDecorator on RecheckTX since diff --git a/x/auth/ante/sigverify_test.go b/x/auth/ante/sigverify_test.go index 522f90ddba..c263563ade 100644 --- a/x/auth/ante/sigverify_test.go +++ b/x/auth/ante/sigverify_test.go @@ -142,7 +142,7 @@ func TestSigVerification(t *testing.T) { {"wrong accnums", []crypto.PrivKey{priv1, priv2, priv3}, []uint64{7, 8, 9}, []uint64{0, 0, 0}, false, true}, {"wrong sequences", []crypto.PrivKey{priv1, priv2, priv3}, []uint64{0, 1, 2}, []uint64{3, 4, 5}, false, true}, {"valid tx", []crypto.PrivKey{priv1, priv2, priv3}, []uint64{0, 1, 2}, []uint64{0, 0, 0}, false, false}, - {"no err on recheck", []crypto.PrivKey{}, []uint64{}, []uint64{}, true, false}, + {"no err on recheck", []crypto.PrivKey{priv1, priv2, priv3}, []uint64{0, 1, 2}, []uint64{0, 0, 0}, true, false}, } for i, tc := range testCases { ctx = ctx.WithIsReCheckTx(tc.recheck)