From e7f71c3f204dd18acedc94475aa1cda9714e3ceb Mon Sep 17 00:00:00 2001 From: Lukasz Cwik <126621805+lcwik@users.noreply.github.com> Date: Thu, 5 Oct 2023 09:52:46 -0700 Subject: [PATCH] [CLOB-914] Ensure that the rate limiting is only occurring during `CheckTx` and `ReCheckTx`. (#488) * [CLOB-914] Ensure that the rate limiting is only occurring during `CheckTx` and `ReCheckTx`. --- protocol/x/clob/keeper/rate_limit.go | 12 ++++++ protocol/x/clob/keeper/rate_limit_test.go | 43 +++++++++++++++++++ .../x/clob/rate_limit/order_rate_limiter.go | 16 +++---- 3 files changed, 61 insertions(+), 10 deletions(-) create mode 100644 protocol/x/clob/keeper/rate_limit_test.go diff --git a/protocol/x/clob/keeper/rate_limit.go b/protocol/x/clob/keeper/rate_limit.go index 42f041ffa5..6079b82987 100644 --- a/protocol/x/clob/keeper/rate_limit.go +++ b/protocol/x/clob/keeper/rate_limit.go @@ -7,7 +7,13 @@ import ( ) // RateLimitCancelOrder passes order cancellations with valid clob pairs to `cancelOrderRateLimiter`. +// The rate limiting is only performed during `CheckTx` and `ReCheckTx`. func (k *Keeper) RateLimitCancelOrder(ctx sdk.Context, msg *types.MsgCancelOrder) error { + // Only rate limit during `CheckTx` and `ReCheckTx`. + if lib.IsDeliverTxMode(ctx) { + return nil + } + _, found := k.GetClobPair(ctx, types.ClobPairId(msg.OrderId.GetClobPairId())) // If the clob pair isn't found then we expect order cancellation validation to fail the order cancellation as // being invalid. @@ -30,7 +36,13 @@ func (k *Keeper) RateLimitCancelOrder(ctx sdk.Context, msg *types.MsgCancelOrder } // RateLimitPlaceOrder passes orders with valid clob pairs to `placeOrderRateLimiter`. +// The rate limiting is only performed during `CheckTx` and `ReCheckTx`. func (k *Keeper) RateLimitPlaceOrder(ctx sdk.Context, msg *types.MsgPlaceOrder) error { + // Only rate limit during `CheckTx` and `ReCheckTx`. + if lib.IsDeliverTxMode(ctx) { + return nil + } + _, found := k.GetClobPair(ctx, msg.Order.GetClobPairId()) // If the clob pair isn't found then we expect order validation to fail the order as being invalid. if !found { diff --git a/protocol/x/clob/keeper/rate_limit_test.go b/protocol/x/clob/keeper/rate_limit_test.go new file mode 100644 index 0000000000..a61ad6e590 --- /dev/null +++ b/protocol/x/clob/keeper/rate_limit_test.go @@ -0,0 +1,43 @@ +package keeper_test + +import ( + testApp "github.com/dydxprotocol/v4-chain/protocol/testutil/app" + "github.com/dydxprotocol/v4-chain/protocol/testutil/constants" + clobtypes "github.com/dydxprotocol/v4-chain/protocol/x/clob/types" + "github.com/stretchr/testify/require" + "testing" +) + +func TestRateLimitPlaceOrderIsNoopOutsideOfCheckTxAndReCheckTx(t *testing.T) { + tApp := testApp.NewTestAppBuilder().WithTesting(t).Build() + checkTxCtx := tApp.AdvanceToBlock(21, testApp.AdvanceToBlockOptions{}) + deliverTxCtx := checkTxCtx.WithIsCheckTx(false).WithIsReCheckTx(false) + msg := clobtypes.NewMsgPlaceOrder(constants.Order_Alice_Num0_Id0_Clob0_Buy5_Price5_GTB20) + + // We expect an error and that the GTB is out of bounds. + require.Error( + t, + tApp.App.ClobKeeper.RateLimitPlaceOrder(checkTxCtx, msg), + "GoodTilBlock 20 is less than the current blockHeight 22", + ) + + // We don't expect any checks from occurring. + require.Nil(t, tApp.App.ClobKeeper.RateLimitPlaceOrder(deliverTxCtx, msg)) +} + +func TestRateLimitCancelOrderIsNoopOutsideOfCheckTxAndReCheckTx(t *testing.T) { + tApp := testApp.NewTestAppBuilder().WithTesting(t).Build() + checkTxCtx := tApp.AdvanceToBlock(21, testApp.AdvanceToBlockOptions{}) + deliverTxCtx := checkTxCtx.WithIsCheckTx(false).WithIsReCheckTx(false) + msg := clobtypes.NewMsgCancelOrderShortTerm(constants.Order_Alice_Num0_Id0_Clob0_Buy5_Price5_GTB20.OrderId, 20) + + // We expect an error and that the GTB is out of bounds. + require.Error( + t, + tApp.App.ClobKeeper.RateLimitCancelOrder(checkTxCtx, msg), + "GoodTilBlock 20 is less than the current blockHeight 22", + ) + + // We don't expect any checks from occurring. + require.Nil(t, tApp.App.ClobKeeper.RateLimitCancelOrder(deliverTxCtx, msg)) +} diff --git a/protocol/x/clob/rate_limit/order_rate_limiter.go b/protocol/x/clob/rate_limit/order_rate_limiter.go index e4c073d9af..bfd3578b1b 100644 --- a/protocol/x/clob/rate_limit/order_rate_limiter.go +++ b/protocol/x/clob/rate_limit/order_rate_limiter.go @@ -29,7 +29,8 @@ var _ RateLimiter[*types.MsgPlaceOrder] = (*placeOrderRateLimiter)(nil) // - how many short term orders per subaccount (by using satypes.SubaccountId). // - how many stateful order per subaccount (by using satypes.SubaccountId). // -// The rate limiting keeps track of orders only placed during CheckTx. +// The rate limiting must only be used during `CheckTx` because the rate limiting information is not recovered +// on application restart preventing it from being deterministic during `DeliverTx`. // // Depending upon the provided types.BlockRateLimitConfiguration, the returned RateLimiter may rely on: // - `ctx.BlockHeight()` in RateLimit to track which block the rate limit should apply to. @@ -83,10 +84,7 @@ func NewPlaceOrderRateLimiter(config types.BlockRateLimitConfiguration) RateLimi } func (r *placeOrderRateLimiter) RateLimit(ctx sdk.Context, msg *types.MsgPlaceOrder) (err error) { - // Only perform rate limiting in CheckTx. - if lib.IsDeliverTxMode(ctx) { - return nil - } + lib.AssertCheckTxMode(ctx) if msg.Order.IsShortTermOrder() { err = r.checkStateShortTermOrderRateLimiter.RateLimit( @@ -153,7 +151,8 @@ var _ RateLimiter[*types.MsgCancelOrder] = (*cancelOrderRateLimiter)(nil) // types.BlockRateLimitConfiguration. The rate limiter currently supports limiting based upon: // - how many short term order cancellations per subaccount (by using satypes.SubaccountId). // -// The rate limiting keeps track of order cancellations placed during CheckTx. +// The rate limiting must only be used during `CheckTx` because the rate limiting information is not recovered +// on application restart preventing it from being deterministic during `DeliverTx`. // // Depending upon the provided types.BlockRateLimitConfiguration, the returned RateLimiter may rely on: // - `ctx.BlockHeight()` in RateLimit to track which block the rate limit should apply to. @@ -190,10 +189,7 @@ func NewCancelOrderRateLimiter(config types.BlockRateLimitConfiguration) RateLim } func (r *cancelOrderRateLimiter) RateLimit(ctx sdk.Context, msg *types.MsgCancelOrder) (err error) { - // Only perform rate limiting in CheckTx. - if lib.IsDeliverTxMode(ctx) { - return nil - } + lib.AssertCheckTxMode(ctx) if msg.OrderId.IsShortTermOrder() { err = r.checkStateShortTermRateLimiter.RateLimit(