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.

Sign up for GitHub

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: signature verification cache #41

Merged
merged 8 commits into from
Jan 7, 2021
Merged

feat: signature verification cache #41

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
bdac6d6
feat: signature verification cache
jinsan-line Dec 8, 2020
3ac69bd
chore: recover to skip to sigverify
jinsan-line Jan 5, 2021
dd56ff5
chore: rename `ok` to `exist` of `checkCache()` return var
jinsan-line Jan 5, 2021
cfc5dca
Merge remote-tracking branch 'upstream/feat/perf' into sig-cache
jinsan-line Jan 6, 2021
865bc14
chore: remove txHashCash if got an error
jinsan-line Jan 6, 2021
f369205
chore: revise `verifySignatureWithCache()` for readability
jinsan-line Jan 7, 2021
9531633
chore: remove naked return
jinsan-line Jan 7, 2021
a77c79a
fix: lint error
jinsan-line Jan 7, 2021
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
34 changes: 18 additions & 16 deletions x/auth/ante/ante_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand All @@ -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 {
Expand All @@ -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")
}
107 changes: 90 additions & 17 deletions x/auth/ante/sigverify.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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) {
jinsan-line marked this conversation as resolved.
Show resolved Hide resolved
// TODO https://github.com/line/link/issues/1136
// no need to verify signatures on recheck tx
if ctx.IsReCheckTx() {
return next(ctx, tx, simulate)
Expand All @@ -181,43 +187,110 @@ 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 {
jinsan-line marked this conversation as resolved.
Show resolved Hide resolved
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 bool, stored bool) {
// #NOTE `genesis` transactions should not use `cache`
if ctx.BlockHeight() == 0 {
return pubKey.VerifyBytes(sigTx.GetSignBytes(ctx, signerAcc), sig), false
}

switch {
case ctx.IsCheckTx() && !ctx.IsReCheckTx(): // CheckTx
if !pubKey.VerifyBytes(sigTx.GetSignBytes(ctx, signerAcc), sig) {
return false, false
}
svd.txHashCache.Store(sigKey, txHash)
Copy link

@egonspace egonspace Dec 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

It seems like some tx's signature will not be removed from the cache if it does not meet DeliverTx. If the mempool is full, the tx will not be able to enter the mempool after passing checkTx, so it will not be able to meet DeliverTx.

Copy link
Contributor Author

@jinsan-line jinsan-line Dec 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

You're correct. A tx couldn't be removed from the cache if the tx is not added to mempool. But I'd like to care it w/ missing transaction issue. Currently, I assume that the tx must be added to mempool if it passes checkTx.

Copy link
Contributor Author

@jinsan-line jinsan-line Dec 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

As another approach, we could remove a tx from the cache when we fail to add a tx to mempool.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another thought, even though we fail to add a tx to mempool after adding it to the cache, it could be removed when same account w/ the same sequence broadcasts a tx again. So, finally after thinking deeply, I have a conclusion that it's not a big deal. Could you agree on it?

stored = true

case ctx.IsReCheckTx(): // ReCheckTx
jinsan-line marked this conversation as resolved.
Show resolved Hide resolved
verified, exist := svd.checkCache(sigKey, txHash)

if !verified {
if exist {
svd.txHashCache.Delete(sigKey)
}
return false, false
}

default: // DeliverTx
verified, exist := svd.checkCache(sigKey, txHash)
if exist {
svd.txHashCache.Delete(sigKey)
}

if !verified && !pubKey.VerifyBytes(sigTx.GetSignBytes(ctx, signerAcc), sig) {
return false, false
}
}

return true, 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
Expand Down
2 changes: 1 addition & 1 deletion x/auth/ante/sigverify_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down