[OTE-456] FNS x OE: stage FinalizeBlock events and emit in Precommit
#2253
Conversation
|
Warning Rate limit exceeded@teddyding has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 26 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes introduce new interfaces and message types to enhance event handling during the Changes
Sequence DiagramsequenceDiagram
participant A as Client
participant B as FullNodeStreamingManager
participant C as Keeper
participant D as OffchainUpdates
A->>C: Request Finalize Block
C->>B: StageFinalizeBlockFill
C->>B: StageFinalizeBlockSubaccountUpdate
B->>D: StreamBatchUpdatesAfterFinalizeBlock
D-->>A: Send Updates
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
|
|
|
||
| finalizedFills, finalizedSubaccountUpdates := sm.getStagedEventsFromFinalizeBlock(ctx) | ||
|
|
||
| // TODO(CT-1190): Stream below in a single batch. |
There was a problem hiding this comment.
Will do this in a follow-up PR
There was a problem hiding this comment.
probably good to just obtain the lock and manually send the updates through in a single batch bypassing the cache
proto/dydxprotocol/clob/query.proto
Outdated
| @@ -198,6 +198,15 @@ message StreamUpdate { | |||
| } | |||
| } | |||
|
|
|||
| // StagedFinalizeBlockEvent is an event staged during `FinalizeBlock`. | |||
There was a problem hiding this comment.
Can we add context to this comment to explain staging more and why it is necessary? + this is not exposed externally, so maybe move it out of query.proto?
There was a problem hiding this comment.
Will add more documentation in code as opposed to proto file
| ctx sdk.Context, | ||
| eventBytes []byte, | ||
| ) { | ||
| noGasCtx := ctx.WithGasMeter(ante_types.NewFreeInfiniteGasMeter()) |
There was a problem hiding this comment.
for my understanding why do we need to add gas meter to ctx? because we write to state?
There was a problem hiding this comment.
Yes any read/write to store incur the gas meter. We do the same thing for indexer
| ) { | ||
| // Flush all pending updates, since we want the onchain updates to arrive in a batch. | ||
| sm.FlushStreamUpdates() | ||
|
|
There was a problem hiding this comment.
Thinking out loud, we don't need to sm.Lock() this right? CheckTx transactions won't run in parallel when we are finalizing the block so the cache couldn't possibly get new updates when we are calling this function? It could be the case that we flush on the timer but it should be fine if cache is empty since we flush it first
EDIT: i see that the three sm.SendOrderbookUpdates() functions already obtain the lock before adding to the cache, i think it's fine
There was a problem hiding this comment.
Yes agreed. In the follow-up PR I will obtain the lock around sending a batch
| localValidatorOperationsQueue, _ := k.MemClob.GetOperationsToReplay(ctx) | ||
| fetchOrdersInvolvedInOpQueue(localValidatorOperationsQueue) | ||
| orderIdsFromLocal := fetchOrdersInvolvedInOpQueue( | ||
| localValidatorOperationsQueue, |
There was a problem hiding this comment.
Think we need to monitor this to make sure the removal of
orderIdsFromProposed := fetchOrdersInvolvedInOpQueue(
operations,
)
will not break correctness
| ) | ||
|
|
||
| // Returns the order updates needed to update the fill amount for the orders | ||
| // from local ops queue, according to the latest onchain state (after FinalizeBlock). |
There was a problem hiding this comment.
| // from local ops queue, according to the latest onchain state (after FinalizeBlock). | |
| // from local ops queue, according to the latest onchain state (after FinalizeBlock). | |
| // Effectively reverts the optimistic fill amounts removed from the CheckTx to DeliverTx state transition. |
teddyding
force-pushed
the
td/fns-oe-refactored
branch
from
5229884 to
ceba058
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
protocol/x/clob/types/streaming.pb.gois excluded by!**/*.pb.go
Files selected for processing (15)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/bundle.ts (2 hunks)
- indexer/packages/v4-protos/src/codegen/gogoproto/bundle.ts (1 hunks)
- indexer/packages/v4-protos/src/codegen/google/bundle.ts (1 hunks)
- proto/dydxprotocol/clob/streaming.proto (1 hunks)
- protocol/app/app.go (4 hunks)
- protocol/lib/metrics/constants.go (1 hunks)
- protocol/lib/metrics/metric_keys.go (1 hunks)
- protocol/streaming/constants.go (1 hunks)
- protocol/streaming/full_node_streaming_manager.go (6 hunks)
- protocol/streaming/noop_streaming_manager.go (1 hunks)
- protocol/streaming/types/interface.go (1 hunks)
- protocol/x/clob/abci.go (1 hunks)
- protocol/x/clob/keeper/grpc_stream_finalize_block.go (1 hunks)
- protocol/x/clob/keeper/process_operations.go (3 hunks)
- protocol/x/subaccounts/keeper/subaccount.go (1 hunks)
Files skipped from review due to trivial changes (2)
- indexer/packages/v4-protos/src/codegen/gogoproto/bundle.ts
- indexer/packages/v4-protos/src/codegen/google/bundle.ts
Files skipped from review as they are similar to previous changes (7)
- protocol/app/app.go
- protocol/lib/metrics/constants.go
- protocol/lib/metrics/metric_keys.go
- protocol/streaming/constants.go
- protocol/streaming/noop_streaming_manager.go
- protocol/x/clob/abci.go
- protocol/x/subaccounts/keeper/subaccount.go
Additional context used
buf
proto/dydxprotocol/clob/streaming.proto
4-4: import "dydxprotocol/subaccounts/streaming.proto": file does not exist
(COMPILE)
Additional comments not posted (23)
proto/dydxprotocol/clob/streaming.proto (1)
10-16: LGTM!The new message type
StagedFinalizeBlockEventis well-defined and serves a clear purpose of representing events staged during theFinalizeBlockprocess. The use ofoneoffor the event field allows for flexibility in the type of event that can be contained within the message.protocol/x/clob/keeper/grpc_stream_finalize_block.go (3)
15-29: LGTM!The function logic for retrieving updates to sync the local operations queue is correct.
As mentioned in the previous review, please monitor the removal of the following code segment to ensure it does not break correctness:
orderIdsFromProposed := fetchOrdersInvolvedInOpQueue( operations, )
37-42: As mentioned in the previous review, please avoid using thetelemetrymodule.
34-50: LGTM!The function logic for streaming batch updates after a block is finalized is correct. The documentation has been improved and accurately describes the purpose of the function.
protocol/streaming/types/interface.go (4)
53-56: LGTM!The
StageFinalizeBlockFillmethod is a valid addition to theFullNodeStreamingManagerinterface. It follows the naming conventions and has appropriate parameters for staging a fill event related to a finalized block.
57-60: LGTM!The
StageFinalizeBlockSubaccountUpdatemethod is a valid addition to theFullNodeStreamingManagerinterface. It follows the naming conventions and has appropriate parameters for staging a subaccount update event related to a finalized block.
61-63: LGTM!The
GetStagedFinalizeBlockEventsmethod is a valid addition to theFullNodeStreamingManagerinterface. It follows the naming conventions and has an appropriate return type for retrieving a list of staged events related to finalized blocks.
65-69: LGTM!The
StreamBatchUpdatesAfterFinalizeBlockmethod is a valid addition to theFullNodeStreamingManagerinterface. It follows the naming conventions and has appropriate parameters for streaming batch updates after a block is finalized, synchronizing updates with the local operations queue, and utilizing a mapping of perpetual IDs to CLOB pair IDs.indexer/packages/v4-protos/src/codegen/dydxprotocol/bundle.ts (3)
39-40: LGTM!The addition of new imports for
clob/streaming,clob/tx, and various submodules underdaemons/is properly done. The imports follow the existing naming conventions, are placed in the correct order, and are grouped with related imports.Also applies to: 41-47
44-123: LGTM!The renaming of existing imports by incrementing their suffixed numbers is consistently applied across all affected imports. The renaming is necessary to maintain uniqueness due to the addition of new imports. It does not introduce any naming conflicts or inconsistencies.
Line range hint
191-404: LGTM!The updates to the exports within the
dydxprotocolnamespace to include the newly added and renamed imports are consistently applied across all affected exports. The updates are necessary to ensure that the newly added and renamed imports are properly exported and accessible. The existing structure and organization of the exports are maintained.protocol/streaming/full_node_streaming_manager.go (10)
381-387: LGTM!The function correctly retrieves the count of staged events from the transient store and handles the case when the count doesn't exist.
390-403: LGTM!The function correctly stages the subaccount update event in the transient store during
FinalizeBlock.
410-423: LGTM!The function correctly stages the fill event in the transient store during
FinalizeBlock.
425-436: LGTM!The function correctly retrieves all staged events from the transient store by iterating over the count and unmarshaling each event.
439-445: LGTM!The function correctly retrieves all staged events from the transient store using a new context with an infinite gas meter to avoid charging gas.
447-461: LGTM!The function correctly stages an event in the transient store by incrementing the count, storing the count, and storing the event bytes in a prefix store using the count as the key.
809-809: LGTM!Flushing all pending updates before streaming the batch updates ensures that the onchain updates arrive in a batch.
838-871: LGTM!The function correctly retrieves staged events from
FinalizeBlock, categorizes them into finalized fills and subaccount updates, and sets appropriate metrics.
Line range hint
873-933: LGTM!The function correctly initializes new streams with orderbook and subaccount snapshots, handling the case when the subaccount snapshot may not exist due to a race condition. It also handles the snapshot block interval logic correctly.
Line range hint
97-136: LGTM!The constructor correctly initializes the
FullNodeStreamingManagerImplinstance with the provided parameters, including the newstreamingManagerTransientStoreKeyparameter. The usage of the transient store key is consistent with the newly added functions.protocol/x/clob/keeper/process_operations.go (2)
Line range hint
533-547: LGTM!The changes simplify the logic related to gRPC streaming and order book updates by staging the updates using the streaming manager's
StageFinalizeBlockFillmethod. This approach potentially improves performance and maintainability by streamlining the management of updates without relying on theMemClobstate.
652-656: LGTM!The changes simplify the logic related to gRPC streaming and order book updates for liquidation matches by staging the updates using the streaming manager's
StageFinalizeBlockFillmethod. This approach is consistent with the changes made inPersistMatchOrdersToStateand potentially improves performance and maintainability by streamlining the management of updates without relying on theMemClobstate.
| syntax = "proto3"; | ||
| package dydxprotocol.clob; | ||
|
|
||
| import "dydxprotocol/subaccounts/streaming.proto"; |
There was a problem hiding this comment.
Fix the missing import file.
The import statement import "dydxprotocol/subaccounts/streaming.proto"; references a file that does not exist, according to the static analysis hints. This could lead to compilation errors.
Please ensure that the file dydxprotocol/subaccounts/streaming.proto exists in the specified location. If you need assistance resolving this issue, I'd be happy to help. Let me know if you'd like me to open a GitHub issue to track this task.
Tools
buf
4-4: import "dydxprotocol/subaccounts/streaming.proto": file does not exist
(COMPILE)
Changelist
Store order fill stream events in a transient store during block processing
Do the following in Precommit, after consensus agrees on a block.
Test Plan
(TODO) FNS integration testing on dev
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
Here are the updated release notes based on the provided changes:
New Features
Improvements
Chores