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.

Sign up for GitHub

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

[OTE-553] Timestamp nonce #1970

Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
a2d9900
Create accountplus module with timestamp nonce proto and genesis
jerryfan01234 Jul 15, 2024
42a35ef
Address PR comments
jerryfan01234 Jul 16, 2024
d24dc06
fix proto comment
jerryfan01234 Jul 16, 2024
741c8cd
Add remainder of module scaffolding
jerryfan01234 Jul 18, 2024
f1a8c12
fix merge conflict
jerryfan01234 Jul 18, 2024
3e469e6
Add unit test
jerryfan01234 Jul 18, 2024
03b7ed1
Add additional scaffolding
jerryfan01234 Jul 19, 2024
952dfdc
Merge branch 'main' into timestamp-nonce
jerryfan01234 Jul 22, 2024
207a4db
Skip increment sequence for timestamp nonce
jerryfan01234 Jul 19, 2024
a6ae93c
Address PR comments
jerryfan01234 Jul 23, 2024
3fdb5b8
resolve merge conflict
jerryfan01234 Jul 23, 2024
9e4e31b
resovle merge conflict from local branch
jerryfan01234 Jul 23, 2024
721f00b
Add unit tests
jerryfan01234 Jul 23, 2024
0742da7
Add ts nonce validation and update to ante handler
jerryfan01234 Jul 23, 2024
9d56920
Resolve merge conflict
jerryfan01234 Jul 23, 2024
2e193fb
Resolve merge conflict
jerryfan01234 Jul 23, 2024
23a90c2
Full implementation of ts nonce in ante handler with tests
jerryfan01234 Jul 24, 2024
5d057b7
Merge branch 'main' into timestamp-nonce
jerryfan01234 Jul 25, 2024
5625b2e
Refactor timestamp nonce ante handler and sigverify
jerryfan01234 Jul 25, 2024
fe97218
address pr comments
jerryfan01234 Jul 26, 2024
d866fa7
revision
jerryfan01234 Jul 26, 2024
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
19 changes: 14 additions & 5 deletions protocol/app/ante.go
View file
Open in desktop
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 All @@ -28,8 +29,10 @@ import (
// struct embedding to include the normal cosmos-sdk `HandlerOptions`.
type HandlerOptions struct {
ante.HandlerOptions
Codec codec.Codec
AuthStoreKey storetypes.StoreKey
Codec codec.Codec
AuthStoreKey storetypes.StoreKey
// TODO: create interface for accountplus keeper when mocking need arises
jerryfan01234 marked this conversation as resolved.
Show resolved Hide resolved
// https://github.com/dydxprotocol/v4-chain/pull/1963#discussion_r1691904072
jerryfan01234 marked this conversation as resolved.
Show resolved Hide resolved
AccountplusKeeper *accountpluskeeper.Keeper
ClobKeeper clobtypes.ClobKeeper
PerpetualsKeeper perpetualstypes.PerpetualsKeeper
Expand Down Expand Up @@ -111,7 +114,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 +153,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 +251,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
48 changes: 21 additions & 27 deletions protocol/app/ante/sigverify.go
View file
Open in desktop
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 Down Expand Up @@ -63,7 +64,10 @@ 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.
// If the lengths are the same, then sigs and signers are ordered correctly. This means that each expected signer
// provided a signature. In cosmos original sigverify, the ordering is explicitly checked.
jerryfan01234 marked this conversation as resolved.
Show resolved Hide resolved
//https://github.com/cosmos/cosmos-sdk/blob/502450cd1ef0b6bd77807922091893611c370c5d/x/auth/ante/sigverify.go#L189
if len(sigs) != len(signers) {
err := errorsmod.Wrapf(
sdkerrors.ErrUnauthorized,
Expand All @@ -78,6 +82,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 +100,24 @@ 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()
start := time.Now()
err = svd.akp.ProcessTimestampNonce(ctx, acc, sig.Sequence)

if !accountpluskeeper.IsValidTimestampNonce(tsNonce, blockTs) {
return ctx, errorsmod.Wrapf(
sdkerrors.ErrWrongSequence,
"timestamp nonce %d not within valid time window", tsNonce,
telemetry.MeasureSince(start, []string{metrics.TimestampNonce, metrics.Latency}...)
jerryfan01234 marked this conversation as resolved.
Show resolved Hide resolved
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())
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 nonce processing duration for better monitoring.

The duration of nonce processing can be logged for better monitoring and performance analysis.

start := time.Now()
err = svd.akp.ProcessTimestampNonce(ctx, acc, sig.Sequence)
duration := time.Since(start)

telemetry.MeasureSince(start, []string{metrics.TimestampNonce, metrics.Latency}...)
if err == nil {
	telemetry.IncrCounterWithLabels(
		[]string{metrics.TimestampNonce, metrics.Valid, metrics.Count},
		1,
		[]gometrics.Label{metrics.GetLabelForIntValue(metrics.ExecMode, int(ctx.ExecMode()))},
	)
	fmt.Printf("Processed valid timestamp nonce %d in %s", sig.Sequence, duration)
	return ctx, nil
} else {
	telemetry.IncrCounterWithLabels(
		[]string{metrics.TimestampNonce, metrics.Invalid, metrics.Count},
		1,
		[]gometrics.Label{metrics.GetLabelForIntValue(metrics.ExecMode, int(ctx.ExecMode()))},
	)
	fmt.Printf("Failed to process timestamp nonce %d in %s: %s", sig.Sequence, duration, err.Error())
	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
View file
Open in desktop

This file was deleted.

75 changes: 0 additions & 75 deletions protocol/app/ante/timestampnonce_test.go
View file
Open in desktop

This file was deleted.

4 changes: 4 additions & 0 deletions protocol/lib/metrics/constants.go
View file
Open in desktop
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
32 changes: 32 additions & 0 deletions protocol/x/accountplus/ante/timestampnonce.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
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")
}

if accountpluskeeper.IsTimestampNonce(signatures[0].Sequence) {
jerryfan01234 marked this conversation as resolved.
Show resolved Hide resolved
return true, nil
} else {
return false, nil
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Ensure comprehensive error handling.

The function IsTimestampNonceTx correctly checks the transaction type and retrieves signatures. However, consider logging the errors for better traceability.

-  return false, errorsmod.Wrap(sdkerrors.ErrTxDecode, "invalid transaction type")
+  err := errorsmod.Wrap(sdkerrors.ErrTxDecode, "invalid transaction type")
+  ctx.Logger().Error(err.Error())
+  return false, err

-  return false, errorsmod.Wrap(sdkerrors.ErrTxDecode, "more than one signature")
+  err := errorsmod.Wrap(sdkerrors.ErrTxDecode, "more than one signature")
+  ctx.Logger().Error(err.Error())
+  return false, err
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// 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")
}
if accountpluskeeper.IsTimestampNonce(signatures[0].Sequence) {
return true, nil
} else {
return false, nil
}
}
// 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 {
err := errorsmod.Wrap(sdkerrors.ErrTxDecode, "invalid transaction type")
ctx.Logger().Error(err.Error())
return false, err
}
signatures, err := sigTx.GetSignaturesV2()
if err != nil {
return false, err
}
if len(signatures) != 1 {
err := errorsmod.Wrap(sdkerrors.ErrTxDecode, "more than one signature")
ctx.Logger().Error(err.Error())
return false, err
}
if accountpluskeeper.IsTimestampNonce(signatures[0].Sequence) {
return true, nil
} else {
return false, nil
}
}

Loading
Loading