Add loadtest script #658
Add loadtest script #658
Conversation
WalkthroughThe changes introduce a new load test feature into the CLI, including the addition of a load testing command, configuration options, and implementation in a dedicated package. The CLI now registers a "loadtest" command that executes a new function invoking load test functionality. Additionally, the blockchain publisher’s waiting period for transaction receipts is increased, and updates to nonce management have been made, including refactoring tests and altering concurrency limit initialization. A new test nonce manager implementation is added in the test utilities package. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI
participant Parser
participant LoadTestCmd
participant LoadTestPkg
participant BlockchainPublisher
participant Database
participant Logger
CLI->>Parser: Parse command-line options
Parser->>CLI: Identify "loadtest" command with options
CLI->>LoadTestCmd: Invoke loadTest() for "loadtest"
LoadTestCmd->>LoadTestPkg: Call LoadTest(logger, options, contractsOptions)
LoadTestPkg->>BlockchainPublisher: Initialize publisher & establish DB connection
BlockchainPublisher->>Database: Open namespaced DB for nonce management
BlockchainPublisher-->>LoadTestPkg: Return publisher instance
LoadTestPkg->>LoadTestPkg: Setup ticker and wait group for sending messages
LoadTestPkg->>BlockchainPublisher: Publish messages at configured rate
BlockchainPublisher->>Logger: Log message publication status and errors
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
🪧 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 (
|
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
neekolas
force-pushed
the
03-20-add_loadtest_script
branch
from
28b5027 to
2c1ef3a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🧹 Nitpick comments (8)
pkg/config/cliOptions.go (1)
83-89: New LoadTestOptions structureA well-structured configuration object for the new load testing feature has been added with appropriate defaults:
- Private key (required)
- Message rate (25/sec default)
- Test duration (60 sec default)
- Message size (7168 bytes default)
- DB connection string (from env variable)
These settings provide good flexibility for configuring various aspects of load testing.
Consider adding validation for these parameters, especially for minimum/maximum values. For example, you might want to enforce:
- Minimum/maximum message rate
- Minimum/maximum duration
- Minimum/maximum message size
This would prevent users from accidentally configuring tests that are either too light to be meaningful or too heavy to be safe.
cmd/cli/main.go (1)
139-141: Registering the loadtest command
Good addition for parsing load test flags. Make sure usage instructions for this new command are clear so users know how to invoke it.pkg/testutils/noncemanager/nonceManager.go (2)
49-78: GetNonce method
- Locking the mutex is correct to protect shared state.
- Pushing abandoned nonces back onto the heap is a useful mechanism for reuse.
- Consider adding a context timeout check if needed for stress testing scenarios, but that might be optional.
88-90: Replenish method
Currently a no-op. If the test harness eventually requires replenishing the nonce manager from an external source, consider implementing it or removing the stub.pkg/loadtest/loadtest.go (1)
65-127: doLoadTest function
- Uses a ticker to schedule messages at a precise rate.
wg.Wait()ensures all goroutines finish.- Consider adding context-based cancellation or early stop conditions if needed for more dynamic load test control.
pkg/blockchain/nonceManager_test.go (3)
35-54: TestGetNonce_RevertMany: Validating Nonce Reversion Behavior
This test verifies that when a nonce is cancelled (vianonce.Cancel()), subsequent calls return the same nonce value (0 in this case). This is important to ensure that cancellation reverts the nonce usage properly. However, adding a brief inline comment to explain the expected behavior—i.e., that cancellation should “revert” the nonce consumption so that the nonce remains at its initial value—could enhance maintainability and clarity for future developers.
56-76: TestGetNonce_ConsumeMany: Sequential Consumption Check
The test iterates over 10 nonce consumptions and verifies that each returned nonce’s value matches the expected sequence. This effectively tests the incrementing logic upon consumption. A short comment explaining the rationale behind expecting the nonce to equal the loop index would be beneficial to maintain readability and clarity for future maintainers.
78-106: TestGetNonce_ConsumeManyConcurrent: Assessing Concurrency Robustness
The concurrent test uses goroutines with async.WaitGroupto simulate multiple clients consuming nonces simultaneously. The error checks ensure that the nonce manager handles concurrent access appropriately. One suggestion would be to consider asserting the uniqueness of nonce values in a concurrent environment if the design contracts require that each nonce, once consumed, be unique. This additional check could improve confidence in the thread-safety and correctness of the nonce incrementation logic under heavy load.
🛑 Comments failed to post (1)
pkg/loadtest/loadtest.go (1)
15-63: 🛠️ Refactor suggestion
LoadTest function
- Properly initializes blockchain client, signer, DB, and publisher before calling
doLoadTest.- Adequate error handling with immediate returns.
- Consider returning an error instead of using
logger.Fatalif partial failures must be handled upstream.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
cmd/cli/main.go (2)
42-42: Add a short doc comment for clarity.
Although optional, adding a brief comment for theLoadTestfield helps maintain consistency with other fields.
803-807: Ensure consistent error handling strategy.
This function terminates execution on error vialogger.Fatal. You may consider returning an error fromloadTestinstead, if you prefer graceful handling, but this approach is acceptable if a load test failure should halt execution.pkg/testutils/noncemanager/nonceManager.go (3)
38-43: TestNonceManager struct is well-defined.
Consider adding a short doc comment explaining usage, especially around theabandonedheap.
45-47: Initialization may need a heap.Init call.
Currently,abandonedis used withheap.Pop, but we never callheap.Init. Although it may work with an empty slice, consider explicitly initializing the heap for safety.func NewTestNonceManager(logger *zap.Logger) *TestNonceManager { + tm := &TestNonceManager{logger: logger} + heap.Init(&tm.abandoned) + return tm - return &TestNonceManager{logger: logger} }
88-90: Method is a no-op.
If this is intentionally a placeholder, consider briefly documenting the reasoning or removing the method if unneeded.pkg/loadtest/loadtest.go (1)
15-63: Ensure consistent error handling in LoadTest.
The function returns an error for most failures, but useslogger.FatalwhenBlockchainPublishercreation fails. Consider returning errors uniformly if partial failures need distinct handling.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
cmd/cli/main.go(7 hunks)pkg/blockchain/blockchainPublisher.go(2 hunks)pkg/blockchain/blockchainPublisher_test.go(2 hunks)pkg/blockchain/nonceManager.go(2 hunks)pkg/blockchain/nonceManager_test.go(1 hunks)pkg/config/cliOptions.go(1 hunks)pkg/loadtest/loadtest.go(1 hunks)pkg/testutils/noncemanager/nonceManager.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/blockchain/blockchainPublisher_test.go
- pkg/blockchain/blockchainPublisher.go
- pkg/blockchain/nonceManager.go
- pkg/config/cliOptions.go
🧰 Additional context used
🧬 Code Definitions (2)
pkg/testutils/noncemanager/nonceManager.go (1)
pkg/blockchain/nonceManager.go (1) (1)
NonceContext(14-18)
cmd/cli/main.go (2)
pkg/loadtest/loadtest.go (1) (1)
LoadTest(15-63)pkg/config/cliOptions.go (1) (1)
LoadTestOptions(83-89)
🔇 Additional comments (19)
cmd/cli/main.go (5)
15-15: No issues with the new import.
70-70: Variable declaration is straightforward.
Looks good; no issues found.
139-141: Load test command registered successfully.
Command name, help text, and error handling align with existing patterns.
170-170: Populating CLI struct with loadTestOptions.
No concerns; this maintains consistency across the CLI’s aggregated options.
763-765: New switch case for 'loadtest'
This case logically parallels other commands. No issues with callingloadTest.pkg/testutils/noncemanager/nonceManager.go (5)
1-3: Package declaration is clear.
No issues found with naming or structure.
4-11: Imports are appropriate.
The chosen packages (heap, context, sync, etc.) are correct for this use case.
13-36: Custom heap implementation deviates from container/heap defaults.
Returning the first element inPopinstead of the last is non-standard for a min-heap. This matches a previous review concern.
80-86: Validate nonce progression upon fast-forward.
Currently, this method unconditionally setstm.nonce. If it’s set to a lower value than the current nonce, subsequent usage might be confusing or lead to collisions.
49-78:❓ Verification inconclusive
Concurrency and context checks appear correct.
Locking around push/pop fromabandonedis good. However, double-check thatheap.Initis called before the first push/pop to avoid unexpected behavior.
🏁 Script executed:
#!/bin/bash # Verify if heap.Init is invoked anywhere in the codebase rg "heap.Init"Length of output: 14
Attention: Potential Missing
heap.InitCall in NonceManager InitializationConcurrency and context handling in
GetNonceare correct, and the locking around push/pop operations onabandonedis properly managed. However, our search for occurrences ofheap.Initdidn’t return any evidence that the abandoned heap is being initialized. Please verify thatheap.Init(&tm.abandoned)is invoked during the initialization (e.g., in the constructor of the TestNonceManager or an init function) before any push/pop operations occur to ensure heap invariants are maintained.pkg/loadtest/loadtest.go (3)
1-13: Package and imports are appropriate.
No immediate issues with dependencies.
65-127: Improper integer iteration indoLoadTest.
Usingfor i := range totalMessageson an integer is invalid. This was previously flagged. Also, be cautious with the goroutine capturing loop variables.-for i := range totalMessages { +for i := 0; i < totalMessages; i++ {
129-140: Error from cryptoRand.Read is discarded.
As previously noted, ignoring potential errors can compromise reliability or security.- _, _ = cryptoRand.Read(b) + if _, err := cryptoRand.Read(b); err != nil { + panic(err) // or handle appropriately + }pkg/blockchain/nonceManager_test.go (6)
3-13: Import statements look well-organized and appropriate.The imports needed for the nonce manager tests are well-structured and include all necessary packages for the SQL-backed nonce manager implementation.
15-33: Simple nonce test looks good.This test verifies the basic functionality of getting a nonce and ensures it starts at the expected value (0). The test properly uses the SQL-backed nonce manager, initializes it, and cleans up resources.
35-54: Multiple nonce reservation/cancellation test is well-structured.This test verifies that when nonces are repeatedly requested and canceled, the same nonce (0) is correctly reused each time. The test demonstrates proper resource management with appropriate setup and teardown.
56-76: Sequential nonce consumption test is robust.This test ensures that when consuming nonces sequentially, each nonce increments properly (0 through 9). The test correctly verifies the behavior of the SQL-backed nonce manager when nonces are consumed rather than canceled.
78-106: Concurrent nonce consumption test is valuable.This test verifies the thread-safety of the nonce manager implementation by requesting and consuming nonces concurrently from multiple goroutines. The use of WaitGroup ensures proper synchronization.
1-106: Refactoring improves code organization.The removal of the
Int64Heaptype andTestNonceManagerstruct from this file and their relocation topkg/testutils/noncemanager/nonceManager.go(as noted in the summary) is a good architectural decision. This refactoring improves code organization by:
- Separating test utilities from the code being tested
- Making the test nonce manager implementation reusable across multiple test files
- Focusing this test file on testing the SQL-backed implementation
This change aligns well with the PR objective of supporting load testing with multiple payers.
|
do you already have results from localhost? I wonder how different anvil is from say staging |
|
I've only tested against testnet staging. I don't think it's hitting staging at its full capacity though. When I look in the block explorer I don't see fees increasing, which would be a clear indicator that the blocks are full. |
neekolas
force-pushed
the
03-20-add_loadtest_script
branch
from
2c1ef3a to
8bbedfb
Compare
Add blockchain load testing functionalityAdded new 📍Where to StartThe main CLI entry point in main.go followed by the core load testing implementation in loadtest.go Macroscope summarized e31a9ea. |
neekolas
force-pushed
the
03-20-add_loadtest_script
branch
2 times, most recently
from
30fc01c to
9562a95
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
♻️ Duplicate comments (3)
pkg/testutils/noncemanager/nonceManager.go (2)
69-73:⚠️ Potential issueUse heap.Push instead of direct access to abandoned heap.
The heap.Push function should be used instead of directly calling Push on the abandoned heap to maintain the heap invariant.
Cancel: func() { tm.mu.Lock() defer tm.mu.Unlock() - tm.abandoned.Push(nonce) + heap.Push(&tm.abandoned, nonce) },
23-29: 💡 Verification agent🧩 Analysis chain
Fix the heap Pop implementation to follow standard heap behavior.
The current Pop implementation removes the first element (smallest) rather than the last element, which is how heap.Pop typically works in Go's container/heap package. This deviation could lead to confusion or subtle bugs.
func (h *Int64Heap) Pop() interface{} { old := *h n := len(old) - x := old[0] // Get the smallest element - *h = old[1:n] + x := old[n-1] // Get the last element + *h = old[:n-1] return x }
🏁 Script executed:
#!/bin/bash # Check Go's container/heap implementation for comparison cat $(fd -t f -p "heap/.+\.go$" /usr/local/go/src/container/ | head -1)Length of output: 1262
Action Required: Correct the Pop method to match Go's container/heap behavior by removing the last element.
- The current implementation incorrectly removes the first element (index 0), while the standard behavior (as seen in Go's container/heap example) removes the last element.
- Update your implementation accordingly:
func (h *Int64Heap) Pop() interface{} { old := *h n := len(old) - x := old[0] // Get the smallest element - *h = old[1:n] + x := old[n-1] // Get the last element + *h = old[:n-1] return x }pkg/loadtest/loadtest.go (1)
94-94:⚠️ Potential issueFix the loop iteration over totalMessages.
Using
rangewith an integer is incorrect syntax in Go. It should be a traditional for loop.- for i := range totalMessages { + for i := 0; i < totalMessages; i++ {
🧹 Nitpick comments (4)
cmd/cli/main.go (1)
139-141: Maintain consistent capitalization in error messages.The error message here starts with "Could" (capitalized), while other similar error messages in the file use "could" (lowercase).
- return nil, fmt.Errorf("Could not add loadtest command: %s", err) + return nil, fmt.Errorf("could not add loadtest command: %s", err)pkg/testutils/noncemanager/nonceManager.go (1)
73-73: Remove misleading comment.The "No-op" comment is misleading since the Cancel function actually has an implementation.
- }, // No-op + },pkg/loadtest/loadtest.go (2)
71-71: Use testutils.RandomGroupID instead of reimplementing.You're importing testutils but then defining a separate randomBytes function. Use the existing testutils implementation for consistency.
- groupID := testutils.RandomGroupID() + groupID := testutils.RandomGroupID()
117-119: Consider implementing a timeout for waitgroup.Using
wg.Wait()without a timeout can cause the function to hang indefinitely if a goroutine doesn't complete, which could happen in load testing scenarios.- wg.Wait() + // Set up a timeout channel + done := make(chan struct{}) + go func() { + wg.Wait() + close(done) + }() + + // Wait with timeout + select { + case <-done: + // All goroutines completed successfully + case <-time.After(time.Duration(options.Duration+30) * time.Second): + logger.Warn("Timed out waiting for all messages to complete") + }
🛑 Comments failed to post (3)
pkg/testutils/noncemanager/nonceManager.go (1)
45-47: 🛠️ Refactor suggestion
Initialize heap in constructor.
The abandoned heap is not properly initialized. You should initialize it and call heap.Init in the constructor to ensure it maintains the heap property.
func NewTestNonceManager(logger *zap.Logger) *TestNonceManager { - return &TestNonceManager{logger: logger} + abandoned := Int64Heap{} + heap.Init(&abandoned) + return &TestNonceManager{ + logger: logger, + abandoned: abandoned, + } }📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.func NewTestNonceManager(logger *zap.Logger) *TestNonceManager { abandoned := Int64Heap{} heap.Init(&abandoned) return &TestNonceManager{ logger: logger, abandoned: abandoned, } }pkg/loadtest/loadtest.go (2)
47-59: 🛠️ Refactor suggestion
Propagate error on publisher creation.
The function returns an early error when creating the client, signer, or DB connection, but calls Fatal when the publisher creation fails, which doesn't match the error handling pattern in the rest of the function.
publisher, err := blockchain.NewBlockchainPublisher( ctx, logger, chainClient, signer, *contractsOptions, nonceManager, ) if err != nil { - logger.Fatal("could not create blockchain publisher", zap.Error(err)) + return err }📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.nonceManager := blockchain.NewSQLBackedNonceManager(dbConn, logger) publisher, err := blockchain.NewBlockchainPublisher( ctx, logger, chainClient, signer, *contractsOptions, nonceManager, ) if err != nil { return err }
60-62: 🛠️ Refactor suggestion
Return errors from doLoadTest.
The doLoadTest function doesn't currently return any errors that might occur during execution. Consider modifying it to return an error and propagate it back to the LoadTest caller.
-func doLoadTest( +func doLoadTest( ctx context.Context, logger *zap.Logger, publisher *blockchain.BlockchainPublisher, options *config.LoadTestOptions, -) { +) error { // Function body... + return nil } // And in LoadTest: - doLoadTest(ctx, logger, publisher, options) - return nil + return doLoadTest(ctx, logger, publisher, options)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
pkg/testutils/noncemanager/nonceManager.go (2)
13-36:⚠️ Potential issueIncorrect heap implementation may cause ordering issues
The
Int64Heapheap implementation has an issue in thePop()method. It removes the first element instead of the last element from the slice, which doesn't follow standard heap operations:func (h *Int64Heap) Pop() interface{} { old := *h n := len(old) - x := old[0] // Get the smallest element - *h = old[1:n] + x := old[n-1] // Get the last element + *h = old[:n-1] return x }The
container/heappackage expectsPop()to remove and return the last element of the slice, while the heap management is handled by the package itself.
67-78:⚠️ Potential issueDirect heap manipulation breaks heap invariants
The
Cancelfunction directly callstm.abandoned.Push(nonce)instead of usingheap.Push(&tm.abandoned, nonce), which bypasses the heap package's functionality to maintain heap invariants:Cancel: func() { tm.mu.Lock() defer tm.mu.Unlock() - tm.abandoned.Push(nonce) + heap.Push(&tm.abandoned, nonce) },This could lead to incorrect ordering when later using
heap.Pop.
🧹 Nitpick comments (4)
pkg/testutils/noncemanager/nonceManager.go (2)
73-73: Remove misleading commentThe
// No-opcomment after thetm.abandoned.Push(nonce)statement is misleading since the function is actually performing an operation. This comment should be removed.
88-90: Consider documenting why Replenish is a no-opThe
Replenishmethod is implemented as a no-op (returns nil) but lacks documentation explaining why this implementation is sufficient for testing purposes.func (tm *TestNonceManager) Replenish(ctx context.Context, nonce big.Int) error { + // No-op for test implementation as we don't need to synchronize with external nonce state return nil }cmd/cli/main.go (2)
139-141: Command description might need more detail.
It could help to include usage examples or information about parameters in the help text to guide users better.
802-807: Consider avoiding fatal exit in case of load test failure.
Usinglogger.Fatalwill terminate the entire CLI run on any error, which may be overly restrictive if partial failures are acceptable in a testing context. Returning an error or using a non-fatal log method would allow the CLI to exit with a controlled status code that can be handled by automation scripts.func loadTest(logger *zap.Logger, options *CLI) { if err := loadtest.LoadTest(logger, &options.LoadTest, &options.Contracts); err != nil { - logger.Fatal("could not run load test", zap.Error(err)) + logger.Error("could not run load test", zap.Error(err)) + os.Exit(1) // or return instead } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
cmd/cli/main.go(7 hunks)pkg/blockchain/blockchainPublisher.go(2 hunks)pkg/blockchain/blockchainPublisher_test.go(2 hunks)pkg/blockchain/nonceManager.go(1 hunks)pkg/blockchain/nonceManager_test.go(1 hunks)pkg/config/cliOptions.go(1 hunks)pkg/loadtest/loadtest.go(1 hunks)pkg/testutils/noncemanager/nonceManager.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/blockchain/nonceManager.go
- pkg/blockchain/blockchainPublisher.go
- pkg/config/cliOptions.go
- pkg/blockchain/blockchainPublisher_test.go
🧰 Additional context used
🧬 Code Definitions (1)
pkg/testutils/noncemanager/nonceManager.go (1)
pkg/blockchain/nonceManager.go (1) (1)
NonceContext(14-18)
🔇 Additional comments (15)
pkg/testutils/noncemanager/nonceManager.go (4)
38-43: LGTM! TestNonceManager structureThe TestNonceManager struct with mutex for synchronization, nonce tracking, logger, and abandoned heap looks well-designed for test purposes.
45-47: LGTM! Factory functionSimple and straightforward factory function.
49-66: LGTM! GetNonce method implementationThe GetNonce method correctly handles:
- Context cancellation
- Thread safety with mutex
- Prioritizing abandoned nonces when available
- Incrementing the nonce counter when needed
- Proper logging
80-86: LGTM! FastForwardNonce implementationThe method correctly handles thread safety with mutex locking and properly updates the nonce value.
pkg/blockchain/nonceManager_test.go (5)
5-7: LGTM! Import adjustmentsThe import of math/big and sync packages is appropriate for the tests.
15-33: LGTM! TestGetNonce_Simple testThe test properly initializes a SQL-backed nonce manager and verifies that:
- The first nonce is 0
- Error handling works correctly
- Resources are properly cleaned up
35-54: LGTM! TestGetNonce_RevertMany testGood test for verifying that canceling nonces allows them to be reused. The test properly checks that the same nonce (0) is returned after each cancellation.
56-76: LGTM! TestGetNonce_ConsumeMany testGood test for verifying sequential nonce consumption. The test properly checks that incrementing nonces (0-9) are returned and can be consumed without errors.
78-106: LGTM! TestGetNonce_ConsumeManyConcurrent testGreat concurrent test that creates 20 goroutines to simultaneously get and consume nonces. This effectively tests the thread safety of the nonce manager implementation.
cmd/cli/main.go (5)
15-15: No concerns about this new import.
42-42: Field name looks consistent.
Storing the load test configuration inLoadTestaligns well with the existing naming pattern for other CLI commands.
70-70: Variable definition is clear.
No issues with introducing this variable; it's immediately used for the new command.
170-170: No concerns.
New field is properly integrated into the CLI struct.
763-765: No issues with registering the new command.
This follows the established pattern for other CLI commands.pkg/loadtest/loadtest.go (1)
1-13: Imports and package definition look appropriate.
No issues identified; all imported packages appear to be used.
neekolas
force-pushed
the
03-20-add_loadtest_script
branch
2 times, most recently
from
9c5c645 to
0f40bbb
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/blockchain/nonceManager_test.go (1)
79-106: Consider verifying uniqueness in concurrency test.The concurrent test correctly checks for errors using a wait group. For stronger validation, you could confirm that each consumed nonce is unique across goroutines. This additional check can help catch subtle concurrency or nonce allocation issues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
cmd/cli/main.go(7 hunks)pkg/blockchain/blockchainPublisher.go(2 hunks)pkg/blockchain/blockchainPublisher_test.go(2 hunks)pkg/blockchain/nonceManager.go(1 hunks)pkg/blockchain/nonceManager_test.go(1 hunks)pkg/config/cliOptions.go(1 hunks)pkg/loadtest/loadtest.go(1 hunks)pkg/testutils/noncemanager/nonceManager.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/blockchain/nonceManager.go
- pkg/blockchain/blockchainPublisher_test.go
- pkg/blockchain/blockchainPublisher.go
- pkg/config/cliOptions.go
🧰 Additional context used
🧬 Code Definitions (1)
pkg/testutils/noncemanager/nonceManager.go (1)
pkg/blockchain/nonceManager.go (1) (1)
NonceContext(14-18)
🔇 Additional comments (7)
pkg/testutils/noncemanager/nonceManager.go (2)
23-29: Preserve heap invariants or document custom 'Pop' behavior.The
Popmethod is returning the first element in the slice instead of removing the last element as the standardheap.Interfaceexpects. This deviates from the default min-heap usage incontainer/heap. Consider returning the last element as required by the standard heap contract or update documentation to clarify your custom approach if intentional.
70-73: Useheap.Pushto maintain correctness.Calling
tm.abandoned.Push(nonce)directly bypasses the invariants maintained byheap.Push(&tm.abandoned, nonce). Consider switching toheap.Push(&tm.abandoned, nonce)so that the heap properties remain intact.cmd/cli/main.go (3)
70-71: Load test command registration looks good.The addition of
loadTestOptionsand registration of"loadtest"command in the parser is consistent with the other commands. Usage details appear correctly configured.Also applies to: 139-141
763-766: New 'loadtest' CLI case is well-integrated.Invoking the
loadTestfunction from the switch statement aligns with the existing pattern of subcommand handling in this CLI. Implementation remains consistent with other commands.
802-806: Load test handler aligns with CLI conventions.The
loadTestfunction callsloadtest.LoadTestand useslogger.Fatalfor errors, matching the CLI’s overall error-handling style. This is clear, straightforward, and maintains consistency.pkg/loadtest/loadtest.go (2)
93-93: Invalid range clause over integer.
for i := range totalMessageswon't compile becausetotalMessagesis an integer, not a slice, channel, or map. Use a traditional loop, for example:-for i := range totalMessages { +for i := 0; i < totalMessages; i++ {
90-106: Concurrent assignment to sharederrvariable can cause data races.Multiple goroutines write to
errwithout synchronization, risking race conditions and lost error details. Use a mutex or another thread-safe mechanism to track errors in parallel computations.
neekolas
force-pushed
the
03-20-add_loadtest_script
branch
from
0f40bbb to
e31a9ea
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
pkg/testutils/noncemanager/nonceManager.go (1)
72-73:⚠️ Potential issueBypassing standard heap invariants in
Cancel.Rather than calling
tm.abandoned.Push(nonce)directly, usingheap.Push(&tm.abandoned, nonce)is generally recommended to maintain correct heap ordering. Currently, you rely on the customPopapproach; if the program expands or changes how the heap is used, this might cause subtle bugs.- tm.abandoned.Push(nonce) + heap.Push(&tm.abandoned, nonce)pkg/loadtest/loadtest.go (1)
93-107:⚠️ Potential issueLogic errors in the send loop: invalid range and unsynchronized error assignment.
for i := range totalMessages {will not compile iftotalMessagesis an integer.- Assigning to
errfrom multiple goroutines without synchronization causes a data race.Below is a possible fix:
- for i := range totalMessages { + var mu sync.Mutex + for i := 0; i < totalMessages; i++ { <-ticker.C wg.Add(1) go func(messageNumber int) { defer wg.Done() if _, publishError := publisher.PublishGroupMessage(ctx, groupID, testutils.RandomBytes(options.MessageSizeBytes)); publishError != nil { - err = publishError + mu.Lock() + if err == nil { + err = publishError + } + mu.Unlock() logger.Error( "Failed to publish message", - zap.Error(err), + zap.Error(publishError), zap.Int("messageNumber", messageNumber+1), ) } }(i) }
🧹 Nitpick comments (1)
pkg/testutils/noncemanager/nonceManager.go (1)
23-29: Pop implementation deviates from the default heap interface behavior.The standard library example for
heap.Interfacetypically swaps and pops from the end of the slice. Here, the smallest element is removed from the front, which can still work if yourLessdefinition is consistent. However, it's unconventional and may surprise maintainers. Consider documenting the rationale or aligning with the standard approach for clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
cmd/cli/main.go(7 hunks)pkg/blockchain/blockchainPublisher.go(2 hunks)pkg/blockchain/blockchainPublisher_test.go(2 hunks)pkg/blockchain/nonceManager.go(1 hunks)pkg/blockchain/nonceManager_test.go(1 hunks)pkg/config/cliOptions.go(1 hunks)pkg/loadtest/loadtest.go(1 hunks)pkg/testutils/noncemanager/nonceManager.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/blockchain/nonceManager.go
- pkg/config/cliOptions.go
- pkg/blockchain/blockchainPublisher_test.go
- pkg/blockchain/blockchainPublisher.go
🧰 Additional context used
🧬 Code Definitions (3)
pkg/testutils/noncemanager/nonceManager.go (1)
pkg/blockchain/nonceManager.go (1)
NonceContext(14-18)
cmd/cli/main.go (2)
pkg/loadtest/loadtest.go (1)
LoadTest(15-61)pkg/config/cliOptions.go (1)
LoadTestOptions(83-89)
pkg/loadtest/loadtest.go (3)
pkg/config/cliOptions.go (1)
LoadTestOptions(83-89)pkg/blockchain/nonceManager.go (1)
NewSQLBackedNonceManager(51-57)pkg/blockchain/blockchainPublisher.go (2)
NewBlockchainPublisher(40-117)BlockchainPublisher(29-38)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test (Node)
- GitHub Check: Lint-Go
🔇 Additional comments (8)
cmd/cli/main.go (6)
15-15: New import for load testing looks correct.This addition appears to properly import the new loadtest package. No issues identified.
42-42: New LoadTest field in CLI struct is appropriate.Exposing
LoadTestoptions on the main CLI struct aligns well with the new command. No concerns noted.
70-70: Definition of loadTestOptions variable is consistent with other commands.This variable is necessary to parse the load test parameters. Looks good.
139-141: Registration of the "loadtest" command is consistent.The added command follows the same command registration pattern as other CLI commands. No issues found.
763-765: Case branch for "loadtest" is coherent.Utilizing the
loadTesthelper function upon selecting the "loadtest" command is in line with the CLI architecture.
803-807: Load test function correctly delegates to the loadtest package.Invoking
loadtest.LoadTesthere is consistent with other CLI commands that delegate logic to underlying packages. The use oflogger.Fatalmatches other error handling patterns in this file.pkg/testutils/noncemanager/nonceManager.go (1)
49-78: Potential concurrency considerations inGetNonce.Although you're locking around the abandoned-heap access and nonce increment, ensure all usage patterns are thoroughly tested under concurrent conditions. The logic appears correct, but concurrency with repeated
Cancel()calls might require additional validation.pkg/blockchain/nonceManager_test.go (1)
5-7: New imports for working with big integers, concurrency, and testing.These changes are aligned with the test logic that requires
math/big,sync, andtesting. No concerns found.
tl;dr
Notes
Having a single payer for the load test limits the pace that we can write messages because of blockchain limitations around nonces. We can only have 64 pending transactions in the mempool. The turnaround time to mark a message as "sent" is slow, which leaves a lot of time waiting for the semaphore.
I don't really see a way around this other than sending from multiple Payers.
Summary by CodeRabbit