Merge PR #2977: Don't serialize Account Number and Sequence Number in…

… signatures * Don't serialize Account Number and Sequence Number in signatures This was not needed to be included within the tx body, as its in the state. * fix lint
cosmos · Dec 5, 2018 · 2848746 · 2848746
1 parent bc772e5
commit 2848746
Show file tree

Hide file tree

Showing 8 changed files with 38 additions and 78 deletions.
diff --git a/PENDING.md b/PENDING.md
@@ -10,6 +10,7 @@ BREAKING CHANGES
 * Gaia
  - [#128](https://github.com/tendermint/devops/issues/128) Updated CircleCI job to trigger website build on every push to master/develop.
 * SDK
+ - [auth] \#2952 Signatures are no longer serialized on chain with the account number and sequence number
 
 * Tendermint
 

diff --git a/cmd/gaia/app/benchmarks/txsize_test.go b/cmd/gaia/app/benchmarks/txsize_test.go
@@ -12,10 +12,11 @@ import (
 
 // This will fail half the time with the second output being 173
 // This is due to secp256k1 signatures not being constant size.
-// This will be resolved when updating to tendermint v0.24.0
 // nolint: vet
 func ExampleTxSendSize() {
 	cdc := app.MakeCodec()
+	var gas uint64 = 1
+
 	priv1 := secp256k1.GenPrivKeySecp256k1([]byte{0})
 	addr1 := sdk.AccAddress(priv1.PubKey().Address())
 	priv2 := secp256k1.GenPrivKeySecp256k1([]byte{1})
@@ -25,11 +26,14 @@ func ExampleTxSendSize() {
 		Inputs:  []bank.Input{bank.NewInput(addr1, coins)},
 		Outputs: []bank.Output{bank.NewOutput(addr2, coins)},
 	}
-	sig, _ := priv1.Sign(msg1.GetSignBytes())
-	sigs := []auth.StdSignature{{nil, sig, 0, 0}}
-	tx := auth.NewStdTx([]sdk.Msg{msg1}, auth.NewStdFee(0, coins...), sigs, "")
+	fee := auth.NewStdFee(gas, coins...)
+	signBytes := auth.StdSignBytes("example-chain-ID",
+		1, 1, fee, []sdk.Msg{msg1}, "")
+	sig, _ := priv1.Sign(signBytes)
+	sigs := []auth.StdSignature{{nil, sig}}
+	tx := auth.NewStdTx([]sdk.Msg{msg1}, fee, sigs, "")
 	fmt.Println(len(cdc.MustMarshalBinaryBare([]sdk.Msg{msg1})))
 	fmt.Println(len(cdc.MustMarshalBinaryBare(tx)))
 	// output: 80
-	// 167
+	// 169
 }
diff --git a/x/auth/ante.go b/x/auth/ante.go
@@ -77,17 +77,14 @@ func NewAnteHandler(am AccountKeeper, fck FeeCollectionKeeper) sdk.AnteHandler {
 		// When simulating, this would just be a 0-length slice.
 		stdSigs := stdTx.GetSignatures()
 		signerAddrs := stdTx.GetSigners()
-
-		// create the list of all sign bytes
-		signBytesList := getSignBytesList(newCtx.ChainID(), stdTx, stdSigs)
 		signerAccs, res := getSignerAccs(newCtx, am, signerAddrs)
 		if !res.IsOK() {
 			return newCtx, res, true
 		}
-		res = validateAccNumAndSequence(ctx, signerAccs, stdSigs)
-		if !res.IsOK() {
-			return newCtx, res, true
-		}
+
+		isGenesis := ctx.BlockHeight() == 0
+		// create the list of all sign bytes
+		signBytesList := getSignBytesList(newCtx.ChainID(), stdTx, signerAccs, isGenesis)
 
 		// first sig pays the fees
 		if !stdTx.Fee.Amount.IsZero() {
@@ -129,31 +126,6 @@ func getSignerAccs(ctx sdk.Context, am AccountKeeper, addrs []sdk.AccAddress) (a
 	return
 }
 
-func validateAccNumAndSequence(ctx sdk.Context, accs []Account, sigs []StdSignature) sdk.Result {
-	for i := 0; i < len(accs); i++ {
-		// On InitChain, make sure account number == 0
-		if ctx.BlockHeight() == 0 && sigs[i].AccountNumber != 0 {
-			return sdk.ErrInvalidSequence(
-				fmt.Sprintf("Invalid account number for BlockHeight == 0. Got %d, expected 0", sigs[i].AccountNumber)).Result()
-		}
-
-		// Check account number.
-		accnum := accs[i].GetAccountNumber()
-		if ctx.BlockHeight() != 0 && accnum != sigs[i].AccountNumber {
-			return sdk.ErrInvalidSequence(
-				fmt.Sprintf("Invalid account number. Got %d, expected %d", sigs[i].AccountNumber, accnum)).Result()
-		}
-
-		// Check sequence number.
-		seq := accs[i].GetSequence()
-		if seq != sigs[i].Sequence {
-			return sdk.ErrInvalidSequence(
-				fmt.Sprintf("Invalid sequence. Got %d, expected %d", sigs[i].Sequence, seq)).Result()
-		}
-	}
-	return sdk.Result{}
-}
-
 // verify the signature and increment the sequence.
 // if the account doesn't have a pubkey, set it.
 func processSig(ctx sdk.Context,
@@ -297,11 +269,15 @@ func setGasMeter(simulate bool, ctx sdk.Context, stdTx StdTx) sdk.Context {
 	return ctx.WithGasMeter(sdk.NewGasMeter(stdTx.Fee.Gas))
 }
 
-func getSignBytesList(chainID string, stdTx StdTx, stdSigs []StdSignature) (signatureBytesList [][]byte) {
-	signatureBytesList = make([][]byte, len(stdSigs))
-	for i := 0; i < len(stdSigs); i++ {
+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,
-			stdSigs[i].AccountNumber, stdSigs[i].Sequence,
+			accNum, accs[i].GetSequence(),
 			stdTx.Fee, stdTx.Msgs, stdTx.Memo)
 	}
 	return

diff --git a/x/auth/ante_test.go b/x/auth/ante_test.go
@@ -74,7 +74,7 @@ func newTestTx(ctx sdk.Context, msgs []sdk.Msg, privs []crypto.PrivKey, accNums
 		if err != nil {
 			panic(err)
 		}
-		sigs[i] = StdSignature{PubKey: priv.PubKey(), Signature: sig, AccountNumber: accNums[i], Sequence: seqs[i]}
+		sigs[i] = StdSignature{PubKey: priv.PubKey(), Signature: sig}
 	}
 	tx := NewStdTx(msgs, fee, sigs, "")
 	return tx
@@ -88,7 +88,7 @@ func newTestTxWithMemo(ctx sdk.Context, msgs []sdk.Msg, privs []crypto.PrivKey,
 		if err != nil {
 			panic(err)
 		}
-		sigs[i] = StdSignature{PubKey: priv.PubKey(), Signature: sig, AccountNumber: accNums[i], Sequence: seqs[i]}
+		sigs[i] = StdSignature{PubKey: priv.PubKey(), Signature: sig}
 	}
 	tx := NewStdTx(msgs, fee, sigs, memo)
 	return tx
@@ -102,7 +102,7 @@ func newTestTxWithSignBytes(msgs []sdk.Msg, privs []crypto.PrivKey, accNums []ui
 		if err != nil {
 			panic(err)
 		}
-		sigs[i] = StdSignature{PubKey: priv.PubKey(), Signature: sig, AccountNumber: accNums[i], Sequence: seqs[i]}
+		sigs[i] = StdSignature{PubKey: priv.PubKey(), Signature: sig}
 	}
 	tx := NewStdTx(msgs, fee, sigs, memo)
 	return tx
@@ -200,7 +200,7 @@ func TestAnteHandlerAccountNumbers(t *testing.T) {
 	// new tx from wrong account number
 	seqs = []uint64{1}
 	tx = newTestTx(ctx, msgs, privs, []uint64{1}, seqs, fee)
-	checkInvalidTx(t, anteHandler, ctx, tx, false, sdk.CodeInvalidSequence)
+	checkInvalidTx(t, anteHandler, ctx, tx, false, sdk.CodeUnauthorized)
 
 	// from correct account number
 	seqs = []uint64{1}
@@ -213,7 +213,7 @@ func TestAnteHandlerAccountNumbers(t *testing.T) {
 	msgs = []sdk.Msg{msg1, msg2}
 	privs, accnums, seqs = []crypto.PrivKey{priv1, priv2}, []uint64{1, 0}, []uint64{2, 0}
 	tx = newTestTx(ctx, msgs, privs, accnums, seqs, fee)
-	checkInvalidTx(t, anteHandler, ctx, tx, false, sdk.CodeInvalidSequence)
+	checkInvalidTx(t, anteHandler, ctx, tx, false, sdk.CodeUnauthorized)
 
 	// correct account numbers
 	privs, accnums, seqs = []crypto.PrivKey{priv1, priv2}, []uint64{0, 1}, []uint64{2, 0}
@@ -260,7 +260,7 @@ func TestAnteHandlerAccountNumbersAtBlockHeightZero(t *testing.T) {
 	// new tx from wrong account number
 	seqs = []uint64{1}
 	tx = newTestTx(ctx, msgs, privs, []uint64{1}, seqs, fee)
-	checkInvalidTx(t, anteHandler, ctx, tx, false, sdk.CodeInvalidSequence)
+	checkInvalidTx(t, anteHandler, ctx, tx, false, sdk.CodeUnauthorized)
 
 	// from correct account number
 	seqs = []uint64{1}
@@ -273,7 +273,7 @@ func TestAnteHandlerAccountNumbersAtBlockHeightZero(t *testing.T) {
 	msgs = []sdk.Msg{msg1, msg2}
 	privs, accnums, seqs = []crypto.PrivKey{priv1, priv2}, []uint64{1, 0}, []uint64{2, 0}
 	tx = newTestTx(ctx, msgs, privs, accnums, seqs, fee)
-	checkInvalidTx(t, anteHandler, ctx, tx, false, sdk.CodeInvalidSequence)
+	checkInvalidTx(t, anteHandler, ctx, tx, false, sdk.CodeUnauthorized)
 
 	// correct account numbers
 	privs, accnums, seqs = []crypto.PrivKey{priv1, priv2}, []uint64{0, 0}, []uint64{2, 0}
@@ -322,7 +322,7 @@ func TestAnteHandlerSequences(t *testing.T) {
 	checkValidTx(t, anteHandler, ctx, tx, false)
 
 	// test sending it again fails (replay protection)
-	checkInvalidTx(t, anteHandler, ctx, tx, false, sdk.CodeInvalidSequence)
+	checkInvalidTx(t, anteHandler, ctx, tx, false, sdk.CodeUnauthorized)
 
 	// fix sequence, should pass
 	seqs = []uint64{1}
@@ -339,14 +339,14 @@ func TestAnteHandlerSequences(t *testing.T) {
 	checkValidTx(t, anteHandler, ctx, tx, false)
 
 	// replay fails
-	checkInvalidTx(t, anteHandler, ctx, tx, false, sdk.CodeInvalidSequence)
+	checkInvalidTx(t, anteHandler, ctx, tx, false, sdk.CodeUnauthorized)
 
 	// tx from just second signer with incorrect sequence fails
 	msg = newTestMsg(addr2)
 	msgs = []sdk.Msg{msg}
 	privs, accnums, seqs = []crypto.PrivKey{priv2}, []uint64{1}, []uint64{0}
 	tx = newTestTx(ctx, msgs, privs, accnums, seqs, fee)
-	checkInvalidTx(t, anteHandler, ctx, tx, false, sdk.CodeInvalidSequence)
+	checkInvalidTx(t, anteHandler, ctx, tx, false, sdk.CodeUnauthorized)
 
 	// fix the sequence and it passes
 	tx = newTestTx(ctx, msgs, []crypto.PrivKey{priv2}, []uint64{1}, []uint64{1}, fee)

diff --git a/x/auth/client/txbuilder/txbuilder.go b/x/auth/client/txbuilder/txbuilder.go
@@ -162,9 +162,7 @@ func (bldr TxBuilder) BuildWithPubKey(name string, msgs []sdk.Msg) ([]byte, erro
 	}
 
 	sigs := []auth.StdSignature{{
-		AccountNumber: msg.AccountNumber,
-		Sequence:      msg.Sequence,
-		PubKey:        info.GetPubKey(),
+		PubKey: info.GetPubKey(),
 	}}
 
 	return bldr.Codec.MarshalBinaryLengthPrefixed(auth.NewStdTx(msg.Msgs, msg.Fee, sigs, msg.Memo))
@@ -206,9 +204,7 @@ func MakeSignature(name, passphrase string, msg StdSignMsg) (sig auth.StdSignatu
 		return
 	}
 	return auth.StdSignature{
-		AccountNumber: msg.AccountNumber,
-		Sequence:      msg.Sequence,
-		PubKey:        pubkey,
-		Signature:     sigBytes,
+		PubKey:    pubkey,
+		Signature: sigBytes,
 	}, nil
 }
diff --git a/x/auth/stdtx.go b/x/auth/stdtx.go
@@ -188,8 +188,6 @@ func StdSignBytes(chainID string, accnum uint64, sequence uint64, fee StdFee, ms
 type StdSignature struct {
 	crypto.PubKey `json:"pub_key"` // optional
 	Signature     []byte           `json:"signature"`
-	AccountNumber uint64           `json:"account_number"`
-	Sequence      uint64           `json:"sequence"`
 }
 
 // logic for standard transaction decoding

diff --git a/x/bank/app_test.go b/x/bank/app_test.go
@@ -123,7 +123,7 @@ func TestMsgSendWithAccounts(t *testing.T) {
 			msgs:       []sdk.Msg{sendMsg1, sendMsg2},
 			accNums:    []uint64{0},
 			accSeqs:    []uint64{0},
-			expSimPass: false,
+			expSimPass: true, // doesn't check signature
 			expPass:    false,
 			privKeys:   []crypto.PrivKey{priv1},
 		},
@@ -136,19 +136,6 @@ func TestMsgSendWithAccounts(t *testing.T) {
 			mock.CheckBalance(t, mapp, eb.addr, eb.coins)
 		}
 	}
-
-	// bumping the tx nonce number without resigning should be an auth error
-	mapp.BeginBlock(abci.RequestBeginBlock{})
-
-	tx := mock.GenTx([]sdk.Msg{sendMsg1}, []uint64{0}, []uint64{0}, priv1)
-	tx.Signatures[0].Sequence = 1
-
-	res := mapp.Deliver(tx)
-	require.EqualValues(t, sdk.CodeUnauthorized, res.Code, res.Log)
-	require.EqualValues(t, sdk.CodespaceRoot, res.Codespace)
-
-	// resigning the tx with the bumped sequence should work
-	mock.SignCheckDeliver(t, mapp.BaseApp, []sdk.Msg{sendMsg1, sendMsg2}, []uint64{0}, []uint64{1}, true, true, priv1)
 }
 
 func TestMsgSendMultipleOut(t *testing.T) {

diff --git a/x/mock/app.go b/x/mock/app.go
@@ -202,10 +202,8 @@ func GenTx(msgs []sdk.Msg, accnums []uint64, seq []uint64, priv ...crypto.PrivKe
 		}
 
 		sigs[i] = auth.StdSignature{
-			PubKey:        p.PubKey(),
-			Signature:     sig,
-			AccountNumber: accnums[i],
-			Sequence:      seq[i],
+			PubKey:    p.PubKey(),
+			Signature: sig,
 		}
 	}
-Original file line number
+Diff line change
@@ Expand Up / @@ -10,6 +10,7 @@ BREAKING CHANGES @@
     * Gaia
      - [#128](https://github.com/tendermint/devops/issues/128) Updated CircleCI job to trigger website build on every push to master/develop.
     * SDK
+     - [auth] \#2952 Signatures are no longer serialized on chain with the account number and sequence number
     * Tendermint
@@ Expand Down @@