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.

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

Remove GetSignatures from SigVerifiableTx #6550

Merged
merged 14 commits into from
Sep 2, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
10 changes: 6 additions & 4 deletions client/testutil/suite.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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")
Expand All @@ -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())
}

Expand Down
5 changes: 4 additions & 1 deletion client/tx/tx_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
37 changes: 33 additions & 4 deletions x/auth/ante/basic.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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")
}
Expand All @@ -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)
aaronc marked this conversation as resolved.
Show resolved Hide resolved

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
}

Expand Down Expand Up @@ -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:
fedekunze marked this conversation as resolved.
Show resolved Hide resolved
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.
Expand Down
108 changes: 61 additions & 47 deletions x/auth/ante/basic_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down Expand Up @@ -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()
Expand All @@ -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() {
Expand Down
12 changes: 9 additions & 3 deletions x/auth/client/cli/cli_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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)
Expand Down Expand Up @@ -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
Expand Down
1 change: 0 additions & 1 deletion x/auth/signing/sig_verifiable_tx.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

Expand Down
26 changes: 17 additions & 9 deletions x/auth/tx/builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit, wrap error with signer info index

}
res[i] = signing.SignatureV2{
PubKey: pubKeys[i],
Data: sigData,
Sequence: si.GetSequence(),
}

}
}

Expand Down