[OTE-176] Reduce the cost of adding an indexer event. #1078
Conversation
Currently we unmarshalled and marshalled all of them which grew linearly with the number of messages so as blocks got larger this became a noticeable cost.
WalkthroughThe updates mainly focus on improving data serialization and storage within the indexer components of a blockchain protocol. Key changes include transitioning from custom or older serialization methods to Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files selected for processing (14)
- protocol/indexer/indexer_manager/events.go (4 hunks)
- protocol/indexer/indexer_manager/on_chain_updates.go (2 hunks)
- protocol/indexer/off_chain_updates/off_chain_updates.go (4 hunks)
- protocol/indexer/off_chain_updates/off_chain_updates_test.go (3 hunks)
- protocol/testing/e2e/trading_rewards/trading_rewards_test.go (3 hunks)
- protocol/testutil/keeper/assets.go (2 hunks)
- protocol/testutil/keeper/keeper.go (1 hunks)
- protocol/testutil/keeper/perpetuals.go (2 hunks)
- protocol/testutil/keeper/prices.go (3 hunks)
- protocol/testutil/keeper/rewards.go (3 hunks)
- protocol/testutil/keeper/subaccounts.go (2 hunks)
- protocol/x/clob/keeper/orders_test.go (3 hunks)
- protocol/x/perpetuals/keeper/perpetual_test.go (4 hunks)
- protocol/x/sending/keeper/transfer_test.go (4 hunks)
Additional comments: 27
protocol/indexer/indexer_manager/on_chain_updates.go (1)
- 21-21: The switch to
proto.Marshalfor marshalingIndexerTendermintBlockaligns with the PR's goal of optimizing serialization. Ensure that corresponding unmarshaling operations are also updated to useproto.Unmarshalfor consistency.protocol/testutil/keeper/keeper.go (1)
- 32-32: Updating the variable name to
indexer_manager.TransientStoreKeyimproves naming clarity and consistency. Ensure that this key is correctly defined and consistently used across the system.protocol/testutil/keeper/assets.go (1)
- 113-113: Replacing
common.UnmarshalerImpl{}withproto.Unmarshal()for unmarshalingAssetCreateEventV1aligns with the PR's optimization goals. Ensure thatAssetCreateEventV1is fully compatible withproto.Unmarshal.protocol/testutil/keeper/rewards.go (1)
- 136-136: Switching to
proto.Unmarshal()for unmarshalingTradingRewardsEventV1is in line with the PR's optimization efforts. Ensure thatTradingRewardsEventV1is fully compatible withproto.Unmarshal.protocol/testutil/keeper/subaccounts.go (1)
- 155-155: Replacing
common.UnmarshalerImpl{}withproto.Unmarshal()for unmarshalingSubaccountUpdateEventV1aligns with the PR's optimization goals. Ensure thatSubaccountUpdateEventV1is fully compatible withproto.Unmarshal.protocol/indexer/indexer_manager/events.go (4)
- 19-23: The introduction of
IndexerEventsCountKeyandIndexerEventsPrefixfor managing indexer events storage is a good optimization. Ensure these keys are consistently used across the system.- 28-49: Refactoring
getIndexerEventsCountandgetIndexerEventsto handle count and individual events retrieval more efficiently is a positive change. Ensure thorough testing to confirm correct functionality.- 112-120: The modifications in
addEventto handle count increment and individual event storage usingproto.Marshalalign with the PR's optimization goals. Ensure compatibility and correctness of serialization.- 130-130: Updating
clearEventsto delete the count key instead of the events key simplifies the event clearing process. Verify that this change does not impact the retrieval of events in any negative way.protocol/testutil/keeper/perpetuals.go (2)
- 6-6: The addition of
github.com/cosmos/gogoproto/protois appropriate for the context of optimizing serialization and deserialization processes. This aligns well with the PR's objectives.- 199-199: Switching to
proto.Unmarshalfor unmarshallingliquidityTierEventis a positive change, enhancing efficiency and maintainability by utilizing the standard library over custom implementations.protocol/testutil/keeper/prices.go (2)
- 5-5: The addition of
github.com/cosmos/gogoproto/protosupports the optimization of data handling processes, specifically for unmarshallingMarketEventV1.- 186-186: Utilizing
proto.Unmarshalfor unmarshallingMarketEventV1is a beneficial change, promoting efficiency and code maintainability by leveraging standardized library functions.protocol/indexer/off_chain_updates/off_chain_updates.go (3)
- 255-255: The use of
proto.MarshalinnewOrderPlaceMessageis a positive change for standardizing serialization. Ensure that all instances where this serialized data is consumed (e.g., deserialized) are compatible with theprotoformat to maintain interoperability.- 276-276: The introduction of
proto.MarshalinnewOrderRemoveMessagealigns with the goal of optimizing serialization. Verify that the serialized data structure matches the expected schema in all contexts where it's used, to prevent any data integrity issues.- 295-295: Using
proto.MarshalinnewOrderUpdateMessagestandardizes the serialization process. It's important to ensure that the serialized format is consistently handled across the system, especially in parts of the codebase that interact with these messages.protocol/x/sending/keeper/transfer_test.go (3)
- 55-55: Replacing custom unmarshalling with
proto.UnmarshalinassertTransferEventInIndexerBlockis a good practice for consistency. Ensure that theTransferEventV1struct is fully compatible with the serialized data to avoid any potential issues during tests.- 80-80: The use of
proto.UnmarshalinassertDepositEventInIndexerBlockfor standardizing unmarshalling is commendable. Double-check that theTransferEventV1struct accurately represents the expected schema for deposit events in all test scenarios.- 105-105: Introducing
proto.UnmarshalinassertWithdrawEventInIndexerBlockenhances consistency in data handling. Verify that theTransferEventV1struct is correctly structured to match the serialized withdraw event data in tests.protocol/testing/e2e/trading_rewards/trading_rewards_test.go (1)
- 5-5: The addition of the
github.com/cosmos/gogoproto/protoimport aligns with the PR's objective to optimize data handling by leveraging standard serialization and deserialization methods. This change is consistent with the broader goal of improving performance and maintainability by using well-supported libraries.protocol/x/clob/keeper/orders_test.go (6)
- 116-127: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
The test suite provides comprehensive coverage for handling stateful and conditional orders. It's well-structured and covers a variety of scenarios, ensuring the system behaves as expected under different conditions.
- 116-127: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
It's commendable that the tests make extensive use of mocks to isolate the system under test and focus on the behavior of the
ClobKeepercomponent. This approach ensures that the tests remain focused and do not inadvertently depend on external systems or state.
- 116-127: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
The tests for
TestPlaceStatefulOrdersFromLastBlockandTestPlaceConditionalOrdersTriggeredInLastBlockeffectively simulate the conditions under which stateful and conditional orders should be placed or ignored. These tests are crucial for ensuring the reliability and correctness of order handling logic in the system.
- 116-127: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
The use of
require.PanicsWithValuein tests likeTestPlaceStatefulOrdersFromLastBlockandTestPlaceConditionalOrdersTriggeredInLastBlockis a good practice for ensuring that the system behaves as expected in error scenarios. It's important to ensure that the system fails gracefully and provides meaningful error messages when encountering unexpected conditions.
- 116-127: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
The tests make good use of the
memClobmock to simulate the behavior of the in-memory order book. This approach allows for precise control over the system's behavior during testing and ensures that the tests can focus on the logic of theClobKeeperwithout being affected by the complexities of order book management.
- 116-127: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
The setup for each test case is thorough and carefully constructs the necessary state and conditions for the test to run. This meticulous setup is essential for ensuring that the tests accurately reflect the scenarios they are intended to simulate and that the assertions made at the end of each test are meaningful and valid.
protocol/x/perpetuals/keeper/perpetual_test.go (1)
- 1968-1968: The function
getFundingBlockEventsFromIndexerBlockdirectly accesses theProduceBlockmethod of theIndexerEventManagerand iterates through all events, looking specifically for funding events. This approach is efficient for extracting funding events but assumes that all events are correctly formatted and relevant to the context.The logic for extracting funding events is clear and straightforward. However, ensure that the broader system guarantees the integrity and relevance of these events to avoid processing irrelevant or malformed data.
| var block indexer_manager.IndexerTendermintBlock | ||
| _ = unmarshaler.Unmarshal(msgSender.GetOnchainMessages()[0].Value, &block) | ||
| _ = proto.Unmarshal(msgSender.GetOnchainMessages()[0].Value, &block) |
There was a problem hiding this comment.
The replacement of a custom unmarshalling implementation with proto.Unmarshal() is a significant improvement in terms of both performance and code maintainability. By using a standard library method, the code becomes more readable, easier to maintain, and potentially more efficient. However, it's crucial to ensure that all data structures being unmarshalled are compatible with the proto package and that this change does not introduce any regressions in how data is interpreted or processed.
One minor suggestion is to add error handling for the proto.Unmarshal() call. Ignoring the error returned by this function can lead to hard-to-debug issues if the unmarshalling fails for any reason (e.g., due to incompatible data structures or corrupted data). Proper error handling would improve the robustness of the test code.
- _ = proto.Unmarshal(msgSender.GetOnchainMessages()[0].Value, &block)
+ err := proto.Unmarshal(msgSender.GetOnchainMessages()[0].Value, &block)
+ require.NoError(t, err, "Failed to unmarshal indexer tendermint block")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.
| _ = proto.Unmarshal(msgSender.GetOnchainMessages()[0].Value, &block) | |
| err := proto.Unmarshal(msgSender.GetOnchainMessages()[0].Value, &block) | |
| require.NoError(t, err, "Failed to unmarshal indexer tendermint block") |
| var fundingEvent indexerevents.FundingEventV1 | ||
| err := unmarshaler.Unmarshal(event.DataBytes, &fundingEvent) | ||
| err := proto.Unmarshal(event.DataBytes, &fundingEvent) |
There was a problem hiding this comment.
The use of proto.Unmarshal directly without checking the event.Subtype against a specific subtype related to funding events might lead to incorrect unmarshalling if the event data does not match the expected structure. It's crucial to ensure that the event data structure matches the expected indexerevents.FundingEventV1 structure to avoid runtime errors.
Consider adding a subtype check or ensuring through other means that the event data structure is indeed what is expected before attempting to unmarshal it.
Using panic within the getFundingBlockEventsFromIndexerBlock function upon encountering an error in proto.Unmarshal is a harsh error handling strategy that could terminate the entire application. It's generally advisable to handle errors gracefully, especially in a blockchain context where stability is crucial.
Recommend replacing panic with a more graceful error handling mechanism, such as returning the error to the caller, to prevent the application from crashing.
- panic(err)
+ return nil, errCommittable 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.
| err := proto.Unmarshal(event.DataBytes, &fundingEvent) | |
| err := proto.Unmarshal(event.DataBytes, &fundingEvent) | |
| return nil, err |
The error handling in getFundingBlockEventsFromIndexerBlock using panic for unmarshalling errors is aggressive and could lead to service disruption. In blockchain applications, where uptime and reliability are paramount, consider handling errors more gracefully.
Suggest logging the error and continuing with the next event or returning an error collection for the caller to handle, rather than using panic.
Changelist
[OTE-176] Reduce the cost of adding an indexer event.
Currently we unmarshalled and marshalled all of them which grew linearly with the number of messages so as blocks got larger this became a noticeable cost during the critical section when we hold effectively a global lock.
After this change:
Test Plan
No functional change, existing tests pass.
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.