Local state index engine #537
Conversation
WalkthroughThe pull request introduces significant enhancements to the state management and transaction processing capabilities of the Flow EVM Gateway. Key modifications include the addition of new fields and methods for state indexing in the Changes
Possibly related issues
Possibly related PRs
Poem
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (3)
Files skipped from review as they are similar to previous changes (3)
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 (
|
![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
Outside diff range, codebase verification and nitpick comments (1)
models/receipt.go (1)
73-141: LGTM with a suggestion for improvement!The
EqualReceiptsfunction is well-structured and follows a clear logic for comparing the receipts. It handles nil values appropriately, performs a comprehensive comparison of the receipts, and returns detailed information about any mismatches.Some positive aspects of the function:
- Checks for nil values and returns an error if either receipt is nil.
- Compares the logs of both receipts, ensuring that the number of logs matches and that each log entry's properties are equivalent.
- Compares various fields of the receipts, such as
TxHash,GasUsed,CumulativeGasUsed,Type,ContractAddress,Status, andBloom.- Returns a boolean indicating whether the receipts are equal and a slice of errors detailing any mismatches.
Suggestion for improvement:
Consider extracting the log comparison logic into a separate function for better readability and maintainability. This would make the
EqualReceiptsfunction more concise and easier to understand.For example:
func compareLogs(gethLogs []*gethTypes.Log, logs []*gethTypes.Log) []error { // ... log comparison logic ... } func EqualReceipts(gethReceipt *gethTypes.Receipt, receipt *Receipt) (bool, []error) { // ... other comparison logic ... logErrs := compareLogs(gethReceipt.Logs, receipt.Logs) errs = append(errs, logErrs...) // ... remaining comparison logic ... }Overall, great work on implementing this comprehensive receipt comparison function!
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- bootstrap/bootstrap.go (13 hunks)
- models/receipt.go (2 hunks)
- models/transaction.go (1 hunks)
- services/state/engine.go (1 hunks)
- services/state/state.go (1 hunks)
- storage/pebble/ledger.go (1 hunks)
- tests/helpers.go (2 hunks)
- tests/state_integration_test.go (1 hunks)
Files skipped from review due to trivial changes (1)
- models/transaction.go
Additional comments not posted (17)
storage/pebble/ledger.go (1)
104-104: LGTM!The code change is approved.
Assigning the result of
index.Next()to theindexvariable is necessary to update theindexwith the next value. This ensures that the subsequent operations use the correct slab index.services/state/state.go (1)
1-93: LGTM!The
Statestruct and its methods provide a clean and modular implementation for managing the state and executing transactions. The code follows good practices and handles errors appropriately.tests/state_integration_test.go (1)
23-124: LGTM!The
Test_StateExecution_Transfersfunction provides a comprehensive integration test for state execution and transfers. It properly sets up the necessary components, performs multiple transactions, and verifies the expected balance changes. The test uses appropriate assertions and waits for new block events to ensure the state is updated before making assertions.services/state/engine.go (1)
1-193: LGTM!The code changes in this new file are approved. The code is well-structured, follows good practices, and implements the state engine functionality as described.
Some positive aspects of the code:
- Implements interfaces for the engine and subscriber, promoting modularity and testability.
- Uses dependency injection for the engine's dependencies, making it easier to test and maintain.
- Handles errors and panics appropriately, ensuring the engine's robustness.
- Logs important events and errors, aiding in debugging and monitoring.
- The code is well-structured and readable, making it easier for other developers to understand and maintain.
Keep up the good work!
tests/helpers.go (2)
85-93: Verify the purpose and potential impact of skipping transaction validation.The new
SkipTransactionValidationfield added to the server's configuration allows the emulator to bypass transaction validation checks. In thestartEmulatorfunction, this field is set totrue.Please ensure that skipping transaction validation is intended for testing purposes only and won't be enabled in production environments. Skipping validation could potentially lead to unexpected behavior or inconsistencies if not used carefully.
Consider adding a comment to clarify the purpose of this field and any potential risks associated with skipping transaction validation.
For example:
SkipTransactionValidation: true, // Set to true only for testing purposes. Do not use in production.Also, verify that skipping transaction validation does not affect the correctness of the tests and that the tests still cover the necessary validation scenarios.
330-331: LGTM!The formatting change in the
evmSignfunction's parameter list, where the closing parenthesis has been moved to a new line, improves readability and aligns with the Go coding style guidelines.This change does not affect the functionality of the code.
bootstrap/bootstrap.go (11)
36-36: LGTM!The addition of the
Ledgerfield to theStoragesstruct is approved.
48-49: LGTM!The changes to the
Bootstrapstruct are approved:
- Renaming the
clientfield toClientis valid if the field needs to be accessed from outside the package.- The addition of the
Requesterfield is approved.
50-51: LGTM!The changes to the
Bootstrapstruct are approved:
- Renaming the
storagesfield toStoragesis valid if the field needs to be accessed from outside the package.- Renaming the
publishersfield toPublishersis valid if the field needs to be accessed from outside the package.
55-57: LGTM!The changes to the
Bootstrapstruct are approved:
- Renaming the
eventsfield toEventsis valid if the field needs to be accessed from outside the package.- Renaming the
tracesfield toTracesis valid if the field needs to be accessed from outside the package.- The addition of the
Statefield is approved.
93-98: LGTM!The changes to the
StartEventIngestionmethod are approved. They correctly use the publicClientandStoragesfields, reflecting the renaming of the previously private fields.
128-136: LGTM!The changes to the
StartEventIngestionmethod are approved. They correctly use the publicStorages,Publishers, andEventsfields, reflecting the renaming of the previously private fields.
Line range hint
141-174: LGTM!The changes to the following methods are approved:
StartEventIngestion: Correctly uses the publicEventsfield, reflecting the renaming of the previously privateeventsfield.StartTraceDownloader: Correctly uses the publicPublishers,Storages, andTracesfields, reflecting the renaming of the previously privatepublishers,storages, andtracesfields.StopTraceDownloader: Correctly uses the publicTracesfield, reflecting the renaming of the previously privatetracesfield.
178-183: LGTM!The changes to the
StopEventIngestionmethod are approved. They correctly use the publicEventsfield, reflecting the renaming of the previously privateeventsfield.
185-208: LGTM!The addition of the
StartStateIndexandStopStateIndexmethods is approved:
StartStateIndex: Correctly initializes the state indexing engine using the newly addedStatefield and the publicStoragesandPublishersfields.StopStateIndex: Correctly manages the lifecycle of the state indexing engine using the publicStatefield.
Line range hint
214-474: LGTM!The changes to the following methods and functions are approved:
StartAPIServer: Correctly uses the publicServer,Storages, andPublishersfields, reflecting the renaming of the previously privateserver,storages, andpublishersfields.StopAPIServer: Correctly uses the publicServerfield, reflecting the renaming of the previously privateserverfield.setupStorage: Correctly returns aLedgerfield as part of theStoragesstruct, consistent with the addition of theLedgerfield to theStoragesstruct.
493-496: LGTM!The change to the
Runfunction is approved. It correctly calls the newly introducedStartStateIndexmethod to ensure that the state indexing engine is started as part of the boot process.
tests/state_integration_test.go
Outdated
| assert.Equal(t, amount.Uint64()+amount2.Uint64(), balance.Uint64()) | ||
| } | ||
|
|
||
| // todo test historic heights |
There was a problem hiding this comment.
Reminder: Implement test case for historic heights.
The todo comment indicates that testing historic heights is pending. It's important to ensure the correctness of the state at different points in time.
Do you want me to generate the test case for historic heights or open a GitHub issue to track this task?
models/receipt.go
Outdated
| if gethReceipt.CumulativeGasUsed != receipt.CumulativeGasUsed { | ||
| errs = append(errs, fmt.Errorf("receipt CumulativeGasUsed mismatch: %d != %d", gethReceipt.CumulativeGasUsed, receipt.CumulativeGasUsed)) | ||
| } | ||
| if gethReceipt.Type != 0 && gethReceipt.Type != receipt.Type { // only compare if not direct call |
There was a problem hiding this comment.
I am not sure if this is safe, because a type of 0 is basically for LegacyTx. We have simply given the same type to direct calls, but we might miss some cases like this.
There was a problem hiding this comment.
Seems like for direct calls, the TxType is never set: https://github.com/onflow/flow-go/blob/master/fvm/evm/types/result.go#L148-L151. Which means that it gets the default value of 0.
There was a problem hiding this comment.
I understand, but not sure if we can do it any other way?
There was a problem hiding this comment.
Maybe we could temporarily assign it here: https://github.com/onflow/flow-evm-gateway/pull/537/files#diff-b22e4b9d0ee4fa7a9557969e2ce56c91237a67f6e94d1709730985317c8866c3R71. Is this receipt supposed to be saved, or not? Even if it is, that's ok I guess. Becase the type we return for direct calls, is taken from: https://github.com/onflow/flow-evm-gateway/blob/main/models/transaction.go#L98. Which is the same as for LegacyTx.
services/state/engine.go
Outdated
| return err | ||
| } | ||
|
|
||
| ctx, err := e.blockContext(block, receipt, uint(i), gasUsed) |
There was a problem hiding this comment.
uint(i) seems to be used for TxCountSoFar, so I am wondering whether this should be 0-based? I guess it should be starting from 1. What do you think?
There was a problem hiding this comment.
it's passing tests, but will take a look into flow-go how it's indexed
Description
Implementation of state engine and state index.
There are two components:
For contributor use:
masterbranchFiles changedin the Github PR explorerSummary by CodeRabbit
New Features
Bug Fixes
Tests