[ENG-1296] set cap on expired orders processed per block #3243
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
anmolagrawal345
temporarily deployed
to
testnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
mainnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
mainnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
testnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
testnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
mainnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
testnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
mainnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
mainnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
testnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
testnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
mainnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
mainnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
testnet
GitHub Actions
Inactive
Contributor
WalkthroughThis PR implements a per-block cap mechanism for removing expired stateful orders in the CLOB module. It introduces a Changes
Sequence Diagram(s)sequenceDiagram
auctor ->> EndBlocker: Trigger
EndBlocker ->> RemoveExpiredStatefulOrders: Remove expired orders
Note over RemoveExpiredStatefulOrders: Check MaxStatefulOrderRemovalsPerBlock cap
alt Cap enabled and reached
RemoveExpiredStatefulOrders -->> RemoveExpiredStatefulOrders: Break early, return partial batch
RemoveExpiredStatefulOrders ->> EndBlocker: Return expired IDs (up to cap)
else Cap disabled or not reached
RemoveExpiredStatefulOrders ->> RemoveExpiredStatefulOrders: Continue processing all
RemoveExpiredStatefulOrders ->> EndBlocker: Return all expired IDs
end
EndBlocker ->> Telemetry: Record gauge (expired order count)
EndBlocker ->> EndBlocker: Emit event
sequenceDiagram
Client ->> CLI: query clob block-limits-config
CLI ->> QueryClient: BlockLimitsConfiguration(request)
QueryClient ->> gRPC: Call BlockLimitsConfiguration
gRPC ->> ClobKeeper: BlockLimitsConfiguration(ctx, req)
ClobKeeper ->> StateStore: GetBlockLimitsConfig
StateStore -->> ClobKeeper: BlockLimitsConfig
ClobKeeper -->> gRPC: QueryBlockLimitsConfigurationResponse
gRPC -->> QueryClient: Response
QueryClient -->> CLI: Response
CLI -->> Client: Display config
sequenceDiagram
Governance ->> MsgServer: UpdateBlockLimitsConfig(authority, config)
MsgServer ->> Authority: Verify HasAuthority
alt Authority valid
Authority -->> MsgServer: OK
MsgServer ->> ClobKeeper: UpdateBlockLimitsConfig(ctx, config)
ClobKeeper ->> StateStore: setBlockLimitsConfig
StateStore -->> ClobKeeper: Stored
ClobKeeper -->> MsgServer: Success
MsgServer -->> Governance: MsgUpdateBlockLimitsConfigResponse
else Authority invalid
Authority -->> MsgServer: ErrInvalidSigner
MsgServer -->> Governance: Error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes Areas requiring extra attention:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (42)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/protocol-build-and-push.yml(1 hunks)protocol/x/clob/abci.go(1 hunks)protocol/x/clob/flags/flags.go(6 hunks)protocol/x/clob/flags/flags_test.go(5 hunks)protocol/x/clob/keeper/stateful_order_state.go(2 hunks)protocol/x/clob/keeper/stateful_order_state_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: hwray
Repo: dydxprotocol/v4-chain PR: 2597
File: indexer/services/ender/src/scripts/handlers/dydx_update_perpetual_v1_handler.sql:16-20
Timestamp: 2024-11-22T18:12:04.606Z
Learning: Avoid suggesting changes to deprecated functions such as `dydx_update_perpetual_v1_handler` in `indexer/services/ender/src/scripts/handlers/dydx_update_perpetual_v1_handler.sql` if they are unchanged in the PR.
Learnt from: anmolagrawal345
Repo: dydxprotocol/v4-chain PR: 2780
File: protocol/x/clob/keeper/twap_order_state.go:137-138
Timestamp: 2025-04-15T16:57:26.546Z
Learning: TWAP order cancellations and expiries will be handled in a dedicated follow-up PR, separate from the initial implementation of TWAP order functionality in the end blocker.
📚 Learning: 2025-04-15T16:58:46.335Z
Learnt from: anmolagrawal345
Repo: dydxprotocol/v4-chain PR: 2780
File: protocol/x/clob/keeper/twap_order_state_test.go:191-209
Timestamp: 2025-04-15T16:58:46.335Z
Learning: For TWAP orders, message validation in message_place_order.go handles edge cases with comprehensive checks for parameters like durations and intervals, making it unnecessary to test invalid parameters in twap_order_state_test.go which only receives pre-validated inputs.
Applied to files:
protocol/x/clob/keeper/stateful_order_state_test.goprotocol/x/clob/flags/flags_test.go
🧬 Code graph analysis (4)
protocol/x/clob/abci.go (4)
protocol/lib/metrics/lib.go (1)
SetGauge(38-40)protocol/x/clob/types/keys.go (1)
ModuleName(6-6)protocol/lib/metrics/metric_keys.go (1)
EndBlocker(91-91)protocol/lib/metrics/constants.go (1)
Count(9-9)
protocol/x/clob/keeper/stateful_order_state.go (2)
protocol/app/flags/flags.go (1)
Flags(13-36)protocol/x/clob/flags/flags.go (1)
MaxStatefulOrderRemovalsPerBlock(30-30)
protocol/x/clob/keeper/stateful_order_state_test.go (4)
protocol/testutil/constants/stateful_orders.go (5)
ConditionalOrder_Alice_Num0_Id0_Clob0_Buy5_Price10_GTBT15_StopLoss20(457-470)ConditionalOrder_Alice_Num1_Id0_Clob0_Sell5_Price10_GTB15(932-943)LongTermOrder_Alice_Num0_Id0_Clob0_Buy5_Price10_GTBT20(131-142)ConditionalOrder_Alice_Num1_Id1_Clob0_Sell50_Price5_GTB30(944-955)LongTermOrder_Alice_Num1_Id1_Clob0_Sell25_Price30_GTBT10(191-202)protocol/x/clob/memclob/memclob.go (1)
NewMemClobPriceTimePriority(57-66)protocol/testutil/keeper/clob.go (1)
NewClobKeepersTestContext(61-73)protocol/x/clob/flags/flags.go (1)
MaxStatefulOrderRemovalsPerBlock(30-30)
protocol/x/clob/flags/flags_test.go (1)
protocol/x/clob/flags/flags.go (2)
MaxStatefulOrderRemovalsPerBlock(30-30)DefaultMaxStatefulOrderRemovalsPerBlock(44-44)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (32)
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-vulcan / (vulcan) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-ender / (ender) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-bazooka-lambda / (bazooka) Build and Push Lambda
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-vulcan / (vulcan) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-bazooka-lambda / (bazooka) Build and Push Lambda
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-roundtable / (roundtable) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-comlink / (comlink) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-ender / (ender) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-comlink / (comlink) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-roundtable / (roundtable) Build and Push
- GitHub Check: golangci-lint
- GitHub Check: benchmark
- GitHub Check: test-coverage-upload
- GitHub Check: build
- GitHub Check: liveness-test
- GitHub Check: test-race
- GitHub Check: unit-end-to-end-and-integration
- GitHub Check: check-sample-pregenesis-up-to-date
- GitHub Check: container-tests
- GitHub Check: build-and-push-testnet
- GitHub Check: build-and-push-mainnet
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Summary
- GitHub Check: build-and-push-dev2
- GitHub Check: build-and-push-dev4
- GitHub Check: build-and-push-staging
- GitHub Check: build-and-push-dev
anmolagrawal345
temporarily deployed
to
testnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
testnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
testnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
testnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
testnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
testnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
mainnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
mainnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
mainnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
testnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
testnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
testnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
mainnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
mainnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
mainnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
mainnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
mainnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
mainnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
mainnet
GitHub Actions
Inactive
jusbar23
reviewed
Nov 10, 2025
| - main | ||
| - 'release/protocol/v[0-9]+.[0-9]+.x' # e.g. release/protocol/v0.1.x | ||
| - 'release/protocol/v[0-9]+.x' # e.g. release/protocol/v1.x | ||
| - 'anmol/eng-1296' |
protocol/x/clob/types/errors.go
Outdated
| 1022, | ||
| "Liquidation conflicts with ClobPair status", | ||
| ) | ||
| ErrInvalidBlockLimitsConfig = errorsmod.Register( |
Contributor
There was a problem hiding this comment.
Do we use this? Doesn't seem like we have any validation logic
Contributor
Author
There was a problem hiding this comment.
deleted - the only validation logic lives in the server itself which verifies the message came from an authorized address. otherwise, think we can just use the value. dont need to check negative values either since its a uint
| // Validate checks that the BlockLimitsConfig is valid. | ||
| // Note: MaxStatefulOrderRemovalsPerBlock can be 0, which means no cap (process all expired orders). | ||
| func (config *BlockLimitsConfig) Validate() error { | ||
| // No validation needed - 0 is a valid value meaning "no cap" |
Contributor
There was a problem hiding this comment.
Do we want to set an upper bound for his?
Contributor
Author
There was a problem hiding this comment.
dont think so since its governance protected. if for some reason we need to bump it past any threshold, we'll have to do an upgrade to support it
anmolagrawal345
temporarily deployed
to
testnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
testnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
testnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
testnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
testnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
testnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
testnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
mainnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
mainnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
mainnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
mainnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
mainnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
mainnet
GitHub Actions
Inactive
anmolagrawal345
temporarily deployed
to
mainnet
GitHub Actions
Inactive
shrenujb
reviewed
Nov 10, 2025
|
|
||
| // MsgUpdateBlockLimitsConfig is a request type for updating the block limits | ||
| // configuration. | ||
| message MsgUpdateBlockLimitsConfig { |
Contributor
There was a problem hiding this comment.
Would this be a gov msg? Are there other limits we already have which we use gov msgs to update?
Contributor
Author
There was a problem hiding this comment.
Yep this follows the same pattern as MsgUpdateLiquidationConfig which also seems gov protected.
| // The maximum number of expired stateful orders that can be removed from | ||
| // state in a single block. This prevents performance degradation when | ||
| // processing a large number of expired orders. | ||
| uint32 max_stateful_order_removals_per_block = 1; |
Contributor
There was a problem hiding this comment.
Is there a strong reason not to put this under BlockRateLimitConfiguration, which is also stored in state?
Not too concerned about the differences between this field (end blocker) and existing block rate limits (checktx)
Doing that would save us from so many stubs/new messages added in this PR
| // The maximum number of expired stateful orders that can be removed from | ||
| // state in a single block. This prevents performance degradation when | ||
| // processing a large number of expired orders. | ||
| uint32 max_stateful_order_removals_per_block = 1; |
Contributor
There was a problem hiding this comment.
For my context, why did we decide to add this recently? Is this an AI from a security report?
| numRemoved := uint32(0) | ||
|
|
||
| // Process orders up to the cap (if set), otherwise process all | ||
| for ; it.Valid(); it.Next() { |
Contributor
There was a problem hiding this comment.
A bit cleaner to put condition in loop guard
maxRemovals := blockLimitsConfig.MaxStatefulOrderRemovalsPerBlock
numRemoved := uint32(0)
// Process orders up to the cap (if set), otherwise process all
for ; it.Valid() && (maxRemovals == 0 || numRemoved < maxRemovals); it.Next() {
var orderId types.OrderId
k.cdc.MustUnmarshal(it.Value(), &orderId)
expiredOrderIds = append(expiredOrderIds, orderId)
store.Delete(it.Key())
numRemoved++
}
return expiredOrderIds
}
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changelist
This change builds off the same pattern for setting caps on deleveraging and liquidations per block. We need to cap the expired orders removal per block to prevent the situation in which the end blocker is stuck removing a large number of orders (a potential attack vector).
Test Plan
[Describe how this PR was tested (if applicable)]
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.Summary by CodeRabbit
New Features
Improvements