Add poor's man identity updates stress test #738
Conversation
WalkthroughA new CLI subcommand Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI (main.go)
participant StressPkg (stress.go)
participant CastCmd (cast.go)
participant EthereumNode
User->>CLI (main.go): Run 'identity-updates-stress' command with options
CLI (main.go)->>StressPkg (stress.go): StressIdentityUpdates(count, contract, rpc, privateKey)
StressPkg (stress.go)->>EthereumNode: Get current nonce for sender address
loop For each transaction (count)
StressPkg (stress.go)->>CastCmd (cast.go): Build and execute cast send command (with nonce)
CastCmd (cast.go)->>EthereumNode: Send transaction to contract
EthereumNode-->>CastCmd (cast.go): Return transaction result
CastCmd (cast.go)-->>StressPkg (stress.go): Return status
end
StressPkg (stress.go)-->>CLI (main.go): Return overall status
CLI (main.go)-->>User: Output result
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (1.64.8)Error: you are using a configuration file for golangci-lint v2 with golangci-lint v1: please use golangci-lint v2 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (5)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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}
fbac
force-pushed
the
04-23-poor_man_stress_test
branch
from
e074963 to
743c5d2
Compare
Add stress testing capability for identity updates in CLI by implementing new
|
![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: 5
🧹 Nitpick comments (1)
pkg/stress/stress.go (1)
16-20: Hardcoded test values limit flexibility.The hardcoded constants for the identity updates signature, inbox ID, and payload limit the flexibility of the stress test.
Consider making these values configurable through function parameters with the current values as defaults:
const ( IDENTITY_UPDATES_SIGNATURE = "addIdentityUpdate(bytes32,bytes)" - IDENTITY_UPDATES_INBOX_ID = "0xdee4b6c2d041591ff89952fb57487e594ec42d07063a94cd4535e80af25c8e2e" - IDENTITY_UPDATES_PAYLOAD = "0a23122102d3be290e677cce90cb6300f584dc29b201b5fa136aac59a5a1eec4ac4d001f592aa0030a790a770a2a30783231383164633965343032633731326566653161633130336564356430373730343965373133333910011a450a430a41ded55d591b4007c54463b3bcdb6ccb7d508b98b6a029e2b898fb364edd0a0a516a700701caa15da918d80dba740777cae1159cae83ab316fb45443fe26922e851c20010ad60112d3010a221220a0cd41533d41444017a9e4fc401b66f4207b9d9eb16d7d9b747aa26fb76b953912450a430a41ded55d591b4007c54463b3bcdb6ccb7d508b98b6a029e2b898fb364edd0a0a516a700701caa15da918d80dba740777cae1159cae83ab316fb45443fe26922e851c1a661a640a40540884bcc933b83cd1089af67818c7c673a4b76ad31aa823d4921a37013ce868617cac8b208cd0be1022733e7186b7e378ce1852544200a3998971e732d8510a1220a0cd41533d41444017a9e4fc401b66f4207b9d9eb16d7d9b747aa26fb76b953910b8cab6c5efa9bd9c181a4064336265323930653637376363653930636236333030663538346463323962323031623566613133366161633539613561316565633461633464303031663539" ) +type IdentityUpdateParams struct { + InboxID string + Payload string +} + +var DefaultIdentityUpdateParams = IdentityUpdateParams{ + InboxID: "0xdee4b6c2d041591ff89952fb57487e594ec42d07063a94cd4535e80af25c8e2e", + Payload: "0a23122102d3be290e677cce90cb6300f584dc29b201b5fa136aac59a5a1eec4ac4d001f592aa0030a790a770a2a30783231383164633965343032633731326566653161633130336564356430373730343965373133333910011a450a430a41ded55d591b4007c54463b3bcdb6ccb7d508b98b6a029e2b898fb364edd0a0a516a700701caa15da918d80dba740777cae1159cae83ab316fb45443fe26922e851c20010ad60112d3010a221220a0cd41533d41444017a9e4fc401b66f4207b9d9eb16d7d9b747aa26fb76b953912450a430a41ded55d591b4007c54463b3bcdb6ccb7d508b98b6a029e2b898fb364edd0a0a516a700701caa15da918d80dba740777cae1159cae83ab316fb45443fe26922e851c1a661a640a40540884bcc933b83cd1089af67818c7c673a4b76ad31aa823d4921a37013ce868617cac8b208cd0be1022733e7186b7e378ce1852544200a3998971e732d8510a1220a0cd41533d41444017a9e4fc401b66f4207b9d9eb16d7d9b747aa26fb76b953910b8cab6c5efa9bd9c181a4064336265323930653637376363653930636236333030663538346463323962323031623566613133366161633539613561316565633461633464303031663539", +}Then modify the function signature and implementation to accept these parameters:
func StressIdentityUpdates( ctx context.Context, logger *zap.Logger, n int, contractAddress, rpc, privateKey string, + params *IdentityUpdateParams, ) error { + // Use default params if none provided + if params == nil { + params = &DefaultIdentityUpdateParams + } // ... rest of implementation cs := &CastSendCommand{ ContractAddress: contractAddress, Function: IDENTITY_UPDATES_SIGNATURE, - FunctionArgs: []string{IDENTITY_UPDATES_INBOX_ID, IDENTITY_UPDATES_PAYLOAD}, + FunctionArgs: []string{params.InboxID, params.Payload}, // ... rest of implementation }
🛑 Comments failed to post (5)
pkg/stress/cast.go (2)
21-41:
⚠️ Potential issuePotential command injection vulnerability in command building.
The command construction doesn't properly escape user-supplied values, which could allow command injection if malicious input is provided.
-func buildCastSendCommand(c *CastSendCommand) string { +func buildCastSendCommand(c *CastSendCommand) (string, error) { var cmd strings.Builder cmd.WriteString(fmt.Sprintf( - "cast send '%s' '%s'", + "cast send %s %s", c.ContractAddress, c.Function, )) for _, arg := range c.FunctionArgs { - cmd.WriteString(fmt.Sprintf(" '%s'", arg)) + cmd.WriteString(fmt.Sprintf(" %s", arg)) } cmd.WriteString(fmt.Sprintf(" --rpc-url %s", c.Rpc)) cmd.WriteString(fmt.Sprintf(" --private-key %s", c.PrivateKey)) if c.Nonce != nil { cmd.WriteString(fmt.Sprintf(" --nonce %d", *c.Nonce)) } - return cmd.String() + return cmd.String(), nil }Consider using exec.Command with direct arguments rather than building a shell command string:
func (c *CastSendCommand) Run(ctx context.Context) error { args := []string{"send", c.ContractAddress, c.Function} args = append(args, c.FunctionArgs...) args = append(args, "--rpc-url", c.Rpc, "--private-key", c.PrivateKey) if c.Nonce != nil { args = append(args, "--nonce", fmt.Sprintf("%d", *c.Nonce)) } cmd := exec.CommandContext(ctx, "cast", args...) // Rest of the implementation... }
43-60: 🛠️ Refactor suggestion
Improve context handling and error reporting.
The function creates a new context instead of using the provided one, and the error doesn't include the original error details.
func (c *CastSendCommand) Run(ctx context.Context) error { - cli := buildCastSendCommand(c) + cli, err := buildCastSendCommand(c) + if err != nil { + return fmt.Errorf("failed to build command: %w", err) + } - ctxwt, cancel := context.WithTimeout(context.Background(), 1*time.Minute) + ctxwt, cancel := context.WithTimeout(ctx, 1*time.Minute) defer cancel() cmd := exec.CommandContext(ctxwt, "bash", "-c", cli) var outBuf, errBuf bytes.Buffer cmd.Stdout = &outBuf cmd.Stderr = &errBuf if err := cmd.Run(); err != nil { - return fmt.Errorf("cast send failed: %s", errBuf.String()) + return fmt.Errorf("cast send failed: %w, stderr: %s", err, errBuf.String()) } return nil }Committable suggestion skipped: line range outside the PR's diff.
cmd/cli/main.go (1)
547-567: 🛠️ Refactor suggestion
Add validation for required parameters with reasonable defaults.
The function doesn't validate that Contract and Rpc are provided, nor does it set reasonable defaults for Count.
func identityUpdatesStress(logger *zap.Logger, options *CLI) { ctx := context.Background() + // Validate required parameters and set defaults + if options.IdentityUpdatesStress.Contract == "" { + logger.Fatal("contract address is required") + } + + if options.IdentityUpdatesStress.Rpc == "" { + // Use the RPC URL from global options + options.IdentityUpdatesStress.Rpc = options.Contracts.RpcUrl + logger.Info("using RPC URL from global options", zap.String("rpc", options.IdentityUpdatesStress.Rpc)) + } + + count := options.IdentityUpdatesStress.Count + if count <= 0 { + count = 10 // Default to 10 transactions + logger.Info("using default transaction count", zap.Int("count", count)) + } + logger.Info( "creating identity updates", - zap.Int("count", options.IdentityUpdatesStress.Count), + zap.Int("count", count), zap.String("contract", options.IdentityUpdatesStress.Contract), ) err := stress.StressIdentityUpdates( ctx, logger, - options.IdentityUpdatesStress.Count, + count, options.IdentityUpdatesStress.Contract, options.IdentityUpdatesStress.Rpc, options.IdentityUpdatesStress.PrivateKey, ) if err != nil { logger.Fatal("could not create identity updates", zap.Error(err)) } }Committable suggestion skipped: line range outside the PR's diff.
pkg/stress/stress.go (2)
80-80:
⚠️ Potential issueAvoid exposing private key in error messages.
The private key is included in the error message, which could lead to accidental exposure in logs.
- return 0, fmt.Errorf("failed to get nonce for address %s: %s", privateKey, err) + return 0, fmt.Errorf("failed to get nonce: %s", 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.return 0, fmt.Errorf("failed to get nonce: %s", err)
22-64: 🛠️ Refactor suggestion
Add concurrency control and error aggregation.
The function launches all transactions at once without limiting concurrency, and doesn't aggregate errors from goroutines.
func StressIdentityUpdates( ctx context.Context, logger *zap.Logger, n int, contractAddress, rpc, privateKey string, ) error { var wg sync.WaitGroup + maxConcurrent := 10 // Limit concurrent transactions + sem := make(chan struct{}, maxConcurrent) + + // Create a channel to collect errors + errorCh := make(chan error, n) startingNonce, err := getCurrentNonce(ctx, privateKey, rpc) if err != nil { return fmt.Errorf("failed to get starting nonce: %s", err) } for i := 0; i < n; i++ { wg.Add(1) nonce := startingNonce + i cs := &CastSendCommand{ ContractAddress: contractAddress, Function: IDENTITY_UPDATES_SIGNATURE, FunctionArgs: []string{IDENTITY_UPDATES_INBOX_ID, IDENTITY_UPDATES_PAYLOAD}, Rpc: rpc, PrivateKey: privateKey, Nonce: &nonce, } go func(idx int) { defer wg.Done() + + // Acquire semaphore + sem <- struct{}{} + defer func() { <-sem }() logger.Info("Starting transaction", zap.Int("idx", idx)) if err := cs.Run(ctx); err != nil { logger.Error("error", zap.Int("idx", idx), zap.Error(err)) + errorCh <- fmt.Errorf("transaction %d failed: %w", idx, err) } else { logger.Info("Completed transaction", zap.Int("idx", idx)) } }(i) } wg.Wait() + close(errorCh) + + // Collect errors + var errors []error + for err := range errorCh { + errors = append(errors, err) + } + + // Return aggregate error if any transactions failed + if len(errors) > 0 { + return fmt.Errorf("%d of %d transactions failed: %v", len(errors), n, errors) + } return nil }📝 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 StressIdentityUpdates( ctx context.Context, logger *zap.Logger, n int, contractAddress, rpc, privateKey string, ) error { var wg sync.WaitGroup // Limit concurrent in-flight transactions maxConcurrent := 10 sem := make(chan struct{}, maxConcurrent) // Channel to collect per-transaction errors errorCh := make(chan error, n) startingNonce, err := getCurrentNonce(ctx, privateKey, rpc) if err != nil { return fmt.Errorf("failed to get starting nonce: %s", err) } for i := 0; i < n; i++ { wg.Add(1) nonce := startingNonce + i cs := &CastSendCommand{ ContractAddress: contractAddress, Function: IDENTITY_UPDATES_SIGNATURE, FunctionArgs: []string{IDENTITY_UPDATES_INBOX_ID, IDENTITY_UPDATES_PAYLOAD}, Rpc: rpc, PrivateKey: privateKey, Nonce: &nonce, } go func(idx int) { defer wg.Done() // Acquire slot sem <- struct{}{} defer func() { <-sem }() logger.Info("Starting transaction", zap.Int("idx", idx)) if err := cs.Run(ctx); err != nil { logger.Error("error", zap.Int("idx", idx), zap.Error(err)) errorCh <- fmt.Errorf("transaction %d failed: %w", idx, err) } else { logger.Info("Completed transaction", zap.Int("idx", idx)) } }(i) } wg.Wait() close(errorCh) // Aggregate errors var errs []error for e := range errorCh { errs = append(errs, e) } if len(errs) > 0 { return fmt.Errorf("%d of %d transactions failed: %v", len(errs), n, errs) } return nil }
neekolas
left a comment
neekolas
left a comment
There was a problem hiding this comment.
Have you run this enough to be sure that there aren't any nonce-related problems if some transactions revert or fail?
There are "nonce too high" problems, because the goroutines are executed without specific order and the mempool rejects when there's a nonce too high. Edit: Fixed it with a cheap |
mkysel
left a comment
mkysel
left a comment
There was a problem hiding this comment.
LGTM. Lets take this puppy for a ride
Add identity updates stress testing functionality to CLI with parallel transaction execution
CastSendCommandfunctionality in cast.go for executing Ethereum transactions via the cast CLI toolIdentityUpdatesStressOptionsstructidentity-updates-stresscommand in main.go to execute stress tests with configurable parameters📍Where to Start
Start with the
identityUpdatesStressfunction in main.go which handles the new CLI command and orchestrates the stress testing flow.Macroscope summarized e074963.
Summary by CodeRabbit