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

refactor(auth): incorporate set pub key decorator into sig verification decorator #19093

Merged
merged 12 commits into from
Jan 22, 2024
4 changes: 2 additions & 2 deletions baseapp/block_gas_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -168,11 +168,11 @@ func TestBaseApp_BlockGas(t *testing.T) {
}
require.Empty(t, okValue)
} else {
require.Equal(t, uint32(0), rsp.TxResults[0].Code)
require.Equal(t, uint32(0), rsp.TxResults[0].Code, "failure", rsp.TxResults[0].Log)
require.Equal(t, []byte("ok"), okValue)
}
// check block gas is always consumed
baseGas := uint64(54436) // baseGas is the gas consumed before tx msg
baseGas := uint64(38798) // baseGas is the gas consumed before tx msg
Copy link
Contributor Author

@testinginprod testinginprod Jan 17, 2024

Choose a reason for hiding this comment

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

we have almost halved the execution costs since we're not redundantly getting and decoding from state, also the account is saved in state only once instead of twice

expGasConsumed := addUint64Saturating(tc.gasToConsume, baseGas)
if expGasConsumed > uint64(simtestutil.DefaultConsensusParams.Block.MaxGas) {
// capped by gasLimit
Expand Down
1 change: 0 additions & 1 deletion simapp/ante.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@ func NewAnteHandler(options HandlerOptions) (sdk.AnteHandler, error) {
ante.NewValidateMemoDecorator(options.AccountKeeper),
ante.NewConsumeGasForTxSizeDecorator(options.AccountKeeper),
ante.NewDeductFeeDecorator(options.AccountKeeper, options.BankKeeper, options.FeegrantKeeper, options.TxFeeChecker),
ante.NewSetPubKeyDecorator(options.AccountKeeper), // SetPubKeyDecorator must be called before all signature verification decorators
ante.NewValidateSigCountDecorator(options.AccountKeeper),
ante.NewSigVerificationDecorator(options.AccountKeeper, options.SignModeHandler, options.SigGasConsumer),
}
Expand Down
4 changes: 1 addition & 3 deletions tests/integration/bank/app_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -419,10 +419,8 @@ func TestMsgSetSendEnabled(t *testing.T) {
},
accSeqs: []uint64{1}, // wrong signer, so this sequence doesn't actually get used.
expInError: []string{
"pubKey does not match signer address",
"cannot be claimed by public key with address",
govAddr,
"with signer index: 0",
"invalid pubkey",
},
},
{
Expand Down
1 change: 1 addition & 0 deletions x/auth/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
### Consensus Breaking Changes

* [#18817](https://github.com/cosmos/cosmos-sdk/pull/18817) SigVerification, GasConsumption, IncreaseSequence ante decorators have all been joined into one SigVerification decorator. Gas consumption during TX validation flow has reduced.
* [#19093](https://github.com/cosmos/cosmos-sdk/pull/19093) SetPubKeyDecorator was merged into SigVerification, gas consumption is almost halved for a simple tx.

### Bug Fixes

Expand Down
1 change: 0 additions & 1 deletion x/auth/ante/ante.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@ func NewAnteHandler(options HandlerOptions) (sdk.AnteHandler, error) {
NewValidateMemoDecorator(options.AccountKeeper),
NewConsumeGasForTxSizeDecorator(options.AccountKeeper),
NewDeductFeeDecorator(options.AccountKeeper, options.BankKeeper, options.FeegrantKeeper, options.TxFeeChecker),
NewSetPubKeyDecorator(options.AccountKeeper), // SetPubKeyDecorator must be called before all signature verification decorators
NewValidateSigCountDecorator(options.AccountKeeper),
NewSigVerificationDecorator(options.AccountKeeper, options.SignModeHandler, options.SigGasConsumer),
}
Expand Down
77 changes: 1 addition & 76 deletions x/auth/ante/ante_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -166,25 +166,6 @@ func TestAnteHandlerSigErrors(t *testing.T) {
false,
sdkerrors.ErrUnknownAddress,
},
{
"save the first account, but second is still unrecognized",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this test is not valid anymore after the change in behaviours, since the pubkey checking step is done separately during sig verification

func(suite *AnteTestSuite) TestCaseArgs {
suite.accountKeeper.SetAccount(suite.ctx, suite.accountKeeper.NewAccountWithAddress(suite.ctx, addr0))
suite.bankKeeper.EXPECT().SendCoinsFromAccountToModule(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil)

privs, accNums, accSeqs := []cryptotypes.PrivKey{priv0, priv1, priv2}, []uint64{0, 1, 2}, []uint64{0, 0, 0}

return TestCaseArgs{
accNums: accNums,
accSeqs: accSeqs,
msgs: msgs,
privs: privs,
}
},
false,
false,
sdkerrors.ErrUnknownAddress,
},
{
"save all the accounts, should pass",
func(suite *AnteTestSuite) TestCaseArgs {
Expand Down Expand Up @@ -1121,62 +1102,6 @@ func TestAnteHandlerSetPubKey(t *testing.T) {
false,
sdkerrors.ErrWrongSequence,
},
{
"make sure previous public key has been set after wrong signature",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this test is not valid anymore because each account has it's own set pub key step

func(suite *AnteTestSuite) TestCaseArgs {
accs := suite.CreateTestAccounts(2)
suite.bankKeeper.EXPECT().SendCoinsFromAccountToModule(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil)

// Make sure public key has not been set from previous test.
acc1 := suite.accountKeeper.GetAccount(suite.ctx, accs[1].acc.GetAddress())
require.Nil(t, acc1.GetPubKey())

privs, accNums, accSeqs := []cryptotypes.PrivKey{accs[1].priv}, []uint64{accs[1].acc.GetAccountNumber()}, []uint64{accs[1].acc.GetSequence()}
msgs := []sdk.Msg{testdata.NewTestMsg(accs[1].acc.GetAddress())}
err := suite.txBuilder.SetMsgs(msgs...)
require.NoError(t, err)
suite.txBuilder.SetFeeAmount(feeAmount)
suite.txBuilder.SetGasLimit(gasLimit)

// Manually create tx, and remove signature.
tx, err := suite.CreateTestTx(suite.ctx, privs, accNums, accSeqs, suite.ctx.ChainID(), signing.SignMode_SIGN_MODE_DIRECT)
require.NoError(t, err)
txBuilder, err := suite.clientCtx.TxConfig.WrapTxBuilder(tx)
require.NoError(t, err)
require.NoError(t, txBuilder.SetSignatures())

// Run anteHandler manually, expect ErrNoSignatures.
_, err = suite.anteHandler(suite.ctx, txBuilder.GetTx(), false)
require.Error(t, err)
require.True(t, errors.Is(err, sdkerrors.ErrNoSignatures))

// Make sure public key has not been set.
acc1 = suite.accountKeeper.GetAccount(suite.ctx, accs[1].acc.GetAddress())
require.Nil(t, acc1.GetPubKey())

// Set incorrect accSeq, to generate incorrect signature.
privs, accNums, accSeqs = []cryptotypes.PrivKey{accs[1].priv}, []uint64{accs[1].acc.GetAccountNumber()}, []uint64{1}

suite.ctx, err = suite.DeliverMsgs(t, privs, msgs, feeAmount, gasLimit, accNums, accSeqs, suite.ctx.ChainID(), false)
require.Error(t, err)

// Make sure public key has been set, as SetPubKeyDecorator
// is called before all signature verification decorators.
acc1 = suite.accountKeeper.GetAccount(suite.ctx, accs[1].acc.GetAddress())
require.Equal(t, acc1.GetPubKey(), accs[1].priv.PubKey())
suite.bankKeeper.EXPECT().SendCoinsFromAccountToModule(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil)

return TestCaseArgs{
accNums: accNums,
accSeqs: accSeqs,
msgs: msgs,
privs: privs,
}
},
false,
false,
sdkerrors.ErrWrongSequence,
},
}

for _, tc := range testCases {
Expand Down Expand Up @@ -1206,7 +1131,7 @@ func generatePubKeysAndSignatures(n int, msg []byte, _ bool) (pubkeys []cryptoty
// privkey = ed25519.GenPrivKey()
// } else {
// privkey = secp256k1.GenPrivKey()
//}
// }
testinginprod marked this conversation as resolved.
Show resolved Hide resolved

pubkeys[i] = privkey.PubKey()
signatures[i], _ = privkey.Sign(msg)
Expand Down
Loading
Loading