Remove GetSignatures from SigVerifiableTx (#6550)

* Remove GetSignatures from SigVerifiableTx * Fix conflicts * update client tests * fix x/auth tests * add MultiSignatureData test case for ConsumeTxSizeGasDecorator test Co-authored-by: Alexander Bezobchuk <[email protected]> Co-authored-by: sahith-narahari <[email protected]> Co-authored-by: Cory Levinson <[email protected]> Co-authored-by: Federico Kunze <[email protected]>
cosmos · Sep 2, 2020 · 9e85e81 · 9e85e81
1 parent cf394ed
commit 9e85e81
Show file tree

Hide file tree

Showing 7 changed files with 130 additions and 69 deletions.
diff --git a/client/testutil/suite.go b/client/testutil/suite.go
@@ -119,7 +119,6 @@ func (s *TxConfigTestSuite) TestTxBuilderSetSignatures() {
 	err = txBuilder.SetSignatures(sig1, msig)
 	s.Require().NoError(err)
 	sigTx = txBuilder.GetTx()
-	s.Require().Len(sigTx.GetSignatures(), 2)
 	sigsV2, err := sigTx.GetSignaturesV2()
 	s.Require().NoError(err)
 	s.Require().Len(sigsV2, 2)
@@ -163,7 +162,6 @@ func (s *TxConfigTestSuite) TestTxBuilderSetSignatures() {
 	err = txBuilder.SetSignatures(sig1, msig)
 	s.Require().NoError(err)
 	sigTx = txBuilder.GetTx()
-	s.Require().Len(sigTx.GetSignatures(), 2)
 	sigsV2, err = sigTx.GetSignaturesV2()
 	s.Require().NoError(err)
 	s.Require().Len(sigsV2, 2)
@@ -267,7 +265,9 @@ func (s *TxConfigTestSuite) TestTxEncodeDecode() {
 	s.Require().Equal(feeAmount, tx3.GetFee())
 	s.Require().Equal(gasLimit, tx3.GetGas())
 	s.Require().Equal(memo, tx3.GetMemo())
-	s.Require().Equal([][]byte{dummySig}, tx3.GetSignatures())
+	tx3Sigs, err := tx3.GetSignaturesV2()
+	s.Require().NoError(err)
+	s.Require().Equal([]signingtypes.SignatureV2{sig}, tx3Sigs)
 	s.Require().Equal([]crypto.PubKey{pubkey}, tx3.GetPubKeys())
 
 	s.T().Log("JSON encode transaction")
@@ -284,7 +284,9 @@ func (s *TxConfigTestSuite) TestTxEncodeDecode() {
 	s.Require().Equal(feeAmount, tx3.GetFee())
 	s.Require().Equal(gasLimit, tx3.GetGas())
 	s.Require().Equal(memo, tx3.GetMemo())
-	s.Require().Equal([][]byte{dummySig}, tx3.GetSignatures())
+	tx3Sigs, err = tx3.GetSignaturesV2()
+	s.Require().NoError(err)
+	s.Require().Equal([]signingtypes.SignatureV2{sig}, tx3Sigs)
 	s.Require().Equal([]crypto.PubKey{pubkey}, tx3.GetPubKeys())
 }
 

diff --git a/client/tx/tx_test.go b/client/tx/tx_test.go
@@ -116,7 +116,10 @@ func TestBuildUnsignedTx(t *testing.T) {
 	tx, err := tx.BuildUnsignedTx(txf, msg)
 	require.NoError(t, err)
 	require.NotNil(t, tx)
-	require.Empty(t, tx.GetTx().(signing.SigVerifiableTx).GetSignatures())
+
+	sigs, err := tx.GetTx().(signing.SigVerifiableTx).GetSignaturesV2()
+	require.NoError(t, err)
+	require.Empty(t, sigs)
 }
 
 func TestSign(t *testing.T) {

diff --git a/x/auth/ante/basic.go b/x/auth/ante/basic.go
@@ -7,7 +7,8 @@ import (
 	"github.com/cosmos/cosmos-sdk/crypto/types/multisig"
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
-	"github.com/cosmos/cosmos-sdk/x/auth/signing"
+	"github.com/cosmos/cosmos-sdk/types/tx/signing"
+	authsigning "github.com/cosmos/cosmos-sdk/x/auth/signing"
 	"github.com/cosmos/cosmos-sdk/x/auth/types"
 )
 
@@ -90,7 +91,7 @@ func NewConsumeGasForTxSizeDecorator(ak AccountKeeper) ConsumeTxSizeGasDecorator
 }
 
 func (cgts ConsumeTxSizeGasDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler) (sdk.Context, error) {
-	sigTx, ok := tx.(signing.SigVerifiableTx)
+	sigTx, ok := tx.(authsigning.SigVerifiableTx)
 	if !ok {
 		return ctx, sdkerrors.Wrap(sdkerrors.ErrTxDecode, "invalid tx type")
 	}
@@ -101,10 +102,15 @@ func (cgts ConsumeTxSizeGasDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, sim
 	// simulate gas cost for signatures in simulate mode
 	if simulate {
 		// in simulate mode, each element should be a nil signature
-		sigs := sigTx.GetSignatures()
+		sigs, err := sigTx.GetSignaturesV2()
+		if err != nil {
+			return ctx, err
+		}
+		n := len(sigs)
+
 		for i, signer := range sigTx.GetSigners() {
 			// if signature is already filled in, no need to simulate gas cost
-			if sigs[i] != nil {
+			if i < n && !isIncompleteSignature(sigs[i].Data) {
 				continue
 			}
 
@@ -141,6 +147,29 @@ func (cgts ConsumeTxSizeGasDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, sim
 	return next(ctx, tx, simulate)
 }
 
+// isIncompleteSignature tests whether SignatureData is fully filled in for simulation purposes
+func isIncompleteSignature(data signing.SignatureData) bool {
+	if data == nil {
+		return true
+	}
+
+	switch data := data.(type) {
+	case *signing.SingleSignatureData:
+		return len(data.Signature) == 0
+	case *signing.MultiSignatureData:
+		if len(data.Signatures) == 0 {
+			return true
+		}
+		for _, s := range data.Signatures {
+			if isIncompleteSignature(s) {
+				return true
+			}
+		}
+	}
+
+	return false
+}
+
 type (
 	// TxTimeoutHeightDecorator defines an AnteHandler decorator that checks for a
 	// tx height timeout.

diff --git a/x/auth/ante/basic_test.go b/x/auth/ante/basic_test.go
@@ -8,6 +8,7 @@ import (
 
 	"github.com/tendermint/tendermint/crypto"
 
+	"github.com/cosmos/cosmos-sdk/crypto/types/multisig"
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	"github.com/cosmos/cosmos-sdk/x/auth/ante"
 )
@@ -91,7 +92,6 @@ func (suite *AnteTestSuite) TestValidateMemo() {
 
 func (suite *AnteTestSuite) TestConsumeGasForTxSize() {
 	suite.SetupTest(true) // setup
-	suite.txBuilder = suite.clientCtx.TxConfig.NewTxBuilder()
 
 	// keys and addresses
 	priv1, _, addr1 := testdata.KeyTestPubAddr()
@@ -100,66 +100,80 @@ func (suite *AnteTestSuite) TestConsumeGasForTxSize() {
 	msg := testdata.NewTestMsg(addr1)
 	feeAmount := testdata.NewTestFeeAmount()
 	gasLimit := testdata.NewTestGasLimit()
-	suite.Require().NoError(suite.txBuilder.SetMsgs(msg))
-	suite.txBuilder.SetFeeAmount(feeAmount)
-	suite.txBuilder.SetGasLimit(gasLimit)
-	suite.txBuilder.SetMemo(strings.Repeat("01234567890", 10))
-
-	privs, accNums, accSeqs := []crypto.PrivKey{priv1}, []uint64{0}, []uint64{0}
-	tx, err := suite.CreateTestTx(privs, accNums, accSeqs, suite.ctx.ChainID())
-	suite.Require().NoError(err)
-
-	txBytes, err := suite.clientCtx.TxConfig.TxJSONEncoder()(tx)
-	suite.Require().Nil(err, "Cannot marshal tx: %v", err)
 
 	cgtsd := ante.NewConsumeGasForTxSizeDecorator(suite.app.AccountKeeper)
 	antehandler := sdk.ChainAnteDecorators(cgtsd)
 
-	params := suite.app.AccountKeeper.GetParams(suite.ctx)
-	expectedGas := sdk.Gas(len(txBytes)) * params.TxSizeCostPerByte
+	testCases := []struct {
+		name  string
+		sigV2 signing.SignatureV2
+	}{
+		{"SingleSignatureData", signing.SignatureV2{PubKey: priv1.PubKey()}},
+		{"MultiSignatureData", signing.SignatureV2{PubKey: priv1.PubKey(), Data: multisig.NewMultisig(2)}},
+	}
+
+	for _, tc := range testCases {
+		suite.Run(tc.name, func() {
+			suite.txBuilder = suite.clientCtx.TxConfig.NewTxBuilder()
+			suite.Require().NoError(suite.txBuilder.SetMsgs(msg))
+			suite.txBuilder.SetFeeAmount(feeAmount)
+			suite.txBuilder.SetGasLimit(gasLimit)
+			suite.txBuilder.SetMemo(strings.Repeat("01234567890", 10))
+
+			privs, accNums, accSeqs := []crypto.PrivKey{priv1}, []uint64{0}, []uint64{0}
+			tx, err := suite.CreateTestTx(privs, accNums, accSeqs, suite.ctx.ChainID())
+			suite.Require().NoError(err)
+
+			txBytes, err := suite.clientCtx.TxConfig.TxJSONEncoder()(tx)
+			suite.Require().Nil(err, "Cannot marshal tx: %v", err)
 
-	// Set suite.ctx with TxBytes manually
-	suite.ctx = suite.ctx.WithTxBytes(txBytes)
+			params := suite.app.AccountKeeper.GetParams(suite.ctx)
+			expectedGas := sdk.Gas(len(txBytes)) * params.TxSizeCostPerByte
 
-	// track how much gas is necessary to retrieve parameters
-	beforeGas := suite.ctx.GasMeter().GasConsumed()
-	suite.app.AccountKeeper.GetParams(suite.ctx)
-	afterGas := suite.ctx.GasMeter().GasConsumed()
-	expectedGas += afterGas - beforeGas
+			// Set suite.ctx with TxBytes manually
+			suite.ctx = suite.ctx.WithTxBytes(txBytes)
 
-	beforeGas = suite.ctx.GasMeter().GasConsumed()
-	suite.ctx, err = antehandler(suite.ctx, tx, false)
-	suite.Require().Nil(err, "ConsumeTxSizeGasDecorator returned error: %v", err)
+			// track how much gas is necessary to retrieve parameters
+			beforeGas := suite.ctx.GasMeter().GasConsumed()
+			suite.app.AccountKeeper.GetParams(suite.ctx)
+			afterGas := suite.ctx.GasMeter().GasConsumed()
+			expectedGas += afterGas - beforeGas
 
-	// require that decorator consumes expected amount of gas
-	consumedGas := suite.ctx.GasMeter().GasConsumed() - beforeGas
-	suite.Require().Equal(expectedGas, consumedGas, "Decorator did not consume the correct amount of gas")
+			beforeGas = suite.ctx.GasMeter().GasConsumed()
+			suite.ctx, err = antehandler(suite.ctx, tx, false)
+			suite.Require().Nil(err, "ConsumeTxSizeGasDecorator returned error: %v", err)
 
-	// simulation must not underestimate gas of this decorator even with nil signatures
-	txBuilder, err := suite.clientCtx.TxConfig.WrapTxBuilder(tx)
-	suite.Require().NoError(err)
-	suite.Require().NoError(txBuilder.SetSignatures(signing.SignatureV2{
-		PubKey: priv1.PubKey(),
-	}))
-	tx = txBuilder.GetTx()
+			// require that decorator consumes expected amount of gas
+			consumedGas := suite.ctx.GasMeter().GasConsumed() - beforeGas
+			suite.Require().Equal(expectedGas, consumedGas, "Decorator did not consume the correct amount of gas")
+
+			// simulation must not underestimate gas of this decorator even with nil signatures
+			txBuilder, err := suite.clientCtx.TxConfig.WrapTxBuilder(tx)
+			suite.Require().NoError(err)
+			suite.Require().NoError(txBuilder.SetSignatures(tc.sigV2))
+			tx = txBuilder.GetTx()
 
-	simTxBytes, err := suite.clientCtx.TxConfig.TxJSONEncoder()(tx)
-	suite.Require().Nil(err, "Cannot marshal tx: %v", err)
-	// require that simulated tx is smaller than tx with signatures
-	suite.Require().True(len(simTxBytes) < len(txBytes), "simulated tx still has signatures")
+			simTxBytes, err := suite.clientCtx.TxConfig.TxJSONEncoder()(tx)
+			suite.Require().Nil(err, "Cannot marshal tx: %v", err)
+			// require that simulated tx is smaller than tx with signatures
+			suite.Require().True(len(simTxBytes) < len(txBytes), "simulated tx still has signatures")
 
-	// Set suite.ctx with smaller simulated TxBytes manually
-	suite.ctx = suite.ctx.WithTxBytes(txBytes)
+			// Set suite.ctx with smaller simulated TxBytes manually
+			suite.ctx = suite.ctx.WithTxBytes(simTxBytes)
 
-	beforeSimGas := suite.ctx.GasMeter().GasConsumed()
+			beforeSimGas := suite.ctx.GasMeter().GasConsumed()
 
-	// run antehandler with simulate=true
-	suite.ctx, err = antehandler(suite.ctx, tx, true)
-	consumedSimGas := suite.ctx.GasMeter().GasConsumed() - beforeSimGas
+			// run antehandler with simulate=true
+			suite.ctx, err = antehandler(suite.ctx, tx, true)
+			consumedSimGas := suite.ctx.GasMeter().GasConsumed() - beforeSimGas
+
+			// require that antehandler passes and does not underestimate decorator cost
+			suite.Require().Nil(err, "ConsumeTxSizeGasDecorator returned error: %v", err)
+			suite.Require().True(consumedSimGas >= expectedGas, "Simulate mode underestimates gas on AnteDecorator. Simulated cost: %d, expected cost: %d", consumedSimGas, expectedGas)
+
+		})
+	}
 
-	// require that antehandler passes and does not underestimate decorator cost
-	suite.Require().Nil(err, "ConsumeTxSizeGasDecorator returned error: %v", err)
-	suite.Require().True(consumedSimGas >= expectedGas, "Simulate mode underestimates gas on AnteDecorator. Simulated cost: %d, expected cost: %d", consumedSimGas, expectedGas)
 }
 
 func (suite *AnteTestSuite) TestTxHeightTimeoutDecorator() {

diff --git a/x/auth/client/cli/cli_test.go b/x/auth/client/cli/cli_test.go
@@ -193,7 +193,9 @@ func (s *IntegrationTestSuite) TestCLISendGenerateSignAndBroadcast() {
 	s.Require().NoError(err)
 	s.Require().Equal(txBuilder.GetTx().GetGas(), uint64(flags.DefaultGasLimit))
 	s.Require().Equal(len(txBuilder.GetTx().GetMsgs()), 1)
-	s.Require().Equal(0, len(txBuilder.GetTx().GetSignatures()))
+	sigs, err := txBuilder.GetTx().GetSignaturesV2()
+	s.Require().NoError(err)
+	s.Require().Equal(0, len(sigs))
 
 	// Test generate sendTx with --gas=$amount
 	limitedGasGeneratedTx, err := bankcli.MsgSendExec(
@@ -215,7 +217,9 @@ func (s *IntegrationTestSuite) TestCLISendGenerateSignAndBroadcast() {
 	s.Require().NoError(err)
 	s.Require().Equal(txBuilder.GetTx().GetGas(), uint64(100))
 	s.Require().Equal(len(txBuilder.GetTx().GetMsgs()), 1)
-	s.Require().Equal(0, len(txBuilder.GetTx().GetSignatures()))
+	sigs, err = txBuilder.GetTx().GetSignaturesV2()
+	s.Require().NoError(err)
+	s.Require().Equal(0, len(sigs))
 
 	resp, err := bankcli.QueryBalancesExec(val1.ClientCtx, val1.Address)
 	s.Require().NoError(err)
@@ -274,7 +278,9 @@ func (s *IntegrationTestSuite) TestCLISendGenerateSignAndBroadcast() {
 	txBuilder, err = val1.ClientCtx.TxConfig.WrapTxBuilder(signedFinalTx)
 	s.Require().NoError(err)
 	s.Require().Equal(len(txBuilder.GetTx().GetMsgs()), 1)
-	s.Require().Equal(1, len(txBuilder.GetTx().GetSignatures()))
+	sigs, err = txBuilder.GetTx().GetSignaturesV2()
+	s.Require().NoError(err)
+	s.Require().Equal(1, len(sigs))
 	s.Require().Equal(val1.Address.String(), txBuilder.GetTx().GetSigners()[0].String())
 
 	// Write the output to disk

diff --git a/x/auth/signing/sig_verifiable_tx.go b/x/auth/signing/sig_verifiable_tx.go
@@ -13,7 +13,6 @@ type SigVerifiableTx interface {
 	types.Tx
 	GetSigners() []types.AccAddress
 	GetPubKeys() []crypto.PubKey // If signer already has pubkey in context, this list will have nil in its place
-	GetSignatures() [][]byte
 	GetSignaturesV2() ([]signing.SignatureV2, error)
 }
 

diff --git a/x/auth/tx/builder.go b/x/auth/tx/builder.go
@@ -237,15 +237,23 @@ func (w *wrapper) GetSignaturesV2() ([]signing.SignatureV2, error) {
 	res := make([]signing.SignatureV2, n)
 
 	for i, si := range signerInfos {
-		var err error
-		sigData, err := ModeInfoAndSigToSignatureData(si.ModeInfo, sigs[i])
-		if err != nil {
-			return nil, err
-		}
-		res[i] = signing.SignatureV2{
-			PubKey:   pubKeys[i],
-			Data:     sigData,
-			Sequence: si.GetSequence(),
+		// handle nil signatures (in case of simulation)
+		if si.ModeInfo == nil {
+			res[i] = signing.SignatureV2{
+				PubKey: pubKeys[i],
+			}
+		} else {
+			var err error
+			sigData, err := ModeInfoAndSigToSignatureData(si.ModeInfo, sigs[i])
+			if err != nil {
+				return nil, err
+			}
+			res[i] = signing.SignatureV2{
+				PubKey:   pubKeys[i],
+				Data:     sigData,
+				Sequence: si.GetSequence(),
+			}
+
 		}
 	}