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] Timestamp nonce #1970

Merged
merged 21 commits into from
Jul 26, 2024
Merged
Show file tree
Hide file tree
Changes from 20 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
13 changes: 10 additions & 3 deletions protocol/app/ante.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import (
"github.com/dydxprotocol/v4-chain/protocol/lib"
libante "github.com/dydxprotocol/v4-chain/protocol/lib/ante"
"github.com/dydxprotocol/v4-chain/protocol/lib/log"
accountplusante "github.com/dydxprotocol/v4-chain/protocol/x/accountplus/ante"
accountpluskeeper "github.com/dydxprotocol/v4-chain/protocol/x/accountplus/keeper"
clobante "github.com/dydxprotocol/v4-chain/protocol/x/clob/ante"
clobtypes "github.com/dydxprotocol/v4-chain/protocol/x/clob/types"
Expand Down Expand Up @@ -111,7 +112,7 @@ func NewAnteHandler(options HandlerOptions) (sdk.AnteHandler, error) {
validateMemo: ante.NewValidateMemoDecorator(options.AccountKeeper),
validateBasic: ante.NewValidateBasicDecorator(),
validateSigCount: ante.NewValidateSigCountDecorator(options.AccountKeeper),
incrementSequence: customante.NewIncrementSequenceDecorator(options.AccountKeeper),
incrementSequence: ante.NewIncrementSequenceDecorator(options.AccountKeeper),
sigVerification: customante.NewSigVerificationDecorator(
options.AccountKeeper,
*options.AccountplusKeeper,
Expand Down Expand Up @@ -150,7 +151,7 @@ type lockingAnteHandler struct {
validateMemo ante.ValidateMemoDecorator
validateBasic ante.ValidateBasicDecorator
validateSigCount ante.ValidateSigCountDecorator
incrementSequence customante.IncrementSequenceDecorator
incrementSequence ante.IncrementSequenceDecorator
sigVerification customante.SigVerificationDecorator
consumeTxSizeGas ante.ConsumeTxSizeGasDecorator
deductFee ante.DeductFeeDecorator
Expand Down Expand Up @@ -248,7 +249,13 @@ func (h *lockingAnteHandler) clobAnteHandle(ctx sdk.Context, tx sdk.Tx, simulate
if isShortTerm, err = clobante.IsShortTermClobMsgTx(ctx, tx); err != nil {
return ctx, err
}
if !isShortTerm {

var isTimestampNonce bool
if isTimestampNonce, err = accountplusante.IsTimestampNonceTx(ctx, tx); err != nil {
return ctx, err
}

if !isShortTerm && !isTimestampNonce {
Comment on lines +253 to +258
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider logging the timestamp nonce check for better debugging.

The check for timestamp nonce transactions can be logged for better debugging and monitoring.

var isTimestampNonce bool
if isTimestampNonce, err = accountplusante.IsTimestampNonceTx(ctx, tx); err != nil {
	return ctx, err
}
fmt.Printf("Transaction %s is a timestamp nonce: %t", tx.GetTxHash(), isTimestampNonce)

if !isShortTerm && !isTimestampNonce {
	if ctx, err = h.incrementSequence.AnteHandle(ctx, tx, simulate, noOpAnteHandle); err != nil {
		return ctx, err
	}
}

if ctx, err = h.incrementSequence.AnteHandle(ctx, tx, simulate, noOpAnteHandle); err != nil {
return ctx, err
}
Expand Down
53 changes: 25 additions & 28 deletions protocol/app/ante/sigverify.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package ante

import (
"fmt"
"time"

errorsmod "cosmossdk.io/errors"
txsigning "cosmossdk.io/x/tx/signing"
Expand All @@ -13,6 +14,7 @@ import (
authsigning "github.com/cosmos/cosmos-sdk/x/auth/signing"
"github.com/dydxprotocol/v4-chain/protocol/lib/metrics"
accountpluskeeper "github.com/dydxprotocol/v4-chain/protocol/x/accountplus/keeper"
accountplustypes "github.com/dydxprotocol/v4-chain/protocol/x/accountplus/types"
gometrics "github.com/hashicorp/go-metrics"
"google.golang.org/protobuf/types/known/anypb"
)
Expand Down Expand Up @@ -63,7 +65,8 @@ func (svd SigVerificationDecorator) AnteHandle(
return ctx, err
}

// check that signer length and signature length are the same
// Check that signer length and signature length are the same.
// The ordering of the sigs and signers have matching ordering (sigs[i] belongs to signers[i]).
if len(sigs) != len(signers) {
err := errorsmod.Wrapf(
sdkerrors.ErrUnauthorized,
Expand All @@ -78,6 +81,7 @@ func (svd SigVerificationDecorator) AnteHandle(
// only messages that use `GoodTilBlock` for replay protection.
skipSequenceValidation := ShouldSkipSequenceValidation(tx.GetMsgs())

// Iterate on sig and signer pairs.
for i, sig := range sigs {
acc, err := sdkante.GetSignerAcc(ctx, svd.ak, signers[i])
if err != nil {
Expand All @@ -95,35 +99,28 @@ func (svd SigVerificationDecorator) AnteHandle(
// `GoodTilBlock` for replay protection.
if !skipSequenceValidation {
if accountpluskeeper.IsTimestampNonce(sig.Sequence) {
tsNonce := sig.Sequence
blockTs := uint64(ctx.BlockTime().UnixMilli())
address := acc.GetAddress()

if !accountpluskeeper.IsValidTimestampNonce(tsNonce, blockTs) {
return ctx, errorsmod.Wrapf(
sdkerrors.ErrWrongSequence,
"timestamp nonce %d not within valid time window", tsNonce,
defer telemetry.ModuleMeasureSince(
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be put within ProcessTimestampNonce so that the defer clause is run when ProcessTimestampNonce returns. This is a common pattern to measure the latency of a function from start to finish

Copy link
Contributor Author

Choose a reason for hiding this comment

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

				defer telemetry.ModuleMeasureSince(
					accountplustypes.ModuleName,
					time.Now(),
					metrics.TimestampNonce,
					metrics.Latency,
				)
				err = svd.akp.ProcessTimestampNonce(ctx, acc, sig.Sequence)

A bit confused. As u said defer run after the function below is run. In this case it is right above
err = svd.akp.ProcessTimestampNonce(ctx, acc, sig.Sequence)
So it will run after ProcessTimestampNonce?

Copy link
Contributor

Choose a reason for hiding this comment

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

A defer statement defers the execution of a function until the surrounding function returns. https://go.dev/tour/flowcontrol/12

accountplustypes.ModuleName,
time.Now(),
metrics.TimestampNonce,
metrics.Latency,
)
err = svd.akp.ProcessTimestampNonce(ctx, acc, sig.Sequence)

if err == nil {
telemetry.IncrCounterWithLabels(
[]string{metrics.TimestampNonce, metrics.Valid, metrics.Count},
1,
[]gometrics.Label{metrics.GetLabelForIntValue(metrics.ExecMode, int(ctx.ExecMode()))},
)
}
accountState, found := svd.akp.GetAccountState(ctx, address)
if !found {
err := svd.akp.InitializeAccountWithTimestampNonceDetails(ctx, address, tsNonce)
if err != nil {
return ctx, errorsmod.Wrapf(
sdkerrors.ErrLogic,
fmt.Sprintf("failed to initialize AccountState for address %d", address),
)
}
return ctx, nil
} else {
accountpluskeeper.EjectStaleTimestampNonces(&accountState, blockTs)
tsNonceAccepted := accountpluskeeper.AttemptTimestampNonceUpdate(tsNonce, &accountState)
if !tsNonceAccepted {
return ctx, errorsmod.Wrapf(
sdkerrors.ErrWrongSequence,
"timestamp nonce %d rejected", tsNonce,
)
}
svd.akp.SetAccountState(ctx, address, accountState)
telemetry.IncrCounterWithLabels(
[]string{metrics.TimestampNonce, metrics.Invalid, metrics.Count},
1,
[]gometrics.Label{metrics.GetLabelForIntValue(metrics.ExecMode, int(ctx.ExecMode()))},
)
return ctx, errorsmod.Wrapf(sdkerrors.ErrWrongSequence, err.Error())
}
} else {
if sig.Sequence != acc.GetSequence() {
Expand Down
57 changes: 0 additions & 57 deletions protocol/app/ante/timestampnonce.go

This file was deleted.

75 changes: 0 additions & 75 deletions protocol/app/ante/timestampnonce_test.go

This file was deleted.

4 changes: 4 additions & 0 deletions protocol/lib/metrics/constants.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ const (
Deterministic = "deterministic"
Distribution = "distribution"
Error = "error"
ExecMode = "exec_mode"
GitCommit = "git_commit"
HttpGet5xx = "http_get_5xx"
HttpGetHangup = "http_get_hangup"
Expand Down Expand Up @@ -407,6 +408,9 @@ const (
ValidatorNumMatchedTakerOrders = "validator_num_matched_taker_orders"
ValidatorVolumeQuoteQuantums = "validator_volume_quote_quantums"

// x/acocuntplus
TimestampNonce = "timestamp_nonce"

// x/ratelimit
Capacity = "capacity"
RateLimitDenom = "rate_limit_denom"
Expand Down
28 changes: 28 additions & 0 deletions protocol/x/accountplus/ante/timestampnonce.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
package ante

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

// IsTimestampNonceTx returns `true` if the supplied `tx` consist of a single signature that uses a timestamp nonce
// value for sequence
func IsTimestampNonceTx(ctx sdk.Context, tx sdk.Tx) (bool, error) {
sigTx, ok := tx.(authsigning.SigVerifiableTx)
if !ok {
return false, errorsmod.Wrap(sdkerrors.ErrTxDecode, "invalid transaction type")
}
signatures, err := sigTx.GetSignaturesV2()
if err != nil {
return false, err
}

if len(signatures) != 1 {
return false, errorsmod.Wrap(sdkerrors.ErrTxDecode, "more than one signature")
}

return accountpluskeeper.IsTimestampNonce(signatures[0].Sequence), nil
}
85 changes: 85 additions & 0 deletions protocol/x/accountplus/ante/timestampnonce_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
package ante_test

import (
"testing"

"github.com/cosmos/cosmos-sdk/codec"
codectypes "github.com/cosmos/cosmos-sdk/codec/types"
"github.com/cosmos/cosmos-sdk/testutil/testdata"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/types/tx/signing"
authtx "github.com/cosmos/cosmos-sdk/x/auth/tx"
"github.com/dydxprotocol/v4-chain/protocol/testutil/constants"
sdktest "github.com/dydxprotocol/v4-chain/protocol/testutil/sdk"
"github.com/dydxprotocol/v4-chain/protocol/x/accountplus/ante"
"github.com/dydxprotocol/v4-chain/protocol/x/accountplus/keeper"
"github.com/stretchr/testify/require"
)

func TestIsTimestampNonceTx(t *testing.T) {
tests := map[string]struct {
seqs []uint64
expectedResult bool
expectedErr bool
}{
"Returns false for non-ts nonce": {
seqs: []uint64{0},
expectedResult: false,
expectedErr: false,
},
"Returns true for ts nonce": {
seqs: []uint64{keeper.TimestampNonceSequenceCutoff},
expectedResult: true,
expectedErr: false,
},
"Returns error for more than one signature": {
seqs: []uint64{keeper.TimestampNonceSequenceCutoff, keeper.TimestampNonceSequenceCutoff},
expectedResult: false,
expectedErr: true,
},
}

// Run tests.
for name, tc := range tests {
t.Run(name, func(t *testing.T) {
// Initialize some test setup which builds a test transaction from a slice of messages.
var reg codectypes.InterfaceRegistry
protoCfg := authtx.NewTxConfig(codec.NewProtoCodec(reg), authtx.DefaultSignModes)
builder := protoCfg.NewTxBuilder()
err := builder.SetMsgs([]sdk.Msg{constants.Msg_Send}...)
require.NoError(t, err)

// Create signatures
var signatures []signing.SignatureV2
for _, seq := range tc.seqs {
signatures = append(signatures, getSignature(seq))
}
err = builder.SetSignatures(signatures...)

require.NoError(t, err)
tx := builder.GetTx()
ctx, _, _ := sdktest.NewSdkContextWithMultistore()

// Invoke the function under test.
result, err := ante.IsTimestampNonceTx(ctx, tx)

// Assert the results.
if tc.expectedErr {
require.NotNil(t, err)
}
require.Equal(t, tc.expectedResult, result)
})
}
}

func getSignature(seq uint64) signing.SignatureV2 {
_, pubKey, _ := testdata.KeyTestPubAddr()
return signing.SignatureV2{
PubKey: pubKey,
Data: &signing.SingleSignatureData{
SignMode: signing.SignMode_SIGN_MODE_DIRECT,
Signature: nil,
},
Sequence: seq,
}
}
Loading
Loading