[CT-1190] Emit FinalizeBlock updates in single batch.
#2260
Conversation
teddyding
changed the title
FinalizeBlock updates in single batch. FinalizeBlock updates in single batch.
WalkthroughThe pull request introduces significant restructuring within the Changes
Possibly related PRs
Suggested labels
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 (
|
| @@ -490,19 +490,11 @@ func (sm *FullNodeStreamingManagerImpl) TracksSubaccountId(subaccountId satypes. | |||
| return exists | |||
| } | |||
|
|
|||
| // SendOrderbookUpdates groups updates by their clob pair ids and | |||
| // sends messages to the subscribers. | |||
| func (sm *FullNodeStreamingManagerImpl) SendOrderbookUpdates( | |||
There was a problem hiding this comment.
For the 3 functions SendOrderbookUpdates, SendOrderbookFillUpdates, SendFinalizedSubaccountUpdates, we are just taking out a few lines into a helper function. No logic change
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post
Actionable comments posted: 1
Outside diff range and nitpick comments (2)
protocol/x/clob/keeper/process_operations.go (2)
545-547: LGTM! The change simplifies the code and improves efficiency.Replacing the previous method of sending order book fill updates with a direct call to
StageFinalizeBlockFillstreamlines the process and reduces complexity. This is a positive change that improves code readability and maintainability.To ensure the robustness of this change, consider adding unit tests that verify:
- The behavior of
StageFinalizeBlockFillin staging the order book fill for streaming.- The end-to-end flow of streaming order book fills to confirm that it still works as expected after this modification.
652-654: LGTM! The change simplifies the code and improves efficiency, similar toPersistMatchOrdersToState.Replacing the previous logic of sending order book fill updates with a direct call to
StageFinalizeBlockFillstreamlines the process and reduces complexity for liquidation fills as well. This is a positive change that improves code readability and maintainability.To ensure the robustness of this change, consider adding unit tests that verify:
- The behavior of
StageFinalizeBlockFillin staging the liquidation fill for streaming.- The end-to-end flow of streaming liquidation fills to confirm that it still works as expected after this modification.
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 (16)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/bundle.ts (2 hunks)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/streaming.ts (1 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 (12 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
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 (34)
protocol/streaming/constants.go (1)
1-13: LGTM!The constants are well-defined and their purposes are clearly documented. The naming conventions are consistent and the constants serve a clear purpose in the
FullNodeStreamingManager.proto/dydxprotocol/clob/streaming.proto (3)
1-2: LGTM!The syntax and package declarations are correct.
7-7: LGTM!The Go package option is correct.
9-16: LGTM!The
StagedFinalizeBlockEventmessage type is well-defined and follows the protocol buffer language syntax. Theoneoffield allows for flexibility in the types of events that can be processed during theFinalizeBlockprocess.protocol/x/clob/keeper/grpc_stream_finalize_block.go (2)
15-29: LGTM!The function
getUpdatesToSyncLocalOpsQueueis well-structured and effectively retrieves the necessary order book updates to synchronize the local operations queue with the latest on-chain state after block finalization. It plays a crucial role in ensuring that the order fill amounts are accurately reflected in the local state.The logic is clear and follows these steps:
- Retrieve the local validator's operations queue.
- Fetch the order IDs involved in the operations queue.
- For each order ID, retrieve the order book updates.
- Append all the updates to a single
OffchainUpdatesobject.The function effectively reverts the optimistic fill amounts that were adjusted during the transaction state transitions, ensuring consistency between the local and on-chain state.
34-50: LGTM!The function
StreamBatchUpdatesAfterFinalizeBlockis well-structured and effectively streams updates after the consensus has finalized a block. It plays a crucial role in ensuring that the local operations queue is synchronized with the latest on-chain state.The logic is clear and follows these steps:
- Measure the time taken for streaming updates using
telemetry.MeasureSince.- Call
getUpdatesToSyncLocalOpsQueueto retrieve the necessary order book updates.- Pass the retrieved updates and the
PerpetualIdToClobPairIdmapping to theStreamBatchUpdatesAfterFinalizeBlockmethod of the full node's streaming manager.The use of telemetry helps in monitoring the performance of this operation, which is valuable for identifying potential bottlenecks or issues.
Overall, the function is well-designed and contributes to maintaining consistency between the local and on-chain state after block finalization.
protocol/streaming/types/interface.go (4)
53-56: LGTM!The
StageFinalizeBlockFillmethod is a valuable addition to theFullNodeStreamingManagerinterface. It allows for the staging of fill events during the block finalization process, which is crucial for managing the flow of events in a streaming architecture.The method signature and parameters are well-defined and appropriate for the intended functionality.
57-60: LGTM!The
StageFinalizeBlockSubaccountUpdatemethod is a valuable addition to theFullNodeStreamingManagerinterface. It allows for the staging of subaccount update events during the block finalization process, which is crucial for handling changes to subaccount states.The method signature and parameters are well-defined and appropriate for the intended functionality.
61-63: LGTM!The
GetStagedFinalizeBlockEventsmethod is a valuable addition to theFullNodeStreamingManagerinterface. It complements the staging methods by providing a way to retrieve the staged events for block finalization.The method signature and parameters are well-defined and appropriate for the intended functionality.
65-69: LGTM!The
StreamBatchUpdatesAfterFinalizeBlockmethod is a valuable addition to theFullNodeStreamingManagerinterface. It enhances the synchronization of updates related to order books and their associated operations after block finalization.The method signature and parameters are well-defined and appropriate for the intended functionality. The
orderBookUpdatesToSyncLocalOpsQueueparameter represents the off-chain updates that need to be synchronized, while theperpetualIdToClobPairIdparameter provides the necessary mapping for associating updates with specific order books.This method is crucial for maintaining consistency between the off-chain updates and the local operations queue in a streaming architecture.
indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/streaming.ts (4)
1-4: LGTM!The imports are correctly defined and follow the appropriate naming conventions.
7-16: LGTM!The
StagedFinalizeBlockEventinterface and its SDK type are correctly defined with the appropriate properties and types.
18-23: LGTM!The
createBaseStagedFinalizeBlockEventfunction correctly returns aStagedFinalizeBlockEventobject withundefinedproperties, which serves as a base for creating new instances of the event.
25-71: LGTM!The
StagedFinalizeBlockEventobject is correctly defined with the following methods:
encode: Serializes the event data into a binary format using Protocol Buffers.decode: Reconstructs the event from a binary input by iterating over the tags and decoding the corresponding properties.fromPartial: Creates aStagedFinalizeBlockEventinstance from a partial object by calling thefromPartialmethods of the respective properties.The implementation follows the expected patterns for encoding, decoding, and creating instances from partial data.
protocol/streaming/noop_streaming_manager.go (4)
82-86: Skipped reviewing the empty method.The
StageFinalizeBlockFillmethod is part of theNoopGrpcStreamingManagerstruct, which is a no-op implementation of the streaming manager. The method body is empty, indicating that the functionality is not yet implemented.
88-92: Skipped reviewing the no-op method.The
GetStagedFinalizeBlockEventsmethod is part of theNoopGrpcStreamingManagerstruct, which is a no-op implementation of the streaming manager. The method always returnsnil, indicating that no events are staged.
94-98: Skipped reviewing the empty method.The
StageFinalizeBlockSubaccountUpdatemethod is part of theNoopGrpcStreamingManagerstruct, which is a no-op implementation of the streaming manager. The method body is empty, indicating that the functionality is not yet implemented.
100-105: Skipped reviewing the empty method.The
StreamBatchUpdatesAfterFinalizeBlockmethod is part of theNoopGrpcStreamingManagerstruct, which is a no-op implementation of the streaming manager. The method body is empty, indicating that the functionality is not yet implemented.protocol/lib/metrics/metric_keys.go (1)
86-88: LGTM!The addition of new gRPC metrics to track the counts of staged updates for finalized blocks aligns with the PR objective and enhances the monitoring capabilities. The metric names follow the existing naming conventions and provide more granular insights into the performance and behavior of block finalization processes.
protocol/x/clob/abci.go (1)
49-58: LGTM!The
Precommitfunction is a well-structured addition that enhances the clob module's functionality by conditionally managing streaming updates based on the state of the streaming manager. The logic is clear, and the function follows best practices by avoiding unnecessary processing when the streaming manager is disabled.The function is unlikely to introduce any breaking changes or compatibility issues, as it is a new addition and does not modify existing behavior.
indexer/packages/v4-protos/src/codegen/dydxprotocol/bundle.ts (2)
39-194: Significant restructuring of imports.The changes to the imports reflect a substantial reorganization of the codebase, with the introduction of several new modules across various domains such as
clob,daemons,delaymsg,epochs,govplus,indexer,listing,perpetuals,prices,ratelimit,revshare,rewards,sending,stats,subaccounts,vault, andvest. Additionally, the order of some existing imports has been modified.These changes suggest an effort to introduce new functionalities and potentially optimize the structure of the code. The broad scope of the modifications indicates a significant impact on the overall protocol.
248-405: Comprehensive expansion of thedydxprotocolnamespace.The
dydxprotocolnamespace has undergone a significant expansion with the addition of several new exported modules such asdaemons,delaymsg,epochs,govplus,indexer,listing,perpetuals,prices,ratelimit,revshare,rewards,sending,stats,subaccounts,vault, andvest. These new modules cover a wide range of aspects within the protocol, indicating a comprehensive upgrade of the system's capabilities.Furthermore, the existing exported modules within the
dydxprotocolnamespace have been updated to include additional components from the newly imported modules, enhancing their functionality.The introduction of the
ClientFactoryexport suggests potential changes in the management of client interactions within the protocol.Overall, the modifications to the exports align with the changes observed in the imports, reflecting a coordinated effort to restructure and enhance the codebase.
protocol/lib/metrics/constants.go (2)
205-205: LGTM!The new constant
StreamBatchUpdatesAfterFinalizeBlockis declared correctly and follows the naming convention used in the file. The constant's name suggests that it is related to a new feature or functionality for streaming batch updates after a block has been finalized.
205-205: Verify the usage of the new constant in the codebase.The new constant
StreamBatchUpdatesAfterFinalizeBlockis public (exported), which indicates that it is intended to be used by other packages. Please ensure that the constant is being used appropriately in the codebase.Run the following script to verify the usage of the new constant:
Verification successful
Constant usage verified and found to be appropriate
The new constant
StreamBatchUpdatesAfterFinalizeBlockis being used correctly and consistently throughout the codebase. It's implemented in streaming managers, used in the CLOB module, defined in the streaming interface, and even used for performance metrics. This indicates that it's a core part of the streaming functionality in the protocol and is being utilized as intended.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the new constant `StreamBatchUpdatesAfterFinalizeBlock` in the codebase. # Test: Search for the constant usage. Expect: At least one occurrence of the constant being used. rg --type go -C 5 $'StreamBatchUpdatesAfterFinalizeBlock'Length of output: 6104
protocol/x/subaccounts/keeper/subaccount.go (1)
448-450: Simplify sending finalized subaccount updates. LGTM!The change simplifies the code by directly passing the
subaccountUpdateobject to theStageFinalizeBlockSubaccountUpdatemethod, instead of wrapping it in a slice. This eliminates the overhead of creating a slice for a single item.The core functionality remains unaltered and the conditional checks are preserved. Overall, this is a clean refactor that potentially improves efficiency without introducing issues.
protocol/x/clob/keeper/process_operations.go (1)
Line range hint
533-544: Verify the impact of removing the gRPC streaming logic.The code block that handled sending absolute fill amounts to a gRPC stream when gRPC streaming was enabled has been removed. This includes the logic to merge order IDs from both local and proposed operation queues and send order book updates based on these merged IDs.
Please ensure that:
- The removal of this code block doesn't break any existing functionality.
- The order book updates are still being handled correctly after this change.
To verify the impact, run the following script:
If Test 1 returns any results, it indicates that there are still references to the removed code block which need to be cleaned up.
If Test 2 doesn't return any results, it suggests that the order book updates might not be sent anymore after this change. In that case, further investigation would be required to ensure the updates are being handled correctly elsewhere.
Verification successful
Verification successful: gRPC streaming removal and order book updates
The removal of the gRPC streaming logic appears to have been implemented correctly:
- No references to the removed code block were found, indicating a clean removal.
- Order book updates are still being sent using
k.SendOrderbookFillUpdatesin theprocess_operations.gofile.These findings suggest that the changes have been made without breaking existing functionality, and order book updates are still being handled appropriately.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: # - Verify that the gRPC streaming functionality is handled correctly elsewhere. # - Check if there are any references to the removed code block. # Test 1: Search for references to the removed code block echo "Searching for references to the removed gRPC streaming code block..." rg --type go -A 5 $'mergedOrderIds := lib.MergeMaps\(localOrderIds, proposedOrderIds\)' # Test 2: Verify that the order book updates are still being sent echo "Verifying if order book updates are still being sent..." rg --type go -A 5 $'k\.SendOrderbookFillUpdates'Length of output: 787
protocol/streaming/full_node_streaming_manager.go (7)
58-60: LGTM!The
streamingManagerTransientStoreKeyfield is a valid addition to theFullNodeStreamingManagerImplstruct. It is properly initialized in the constructor and will be useful for managing staged events in the full node streaming process.Also applies to: 97-97, 115-116
381-387: LGTM!The
getStagedEventsCountfunction is a well-structured helper that encapsulates the logic of retrieving the staged events count from the transient store. It handles the case when the count bytes are not found and follows good practices.
389-423: LGTM!The
StageFinalizeBlockSubaccountUpdateandStageFinalizeBlockFillmethods provide a clean way to stage specific events during theFinalizeBlockprocess. By storing the events in the transient store, they ensure correct processing even with multipleFinalizeBlockcalls. The methods are well-structured and utilize thestageFinalizeBlockEventhelper effectively.
425-445: LGTM!The
getStagedFinalizeBlockEventsfunction andGetStagedFinalizeBlockEventsmethod provide an efficient and clean way to retrieve all stagedFinalizeBlockevents from the transient store. The function encapsulates the retrieval logic well, first getting the count and then iterating over the stored event bytes. The method exposes this functionality using a gas-free context. Both are well-structured and follow good practices.
447-461: LGTM!The
stageFinalizeBlockEventfunction encapsulates the common logic of storing a stagedFinalizeBlockevent in the transient store. It efficiently manages the event count and stores the event bytes using a gas-free context. The function is well-structured and follows good practices.
Line range hint
493-530: LGTM!The refactored
getStreamUpdatesFromOffchainUpdatesfunction and the newgetStreamUpdatesForOrderbookFillsandgetStreamUpdatesForSubaccountUpdatesfunctions improve the modularity and readability of the code.
- The refactored
getStreamUpdatesFromOffchainUpdatesreturns both stream updates and clob pair IDs, making it easier to associate updates with their identifiers.- The
getStreamUpdatesForOrderbookFillsfunction encapsulates the logic of getting stream updates for orderbook fills, handling deleveraging fills as well.- Similarly, the
getStreamUpdatesForSubaccountUpdatesfunction encapsulates the logic for subaccount updates.These changes enhance the structure and organization of the code.
Also applies to: 551-586, 640-663
843-924: LGTM!The
StreamBatchUpdatesAfterFinalizeBlockmethod and its related functions significantly improve the streaming process after a block is finalized.
- The method consolidates the streaming of updates, ensuring that all relevant updates, including staged events and orderbook updates, are sent in a batch. This enhances the efficiency of the update handling process.
- The
addBatchUpdatesToCachefunction encapsulates the logic of adding batch updates to the cache, improving modularity.- The
getStagedEventsFromFinalizeBlockfunction provides a clear separation of retrieving staged events after consensus, enhancing readability.These changes streamline the post-finalization streaming process and improve the overall structure of the code.
Also applies to: 926-961
protocol/app/app.go (1)
Line range hint
2064-2078: Streaming manager now utilizes a transient store.The changes look good. The streaming manager's constructor now accepts a
streamingManagerTransientStoreKeyargument, allowing it to interact with a dedicated transient store. This enhances the streaming manager's capabilities.Verify that all calls to
getFullNodeStreamingManagerFromOptionshave been updated to pass the newstreamingManagerTransientStoreKeyargument. Run this script to check:Verification successful
Function call to
getFullNodeStreamingManagerFromOptionscorrectly updatedThe verification process confirms that the call to
getFullNodeStreamingManagerFromOptionshas been properly updated to include the newstreamingManagerTransientStoreKeyargument. The function is called withtkeys[streaming.StreamingManagerTransientStoreKey]as the last argument, which correctly corresponds to the new parameter in the function signature.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all calls to `getFullNodeStreamingManagerFromOptions` pass the new argument. # Test: Search for the function calls. Expect: All calls passing the new argument. rg --type go $'getFullNodeStreamingManagerFromOptions' -A 5Length of output: 976
Comments failed to post (1)
proto/dydxprotocol/clob/streaming.proto (1)
4-4: Fix the missing import file.
The static analysis tool reports that the file
dydxprotocol/subaccounts/streaming.protodoes not exist. Please ensure that the file exists and the import path is correct.Do you want me to help you find the correct import path or open a GitHub issue to track this task?
Tools
buf
4-4: import "dydxprotocol/subaccounts/streaming.proto": file does not exist
(COMPILE)
| sm.Lock() | ||
| defer sm.Unlock() |
There was a problem hiding this comment.
for my understanding - why do we need to lock before getStreamUpdatesForSubaccountUpdates?
There was a problem hiding this comment.
We don't; just wanted to lock before the 1st flush. I re-ordered the code blocks a bit to optimize
teddyding
force-pushed
the
td/batch-update
branch
from
e996b1a
to
dfa1e47
Compare
teddyding
force-pushed
the
td/batch-update
branch
from
305303e
to
d9c2ef5
Compare
| @@ -796,6 +840,42 @@ func (sm *FullNodeStreamingManagerImpl) GetSubaccountSnapshotsForInitStreams( | |||
| return ret | |||
| } | |||
|
|
|||
| func (sm *FullNodeStreamingManagerImpl) addBatchUpdatesToCache( | |||
There was a problem hiding this comment.
Docstring comment for when this is used / what exactly the batching does?
There was a problem hiding this comment.
might be good to add a comments saying this should never be called without a lock since we're modifying streamUpdateCache
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
protocol/streaming/full_node_streaming_manager.go (1)
843-882: Looks good, but add a comment about the lock requirement.The function correctly adds the batched updates to the cache and maintains the subscription mappings. The logic is clear and well-structured.
Add a comment at the beginning of the function to make it clear that the caller must acquire the lock before calling this function:
// addBatchUpdatesToCacheWithLock adds batched updates to the cache. // Note: The caller must acquire the lock before calling this function. func (sm *FullNodeStreamingManagerImpl) addBatchUpdatesToCacheWithLock(...) { ... }
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- protocol/streaming/full_node_streaming_manager.go (8 hunks)
Additional comments not posted (6)
protocol/streaming/full_node_streaming_manager.go (6)
Line range hint
493-530: LGTM!The function correctly groups offchain updates by clob pair id and converts them to stream updates. The logic is clear and the implementation looks good.
533-549: Looks good!The refactoring to use
getStreamUpdatesFromOffchainUpdatessimplifies the function and improves code reuse. The logic flow is straightforward.
Line range hint
551-586: Nicely done!The function correctly handles both regular and deleveraging fills, and groups them by clob pair id to create stream updates. The logic is well-structured and easy to follow.
590-607: Looks good to me!The refactoring to use
getStreamUpdatesForOrderbookFillssimplifies the function and improves code modularity. The logic flow is clear and concise.
640-663: Looks good!The function correctly groups subaccount updates and converts them to stream updates. The implementation is straightforward and easy to understand.
Line range hint
883-930: Great work!The refactoring to use the new helper functions improves code modularity and readability. The logic flow is clear and well-structured, with the lock being acquired and released appropriately.
Batching the updates and flushing them before and after adding the staged updates ensures that the onchain updates are emitted in a single batch, which is great for performance and consistency.
This reverts commit 72873fb.
This reverts commit 72873fb.
Changelist
Stream FinalizeBlock updates in a batch. Add to cache and flush before and after.
Test Plan
TODO: test in validation mode
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
Summary by CodeRabbit
New Features
StagedFinalizeBlockEventfor managing events during block finalization.Bug Fixes
Refactor
dydxprotocolnamespace for improved clarity and maintainability.FullNodeStreamingManagerto better manage event staging and streaming.Documentation