Local state transaction re-execution #712
Conversation
m-Peter
requested review from
janezpodhostnik,
peterargue and
zhangchiqing
as code owners
WalkthroughThis pull request introduces significant changes to the Flow EVM Gateway, focusing on transaction state validation, key management, and API improvements. The primary modifications include adding a new command-line argument for transaction state validation, implementing a key store mechanism, and refactoring error handling and logging across multiple components. The changes enhance the flexibility of transaction processing, introduce more robust key management, and provide more granular control over transaction validation strategies. Changes
Possibly Related PRs
Suggested Labels
Suggested Reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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: 5
🧹 Nitpick comments (35)
metrics/collector.go (2)
39-39: Fix field name inconsistencyThe field name
availableSigningkeysuses a lowercase 'k' while the metric name and method use 'Keys'. Consider updating for consistency:- availableSigningkeys prometheus.Gauge + availableSigningKeys prometheus.Gauge
111-111: Ensure consistent naming in metric registration and struct initializationThe field name inconsistency is carried through to the struct initialization.
- availableSigningkeys: availableSigningKeys, + availableSigningKeys: availableSigningKeys,Also applies to: 129-129
services/requester/kms_key_signer.go (3)
15-19: Consider referencing official docs inline for cloudkms.Signer usage.
Providing a brief mention of how the Cloud KMS signing key is set up in addition to the existing link can be helpful.
37-45: Leverage more detailed logging around key ID.
It might be helpful to log the actual fully qualified KMS key resource name when reporting the error here, for better traceability in production.
57-62: Validate message correctness before signing.
Although this snippet simply logs a duration, consider validating that the input message isn’t empty or invalid for your domain logic.services/requester/pool.go (3)
34-34: Config field usage clarity.
When referencing config in the TxPool, consider adding doc comments or method-level commentary on how config influences the pool’s behavior.
42-49: TxPool constructor consistency.
The new config parameter is nicely integrated, but if other configurations apply to subsets of TxPool logic, consider splitting them to avoid a large monolithic config reference.
102-102: Return early.
If validation is disabled, returning nil is correct. Nonetheless, consider logging a debug statement to clarify that the transaction is not being sealed.bootstrap/create-multi-key-account.go (2)
21-23: Global variables for addresses.
Using global variables for addresses is convenient, but consider how this might affect usage in testing or multiple environments—particularly if other code changes these vars at runtime.
57-57: Simplified creation call.
Refactoring to one call that returns both address and key is logical, removing extra complexity.tests/integration_test.go (3)
64-76: Refactor suggestion for configuration.
Consider factoring out repeated configuration struct creation into a helper function to avoid duplication across multiple tests. This enhances maintainability if more fields need to be added or updated.
234-244: Assertion wait times.
The test uses assert.Eventually with a short interval. If the environment is slow, this could lead to flaky tests. Consider a slightly longer duration or adapt the interval for slow environments.
267-267: Additional coverage for multi-key tests.
The separate multi-key account creation flows in this test file appear consistent. Adding negative test cases, such as invalid keys or insufficient keys, could further improve resilience.Also applies to: 271-272, 286-286
api/debug.go (2)
32-33: Offchain package clarity.
The alias "offchain" for storage might cause confusion. Consider a more explicit alias or rely on the default import name for better readability.
70-70: Parameter list complexity.
Adding a new parameter to NewDebugAPI can increase complexity. If more optional parameters are introduced, consider a configuration struct.bootstrap/bootstrap.go (1)
221-229: Filtering out keys that don’t match signer’s public key.
This approach is logical. Consider logging if no matching key is found to help debug misconfiguration.bootstrap/utils.go (2)
23-34: Consider refactoring the switch statement for better maintainabilityThe current switch implementation could be improved to be more maintainable and explicit.
Consider this alternative structure:
- switch { - case config.COAKey != nil: - signer, err = crypto.NewInMemorySigner(config.COAKey, crypto.SHA3_256) - case config.COACloudKMSKey != nil: - signer, err = requester.NewKMSKeySigner( - ctx, - *config.COACloudKMSKey, - logger, - ) - default: - return nil, fmt.Errorf("must provide either single COA / Cloud KMS key") - } + if config.COAKey != nil && config.COACloudKMSKey != nil { + return nil, fmt.Errorf("cannot provide both COA and Cloud KMS keys") + } + + if config.COAKey != nil { + return crypto.NewInMemorySigner(config.COAKey, crypto.SHA3_256) + } + + if config.COACloudKMSKey != nil { + return requester.NewKMSKeySigner( + ctx, + *config.COACloudKMSKey, + logger, + ) + } + + return nil, fmt.Errorf("must provide either single COA or Cloud KMS key")This structure:
- Explicitly prevents both keys being provided
- Makes the logic flow more clear
- Removes the need for intermediate variables
16-40: Add unit tests for the createSigner functionThis new utility function needs test coverage to ensure it handles all cases correctly:
- COA key provided
- Cloud KMS key provided
- Neither key provided
- Both keys provided (edge case)
Would you like me to help create a test file with these test cases?
metrics/handler.go (2)
38-39: Review error logging levelConsider using Error level instead of Debug for method extraction failures, as this could indicate client issues or potential attacks.
- h.logger.Debug().Err(err).Msg("error extracting method") + h.logger.Error().Err(err).Msg("failed to extract method from request")
Line range hint
48-52: Enhance panic recovery loggingThe current panic recovery could provide more context about the failure.
- logger.Error().Msgf("failed to extract method: %v", r) + logger.Error(). + Interface("request", r). + Interface("panic", rec). + Str("url", r.URL.String()). + Msg("panic while extracting method")eth/utils.go (2)
6-62: Consider adding version information for each methodThe valid methods map is well-organized by namespace. Consider adding version information or deprecation status for each method to help with API versioning and backward compatibility.
var validMethods = map[string]struct{}{ - // eth namespace - "eth_blockNumber": {}, + // eth namespace (https://ethereum.org/en/developers/docs/apis/json-rpc/) + "eth_blockNumber": {}, // Since 1.0
66-69: Consider enhancing method validationThe validation is simple and efficient. Consider extending it to validate method parameters or return custom error types for different validation failures.
-func IsValidMethod(methodName string) bool { - _, ok := validMethods[methodName] - return ok +func IsValidMethod(methodName string) error { + if _, ok := validMethods[methodName]; !ok { + return fmt.Errorf("unsupported method: %s", methodName) + } + return nil }tests/web3js/eth_failure_handling_test.js (2)
Line range hint
6-35: Consider extracting common test setupThe test setup code is duplicated across test cases. Consider extracting the common setup into a helper function.
+async function setupTransactionTest() { + const receiver = web3.eth.accounts.create() + return { receiver } +} it('should fail when nonce too low', async () => { - let receiver = web3.eth.accounts.create() + const { receiver } = await setupTransactionTest()
88-116: Consider parameterizing balance test casesThe two insufficient balance test cases are similar. Consider using test.each or a similar mechanism to parameterize these tests with different balance scenarios.
const balanceTestCases = [ { value: 10_100_000, expectedOvershot: 3550000, description: 'transfer exceeds balance' }, { value: 7_000_000, expectedOvershot: 450000, description: 'transfer plus gas exceeds balance' } ]; balanceTestCases.forEach(({ value, expectedOvershot, description }) => { it(`should fail when ${description}`, async () => { // Test implementation using parameters }); });services/evm/executor.go (2)
102-108: Essential gas validation addedThe new gas validation ensures consistency between executed and recorded gas consumption, preventing potential state inconsistencies.
Consider adding metrics/logging for gas mismatches to help monitor and debug such occurrences in production.
Line range hint
121-146: Improved block context creationThe refactoring of block context creation improves code maintainability by:
- Using the dedicated
blocks.NewBlockContextfunction- Maintaining clear separation of concerns
- Properly handling block hash retrieval
However, the error handling for block hash retrieval could be improved.
Consider enhancing the block hash retrieval error handling:
func(n uint64) common.Hash { block, err := s.blocks.GetByHeight(n) if err != nil { + s.logger.Error().Err(err).Uint64("height", n).Msg("failed to get block by height") return common.Hash{} } blockHash, err := block.Hash() if err != nil { + s.logger.Error().Err(err).Uint64("height", n).Msg("failed to compute block hash") return common.Hash{} } return blockHash }config/config.go (1)
57-58: Key management simplification and validation configurationThe changes include:
- Simplified key management by using a single Cloud KMS key
- Added transaction validation configuration
This might require migration steps for existing deployments using multiple keys.
Consider:
- Adding validation for the TxStateValidation field to ensure only valid values are accepted
- Providing migration documentation for users transitioning from multiple keys to single key setup
Also applies to: 93-95
api/utils.go (1)
15-15: LGTM! Consider grouping related error cases.The addition of new error cases from the
corepackage enhances error handling for common Ethereum transaction scenarios. The implementation correctly propagates these errors to the client.Consider grouping related error cases together for better maintainability:
case errors.Is(err, errs.ErrEntityNotFound): return zero, nil case errors.Is(err, errs.ErrInvalid): return zero, err case errors.Is(err, errs.ErrFailedTransaction): return zero, err case errors.As(err, &revertedErr): return zero, revertedErr +// Transaction-specific errors case errors.Is(err, core.ErrNonceTooLow): return zero, err case errors.Is(err, core.ErrNonceTooHigh): return zero, err case errors.Is(err, core.ErrInsufficientFunds): return zero, errAlso applies to: 128-133
cmd/run/cmd.go (1)
286-286: Consider enhancing the flag description.The description could be more detailed about the implications of each validation mechanism.
-Cmd.Flags().StringVar(&txStateValidation, "tx-state-validation", "tx-seal", "Sets the transaction validation mechanism. It can validate using the local state index, or wait for the outer Flow transaction to seal. Available values ('local-index' / 'tx-seal'), defaults to 'tx-seal'.") +Cmd.Flags().StringVar(&txStateValidation, "tx-state-validation", "tx-seal", "Sets the transaction validation mechanism. Options:\n"+ + "- 'local-index': Validates using local state index (faster but potentially less secure)\n"+ + "- 'tx-seal': Waits for the outer Flow transaction to seal (slower but more secure)\n"+ + "Defaults to 'tx-seal'.")tests/helpers.go (1)
144-154: Consider adding validation for keyCount.While the implementation is correct, consider adding validation for the
keyCountparameter to ensure it's within reasonable bounds.keyCount := 5 +if keyCount <= 0 || keyCount > 10 { + return nil, nil, fmt.Errorf("keyCount must be between 1 and 10") +}services/ingestion/event_subscriber_test.go (1)
46-46: Consistent update of NewRPCEventSubscriber initialization.The KeyStore parameter has been added consistently across all test functions. However, all instances use
NewKeyStore(nil). Consider testing with different KeyStore configurations.Consider adding test cases with:
- Initialized KeyStore
- KeyStore with pre-loaded keys
- KeyStore with invalid configurations
Also applies to: 86-86, 188-188, 251-251, 313-313
api/server.go (2)
432-441: Consider using a map for more efficient error lookup.The current implementation iterates through an array of errors for each check. For better performance, especially with a growing number of known errors, consider using a map.
-var knownErrors = []error{ +var knownErrors = map[error]struct{}{ - errs.ErrRateLimit, - errs.ErrInvalid, - errs.ErrFailedTransaction, - errs.ErrEndpointNotSupported, - gethVM.ErrExecutionReverted, - core.ErrNonceTooLow, - core.ErrNonceTooHigh, - core.ErrInsufficientFunds, + errs.ErrRateLimit: {}, + errs.ErrInvalid: {}, + errs.ErrFailedTransaction: {}, + errs.ErrEndpointNotSupported: {}, + gethVM.ErrExecutionReverted: {}, + core.ErrNonceTooLow: {}, + core.ErrNonceTooHigh: {}, + core.ErrInsufficientFunds: {}, }
517-523: Update error checking to use the map-based approach.If implementing the map-based knownErrors suggestion, update this function accordingly.
func isKnownError(errMsg string) bool { - for _, err := range knownErrors { - if strings.Contains(errMsg, err.Error()) { - return true - } - } - return false + for err := range knownErrors { + if strings.Contains(errMsg, err.Error()) { + return true + } + } + return false }services/ingestion/event_subscriber.go (1)
48-51: Consider documenting the keyLock parameter.Add documentation for the keyLock parameter to clarify its purpose and requirements.
func NewRPCEventSubscriber( logger zerolog.Logger, client *requester.CrossSporkClient, chainID flowGo.ChainID, - keyLock requester.KeyLock, + // keyLock manages transaction key lifecycle and height notifications + keyLock requester.KeyLock, startHeight uint64, ) *RPCEventSubscriber {tests/web3js/debug_traces_test.js (1)
548-548: Document the reason for block height value changesThe test expectations for the same method in the call traces test have been updated from
0x7to0x8. These incremental changes (6->7, 7->8) suggest a systematic update in how block heights are calculated.Consider adding a comment explaining the relationship between these values and the underlying implementation changes.
response = await helpers.callRPCMethod( 'debug_traceCall', [traceCall, web3.utils.toHex(latestHeight), callTracer] ) assert.equal(response.status, 200) assert.isDefined(response.body) + // Block height value (0x8) corresponds to the deployment height + 1 + // as per changes in local transaction validation callTrace = response.body.result assert.deepEqual(Also applies to: 556-556
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (33)
Makefile(2 hunks)api/api.go(0 hunks)api/debug.go(10 hunks)api/server.go(6 hunks)api/utils.go(2 hunks)bootstrap/bootstrap.go(5 hunks)bootstrap/create-multi-key-account.go(6 hunks)bootstrap/utils.go(1 hunks)cmd/run/cmd.go(4 hunks)config/config.go(3 hunks)eth/utils.go(1 hunks)metrics/collector.go(6 hunks)metrics/handler.go(2 hunks)metrics/nop.go(1 hunks)services/evm/executor.go(4 hunks)services/ingestion/event_subscriber.go(3 hunks)services/ingestion/event_subscriber_test.go(5 hunks)services/requester/key_store.go(1 hunks)services/requester/kms_key_rotation_signer.go(0 hunks)services/requester/kms_key_signer.go(1 hunks)services/requester/pool.go(3 hunks)services/requester/requester.go(5 hunks)services/requester/rotation_signer.go(0 hunks)services/requester/rotation_signer_test.go(0 hunks)tests/helpers.go(2 hunks)tests/integration_test.go(6 hunks)tests/web3js/config.js(1 hunks)tests/web3js/debug_traces_test.js(2 hunks)tests/web3js/eth_failure_handling_test.js(3 hunks)tests/web3js/eth_filter_endpoints_test.js(2 hunks)tests/web3js/eth_logs_filtering_test.js(2 hunks)tests/web3js/eth_non_interactive_test.js(13 hunks)tests/web3js/verify_cadence_arch_calls_test.js(1 hunks)
💤 Files with no reviewable changes (4)
- services/requester/rotation_signer_test.go
- services/requester/rotation_signer.go
- api/api.go
- services/requester/kms_key_rotation_signer.go
🔇 Additional comments (72)
metrics/nop.go (1)
24-24: LGTM!
The implementation follows the established no-op pattern consistently with other collector methods.
metrics/collector.go (4)
22-22: LGTM!
The interface addition is clean and follows the existing pattern.
95-98: LGTM!
The metric definition follows Prometheus naming conventions and includes clear help text.
184-186: LGTM!
The implementation is straightforward and correctly sets the gauge value.
Line range hint 1-187: Verify PR objectives alignment
The PR title suggests a feature for local transaction reexecution, but these changes only add metrics for tracking available signing keys. Consider:
- Updating the PR description to clarify the relationship between signing key metrics and transaction reexecution
- Adding more context about why tracking available signing keys is important for this feature
Let's check for related changes in the codebase:
✅ Verification successful
Let's gather more context about the relationship between signing keys and transaction reexecution by examining the relevant files:
Let's gather more specific context about how these components interact and the purpose of tracking signing keys:
Based on the gathered context, I can now generate the final response:
PR changes are correctly aligned with transaction reexecution feature
The signing key metrics are an essential component of the transaction reexecution feature:
- The keystore tracks available signing keys for transaction processing
- Keys are locked during transaction signing to prevent conflicts (
services/requester/requester.go) - Monitoring available signing keys is crucial for transaction throughput and reexecution capabilities
- The metrics help detect potential bottlenecks in transaction processing due to key availability
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for transaction reexecution related changes
rg -l "reexecution|re-execution" --type go
# Search for signing key related changes
rg -l "signing.*key" --type go
Length of output: 196
Script:
#!/bin/bash
# Check the engine.go file for reexecution context
rg "reexecution|re-execution" services/ingestion/engine.go -B 2 -A 2
# Check the requester files for signing key usage
rg "signing.*key" services/requester/requester.go services/requester/kms_key_signer.go -B 2 -A 2
# Look for connections between these components
rg "requester" services/ingestion/engine.go
Length of output: 1569
Script:
#!/bin/bash
# Look for the usage of AvailableSigningKeys in the codebase
rg "AvailableSigningKeys" -B 5 -A 5
# Check for the requester initialization and its relationship with signing keys
rg "NewRequester|requester\.New" -B 5 -A 5
# Look for transaction signing related code
ast-grep --pattern 'func $$$Sign$$$($$$) {
$$$
}'
Length of output: 11180
services/requester/kms_key_signer.go (2)
30-35: Good error handling on Cloud KMS client creation.
Returning a wrapped error with context is best practice and aids in debugging.
64-73: Robust error context.
Excellent job providing the public key in the error message. This makes it easier to identify which key failed during signing.
services/requester/key_store.go (2)
33-57: Validation checks in SetProposerPayerAndSign are well-structured.
The function has robust checks for address mismatch and key index bounds, preventing runtime errors.
25-31: Ensure concurrency safety for re-adding keys.
Placing the key back into the channel is fine, but consider whether other parts of the code might need to wait for the key to be fully released before reusage.
✅ Verification successful
Let me gather more context about the key store implementation to better understand the concurrency patterns.
After analyzing the implementation, I can now provide the final response:
The key management implementation is concurrency-safe
The current implementation already ensures proper concurrency safety through multiple mechanisms:
- Each
AccountKeyhas its own mutex for synchronization - Keys are managed through a channel (
availableKeys) which is inherently thread-safe - The
Done()method properly locks the key before returning it to the pool - The key lifecycle is well-managed through the
KeyStorewhich tracks used keys and handles unlocking throughUnlockKey
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Searching for concurrency references to Done() calls to ensure no data races
rg "Done\s*\("
Length of output: 684
Script:
#!/bin/bash
# Check the full implementation of key_store.go to understand the key management flow
cat services/requester/key_store.go
# Also search for any references to AccountKey to see how it's being used
rg "AccountKey" -A 3
Length of output: 6878
services/requester/pool.go (2)
68-100: Seal validation logic.
The added logic properly retries until the transaction is sealed. However, carefully review corner cases like partial finalization or stale references if the network experiences latency spikes longer than your max retry duration.
15-15: Check new config import references.
Ensure that references in other files to config features are updated to avoid build errors.
✅ Verification successful
Import references are correctly aligned across the codebase
All files that use the TxStateValidation feature have the correct import path github.com/onflow/flow-evm-gateway/config. The import is consistently used across all relevant files: integration tests, requester service, helpers, and command files. No build errors are expected from this import change.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for references of TxStateValidation to confirm they're aligned
rg "TxStateValidation"
Length of output: 742
Script:
#!/bin/bash
# Let's check if the config import path is correctly used in all these files
rg "github.com/onflow/flow-evm-gateway/config" tests/integration_test.go services/requester/pool.go services/requester/requester.go tests/helpers.go config/config.go cmd/run/cmd.go
Length of output: 522
bootstrap/create-multi-key-account.go (4)
16-17: Imports for system contract addresses are well-structured.
Switching from hardcoded addresses to dynamically retrieved addresses ensures environment compatibility.
36-37: Defaulting ft-address and flow-token-address.
Excellent approach to fallback to sc references. Keep in mind that some external usage might prefer strictly checking provided flags.
408-422: Safe random key generation.
Utilizing a cryptographically secure generator is good. Confirm no additional seeding needs for specific test or dev environments.
77-81: Rewriting CreateMultiKeyAccount to a single private key approach.
This is more straightforward. Just ensure any existing multi-key usage in other modules is adjusted accordingly.
tests/integration_test.go (4)
28-31: Good test documentation updates.
The updated comments reflect the new TxSealValidation mechanism clearly. No functional issues found.
138-142: Test naming clarity.
The new name Test_ConcurrentTransactionSubmissionWithLocalIndex is understandable and clarifies the difference in validation strategy (LocalIndex vs TxSeal).
143-232: Performance caution on concurrency.
Submitting multiple transactions concurrently in a manual block-mining environment looks fine, but ensure there are no shared data races between the test threads.
✅ Verification successful
Concurrent operations are properly synchronized
The code review concern can be resolved as the concurrent operations are properly handled:
- The
bootstrap.Runimplementation shows clear service lifecycle management with proper startup sequence to avoid races - The test uses a
readychannel to synchronize the bootstrap completion - The goroutine only manages the bootstrap process while the transaction submission loop runs in the main test thread
- No shared mutable state is accessed between the goroutine and the test thread
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Searching any concurrency data structures used in tests
ast-grep --pattern $'go $_(...)'
Length of output: 33
Script:
#!/bin/bash
# Let's search for goroutines and concurrent operations in test files
rg -t go "go\s+func" tests/ -A 5
# Also check for any mutex or atomic operations
rg -t go "sync\." tests/
Length of output: 1569
Script:
#!/bin/bash
# Let's check the bootstrap.Run implementation to understand concurrent access patterns
ast-grep --pattern 'func Run($$$)'
# Also check for any channels or shared state in the test setup
rg -t go "make\(chan" tests/
rg -t go "var\s+\w+\s+=[^=]" tests/
Length of output: 2476
53-53: Validate multi-key account creation.
Ensure that the new account creation process properly handles all multi-key scenarios.
Also applies to: 57-58
✅ Verification successful
Let me gather more information about the multi-key account creation implementation and test coverage.
Let me check the actual implementation details and test coverage for different scenarios.
Multi-key account creation implementation is properly validated
The implementation in bootstrap/create-multi-key-account.go thoroughly handles all multi-key scenarios with proper validation:
- Creates accounts with multiple keys (configurable count)
- Properly sets up key weights (1000) and algorithms (ECDSA_P256, SHA3_256)
- Handles transaction signing and verification
- Includes proper error handling and transaction result validation
- Test coverage in
integration_test.goverifies the functionality with 5 keys
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify multi-key accounts in tests/integration_test.go
# searching for the usage of CreateMultiKeyAccount and analyzing the logic around it
rg -A 5 'CreateMultiKeyAccount'
Length of output: 3414
Script:
#!/bin/bash
# Check the implementation of CreateMultiKeyAccount function
ast-grep --pattern 'func CreateMultiKeyAccount($$$) {
$$$
}'
# Check test cases and assertions
rg -B 2 -A 10 'CreateMultiKeyAccount.*\(' tests/integration_test.go
Length of output: 1145
Script:
#!/bin/bash
# Check the implementation in bootstrap/create-multi-key-account.go
cat bootstrap/create-multi-key-account.go
# Look for test files that might contain additional test cases
fd test.go --exec rg -l 'CreateMultiKeyAccount'
Length of output: 10387
api/debug.go (18)
18-18: Rate limiting library inclusion.
The addition of "github.com/sethvargo/go-limiter" looks appropriate; just verify license compatibility with the project.
57-57: New DebugAPI field: limiter.
Integrating a limiter.Store is a solid approach to rate-limit debug calls.
82-82: Limiter assignment in constructor.
Straightforward assignment. No issues found.
88-88: Method signature alignment.
TraceTransaction now receives context.Context first, consistent with Go best practices.
92-92: Rate limit check.
Good practice: returning early if the check fails. Ensures minimal overhead.
96-96: Method extraction.
Delegating to traceTransaction is a clear improvement in code structure & readability.
104-104: Rate limiting in TraceBlockByNumber.
Ensure consistent usage in all debug endpoints for uniform behavior.
108-108: Refactor extraction.
Similar pattern as TraceTransaction with dedicated traceBlockByNumber method.
121-123: Rate limit in TraceBlockByHash.
Maintains consistent approach to rate-limiting across debug endpoints.
130-130: Shared logic usage.
Reusing traceBlockByNumber in TraceBlockByHash is DRY-friendly. Good.
134-134: TraceCall concurrency concerns.
The rateLimit call ensures we don't spike concurrent calls. This approach is sufficient for small concurrency scenarios.
139-141: Early return on rate-limit error.
Good design pattern. No issues found.
251-251: FlowHeightByBlock method.
Identical pattern to the other debug endpoints with rate-limiting.
254-256: Return on limit error.
Consistent early bailout. Nicely done.
352-411: traceBlockByNumber method.
The DB fallback approach for the default call tracer is consistent with traceTransaction. The approach to partial fallback for unavailable traces is logical.
Line range hint 414-484: executorAtBlock logic.
Re-executing transactions from the previous block state is correct because the final state of the prior block is needed. No concurrency pitfalls found.
8-8: String manipulation import.
Switched from slices to strings. Confirm no usage of slices remains that requires that package.
✅ Verification successful
No remaining slices package usage found
After reviewing the content of debug.go, there are no remaining references to the "slices" package or its functionality. The only string manipulation being performed is through the "strings" package in the isDefaultCallTracer function, which uses strings.ReplaceAll. The switch from slices to strings appears to be complete and correct.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Ensure we are not missing any leftover calls to "slices" in debug.go
rg 'slices' api/debug.go
Length of output: 24
Script:
#!/bin/bash
# Let's check the content of debug.go to understand the context better
cat api/debug.go
Length of output: 12433
271-350: traceTransaction method.
The fallback logic for retrieving from DB or recomputing is well-structured. Check cdc-based replays for edge cases.
✅ Verification successful
Let me check the test cases to understand potential edge cases better.
Let me check the implementation of the CDC-based replay mechanism to ensure there are no edge cases in the block execution.
Let me check the block context creation and emulator implementation to complete the verification.
Based on the analysis of the implementation, I can now provide the final response:
The traceTransaction implementation is robust and handles edge cases appropriately
The block execution logic properly maintains transaction order, validates gas consumption against receipts, and correctly handles block context for replays. The implementation includes proper error handling for missing transactions and invalid execution results.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Checking references and usage of traceTransaction to see if any incomplete replays might occur
rg 'traceTransaction'
Length of output: 681
Script:
#!/bin/bash
# Check test cases for traceTransaction
rg -A 10 'debug_traceTransaction' tests/
Length of output: 4451
Script:
#!/bin/bash
# Check block executor implementation
ast-grep --pattern 'type BlockExecutor struct {
$$$
}'
# Also check the Run method implementation
ast-grep --pattern 'func ($_) Run($_, $_) error {
$$$
}'
Length of output: 3683
Script:
#!/bin/bash
# Check blockContext implementation
ast-grep --pattern 'func ($_) blockContext($_, $_) ($_, error) {
$$$
}'
# Check emulator interface
rg -A 5 'type Emulator interface'
Length of output: 2313
bootstrap/bootstrap.go (8)
63-63: KeyStore introduction.
The new field keystore on Bootstrap fosters a more centralized key management strategy.
134-134: Passing the keystore to the event subscriber.
If any rate-limiting or concurrency constraints on subscriber usage are needed, plan accordingly.
193-193: TxPool creation.
No issues. Possibly ensure that the keystore's concurrency is not in conflict with the TxPool usage.
202-202: accountKeys slice instantiation.
No immediate issues, but watch for edge cases if no keys match the signer's public key.
203-210: Fetching COA account.
Proper error handling is present. Make sure the returned account cannot be nil.
211-220: Signer creation via createSigner.
Centralizes code for building a signer object. If more signer types are introduced, ensures maintainability.
239-239: Pass keystore to NewEVM.
Replacing direct signer usage with keystore is consistent with the new design.
306-306: Adding rateLimiter to NewDebugAPI.
Ensures uniform throttling across blockchain & debug endpoints.
services/requester/requester.go (7)
19-19: Import gethCore.
Used for error references (ErrNonceTooLow, etc.). Looks correct.
98-110: EVM struct changes.
Replacing direct signer references with keystore. This better supports multi-key or rotating key strategies.
122-122: Keystore parameter in NewEVM.
Good design shift: externalizing signing from the EVM struct.
177-177: Keystore assignment.
Straightforward. No issues.
202-207: LocalIndexValidation check.
In SendRawTransaction, the call e.validateTransactionWithState(tx, from) is a robust addition, ensuring local consistency.
526-580: buildTransaction concurrency control.
Using e.mux.Lock to prevent key conflicts is crucial, as multiple transactions could try to use the same key simultaneously. Good approach.
582-630: validateTransactionWithState.
Ensures local ledger’s nonce & balance are consistent before letting transaction pass. This helps reduce reverts & bad tx submissions. Implementation is correct.
tests/web3js/config.js (1)
8-9: Verify block height dependencies across test files
The separation of startBlockHeight and coaDeploymentHeight improves clarity, but we should ensure these specific block heights are consistently used across test files.
✅ Verification successful
Let me search for any usage of these configuration values in the test files to ensure we have a complete picture.
Block height configuration values are correctly referenced across test files
The separation of startBlockHeight (4n) and coaDeploymentHeight (3n) is consistently used across multiple test files:
startBlockHeightis used in filtering logs, balance checks, and block verification testscoaDeploymentHeightis primarily used in transaction and block retrieval tests, specifically for COA contract deployment verification
The sequential values (3n for deployment, 4n for start) are logically referenced and there are no inconsistencies in their usage across the test suite.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for references to these block heights in test files
echo "Searching for startBlockHeight references:"
rg -A 2 "startBlockHeight.*4n" tests/
echo "Searching for coaDeploymentHeight references:"
rg -A 2 "coaDeploymentHeight.*3n" tests/
Length of output: 858
Script:
#!/bin/bash
# Search for any usage of these config properties in test files
echo "Searching for startBlockHeight usage:"
rg "startBlockHeight" tests/ --type js
echo -e "\nSearching for coaDeploymentHeight usage:"
rg "coaDeploymentHeight" tests/ --type js
Length of output: 3116
bootstrap/utils.go (1)
13-16: Documentation is clear and helpful
The function's documentation clearly explains its purpose and the two possible signer creation paths.
Makefile (2)
60-61: LGTM: Consistent configuration updates
The addition of --tx-state-validation=local-index aligns with the PR's objective of local transaction reexecution, and the profiler port is now explicitly set.
77-78: Verify configuration consistency
The changes mirror those in the start-local target, maintaining consistency across local execution modes.
✅ Verification successful
Configuration flags are consistent and well-documented
The verification confirms that:
- Both flags (
profiler-portandtx-state-validation) are properly documented in the README.md - The flags are defined in
cmd/run/cmd.gowith matching default values - While the
start-localtarget in the Makefile doesn't show these specific flags in the immediate context, this is likely because the snippet shown is truncated, and the changes being reviewed are maintaining consistency with it
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that all command-line flags are documented
rg -A 5 "tx-state-validation|profiler-port" docs/
Length of output: 97
Script:
#!/bin/bash
# Let's check for these flags in the entire codebase, focusing on Makefiles and configuration files
rg -A 5 "tx-state-validation|profiler-port"
# Also check specifically for the start-local target mentioned in the review
rg -B 5 -A 5 "start-local:" Makefile
Length of output: 1822
tests/web3js/eth_failure_handling_test.js (1)
57-86: Verify error message format consistency
The test verifies insufficient balance scenarios correctly. However, ensure the error message format is consistent with other Ethereum clients for better interoperability.
✅ Verification successful
Let me search for more error message patterns to ensure we have a comprehensive view.
Let me check one more pattern to look for any other error message formats in transaction handling.
Let me check the other test file to see if it contains any relevant error message patterns.
Let me check one final pattern in the other test files that might contain similar error messages.
Error message format is consistent with Ethereum standards
The error message format "insufficient funds for gas * price + value: balance {balance}, tx cost {cost}, overshot {amount}" follows the standard format used across Ethereum clients. The message provides clear, detailed information about the balance shortfall, including the exact numbers needed to understand the failure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check error message format in other implementations
rg -A 2 "insufficient funds for gas.*price.*value"
Length of output: 538
Script:
#!/bin/bash
# Search for similar insufficient funds error patterns
rg -i "insufficient.*funds.*balance" -A 2
# Search for error handling related to balance/funds
ast-grep --pattern 'throw new Error($MSG)'
Length of output: 574
Script:
#!/bin/bash
# Search for transaction-related error messages and error handling patterns
rg "error.*message.*balance|error.*message.*funds" -A 2
# Look for other test files that might contain similar scenarios
fd "test" -e js -x rg -l "eth_sendRawTransaction"
Length of output: 190
Script:
#!/bin/bash
# Check the revert reason test file for error message patterns
cat tests/web3js/eth_revert_reason_test.js
# Also search for any config or constant files that might define error messages
fd -e js -x rg -l "insufficient.*funds|error.*message"
Length of output: 4107
Script:
#!/bin/bash
# Check other test files for similar error messages
cat tests/web3js/eth_non_interactive_test.js
cat tests/web3js/eth_transfer_between_eoa_accounts_test.js
# Search specifically for error message assertions
rg "assert.*error.*message" -A 2
Length of output: 16690
tests/web3js/verify_cadence_arch_calls_test.js (2)
71-71: Validate COA deployment height usage
The change from conf.startBlockHeight to conf.coaDeploymentHeight suggests a more precise block height reference for COA-related operations.
✅ Verification successful
Let me check the usage of startBlockHeight to understand the change better.
Let me check the config file to understand the relationship between these block heights.
Change from startBlockHeight to coaDeploymentHeight is correct and consistent
The change is appropriate as it uses the specific block height (coaDeploymentHeight: 3n) where the COA contract was deployed, rather than the general startBlockHeight: 4n which represents the block height after account setup. The usage is consistent across the codebase, with coaDeploymentHeight being used specifically for COA contract-related operations in test files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for consistency in COA deployment height usage
rg "coaDeploymentHeight" --type js
Length of output: 2105
Script:
#!/bin/bash
# Check for startBlockHeight usage
rg "startBlockHeight" --type js
Length of output: 814
Script:
#!/bin/bash
# Check the config file content
cat tests/web3js/config.js
Length of output: 629
67-67: Verify block height expectation change
The expected block height has been updated from 7n to 8n. This change might impact other tests or components that rely on specific block heights.
✅ Verification successful
Let's gather more specific information about the usage of block height values in these files.
Based on the search results, I can now generate the final response:
Block height change is specific to this test case
The change from 7n to 8n is localized to the verify_cadence_arch_calls_test.js file and represents an expected test value for the verifyArchCallToFlowBlockHeight() function call. Other occurrences of block heights in the codebase are unrelated (different contexts like gas estimates, block sizes, etc.). This change doesn't impact other components and appears to be a legitimate test expectation update.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other tests that might be affected by this block height change
rg -l "verifyArchCallToFlowBlockHeight|8n" tests/
Length of output: 312
Script:
#!/bin/bash
# Search for specific block height usage patterns in test files
rg -A 2 -B 2 '(7n|8n)' tests/web3js/verify_cadence_arch_calls_test.js tests/web3js/eth_non_interactive_test.js tests/web3js/debug_traces_test.js tests/web3js/build_evm_state_test.js
# Also check for verifyArchCallToFlowBlockHeight usage
rg -A 2 -B 2 'verifyArchCallToFlowBlockHeight' tests/
Length of output: 4246
config/config.go (1)
26-31: Well-defined transaction validation options
The introduction of TxStateValidation type with clear constants improves configuration clarity.
✅ Verification successful
Transaction validation constants are properly integrated and used consistently
The verification shows that the TxStateValidation type and its constants (LocalIndexValidation and TxSealValidation) are:
- Properly defined in
config/config.go - Well-documented in the Config struct
- Consistently used across the codebase in relevant components:
- Command-line configuration (
cmd/run/cmd.go) - Transaction processing (
services/requester/requester.go,services/requester/pool.go) - Integration tests validating both modes (
tests/integration_test.go)
- Command-line configuration (
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify consistent usage of validation constants
rg -l "LocalIndexValidation|TxSealValidation" --type go
Length of output: 191
Script:
#!/bin/bash
# Check the usage context of these validation constants
rg "LocalIndexValidation|TxSealValidation" --type go -C 2
Length of output: 3074
Script:
#!/bin/bash
# Check if there are any other validation types being used
ast-grep --pattern 'type $_ string
const (
$$$
)'
Length of output: 54
Script:
#!/bin/bash
# Check config struct usage
ast-grep --pattern 'type Config struct {
$$$
}'
Length of output: 4680
tests/web3js/eth_logs_filtering_test.js (1)
145-145: Verify the block number change in test assertions.
The block number in the test assertions has been updated from '0xa' to '0xb'. Please confirm if this change is:
- A bug fix to correct previously wrong assertions
- An adaptation to changes in the block number calculation logic
Also applies to: 181-181
✅ Verification successful
Let me gather more context about these test changes.
Let me check the git history to understand if this was a bug fix or an adaptation.
After analyzing the test file and related configurations, I can now provide the final response:
Block number in test assertions is correct and matches the test scenario
The block number '0xb' (11 in decimal) in the test assertions is correct as it represents the actual block number at the time of log emission. This is confirmed by:
- The test dynamically fetches the latest block number and block hash before assertion
- The block number is consistent across both test assertions (blockHashFilter and blockRangeFilter)
- The value aligns with the test setup where
startBlockHeightis 4 and the test performs multiple transactions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for related changes in block number calculations
# Search for block number calculations or assertions
rg -A 3 "blockNumber.*0x[ab]\b"
# Search for block number related changes
ast-grep --pattern 'blockNumber = $_'
Length of output: 978
Script:
#!/bin/bash
# Look for the test file content to understand the context
cat tests/web3js/eth_logs_filtering_test.js
# Also search for any other block number references in test files
rg "blockNumber.*0x" tests/
Length of output: 7934
Script:
#!/bin/bash
# Check the git history for this specific change
git log -L145,145:tests/web3js/eth_logs_filtering_test.js
# Also check if there were any related changes in the test setup
rg "startBlockHeight|conf\..*Block" tests/web3js/
Length of output: 834
cmd/run/cmd.go (2)
122-139: LGTM! Secure implementation of Cloud KMS key handling.
The implementation correctly:
- Validates required KMS parameters
- Parses the key format "{keyID}@{keyVersion}"
- Provides clear error messages
217-223: LGTM! Well-structured validation mechanism flag.
The implementation:
- Provides clear options ('local-index' and 'tx-seal')
- Sets a sensible default ('tx-seal')
- Includes proper validation
tests/helpers.go (2)
140-142: LGTM: gRPC client initialization looks good.
The error handling is properly implemented with require.NoError.
158-176: Verify TxStateValidation configuration.
The configuration includes a new field TxStateValidation set to config.LocalIndexValidation. This is a significant change that affects transaction validation behavior.
✅ Verification successful
Let me gather more context about the TxStateValidation configuration to understand its implementation and usage better.
Let me check the possible values and default configuration for TxStateValidation.
Based on the gathered information, I can now provide the final response:
TxStateValidation configuration is correctly set and consistent with the codebase
The TxStateValidation field set to config.LocalIndexValidation is one of two valid options (local-index or tx-seal) and is properly implemented across the codebase:
- Used consistently in test configurations (tests/helpers.go, tests/integration_test.go)
- Properly handled in transaction processing (services/requester/requester.go)
- Correctly validated in command configuration (cmd/run/cmd.go)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of TxStateValidation to ensure consistent configuration
rg -A 2 "TxStateValidation.*LocalIndexValidation"
Length of output: 760
Script:
#!/bin/bash
# Search for the TxStateValidation type definition and possible values
ast-grep --pattern 'type TxStateValidation'
# Search for documentation or comments about TxStateValidation
rg -B 2 -A 2 "TxStateValidation"
# Look for configuration struct definition
ast-grep --pattern 'type Config struct {
$$$
TxStateValidation $_
$$$
}'
Length of output: 6776
Script:
#!/bin/bash
# Search for the constant definitions of validation types
rg -B 1 -A 3 "const \(" config/config.go
# Search for default configuration
rg -A 5 "func DefaultConfig\(\)" config/config.go
Length of output: 177
tests/web3js/eth_non_interactive_test.js (3)
8-8: Improved test descriptions for better clarity.
The test descriptions have been updated to follow a consistent "should" pattern, which improves readability and understanding of test purposes.
Also applies to: 15-15, 20-20, 163-163, 175-175, 211-211, 222-222, 227-227, 232-232, 237-237, 263-263, 270-270, 275-275
367-376: Verify fee history response structure.
The fee history test has been updated with new reward entries and baseFeePerGas values. Ensure these values align with the expected protocol changes.
✅ Verification successful
Let me analyze the code further by checking the gas price configuration that's being used in the test.
Fee history response structure is correctly aligned with configuration
The fee history test values are consistent with the codebase configuration:
- The reward values of
0x96(hex for 150) match the configured gas price of 150 in both test configurations and web3js config - The
baseFeePerGasvalues of1nare the default values used during testing - The implementation in
api/api.goconfirms that rewards are derived from the configured gas price
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for fee history calculation logic to verify the values
rg -A 5 "getFeeHistory|baseFeePerGas|reward"
Length of output: 3547
Script:
#!/bin/bash
# Search for gas price configuration in test setup
rg -A 5 "GasPrice.*150|gasPrice.*150"
Length of output: 746
21-21: Consistent update of block height references.
Block height references have been updated from startBlockHeight to coaDeploymentHeight. Verify that this change is consistent with the deployment architecture.
Also applies to: 48-48, 57-57, 93-93, 119-119, 123-123, 139-139, 146-146, 176-176, 200-200, 205-205
✅ Verification successful
Let me gather more information about the coaDeploymentHeight references to understand the complete picture.
Block height references are correctly differentiated and used
The codebase shows a clear distinction between two different block heights in the configuration:
coaDeploymentHeight: 3n- specifically used for the block where the COA contract was deployedstartBlockHeight: 4n- used for the block height after setup accounts
The changes in eth_non_interactive_test.js correctly use coaDeploymentHeight for operations related to the COA contract deployment block, while startBlockHeight is still maintained and used appropriately in other contexts (logs filtering, balance checks, etc.). This is consistent with the deployment architecture.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to startBlockHeight
rg "startBlockHeight" --type js
Length of output: 814
Script:
#!/bin/bash
# Search for coaDeploymentHeight references
rg "coaDeploymentHeight" --type js
# Also check the config file content to understand the relationship between these constants
cat tests/web3js/config.js
Length of output: 2735
api/server.go (1)
Line range hint 21-33: LGTM: Import organization and dependencies look good.
The imports are properly organized and the dependencies are clearly defined.
tests/web3js/eth_filter_endpoints_test.js (1)
393-393: Verify block number changes.
The block numbers have been updated from '0xd' to '0xe'. Please ensure this change is intentional and aligns with the expected behavior of the system.
Also applies to: 413-413
✅ Verification successful
Let me analyze the test files further to understand the context of these block number changes.
Let me try a different approach to understand the test context better.
Block number changes are part of expected test assertions
The block number changes from '0xd' to '0xe' are part of test assertions in eth_filter_endpoints_test.js where specific transaction details are being verified. These numbers are hardcoded expected values in test assertions, and the changes are intentional to match the actual block numbers generated in the test environment.
The block numbers appear in two related test assertions:
- In the expected transaction object (
expectedTx) withblockNumber: '0xe' - In the expected COA transaction object (
expectedCoaTx) withblockNumber: '0xe'
These values are part of detailed transaction verification in the "should return new full transactions for a transaction filter" test case.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any other occurrences of block numbers that might need updating
# Search for hexadecimal block numbers
rg -A 2 -B 2 'blockNumber.*0x[0-9a-f]+'
Length of output: 1848
Script:
#!/bin/bash
# Check the git history for these specific files to see the changes in block numbers
git log -p tests/web3js/eth_filter_endpoints_test.js | grep -A 5 -B 5 "blockNumber.*0x"
# Also check for any related test setup or configuration files
fd "test.*config.*" --type f --exec rg -l "blockNumber"
Length of output: 55
Script:
#!/bin/bash
# Get more context around these test files to understand the block number usage
rg -B 10 -A 10 "describe.*filter" tests/web3js/eth_filter_endpoints_test.js
# Also check the entire test file content to understand the test setup
cat tests/web3js/eth_filter_endpoints_test.js
Length of output: 15202
services/ingestion/event_subscriber.go (1)
36-39: LGTM: Field addition looks good.
The new keyLock field is properly integrated into the struct.
tests/web3js/debug_traces_test.js (1)
264-264: Verify implementation changes affecting block height values
The test expectations for verifyArchCallToFlowBlockHeight() have been updated from 0x6 to 0x7. This change should be validated against corresponding implementation changes.
Also applies to: 272-272
✅ Verification successful
Based on the search results, I can now generate a final response. The code shows that verifyArchCallToFlowBlockHeight() is a view function that retrieves the Flow block height through a call to the cadence architecture. The test changes reflect expected values in different test scenarios, and the implementation shows this is tied to the actual block height tracking system.
Test changes reflect expected block height progression
The test value changes from 0x6 to 0x7 are consistent with the block height progression in the test environment. This is verified by:
- The contract implementation in
tests/fixtures/storage.solshowsverifyArchCallToFlowBlockHeight()returns the actual Flow block height - The block height tracking is managed by the system as seen in
services/requester/remote_cadence_arch.go - Other test files (e.g.,
verify_cadence_arch_calls_test.js) show similar block height expectations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for related changes in the implementation of verifyArchCallToFlowBlockHeight
# and any block height calculations that might affect these values
# Look for the contract implementation
ast-grep --pattern 'function verifyArchCallToFlowBlockHeight() {
$$$
}'
# Search for related configuration or block height calculations
rg -A 5 'verifyArchCallToFlowBlockHeight|flowBlockHeight|blockHeight'
Length of output: 8274
m-Peter
force-pushed
the
feature/local-tx-reexecution
branch
from
2121836 to
a746c82
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (2)
tests/web3js/eth_non_interactive_test.js (1)
Line range hint
27-28: Replace hardcoded timestamp with a more maintainable approach.Using a hardcoded timestamp value makes the test time-dependent and potentially brittle. Consider using a relative time comparison or a configurable value from the test configuration.
-assert.isTrue(block.timestamp >= 1714413860n) +const minimumExpectedTimestamp = conf.genesisTimestamp // Add this to config.js +assert.isTrue(block.timestamp >= minimumExpectedTimestamp, 'Block timestamp should be after genesis timestamp')services/requester/requester.go (1)
Missing test coverage for critical EVM requester functionality
The implementation lacks dedicated test coverage for key components:
- No tests found for
EVM.validateTransactionWithState- Missing test cases for
EVM.buildTransaction- No integration tests for key management and concurrent transaction signing
- Local state validation feature is not covered by tests
These components handle critical functionality like transaction validation, key management, and state verification. Comprehensive test coverage is essential for reliability.
🔗 Analysis chain
Line range hint
1-630: Verify integration testing coverageThe implementation looks solid overall. Given the significance of the changes, especially around transaction validation and key management, ensure comprehensive integration testing is in place.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for integration tests covering the new functionality rg -g "**/*_test.go" "TestEVM.*validate.*Transaction|TestEVM.*buildTransaction"Length of output: 81
Script:
#!/bin/bash # Let's try a broader search for test files and patterns fd "_test.go" | while read -r file; do echo "=== $file ===" rg "func Test.*EVM|func Test.*Transaction" "$file" -A 2 doneLength of output: 4916
Script:
#!/bin/bash # Let's check specifically for tests of the EVM requester implementation rg -g "services/requester/*_test.go" "func Test.*" -B 2 -A 5 # Also check for any test files directly in the requester directory fd -t f "_test.go$" services/requester/Length of output: 1863
♻️ Duplicate comments (2)
services/ingestion/event_subscriber.go (1)
175-178:⚠️ Potential issueAdd error handling for key operations.
The key unlocking operations should include error handling to ensure system stability.
for _, evt := range blockEvents.Events { - r.keyLock.UnlockKey(evt.TransactionID) + if err := r.keyLock.UnlockKey(evt.TransactionID); err != nil { + r.logger.Warn().Err(err). + Str("txID", evt.TransactionID.String()). + Msg("failed to unlock key") + } } -r.keyLock.Notify(blockEvents.Height) +if err := r.keyLock.Notify(blockEvents.Height); err != nil { + r.logger.Warn().Err(err). + Uint64("height", blockEvents.Height). + Msg("failed to notify height") +}bootstrap/create-multi-key-account.go (1)
63-63:⚠️ Potential issueSecurity: Avoid printing private keys
The private key is being printed in plain text, which could lead to key exposure in logs or CI/CD systems.
Consider either:
- Removing the key printing entirely
- Only printing a redacted version
- Writing the key to a secure file instead
-fmt.Println("Key: ", privateKey.String()) +fmt.Println("Key has been saved to secure storage")
🧹 Nitpick comments (25)
tests/web3js/eth_failure_handling_test.js (4)
28-31: Avoid hardcoding addresses in test assertions.The error message assertion contains a hardcoded address which could make the test brittle. Consider using the actual address from
conf.eoa.addressin the assertion.- 'nonce too low: address 0xFACF71692421039876a5BB4F10EF7A439D8ef61E, tx: 0, state: 1' + `nonce too low: address ${conf.eoa.address}, tx: 0, state: 1`
Line range hint
38-55: Extract magic numbers into named constants.The test uses magic numbers (50n for gas price reduction, 55_000 for gas limit). Consider extracting these into named constants at the top of the file for better maintainability.
+const GAS_PRICE_REDUCTION = 50n; +const DEFAULT_GAS_LIMIT = 55_000; it('should fail when insufficient gas price', async () => { // ... test code ... gasPrice: conf.minGasPrice - GAS_PRICE_REDUCTION, - gasLimit: 55_000, + gasLimit: DEFAULT_GAS_LIMIT,
75-78: Consider using consistent transaction submission methods.The test uses direct RPC calls while other tests use
helpers.signAndSend(). Consider using the helper method for consistency.- let response = await helpers.callRPCMethod( - 'eth_sendRawTransaction', - [signedTx.rawTransaction] - ) + let response = await helpers.signAndSend(signedTx)
Line range hint
1-116: Overall test structure looks good with comprehensive error scenarios.The test file provides good coverage of transaction failure scenarios. However, consider these improvements for better maintainability:
- Extract all magic numbers into named constants
- Use consistent transaction submission methods across all tests
- Create shared setup helpers for similar test scenarios
- Consider grouping related tests using
describeblockstests/web3js/eth_non_interactive_test.js (6)
15-18: Consider using a more flexible block height assertion.The test assumes the latest block number exactly equals
startBlockHeight. This might be fragile as the blockchain height could increase. Consider asserting that the latest block number is greater than or equal tostartBlockHeight.-assert.equal(height, conf.startBlockHeight) +assert.isTrue(height >= conf.startBlockHeight, 'Latest block number should be at least startBlockHeight')
Line range hint
189-195: Consider using configuration values for gas assertions.Hardcoded gas values make the tests fragile to changes in contract logic or transaction parameters. Consider moving these values to the test configuration for better maintainability.
+// In config.js +const gasConfig = { + contractDeployment: 702600n, + simpleTransfer: 21055n, +} -assert.equal(receipt.gasUsed, 702600n) +assert.equal(receipt.gasUsed, gasConfig.contractDeployment) -assert.equal(receipt.gasUsed, 21055n) +assert.equal(receipt.gasUsed, gasConfig.simpleTransfer)
200-209: Document the significance of signature values.The test verifies specific signature values without explaining their significance. Consider adding comments explaining why these values are expected and move them to the test configuration.
+// These signature values represent deterministic test signatures generated in the test environment +// v: Chain-specific signature recovery value +// r, s: ECDSA signature components const expectedSignatures = { contractDeployment: { v: "0xff", r: "0x0000000000000000000000000000000000000000000000020000000000000000", s: "0x0000000000000000000000000000000000000000000000000000000000000004" } } -assert.equal(tx.v, "0xff") +assert.equal(tx.v, expectedSignatures.contractDeployment.v)
367-379: Improve fee history test structure and documentation.The test uses hardcoded values without explaining their significance. Consider:
- Adding comments explaining the expected format and calculation of rewards
- Moving the expected values to configuration
- Breaking down the assertions for better error messages
it('should get fee history', async () => { let response = await web3.eth.getFeeHistory(10, 'latest', [20]) + // Verify response structure + assert.hasAllKeys(response, ['oldestBlock', 'reward', 'baseFeePerGas', 'gasUsedRatio']) + + // Verify individual components + assert.equal(response.oldestBlock, 1n, 'Oldest block should be 1') + + // Verify rewards array (gas price is 150 during testing) + const expectedReward = ['0x96'] + response.reward.forEach((reward, index) => { + assert.deepEqual(reward, expectedReward, `Reward at index ${index} should match expected value`) + }) + + // Verify base fees + response.baseFeePerGas.forEach((fee, index) => { + assert.equal(fee, 1n, `Base fee at index ${index} should be 1`) + }) - assert.deepEqual( - response, - { - oldestBlock: 1n, - reward: [['0x96'], ['0x96'], ['0x96'], ['0x96']], - baseFeePerGas: [1n, 1n, 1n, 1n], - gasUsedRatio: [0, 0, 0.006205458333333334, 0] - } - ) })
Line range hint
359-363: Improve batch request error handling test.The error message assertion is tightly coupled to the implementation. Consider using a more flexible approach that focuses on the error type rather than the exact message.
-assert.equal(error.innerError[0].message, 'batch too large') +// Define expected error types in config +const BATCH_ERRORS = { + TOO_LARGE: 'batch too large' +} +assert.include(error.innerError[0].message, BATCH_ERRORS.TOO_LARGE, 'Should fail with batch size limit error')
Line range hint
1-379: Consider implementing test utilities for common patterns.The test file contains several repeated patterns and magic numbers. Consider:
- Creating helper functions for common assertions
- Moving magic numbers to configuration
- Implementing shared error handling utilities
Example utility structure:
// test-utils.js const assertBlockFormat = (block) => { assert.isString(block.hash, 'Block should have a hash') assert.isString(block.parentHash, 'Block should have a parent hash') assert.lengthOf(block.logsBloom, 514, 'Block should have correct logs bloom length') // ... more common assertions } const assertTransactionFormat = (tx) => { assert.isString(tx.hash, 'Transaction should have a hash') assert.isDefined(tx.blockNumber, 'Transaction should have a block number') // ... more common assertions }services/ingestion/event_subscriber.go (1)
36-39: Add documentation for the keyLock field.Consider adding a comment to document the purpose and lifecycle of the
keyLockfield in theRPCEventSubscriberstruct.type RPCEventSubscriber struct { logger zerolog.Logger client *requester.CrossSporkClient chain flowGo.ChainID + // keyLock manages transaction key locking/unlocking during event processing keyLock requester.KeyLock height uint64services/requester/pool.go (2)
73-73: Consider making the retry duration configurableThe retry timeout is hardcoded to 1 minute which might be insufficient for some networks, especially during high congestion periods.
Consider making this configurable through the Config struct:
-backoff := retry.WithMaxDuration(time.Minute*1, retry.NewConstant(time.Second*1)) +backoff := retry.WithMaxDuration(t.config.RetryTimeout, retry.NewConstant(t.config.RetryInterval))
73-74: Consider using exponential backoffThe current implementation uses a constant backoff strategy which might not be optimal for handling network congestion.
Consider using exponential backoff instead:
-backoff := retry.WithMaxDuration(time.Minute*1, retry.NewConstant(time.Second*1)) +backoff := retry.WithMaxDuration(time.Minute*1, retry.NewExponential(100*time.Millisecond))config/config.go (1)
93-95: Improve documentation for TxStateValidation fieldWhile the comment explains the purpose, it could be more specific about the implications of each validation mechanism.
Consider expanding the documentation:
-// TxStateValidation sets the transaction validation mechanism. It can validate -// using the local state index, or wait for the outer Flow transaction to seal. +// TxStateValidation sets the transaction validation mechanism: +// - LocalIndexValidation: Validates using the local state index for faster feedback +// - TxSealValidation: Waits for the outer Flow transaction to seal, providing stronger guarantees +// Default: LocalIndexValidationcmd/run/cmd.go (1)
122-140: LGTM! Consider enhancing key format validation.The Cloud KMS key configuration is well-structured with proper validation of required fields.
Consider using a regex pattern to validate the key format more strictly:
- keyParts := strings.Split(cloudKMSKey, "@") - if len(keyParts) != 2 { - return fmt.Errorf("wrong format for Cloud KMS key: %s", key) - } + const keyPattern = `^[a-zA-Z0-9-]+@[0-9]+$` + if !regexp.MustCompile(keyPattern).MatchString(cloudKMSKey) { + return fmt.Errorf("wrong format for Cloud KMS key (expected {keyID}@{keyVersion}): %s", cloudKMSKey) + } + keyParts := strings.Split(cloudKMSKey, "@")tests/helpers.go (1)
140-176: Consider implementing a more robust startup wait mechanism.While the current implementation works, the fixed sleep duration might be insufficient on slower systems.
Consider implementing a health check instead:
- time.Sleep(1000 * time.Millisecond) // give it some time to start, dummy but ok for test + // Implement health check with timeout + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + for { + select { + case <-ctx.Done(): + return nil, fmt.Errorf("timeout waiting for service to start") + default: + if err := client.Ping(ctx); err == nil { + goto serviceReady + } + time.Sleep(100 * time.Millisecond) + } + } +serviceReady:tests/integration_test.go (1)
205-205: Replace arbitrary sleep with deterministic wait.The 3-second sleep is arbitrary and could be flaky on slower systems.
Consider implementing a health check similar to the one suggested for the
servicesSetupfunction.api/debug.go (3)
92-94: Consider extracting rate limiting to middleware.The rate limiting check is duplicated across all public methods. Consider implementing it as middleware to reduce code duplication and ensure consistent rate limiting behavior.
Also applies to: 104-106, 121-123, 139-141, 254-256
352-411: Enhance error handling in traceBlockByNumber.The error handling in this method could be more descriptive to aid in debugging:
- Add context to error messages
- Consider logging errors before returning them
func (d *DebugAPI) traceBlockByNumber( number rpc.BlockNumber, config *tracers.TraceConfig, ) ([]*txTraceResult, error) { block, err := d.blocks.GetByHeight(uint64(number.Int64())) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to get block at height %d: %w", number.Int64(), err) } results := make([]*txTraceResult, len(block.TransactionHashes)) if isDefaultCallTracer(config) { for i, hash := range block.TransactionHashes { trace, err := d.traceTransaction(hash, config) if err != nil { + d.logger.Debug().Err(err).Msgf("failed to trace transaction %s", hash) results[i] = &txTraceResult{TxHash: hash, Error: err.Error()} } else { results[i] = &txTraceResult{TxHash: hash, Result: trace} } } return results, nil }
Line range hint
413-441: Improve error context in executorAtBlock.The error handling could provide more context about why the operation failed:
func (d *DebugAPI) executorAtBlock(block *models.Block) (*evm.BlockExecutor, error) { previousBlock, err := d.blocks.GetByHeight(block.Height - 1) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to get previous block at height %d: %w", block.Height-1, err) } snapshot, err := d.registerStore.GetSnapshotAt(previousBlock.Height) if err != nil { return nil, fmt.Errorf( "failed to get register snapshot at block height %d: %w", previousBlock.Height, err, ) }api/server.go (2)
432-441: Consider using a map for known errors.The current slice implementation requires linear search. A map would provide O(1) lookup and make the code more maintainable.
-var knownErrors = []error{ - errs.ErrRateLimit, - errs.ErrInvalid, - errs.ErrFailedTransaction, - errs.ErrEndpointNotSupported, - gethVM.ErrExecutionReverted, - core.ErrNonceTooLow, - core.ErrNonceTooHigh, - core.ErrInsufficientFunds, -} +var knownErrors = map[string]struct{}{ + errs.ErrRateLimit.Error(): {}, + errs.ErrInvalid.Error(): {}, + errs.ErrFailedTransaction.Error(): {}, + errs.ErrEndpointNotSupported.Error(): {}, + gethVM.ErrExecutionReverted.Error(): {}, + core.ErrNonceTooLow.Error(): {}, + core.ErrNonceTooHigh.Error(): {}, + core.ErrInsufficientFunds.Error(): {}, +}
517-523: Optimize isKnownError function.With the suggested map implementation above, this function could be more efficient:
func isKnownError(errMsg string) bool { - for _, err := range knownErrors { - if strings.Contains(errMsg, err.Error()) { - return true - } - } - return false + _, ok := knownErrors[errMsg] + return ok }bootstrap/bootstrap.go (1)
228-228: Add validation for keystore initialization.The keystore initialization should validate the account keys:
- b.keystore = requester.NewKeyStore(accountKeys) + if len(accountKeys) == 0 { + return fmt.Errorf("cannot initialize keystore with empty account keys") + } + b.keystore = requester.NewKeyStore(accountKeys)services/requester/requester.go (2)
526-580: Consider using more idiomatic error handlingWhile the concurrent implementation is excellent, the error handling could be more idiomatic.
Consider this refactoring:
- var ( - g = errgroup.Group{} - err1, err2 error - latestBlock *flow.Block - account *flow.Account - ) + var ( + g errgroup.Group + latestBlock *flow.Block + account *flow.Account + )LGTM: Excellent concurrent implementation
The concurrent fetching of block and account information improves performance, while the mutex ensures thread safety for key management operations.
582-630: Consider enhancing error handling with errors.WrapWhile the validation logic is solid, the error handling could be more idiomatic using
errors.Wrap.Consider this pattern for error handling:
- return fmt.Errorf( - "%w: address %s, tx: %v, state: %v", - gethCore.ErrNonceTooLow, - from, - tx.Nonce(), - nonce, - ) + return errors.Wrapf( + gethCore.ErrNonceTooLow, + "address %s, tx: %v, state: %v", + from, + tx.Nonce(), + nonce, + )LGTM: Thorough transaction state validation
The implementation provides comprehensive validation of both nonce and balance with detailed error context.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (33)
Makefile(1 hunks)api/api.go(0 hunks)api/debug.go(10 hunks)api/server.go(6 hunks)api/utils.go(2 hunks)bootstrap/bootstrap.go(5 hunks)bootstrap/create-multi-key-account.go(6 hunks)bootstrap/utils.go(1 hunks)cmd/run/cmd.go(4 hunks)config/config.go(3 hunks)eth/utils.go(1 hunks)metrics/collector.go(6 hunks)metrics/handler.go(2 hunks)metrics/nop.go(1 hunks)services/evm/executor.go(4 hunks)services/ingestion/event_subscriber.go(3 hunks)services/ingestion/event_subscriber_test.go(5 hunks)services/requester/key_store.go(1 hunks)services/requester/kms_key_rotation_signer.go(0 hunks)services/requester/kms_key_signer.go(1 hunks)services/requester/pool.go(3 hunks)services/requester/requester.go(5 hunks)services/requester/rotation_signer.go(0 hunks)services/requester/rotation_signer_test.go(0 hunks)tests/helpers.go(2 hunks)tests/integration_test.go(6 hunks)tests/web3js/config.js(1 hunks)tests/web3js/debug_traces_test.js(2 hunks)tests/web3js/eth_failure_handling_test.js(3 hunks)tests/web3js/eth_filter_endpoints_test.js(2 hunks)tests/web3js/eth_logs_filtering_test.js(2 hunks)tests/web3js/eth_non_interactive_test.js(13 hunks)tests/web3js/verify_cadence_arch_calls_test.js(1 hunks)
💤 Files with no reviewable changes (4)
- api/api.go
- services/requester/rotation_signer_test.go
- services/requester/rotation_signer.go
- services/requester/kms_key_rotation_signer.go
🚧 Files skipped from review as they are similar to previous changes (16)
- metrics/nop.go
- tests/web3js/config.js
- metrics/handler.go
- api/utils.go
- Makefile
- metrics/collector.go
- tests/web3js/eth_logs_filtering_test.js
- tests/web3js/verify_cadence_arch_calls_test.js
- bootstrap/utils.go
- eth/utils.go
- services/ingestion/event_subscriber_test.go
- tests/web3js/debug_traces_test.js
- tests/web3js/eth_filter_endpoints_test.js
- services/evm/executor.go
- services/requester/kms_key_signer.go
- services/requester/key_store.go
🔇 Additional comments (10)
services/ingestion/event_subscriber.go (1)
175-178: Consider implementing a cleanup mechanism for partial failures.
The current implementation might leave keys locked if event processing fails mid-way. Consider implementing a cleanup mechanism or using a defer pattern to ensure keys are properly released even in error scenarios.
Let's verify if there are any error paths that could lead to locked keys:
services/requester/pool.go (1)
34-34: LGTM: Config integration looks good!
The config field addition and constructor update are properly implemented.
Also applies to: 42-42, 49-49
bootstrap/create-multi-key-account.go (1)
407-422: LGTM: Secure random key generation implementation
The randomPrivateKey function correctly:
- Uses crypto-secure random source
- Implements proper error handling
- Uses appropriate key algorithm and seed length
cmd/run/cmd.go (2)
217-223: LGTM! Clean implementation of transaction validation configuration.
The validation logic is clear and uses well-defined constants with proper error handling.
279-286: LGTM! Well-documented flag definitions.
The flags are clearly documented with helpful descriptions and examples.
tests/integration_test.go (2)
Line range hint 28-137: LGTM! Well-structured test with clear documentation.
The test effectively validates the transaction sealing mechanism with proper concurrent execution handling.
343-385: LGTM! Clean implementation of Cloud KMS test configuration.
The test properly handles the new single-key KMS configuration with appropriate error handling and validation.
services/requester/requester.go (3)
98-110: LGTM: Struct changes align with key management improvements
The replacement of signer with keystore in the EVM struct indicates a shift towards more robust key management, which is a good architectural improvement.
Line range hint 113-179: LGTM: Constructor properly adapted for keystore integration
The constructor changes appropriately reflect the shift from signer to keystore while maintaining all necessary validations and configurations.
202-206: LGTM: Local transaction validation properly implemented
The addition of local state validation is well-implemented with proper configuration control, aligning with the PR's objective of local transaction reexecution.
m-Peter
changed the title
Description
Includes the final changes for the complete v1.0.0 release.
For contributor use:
masterbranchFiles changedin the Github PR explorerSummary by CodeRabbit
New Features
DebugAPIwith rate limiting functionality.Bug Fixes
Documentation
Tests
Chores