Merge PR #3121: x/auth: fetch one account after another

* x/auth: fetch one account after another

- Don't read all accounts in one go as signature verification
  could fail before last signature is checked.

- TestAnteHandlerFees now checks whether fees were actually
  deducted from the account as well as the fee keeper collected
  them.

Thanks: @ValarDragon for pointing this out
Closes: #3093

PENDING.md

@@ -39,6 +39,8 @@ IMPROVEMENTS
 * Gaia
 
 * SDK
+  * [\#3093](https://github.com/cosmos/cosmos-sdk/issues/3093) Ante handler does no longer read all accounts in one go when processing signatures as signature
+    verification may fail before last signature is checked.
 
 * Tendermint
 

x/auth/ante.go

@@ -76,12 +76,17 @@ func NewAnteHandler(am AccountKeeper, fck FeeCollectionKeeper) sdk.AnteHandler {
 
 		newCtx.GasMeter().ConsumeGas(memoCostPerByte*sdk.Gas(len(stdTx.GetMemo())), "memo")
 
-		signerAccs, res := GetSignerAccs(newCtx, am, stdTx.GetSigners())
+		// stdSigs contains the sequence number, account number, and signatures.
+		// When simulating, this would just be a 0-length slice.
+		signerAddrs := stdTx.GetSigners()
+		signerAccs := make([]Account, len(signerAddrs))
+		isGenesis := ctx.BlockHeight() == 0
+
+		// fetch first signer, who's going to pay the fees
+		signerAccs[0], res = GetSignerAcc(newCtx, am, signerAddrs[0])
 		if !res.IsOK() {
 			return newCtx, res, true
 		}
-
-		// the first signer pays the transaction fees
 		if !stdTx.Fee.Amount.IsZero() {
 			signerAccs[0], res = DeductFees(signerAccs[0], stdTx.Fee)
 			if !res.IsOK() {
@@ -91,16 +96,21 @@ func NewAnteHandler(am AccountKeeper, fck FeeCollectionKeeper) sdk.AnteHandler {
 			fck.AddCollectedFees(newCtx, stdTx.Fee.Amount)
 		}
 
-		isGenesis := ctx.BlockHeight() == 0
-		signBytesList := GetSignBytesList(newCtx.ChainID(), stdTx, signerAccs, isGenesis)
-
 		// stdSigs contains the sequence number, account number, and signatures.
 		// When simulating, this would just be a 0-length slice.
 		stdSigs := stdTx.GetSignatures()
 
 		for i := 0; i < len(stdSigs); i++ {
+			// skip the fee payer, account is cached and fees were deducted already
+			if i != 0 {
+				signerAccs[i], res = GetSignerAcc(newCtx, am, signerAddrs[i])
+				if !res.IsOK() {
+					return newCtx, res, true
+				}
+			}
 			// check signature, return account with incremented nonce
-			signerAccs[i], res = processSig(newCtx, signerAccs[i], stdSigs[i], signBytesList[i], simulate)
+			signBytes := GetSignBytes(newCtx.ChainID(), stdTx, signerAccs[i], isGenesis)
+			signerAccs[i], res = processSig(newCtx, signerAccs[i], stdSigs[i], signBytes, simulate)
 			if !res.IsOK() {
 				return newCtx, res, true
 			}
@@ -116,18 +126,12 @@ func NewAnteHandler(am AccountKeeper, fck FeeCollectionKeeper) sdk.AnteHandler {
 	}
 }
 
-// GetSignerAccs returns a list of signers for a given list of addresses that
-// are expected to sign a transaction.
-func GetSignerAccs(ctx sdk.Context, am AccountKeeper, addrs []sdk.AccAddress) (accs []Account, res sdk.Result) {
-	accs = make([]Account, len(addrs))
-	for i := 0; i < len(accs); i++ {
-		accs[i] = am.GetAccount(ctx, addrs[i])
-		if accs[i] == nil {
-			return nil, sdk.ErrUnknownAddress(addrs[i].String()).Result()
-		}
+// GetSignerAcc a signers for a given address that is expected to sign a transaction.
+func GetSignerAcc(ctx sdk.Context, am AccountKeeper, addr sdk.AccAddress) (Account, sdk.Result) {
+	if acc := am.GetAccount(ctx, addr); acc != nil {
+		return acc, sdk.Result{}
 	}
-
-	return
+	return nil, sdk.ErrUnknownAddress(addr.String()).Result()
 }
 
 // verify the signature and increment the sequence. If the account doesn't have
@@ -293,18 +297,14 @@ func SetGasMeter(simulate bool, ctx sdk.Context, stdTx StdTx) sdk.Context {
 	return ctx.WithGasMeter(sdk.NewGasMeter(stdTx.Fee.Gas))
 }
 
-// GetSignBytesList returns a slice of bytes to sign over for a given transaction
-// and list of accounts.
-func GetSignBytesList(chainID string, stdTx StdTx, accs []Account, genesis bool) (signatureBytesList [][]byte) {
-	signatureBytesList = make([][]byte, len(accs))
-	for i := 0; i < len(accs); i++ {
-		accNum := accs[i].GetAccountNumber()
-		if genesis {
-			accNum = 0
-		}
-		signatureBytesList[i] = StdSignBytes(chainID,
-			accNum, accs[i].GetSequence(),
-			stdTx.Fee, stdTx.Msgs, stdTx.Memo)
+// GetSignBytes returns a slice of bytes to sign over for a given transaction
+// and an account.
+func GetSignBytes(chainID string, stdTx StdTx, acc Account, genesis bool) []byte {
+	accNum := acc.GetAccountNumber()
+	if genesis {
+		accNum = 0
 	}
-	return
+	return StdSignBytes(chainID,
+		accNum, acc.GetSequence(),
+		stdTx.Fee, stdTx.Msgs, stdTx.Memo)
 }

x/auth/ante_test.go

@@ -396,12 +396,14 @@ func TestAnteHandlerFees(t *testing.T) {
 	checkInvalidTx(t, anteHandler, ctx, tx, false, sdk.CodeInsufficientFunds)
 
 	require.True(t, feeCollector.GetCollectedFees(ctx).IsEqual(emptyCoins))
+	require.True(t, mapper.GetAccount(ctx, addr1).GetCoins().AmountOf("atom").Equal(sdk.NewInt(149)))
 
 	acc1.SetCoins(sdk.Coins{sdk.NewInt64Coin("atom", 150)})
 	mapper.SetAccount(ctx, acc1)
 	checkValidTx(t, anteHandler, ctx, tx, false)
 
 	require.True(t, feeCollector.GetCollectedFees(ctx).IsEqual(sdk.Coins{sdk.NewInt64Coin("atom", 150)}))
+	require.True(t, mapper.GetAccount(ctx, addr1).GetCoins().AmountOf("atom").Equal(sdk.NewInt(0)))
 }
 
 // Test logic around memo gas consumption.
@@ -646,8 +648,10 @@ func TestProcessPubKey(t *testing.T) {
 	ctx := sdk.NewContext(ms, abci.Header{ChainID: "mychainid"}, false, log.NewNopLogger())
 	// keys
 	_, addr1 := privAndAddr()
-	priv2, _ := privAndAddr()
+	priv2, addr2 := privAndAddr()
 	acc1 := mapper.NewAccountWithAddress(ctx, addr1)
+	acc2 := mapper.NewAccountWithAddress(ctx, addr2)
+	acc2.SetPubKey(priv2.PubKey())
 	type args struct {
 		acc      Account
 		sig      StdSignature
@@ -660,6 +664,7 @@ func TestProcessPubKey(t *testing.T) {
 	}{
 		{"no sigs, simulate off", args{acc1, StdSignature{}, false}, true},
 		{"no sigs, simulate on", args{acc1, StdSignature{}, true}, false},
+		{"no sigs, account with pub, simulate on", args{acc2, StdSignature{}, true}, false},
 		{"pubkey doesn't match addr, simulate off", args{acc1, StdSignature{PubKey: priv2.PubKey()}, false}, true},
 		{"pubkey doesn't match addr, simulate on", args{acc1, StdSignature{PubKey: priv2.PubKey()}, true}, false},
 	}