fix statedb error during mempool #1702
Conversation
WalkthroughThe recent updates to the codebase mainly focus on simplifying and refining the control flow across various components. The 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 X ? TipsChat with CodeRabbit Bot (
|
![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: 0
Configuration used: CodeRabbit UI
Files selected for processing (4)
- go/enclave/evm/ethchainadapter/eth_chainadapter.go (2 hunks)
- go/enclave/storage/interfaces.go (1 hunks)
- go/enclave/storage/storage.go (1 hunks)
- go/enclave/txpool/txpool_mock_test.go (1 hunks)
Additional comments: 6
go/enclave/evm/ethchainadapter/eth_chainadapter.go (2)
106-106: The change to the
StateAtfunction to directly create a state DB using the provided root hash is consistent with the PR's objective to utilize the "state root" correctly during mempool processing.103-103: Returning
nil, nilfromStateAtwhen the root hash is the empty code hash could lead to ambiguity in error handling. Ensure that all callers ofStateAtcorrectly handle this case.go/enclave/storage/interfaces.go (1)
- 142-147: The addition of the
StateDB()method to theStorageinterface is consistent with the PR's objective to ensure the correct state DB is used during mempool processing. Ensure that all structs implementing theStorageinterface are updated to include this new method.go/enclave/storage/storage.go (2)
111-113: The implementation of the
StateDB()method in thestorageImplstruct is straightforward and matches the PR's objective to provide access to the underlying state database. This method will allow other parts of the system to retrieve thestate.Databaseinstance held bystorageImpl.111-113: Verify that all implementations of the
Storageinterface have been updated to include the newStateDB()method to maintain compatibility across the codebase.go/enclave/txpool/txpool_mock_test.go (1)
- 344-347: The new
StateDBmethod inmockStorageis currently a stub and will panic if called. Ensure that this method is implemented before merging the PR or ensure that tests which might invoke this method handle the panic appropriately.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- go/enclave/evm/ethchainadapter/eth_chainadapter.go (3 hunks)
Additional comments: 3
go/enclave/evm/ethchainadapter/eth_chainadapter.go (3)
3-6: The removal of the
fmtimport indicates that string formatting has been refactored out of this file. This is consistent with the changes in theStateAtfunction.83-112: The refactoring of the
GetBlockfunction to check for cached batches before querying the database is a performance optimization. Ensure that the logic for fetching the head batch and its parent is correctly implemented and that the error handling is appropriate.121-125: The
StateAtfunction has been simplified to directly create a new state DB using the provided root hash. This change is in line with the PR's objective to correctly utilize the "state root" during mempool processing. Confirm that this new approach is correctly integrated with the rest of the system, and that theStateDB()method is correctly implemented in thestoragepackage.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- go/enclave/txpool/txpool_mock_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- go/enclave/txpool/txpool_mock_test.go
| defer func(faucetContainer *container.FaucetContainer) { | ||
| err := faucetContainer.Stop() | ||
| if err != nil { | ||
| fmt.Printf("Could not stop faucet %s", err.Error()) | ||
| } |
There was a problem hiding this comment.
The addition of the defer function to stop the faucetContainer is a good practice to ensure resources are cleaned up after the test is done. However, the error handling within the defer function could be improved by using the testing object t to log the error instead of printing it to stdout, which is more idiomatic in test cases.
defer func(faucetContainer *container.FaucetContainer) {
err := faucetContainer.Stop()
if err != nil {
- fmt.Printf("Could not stop faucet %s", err.Error())
+ t.Errorf("Could not stop faucet: %s", err.Error())
}
}(faucetContainer)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.
| defer func(faucetContainer *container.FaucetContainer) { | |
| err := faucetContainer.Stop() | |
| if err != nil { | |
| fmt.Printf("Could not stop faucet %s", err.Error()) | |
| } | |
| defer func(faucetContainer *container.FaucetContainer) { | |
| err := faucetContainer.Stop() | |
| if err != nil { | |
| t.Errorf("Could not stop faucet: %s", err.Error()) | |
| } |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- integration/faucet/faucet_test.go (2 hunks)
- tools/walletextension/test/wallet_extension_test.go (1 hunks)
Files skipped from review due to trivial changes (1)
- tools/walletextension/test/wallet_extension_test.go
Files skipped from review as they are similar to previous changes (1)
- integration/faucet/faucet_test.go
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (2)
- integration/obscurogateway/tengateway_test.go (1 hunks)
- integration/simulation/simulation.go (1 hunks)
Additional comments: 2
integration/simulation/simulation.go (2)
292-292: The change to include the specific error message in the panic is a good improvement for debugging purposes.
289-295: Verify if using
panicfor handling health check failures is consistent with the error handling strategy throughout the codebase. Consider a less disruptive error reporting mechanism if this is not the case.Verification successful
The use of
panicfor handling health check failures in thecheckHealthStatusmethod is consistent with the error handling strategy throughout the codebase. The pattern of usingpanicis evident in various parts of the main application code, indicating that it is an accepted practice for handling critical errors that require immediate attention and should halt execution. Therefore, no change is necessary regarding the use ofpanicin this context.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for panic usage in the codebase to understand its consistency. rg 'panic\('Length of output: 21476
Script:
#!/bin/bash # Filter out test files and tools to focus on the main application code usage of panic rg 'panic\(' --glob '!*test.go' --glob '!tools/*'Length of output: 13893
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return integrationCommon.AwaitReceiptEth(context.Background(), client, signedTx.Hash(), 2*time.Second) | ||
| return integrationCommon.AwaitReceiptEth(context.Background(), client, signedTx.Hash(), 20*time.Second) |
There was a problem hiding this comment.
The timeout duration for AwaitReceiptEth in the transferETHToAddress function has been increased from 2 seconds to 20 seconds. This change could potentially reduce flakiness in tests due to network delays or other issues. However, it's important to ensure that this increased timeout does not negatively impact the overall test suite's performance, especially if this function is called multiple times in a test run. If the longer timeout is necessary due to network conditions, consider making it configurable or documenting the reason for this specific value.
| @@ -289,7 +289,7 @@ func (s *Simulation) prefundL1Accounts() { | |||
| func (s *Simulation) checkHealthStatus() { | |||
| for _, client := range s.RPCHandles.ObscuroClients { | |||
| if healthy, err := client.Health(); !healthy || err != nil { | |||
| panic("Client is not healthy") | |||
| panic("Client is not healthy: " + err.Error()) | |||
There was a problem hiding this comment.
Potential nil pointer dereference when calling err.Error() if err is nil. Add a check to ensure err is not nil before using it.
- panic("Client is not healthy: " + err.Error())
+ if err != nil {
+ panic("Client is not healthy: " + err.Error())
+ } else {
+ panic("Client is not healthy and no error was provided")
+ }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.
| panic("Client is not healthy: " + err.Error()) | |
| if err != nil { | |
| panic("Client is not healthy: " + err.Error()) | |
| } else { | |
| panic("Client is not healthy and no error was provided") | |
| } |
Why this change is needed
Bug mempool:
The mempool needs a statedb during processing.
In the current implementation we were ignoring the "state root" passed in and always returned the head.
This might have caused some weird edge cases ( and maybe flakyness!)
Perf issue mempool:
avoid db query for each tx with new account
Flakyness
get a green build
What changes were made as part of this PR
PR checks pre-merging
Please indicate below by ticking the checkbox that you have read and performed the required
PR checks