Integrate local state index #541
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe pull request introduces significant changes across multiple files, primarily focusing on the integration of Ethereum Virtual Machine (EVM) functionalities. Key modifications include the introduction of new structs and interfaces for handling EVM operations, the refactoring of existing structures to enhance clarity and maintainability, and the addition of comprehensive testing for state execution and transaction handling. The changes also involve renaming and restructuring fields and methods to improve accessibility and consistency throughout the codebase. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ClientHandler
participant EVMClient
participant LocalClient
participant RemoteClient
participant Engine
participant BlockState
User->>ClientHandler: Send transaction
ClientHandler->>LocalClient: Execute transaction
alt LocalClient fails
ClientHandler->>RemoteClient: Execute transaction
end
ClientHandler->>Engine: Notify transaction
Engine->>BlockState: Execute transaction
BlockState-->>Engine: Return result
Engine-->>ClientHandler: Return transaction status
ClientHandler-->>User: Return transaction confirmation
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 (
|
sideninja
changed the base branch from
gregor/local-state/main
to
gregor/local-state/historic
![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 and nitpick comments (4)
storage/pebble/register_test.go (1)
55-120: LGTM, but consider adding more examples.The subtest is correctly testing the
SetValueandGetValuemethods for multiple accounts at different heights.However, consider adding more examples to cover additional scenarios, such as:
- Setting and getting values for the same owner and key at non-consecutive heights.
- Setting and getting values for the same owner and different keys at the same height.
- Setting and getting values for different owners and the same key at the same height.
services/state/engine.go (1)
82-85: LGTM, but consider adding cleanup logic.The
Stopfunction is correctly implemented and marks the engine as stopped.However, consider adding cleanup logic to gracefully shutdown the engine and release any resources.
services/state/state.go (2)
21-31: LGTM, but address thetodocomment.The struct is well-defined and contains all the necessary fields to represent the state of a block.
However, please address the
todocomment to changetypes.StateDBtotypes.ReadOnlyViewin the future.
138-177: LGTM, but address thetodocomments.The function is correctly implemented and initializes the block context with the necessary fields. The
GetHashFuncclosure allows retrieving the hash of a block by height.However, please address the following
todocomments:
- At line 161-162: Set the
Tracerfield appropriately.- At line 166-167: Consider failing and executing calls using a remote client when cadence architecture is used.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (15)
- api/api.go (2 hunks)
- bootstrap/bootstrap.go (12 hunks)
- models/receipt.go (2 hunks)
- models/transaction.go (1 hunks)
- services/requester/client_handler.go (1 hunks)
- services/requester/evm.go (1 hunks)
- services/requester/local_client.go (1 hunks)
- services/requester/remote_client.go (17 hunks)
- services/requester/remote_client_test.go (1 hunks)
- services/state/engine.go (1 hunks)
- services/state/state.go (1 hunks)
- storage/pebble/register.go (1 hunks)
- storage/pebble/register_test.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 (78)
services/requester/evm.go (8)
11-13: LGTM!The code changes are approved.
15-17: LGTM!The code changes are approved.
19-22: LGTM!The code changes are approved.
24-27: LGTM!The code changes are approved.
29-30: LGTM!The code changes are approved.
32-34: LGTM!The code changes are approved.
36-37: LGTM!The code changes are approved.
39-40: LGTM!The code changes are approved.
services/requester/local_client.go (7)
16-21: LGTM!The code changes are approved.
28-33: LGTM!The code changes are approved.
35-42: LGTM!The code changes are approved.
60-71: LGTM!The code changes are approved.
73-79: LGTM!The code changes are approved.
81-87: LGTM!The code changes are approved.
89-96: LGTM!The code changes are approved.
storage/pebble/register_test.go (2)
11-26: LGTM!The subtest is correctly testing the
SetValueandGetValuemethods at the same height.
28-53: LGTM!The subtest is correctly testing the
SetValueandGetValuemethods at the latest height.services/state/engine.go (7)
19-28: LGTM!The
Enginestruct is well-defined and has all the necessary fields for the state engine.
30-51: LGTM!The
NewStateEnginefunction is correctly implemented and initializes all the fields of theEnginestruct.
59-74: LGTM!The
Notifyfunction is correctly implemented and handles the block execution.
76-80: LGTM!The
Runfunction is correctly implemented and sets up the engine to receive blocks from the publisher.
87-89: LGTM!The
Donefunction is correctly implemented and returns the done channel from the engine status.
91-93: LGTM!The
Readyfunction is correctly implemented and returns the ready channel from the engine status.
108-126: LGTM!The
executeBlockfunction is correctly implemented and executes the transactions in the block.storage/pebble/register.go (8)
16-20: LGTM!The
Registerstruct is well-defined with appropriate fields for height, store, and synchronization.
22-28: LGTM!The
NewRegisterfunction correctly initializes all the fields of theRegisterstruct.
30-60: LGTM!The
GetValuemethod is correctly implemented with proper locking, iterator usage, and error handling. It also adheres to the interface expectation of returning nil when the value is not found.
62-77: LGTM!The
SetValuemethod is correctly implemented with proper locking and error handling. It delegates the storage operation to the underlying store.
79-86: LGTM!The
ValueExistsmethod is correctly implemented by leveraging theGetValuemethod and checking the returned value.
88-122: LGTM!The
AllocateSlabIndexmethod is correctly implemented with proper locking, retrieval of the current slab index, incrementing it, and persisting the new index back to the store. It also handles the case when the slab index is not found in the store.
132-136: LGTM!The
idmethod correctly calculates the ledger ID by appending the owner, key, and height bytes, following the schema mentioned in the comments.
138-145: LGTM!The
idUpperandidLowermethods correctly calculate the upper and lower bounds for the ledger ID range. The upper bound is exclusive and increments the height by 1, while the lower bound always uses a height of 0.Also applies to: 147-153
tests/state_integration_test.go (1)
22-163: Comprehensive state execution test.The test function
Test_StateExecution_Transfersis well-structured and covers important aspects of state execution and balance updates. It follows a logical flow and tests the following scenarios:
- Setting up the emulator and required services.
- Configuring and starting the state index, API server, and event ingestion.
- Verifying the initial balance of a test address.
- Sending EVM transactions and waiting for new block events.
- Verifying the updated balances after each transaction.
- Reading balances at historic heights and verifying the expected values.
The test function is comprehensive and ensures the correctness of state execution and balance updates across different block heights.
services/state/state.go (3)
33-65: LGTM!The function is correctly implemented and initializes all the necessary fields of the
BlockStatestruct.
67-114: LGTM, but verify the error handling behavior.The function is correctly implemented and supports executing both direct calls and transaction calls. It also verifies the result receipt against the retrieved receipt to ensure state consistency.
However, please verify the error handling behavior mentioned in the
todocomment at line 94-95. Ensure that restarting and retrying the service on error is the desired behavior.
116-133: LGTM!The function is correctly implemented and allows performing a call without modifying the state.
models/receipt.go (1)
74-141: LGTM!The
EqualReceiptsfunction is well-structured and correctly compares thegethReceipt type with the customReceipttype. It handles nil receipts, compares logs, and compares receipt data fields. The function returns a boolean indicating whether the receipts are equal and a slice of errors containing any mismatches found during the comparison.The code is readable, maintainable, and follows best practices.
services/requester/remote_client_test.go (1)
Line range hint
213-227: LGTM!The changes to the
createEVMtest helper function are consistent with the updated return type of*RemoteClient. The instantiation of theRemoteClientstruct uses the same fields as the previousEVMstruct, indicating that theRemoteClientstruct likely has a similar structure and functionality.The changes are localized to the test helper function and do not affect the behavior of the tests.
services/requester/client_handler.go (12)
29-37: LGTM!The
ClientHandlerstruct is well-defined with appropriate fields to handle EVM operations using remote and local clients.
39-64: LGTM!The
NewClientHandlerfunction is correctly implemented and handles the creation of a newClientHandlerinstance with the provided dependencies.
66-69: LGTM!The
SendRawTransactionmethod is correctly implemented and always uses the remote client to send the raw transaction.
71-86: LGTM!The
GetBalancemethod is correctly implemented and uses thehandleCallfunction to execute the call using both local and remote clients and handle errors.
88-104: LGTM!The
Callmethod is correctly implemented and uses thehandleCallfunction to execute the call using both local and remote clients and handle errors.
106-122: LGTM!The
EstimateGasmethod is correctly implemented and uses thehandleCallfunction to execute the call using both local and remote clients and handle errors.
124-139: LGTM!The
GetNoncemethod is correctly implemented and uses thehandleCallfunction to execute the call using both local and remote clients and handle errors.
141-156: LGTM!The
GetCodemethod is correctly implemented and uses thehandleCallfunction to execute the call using both local and remote clients and handle errors.
158-169: LGTM!The
GetLatestEVMHeightmethod is correctly implemented and uses thehandleCallfunction to execute the call using both local and remote clients and handle errors.
171-187: LGTM!The
GetStorageAtmethod is correctly implemented and uses thehandleCallfunction to execute the call using both local and remote clients and handle errors.
189-213: LGTM!The
localClientmethod is correctly implemented and handles the creation of a newLocalClientinstance for a given block height using the retrieved block and other dependencies.
215-290: LGTM!The
handleCallfunction is well-implemented and handles the execution and error handling of local and remote calls effectively. It covers various error scenarios and returns the appropriate result based on the conditions. The use of goroutines for concurrent execution and the comparison of results in case of no errors adds to the robustness of the function.tests/helpers.go (2)
85-93: Approve addition ofSkipTransactionValidationfield with a note of caution.The addition of the
SkipTransactionValidationfield to the emulator server configuration is approved.This field could be useful in certain test scenarios to improve performance or test specific behaviors by skipping transaction validation.
However, please ensure that skipping validation in tests does not inadvertently hide any potential issues that could occur in production where validation is enabled. Consider adding some tests with validation enabled to catch such issues.
330-331: Approve line break change.The line break added before the closing parenthesis of the
evmSignfunction parameter list is approved.This change enhances code readability without altering the function's behavior.
bootstrap/bootstrap.go (10)
36-36: LGTM!The addition of the
Registersfield to theStoragesstruct is approved.
48-57: LGTM!The changes to the
Bootstrapstruct, including the addition of new fields and renaming of existing fields to uppercase, are approved. These changes improve the organization and consistency of the struct.
75-83: LGTM!The changes to the
Newfunction, which initialize the new fields added to theBootstrapstruct, are approved. The initialization logic is correct and consistent with the struct changes.
Line range hint
93-141: LGTM!The changes to the
StartEventIngestionmethod, which update the usage of theBootstrapstruct fields to match the new public field names, are approved. The field names are used correctly and consistently throughout the method.
156-165: LGTM!The changes to the
StartTraceDownloadermethod, which update the usage of theBootstrapstruct fields to match the new public field names, are approved. The field names are used correctly and consistently throughout the method.
170-174: LGTM!The change to the
StopTraceDownloadermethod, which updates the usage of theBootstrapstruct field to match the new public field nameTraces, is approved. The field name is used correctly.
178-183: LGTM!The change to the
StopEventIngestionmethod, which updates the usage of theBootstrapstruct field to match the new public field nameEvents, is approved. The field name is used correctly.
185-201: LGTM!The addition of the
StartStateIndexmethod to theBootstrapstruct is approved. The method correctly initializes the state indexing engine using the newStatefield and handles the startup process appropriately.
203-208: LGTM!The addition of the
StopStateIndexmethod to theBootstrapstruct is approved. The method correctly handles the stopping of the state indexing engine by checking if theStatefield is nil before calling theStopmethod.
Line range hint
214-341: LGTM!The changes to the
StartAPIServermethod, which update the usage of theBootstrapstruct fields to match the new public field names and initialize therequester.NewClientHandlerwith the newEVMClientfield, are approved. The field names are used correctly and consistently throughout the method, and the initialization of the client handler is valid.services/requester/remote_client.go (14)
Line range hint
84-101: LGTM!The
RemoteClientstruct looks good. It provides a clear structure for the remote client implementation and contains all the necessary dependencies and configurations.
Line range hint
103-193: LGTM!The
NewRemoteconstructor function looks good. It properly initializes theRemoteClientinstance with the provided dependencies and configurations. It also performs necessary checks on the COA account and creates the COA resource if required.
Line range hint
195-252: LGTM!The
SendRawTransactionmethod looks good. It properly validates the transaction, checks the gas price, builds a Flow transaction with the EVM transaction data, and sends it to the transaction pool.
Line range hint
254-302: LGTM!The
buildTransactionmethod looks good. It properly builds a Flow transaction from the provided script and arguments, concurrently retrieves the necessary information, sets the transaction fields, and signs the transaction envelope.
Line range hint
304-348: LGTM!The
GetBalancemethod looks good. It properly retrieves the balance of an address at a specific EVM height by converting the height, executing a script, and returning the balance as a big integer.
Line range hint
350-401: LGTM!The
GetNoncemethod looks good. It properly retrieves the nonce of an address at a specific EVM height by converting the height, executing a script, and returning the nonce as a uint64.
Line range hint
403-424: LGTM!The
stateAtmethod looks good. It properly retrieves the state database at a specific EVM height by converting the height, creating a remote ledger, and returning a new state database instance.
Line range hint
426-439: LGTM!The
GetStorageAtmethod looks good. It properly retrieves the storage value at a specific address and hash at a specific EVM height by retrieving the state database and getting the storage value.
Line range hint
441-495: LGTM!The
Callmethod looks good. It properly executes a read-only EVM call at a specific height by converting the height, executing a script, parsing the result, and returning the returned data.
Line range hint
497-551: LGTM!The
EstimateGasmethod looks good. It properly estimates the gas consumption of an EVM transaction at a specific height by converting the height, executing a script, parsing the result, and returning the gas consumed.
Line range hint
553-605: LGTM!The
GetCodemethod looks good. It properly retrieves the code of an address at a specific EVM height by converting the height, executing a script, converting the result to bytes, and returning the code.
Line range hint
607-631: LGTM!The
GetLatestEVMHeightmethod looks good. It properly retrieves the latest EVM height by executing a script at the latest block height, converting the result to uint64, and returning the height.
Line range hint
633-658: LGTM!The
getSignerNetworkInfomethod looks good. It properly retrieves the signer account information from the network and returns the key index and sequence number of the signer account.
Line range hint
660-678: LGTM!The
replaceAddressesmethod looks good. It properly replaces the addresses in the script based on the network by iterating over the system contracts, replacing the import statements with the actual contract addresses, and replacing the COA address if used.api/api.go (2)
86-86: LGTM!The change to the
evmfield type fromrequester.Requestertorequester.EVMClientis approved.Using a more specialized
EVMClientinterface improves type safety and makes the EVM interactions clearer.
99-99: LGTM!The change to the
evmparameter type fromrequester.Requestertorequester.EVMClientin the constructor is approved.This change is consistent with the
BlockChainAPIstruct field type change and ensures proper instantiation.
services/requester/local_client.go
Outdated
| func (l *LocalClient) Call( | ||
| ctx context.Context, | ||
| data []byte, | ||
| from common.Address, | ||
| evmHeight int64, | ||
| ) ([]byte, error) { | ||
| res, err := l.state.Call(from, data) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| // todo what if it failed? | ||
|
|
||
| return res.ReturnedData, nil | ||
| } |
There was a problem hiding this comment.
Handle the case of a failed call.
The method is mostly implemented correctly, but the case of a failed call is not handled.
Consider adding an error check and return the error if the call fails:
res, err := l.state.Call(from, data)
if err != nil {
return nil, err
}
if res.Failed() {
return nil, fmt.Errorf("call failed: %s", res.Err)
}
return res.ReturnedData, nil
services/requester/local_client.go
Outdated
| } | ||
|
|
||
| func (l *LocalClient) GetLatestEVMHeight(ctx context.Context) (uint64, error) { | ||
| return l.blocks.LatestEVMHeight() |
There was a problem hiding this comment.
I am not sure that this method makes sense for the local client. The requester would use a Cadence script to fetch the latest EVM block on the Flow network, which doesn't have to be the same as what any EVM Gateway operator has ingested.
| address common.Address, | ||
| evmHeight int64, | ||
| ) (*big.Int, error) { | ||
| bal := l.state.GetBalance(address) |
There was a problem hiding this comment.
How do we know that the *state.BlockState matches the given evmHeight 🤔 ? The same goes for all methods that accept historical blocks. I can see that the ClientHandler takes care of that, it's just seems not very intuitive to have a function argument which is unused/ignored.
| return err | ||
| } | ||
|
|
||
| ctx, err := s.blockContext(receipt) |
There was a problem hiding this comment.
| ctx, err := s.blockContext(receipt) | |
| ctx, err := s.blockContext(receipt) | |
| if err != nil { | |
| return err | |
| } |
| return nil, err | ||
| } | ||
|
|
||
| return bv.DryRunTransaction(tx, from) |
services/state/state.go
Outdated
| } | ||
|
|
||
| if ok, errs := models.EqualReceipts(res.Receipt(), receipt); !ok { |
There was a problem hiding this comment.
I believe that we may be able to get away without this models.EqualReceipts method, if we used the blocks'
TransactionsHashRootReceiptsRoot
fields.
We do something similar:
txHashes := evmTypes.TransactionHashes{}
for _, tx := range e.transactions {
txHashes = append(txHashes, tx.Hash())
}
if e.block.TransactionHashRoot != txHashes.RootHash() {
return nil, fmt.Errorf(
"block %d references missing transaction/s",
e.block.Height,
)
}And
b.ReceiptRoot = gethTypes.DeriveSha(receipts, gethTrie.NewStackTrie(nil))There was a problem hiding this comment.
It seems to me that it is more bullet-proof, to compare them this way. But can be done in a follow-up PR.
…regor/local-state/state
# Conflicts: # services/requester/client_handler.go # services/requester/local_client.go
|
mreged into feature/state |
Description
Integrate local state index complying to the EVM client interface. A client handler was added which is a way to execute both remote and local client and compare results. We only use the local state result if the remote client fails. We also can then change this handler to be smarter in ways it chooses which client to use (for example as long as the state index is healthy use local otherwise use remote).
Please note there are a lot of changes that are results of renaming types.
For contributor use:
masterbranchFiles changedin the Github PR explorerSummary by CodeRabbit
New Features
ClientHandlerfor enhanced EVM operations with local and remote client support.EVMClientinterface for structured EVM interactions.EngineandBlockStatefor robust blockchain state management.Bug Fixes
Documentation
Tests
Registerfunctionality and new integration tests for state execution.