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

[OTE-553] Skip sequence increment for timestamp nonce #1956

Merged
merged 12 commits into from
Jul 23, 2024
4 changes: 2 additions & 2 deletions protocol/app/ante.go
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ func NewAnteHandler(options HandlerOptions) (sdk.AnteHandler, error) {
validateMemo: ante.NewValidateMemoDecorator(options.AccountKeeper),
validateBasic: ante.NewValidateBasicDecorator(),
validateSigCount: ante.NewValidateSigCountDecorator(options.AccountKeeper),
incrementSequence: ante.NewIncrementSequenceDecorator(options.AccountKeeper),
incrementSequence: customante.NewIncrementSequenceDecorator(options.AccountKeeper),
sigVerification: customante.NewSigVerificationDecorator(options.AccountKeeper, options.SignModeHandler),
consumeTxSizeGas: ante.NewConsumeGasForTxSizeDecorator(options.AccountKeeper),
deductFee: ante.NewDeductFeeDecorator(
Expand Down Expand Up @@ -140,7 +140,7 @@ type lockingAnteHandler struct {
validateMemo ante.ValidateMemoDecorator
validateBasic ante.ValidateBasicDecorator
validateSigCount ante.ValidateSigCountDecorator
incrementSequence ante.IncrementSequenceDecorator
incrementSequence customante.IncrementSequenceDecorator
sigVerification customante.SigVerificationDecorator
consumeTxSizeGas ante.ConsumeTxSizeGasDecorator
deductFee ante.DeductFeeDecorator
Expand Down
55 changes: 55 additions & 0 deletions protocol/app/ante/timestampnonce.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
package ante

import (
errorsmod "cosmossdk.io/errors"
sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
ante "github.com/cosmos/cosmos-sdk/x/auth/ante"
authsigning "github.com/cosmos/cosmos-sdk/x/auth/signing"
accountpluskeeper "github.com/dydxprotocol/v4-chain/protocol/x/accountplus/keeper"
)

// TODO: combine increment sequence and sequence verification into one decorator
// https://github.com/cosmos/cosmos-sdk/pull/18817
type IncrementSequenceDecorator struct {
ak ante.AccountKeeper
}

func NewIncrementSequenceDecorator(ak ante.AccountKeeper) IncrementSequenceDecorator {
Copy link
Contributor

@teddyding teddyding Jul 25, 2024

Choose a reason for hiding this comment

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

What's the rationale behind forking this decorator here, vs updating it in the dydxprotocol/cosmo-sdk fork? So that we can call accountpluskeeper helper function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was under the impression that updating the fork required additional release complexity.

There is actually a ticket to deprecate IncrementSequenceDecorator altogether by adding the incrementation into sigverify. (https://linear.app/dydx/issue/OTE-624/combine-incrementsequencedecorator-into-sigverificationdecorator)

I think I will have time to address this ticket and the other comments before v6 release. Will publish new PR soon

return IncrementSequenceDecorator{
ak: ak,
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Add a comment saying this is forked from x/auth/ante/sigverify.go

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will add in upcoming PR

func (isd IncrementSequenceDecorator) AnteHandle(
ctx sdk.Context,
tx sdk.Tx,
simulate bool,
next sdk.AnteHandler,
) (sdk.Context, error) {
sigTx, ok := tx.(authsigning.SigVerifiableTx)
if !ok {
return ctx, errorsmod.Wrap(sdkerrors.ErrTxDecode, "invalid transaction type")
}

signatures, err := sigTx.GetSignaturesV2()
if err != nil {
return sdk.Context{}, err
}

for _, signature := range signatures {
Copy link
Contributor

@teddyding teddyding Jul 25, 2024

Choose a reason for hiding this comment

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

Note this technically changes the semantics of IncrementSequenceDecorator. It used to increment sequence for all sigTx.GetSigners() (everyone who's supposed to sign the transaction), now it's iterating over all the signatures (everyone who signed the transactions). These are not inherently the same set, so it's relying on upstream logic to call SigVerifyDecorator correctly (checking they are the same set).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a need to revert to iterating on signers and check that the signer has a signature?

if accountpluskeeper.IsTimestampNonce(signature.Sequence) {
Copy link
Contributor

@teddyding teddyding Jul 25, 2024

Choose a reason for hiding this comment

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

We have a very specific use case for timestamp nonce feature, and it seems to me the following will never happen:

  • timestamp nonce transaction that requires multiple signers
  • (vacuously true from above) timestamp nonce being used in combination with regular sequence

Given this and previous comment, the easiest/least disruptive change (and avoiding a fork) seems to be:

  • Implement a function IsTimestampNounceTx similar to this
  • Update this logic to
	if !isShortTerm && !useTimestampNonce {
		if ctx, err = h.incrementSequence.AnteHandle(ctx, tx, simulate, noOpAnteHandle); err != nil {
			return ctx, err
		}
	}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

// Skip increment for this signature
continue
}

acc := isd.ak.GetAccount(ctx, signature.PubKey.Address().Bytes())
if err := acc.SetSequence(acc.GetSequence() + 1); err != nil {
panic(err)
}

isd.ak.SetAccount(ctx, acc)
}

return next(ctx, tx, simulate)
}
75 changes: 75 additions & 0 deletions protocol/app/ante/timestampnonce_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
package ante_test

import (
"testing"

testante "github.com/dydxprotocol/v4-chain/protocol/testutil/ante"
"github.com/stretchr/testify/require"

cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types"
"github.com/cosmos/cosmos-sdk/testutil/testdata"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/types/tx/signing"

customante "github.com/dydxprotocol/v4-chain/protocol/app/ante"
accountpluskeeper "github.com/dydxprotocol/v4-chain/protocol/x/accountplus/keeper"
)

// Modified from cosmossdk test for IncrementSequenceDecorator
func TestDydxIncrementSequenceDecorator(t *testing.T) {
suite := testante.SetupTestSuite(t, true)
suite.TxBuilder = suite.ClientCtx.TxConfig.NewTxBuilder()

priv, _, addr := testdata.KeyTestPubAddr()
acc := suite.AccountKeeper.NewAccountWithAddress(suite.Ctx, addr)
require.NoError(t, acc.SetAccountNumber(uint64(50)))
suite.AccountKeeper.SetAccount(suite.Ctx, acc)

msgs := []sdk.Msg{testdata.NewTestMsg(addr)}
require.NoError(t, suite.TxBuilder.SetMsgs(msgs...))
privs := []cryptotypes.PrivKey{priv}
accNums := []uint64{suite.AccountKeeper.GetAccount(suite.Ctx, addr).GetAccountNumber()}
feeAmount := testdata.NewTestFeeAmount()
gasLimit := testdata.NewTestGasLimit()
suite.TxBuilder.SetFeeAmount(feeAmount)
suite.TxBuilder.SetGasLimit(gasLimit)

isd := customante.NewIncrementSequenceDecorator(suite.AccountKeeper)
antehandler := sdk.ChainAnteDecorators(isd)

testCases := []struct {
ctx sdk.Context
simulate bool
// This value need not be valid (accountSeq + 1). Validity is handed in customante.NewSigVerificationDecorator
signatureSeq uint64
expectedSeq uint64
}{
// tests from cosmossdk checking incrementing seqence
{suite.Ctx.WithIsReCheckTx(true), false, 0, 1},
{suite.Ctx.WithIsCheckTx(true).WithIsReCheckTx(false), false, 0, 2},
{suite.Ctx.WithIsReCheckTx(true), false, 0, 3},
{suite.Ctx.WithIsReCheckTx(true), false, 0, 4},
{suite.Ctx.WithIsReCheckTx(true), true, 0, 5},

// tests checking that tx with timestamp nonces will not increment sequence
{suite.Ctx.WithIsReCheckTx(true), true, accountpluskeeper.TimestampNonceSequenceCutoff, 5},
{suite.Ctx.WithIsReCheckTx(true), true, accountpluskeeper.TimestampNonceSequenceCutoff + 100000, 5},
}

for i, tc := range testCases {
accSeqs := []uint64{tc.signatureSeq}
tx, err := suite.CreateTestTx(
suite.Ctx,
privs,
accNums,
accSeqs,
suite.Ctx.ChainID(),
signing.SignMode_SIGN_MODE_DIRECT,
)
require.NoError(t, err)

_, err = antehandler(tc.ctx, tx, tc.simulate)
require.NoError(t, err, "unexpected error; tc #%d, %v", i, tc)
require.Equal(t, tc.expectedSeq, suite.AccountKeeper.GetAccount(suite.Ctx, addr).GetSequence())
}
}
2 changes: 2 additions & 0 deletions protocol/x/accountplus/keeper/keeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,8 @@ func (k Keeper) SetGenesisState(ctx sdk.Context, data types.GenesisState) error
return nil
}

// TODO: refactor this function -> InitializeWithTimestampNonceDetails
// Writing to store is expensive so directly write with ts-nonce instead of initializing an empty account then setting.
func (k Keeper) InitializeAccount(ctx sdk.Context, address sdk.AccAddress) error {
if _, found := k.GetAccountState(ctx, address); found {
return errors.New(
Expand Down
6 changes: 5 additions & 1 deletion protocol/x/accountplus/keeper/timestampnonce.go
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
package keeper

func Placeholder() {}
const TimestampNonceSequenceCutoff uint64 = 1 << 40 // 2^40

func IsTimestampNonce(ts uint64) bool {
return ts >= TimestampNonceSequenceCutoff
}
9 changes: 8 additions & 1 deletion protocol/x/accountplus/keeper/timestampnonce_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,13 @@ package keeper_test

import (
"testing"

"github.com/dydxprotocol/v4-chain/protocol/x/accountplus/keeper"
"github.com/stretchr/testify/require"
)

func Placeholder(t *testing.T) {}
func TestIsTimestampNonce(t *testing.T) {
require.True(t, keeper.IsTimestampNonce(keeper.TimestampNonceSequenceCutoff))
require.True(t, keeper.IsTimestampNonce(keeper.TimestampNonceSequenceCutoff+uint64(1)))
require.False(t, keeper.IsTimestampNonce(keeper.TimestampNonceSequenceCutoff-uint64(1)))
}
jerryfan01234 marked this conversation as resolved.
Show resolved Hide resolved
Loading