test: add Solana E2E performance tests and improve Solana outbounds performance #3276
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces several modifications across multiple files, primarily focusing on enhancing Solana-related functionalities within the ZetaChain node. Key changes include the addition of performance testing routines for Solana deposits and withdrawals, updates to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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
Documentation and Community
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3276 +/- ##
===========================================
- Coverage 61.70% 61.62% -0.09%
===========================================
Files 432 433 +1
Lines 30994 31037 +43
===========================================
Hits 19126 19126
- Misses 11009 11052 +43
Partials 859 859
|
skosito
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (10)
e2e/e2etests/test_stress_spl_withdraw.go (2)
49-82: Optimize synchronization by replacingsync.Mutexwith a thread-safe data structure.Using a mutex to protect access to the
withdrawDurationsslice can lead to contention under high concurrency. Consider using a concurrent data structure likesync.ConcurrentSliceor channel-based aggregation to improve performance and reduce complexity.Refactor the code to use a channel for collecting durations:
withdrawDurationsChan := make(chan float64, numWithdrawalsSPL) // In the goroutine: withdrawDurationsChan <- timeToComplete.Seconds() // After eg.Wait(): close(withdrawDurationsChan) // Collect durations from the channel for duration := range withdrawDurationsChan { withdrawDurations = append(withdrawDurations, duration) }This approach eliminates the need for explicit locking and can improve throughput.
95-102: Enhance the latency report by formatting output consistently.The current logging statements use different formats, which might lead to inconsistent and less readable output. Standardize the formatting for clarity.
Update the logging statements for consistent alignment:
r.Logger.Print("Latency report:") r.Logger.Print("min: %.2f seconds", desc.Min) r.Logger.Print("max: %.2f seconds", desc.Max) r.Logger.Print("mean: %.2f seconds", desc.Mean) r.Logger.Print("std: %.2f seconds", desc.Std) for _, p := range desc.DescriptionPercentiles { r.Logger.Print("p%2.0f: %.2f seconds", p.Percentile, p.Value) }This change improves the readability of the latency report.
e2e/e2etests/test_stress_spl_deposit.go (1)
36-36: Clarify usage ofmonitorDepositfunctionThe function
monitorDepositis called but is not defined within this file. Since it's defined intest_stress_solana_deposit.go, consider movingmonitorDepositto a shared utility file within thee2etestspackage for better code reuse and maintainability.Apply this diff to reorganize the code:
-Move the `monitorDeposit` function from `test_stress_solana_deposit.go` to a new file `utils.go` within the `e2etests` package.e2e/e2etests/test_stress_solana_deposit.go (1)
45-60: Add timeout handling tomonitorDepositIn the
monitorDepositfunction, consider implementing a timeout mechanism or utilizing a context with timeout to prevent indefinite blocking in case of network delays or issues with the cross-chain transaction.Apply this diff to enhance robustness:
func monitorDeposit(r *runner.E2ERunner, sig solana.Signature, index int, startTime time.Time) error { + ctx, cancel := context.WithTimeout(r.Ctx, r.ReceiptTimeout) + defer cancel() - cctx := utils.WaitCctxMinedByInboundHash(r.Ctx, sig.String(), r.CctxClient, r.Logger, r.ReceiptTimeout) + cctx := utils.WaitCctxMinedByInboundHash(ctx, sig.String(), r.CctxClient, r.Logger, r.ReceiptTimeout)cmd/zetae2e/local/performance.go (2)
113-160: Consider extracting timeout values as constants.The implementation is well-structured and follows the established patterns. Consider extracting the magic numbers for timeout values (15 minutes) into named constants for better maintainability.
+const ( + DefaultReceiptTimeout = 15 * time.Minute + DefaultCctxTimeout = 15 * time.Minute +)
162-229: Consider parameterizing the deposit amount.The implementation is solid, but the hardcoded deposit amount of 100 SOL could limit test flexibility. Consider making this configurable through the test configuration.
-amount := big.NewInt(0).Mul(big.NewInt(1e9), big.NewInt(100)) // 100 sol in lamports +amount := big.NewInt(0).Mul(big.NewInt(1e9), big.NewInt(conf.SolanaTestDepositAmount)) // Convert configured SOL to lamportscmd/zetae2e/local/local.go (1)
386-403: Consider adding error handling for concurrent test executionWhile the parallel execution of Solana performance tests follows the existing pattern, consider adding error handling for potential race conditions or resource contention when running multiple stress tests concurrently.
eg.Go( - solanaDepositPerformanceRoutine( + func() error { + // Add semaphore or rate limiting here + return solanaDepositPerformanceRoutine( conf, deployerRunner, verbose, e2etests.TestStressSolanaDepositName, e2etests.TestStressSPLDepositName, - ), + ) + }, )changelog.md (3)
Line range hint
1-1: Add version dates to improve changelog tracking.The changelog should include dates for each version release to help track when changes were made.
Add release dates in ISO 8601 format (YYYY-MM-DD) after each version number. For example:
- # CHANGELOG + # CHANGELOG + + All notable changes to this project will be documented in this file. + The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
Line range hint
3-24: Standardize version prefix format.The version numbers should consistently use the 'v' prefix.
Apply consistent version prefixing:
-## Unreleased +## [Unreleased] -### Features +## [v23.0.0] - YYYY-MM-DD
Line range hint
1-1000: Standardize section headers across versions.The changelog uses inconsistent section headers across different versions. Some use H2 (##) while others use H3 (###).
Standardize all version headers to H2 (##) and all category headers (Features, Fixes, etc.) to H3 (###).
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (12)
Makefile(1 hunks)changelog.md(1 hunks)cmd/zetae2e/local/local.go(4 hunks)cmd/zetae2e/local/performance.go(2 hunks)contrib/localnet/docker-compose.yml(1 hunks)contrib/localnet/solana/start-solana.sh(1 hunks)e2e/e2etests/e2etests.go(2 hunks)e2e/e2etests/test_stress_solana_deposit.go(1 hunks)e2e/e2etests/test_stress_solana_withdraw.go(1 hunks)e2e/e2etests/test_stress_spl_deposit.go(1 hunks)e2e/e2etests/test_stress_spl_withdraw.go(1 hunks)e2e/runner/solana.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
contrib/localnet/solana/start-solana.sh (1)
Pattern **/*.sh: Review the shell scripts, point out issues relative to security, performance, and maintainability.
e2e/e2etests/test_stress_spl_deposit.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/e2etests/test_stress_spl_withdraw.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/e2etests/test_stress_solana_deposit.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/e2etests/test_stress_solana_withdraw.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/runner/solana.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
cmd/zetae2e/local/local.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/e2etests/e2etests.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
cmd/zetae2e/local/performance.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
🔇 Additional comments (9)
contrib/localnet/solana/start-solana.sh (1)
12-13: Approved
The increased SOL airdrop amounts are appropriate for stress testing and help prevent insufficient fund errors during high-load scenarios.
contrib/localnet/docker-compose.yml (1)
226-227: LGTM: Service configuration aligns with stress testing requirements.
The addition of the stress profile and restart policy is appropriate for maintaining service availability during stress tests.
e2e/runner/solana.go (1)
277-277: Verify SPL token amount implications.
The significant increase in minting amount (100 trillion tokens) warrants verification:
- Ensure this amount doesn't exceed any Solana program limits
- Verify that this amount is sufficient but not excessive for stress tests
✅ Verification successful
Let me gather more specific information about token-related constants and usage in the Solana context.
Let me search specifically for Solana token-related configurations and decimals.
SPL token amount increase appears safe for testing purposes
The increase to 100 trillion tokens (100_000_000_000_000) is acceptable for the following reasons:
- The amount is within reasonable bounds for SPL token testing
- No explicit maximum supply limitations were found in the codebase
- The change is isolated to test environment setup
- Other chains in the codebase (TON, EVM) commonly use similar large denominations for testing
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any token amount limitations in the codebase
# Search for token amount validations in Solana-related code
rg -A 2 "amount.*exceed|max.*amount|limit.*amount" --type go
# Look for existing token amount constants that might indicate limits
ast-grep --pattern 'const $_: $_ = $_{1e9}'
Length of output: 2424
Script:
#!/bin/bash
# Search for token amount definitions and usage in Solana-related files
rg -A 2 "token.*amount|mint.*amount|NewMintTo" --type go
# Look for any existing SPL token amount patterns
rg -A 2 "1_000_000_000|100_000_000_000_000" --type go
# Search for any Solana token decimals or precision configurations
rg "decimals.*solana|precision.*token" --type go
Length of output: 36416
Script:
#!/bin/bash
# Search for Solana token configurations and decimals
rg -A 2 "solana.*token.*decimal|spl.*token.*decimal" --type go
# Look for any token mint amount validations in Solana-related code
rg -A 2 "mint.*amount.*check|token.*amount.*check" --type go
# Search for any Solana SPL token standards or specifications
rg -A 2 "spl.*token.*standard|solana.*token.*standard" --type go
Length of output: 194
Makefile (1)
278-280: LGTM: Correct dependency addition for Solana tests
The addition of the solana dependency to start-e2e-performance-test target ensures that the Solana environment is properly built before running performance tests.
cmd/zetae2e/local/local.go (2)
113-113: LGTM: Well-structured flag combination
The setupSolana flag elegantly combines both test scenarios requiring Solana setup, following the DRY principle.
Line range hint 231-247: LGTM: Proper conditional Solana setup
The Solana setup is correctly guarded by the setupSolana flag and properly integrated into the deployment flow.
e2e/e2etests/e2etests.go (2)
114-117: LGTM: Consistent test naming convention
The new test name constants follow the established naming pattern and are self-descriptive.
774-809: Verify the default test values for production use
The test definitions are well-structured, but consider:
- The default values (1,200,000 lamports ≈ 0.0012 SOL) might need adjustment based on production requirements
- The count of 30 iterations aligns with the timeout constraints mentioned in PR objectives
✅ Verification successful
Default test values appear to be appropriately configured
The stress test values for Solana operations are well-calibrated when compared to other chain configurations in the test suite:
- ETH stress tests use 100 iterations vs. Solana's 30, which is appropriate given Solana's faster block times
- SOL deposit amount (1.2M lamports ≈ 0.0012 SOL) aligns with test amounts used for other chains (e.g., 0.001 BTC)
- The iteration count of 30 is consistent across all Solana-related tests (both SOL and SPL)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if similar tests exist in the codebase with different default values
# Search for other stress test definitions with default values
rg -A 5 'TestStress.*Name.*DefaultValue' --type go
Length of output: 52
Script:
#!/bin/bash
# Let's try a broader search to find stress test definitions and their values
rg -B 2 -A 5 'TestStress.*Name.*' --type go
# Also search for any test configuration or constants that might define these values
rg -B 2 -A 2 'const.*[Ll]amports|const.*SPL' --type go
# Search for any documentation about test values
fd README.md | xargs rg -i "test.*value|stress.*test"
Length of output: 6317
changelog.md (1)
14-14:
Fix broken link reference.
The link reference [3276] is missing its URL.
Add the missing URL:
-* [3276](https://github.com/zeta-chain/node/pull/3276) - solana performance tests
+* [3276](https://github.com/zeta-chain/node/pull/3276) - solana performance testsLikely invalid or redundant comment.
i tried |
skosito
changed the title
skosito
changed the title
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 14567965 | Triggered | Generic Password | cb0bb04 | cmd/zetaclientd/start.go | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
…erformance (#3276) * stress tests deposit solana * perf tests for sol withdrawals * split withdrawal stress tests into separate tests * separate deposit stress tests * add restart always to solana container * tweak params * fmt and changelog * cleanup * making tests execution more granular * revert last change * split perf tests * tweak params * decrease num of withdrawals back to 30 * run only withdrawals * run sequentiallyx * uncomment tests * params * fix: speed up Solana withdraw stress test in two go routines (#3295) * PR comment and lint * changelog * move account info decoding to solana package * revert timeouts to prev values --------- Co-authored-by: Charlie Chen <[email protected]>
Description
contains fixes from this PR to improve outbounds performance: #3295 which should probably be backported (cc: @lumtis)
How Has This Been Tested?
Summary by CodeRabbit
Release Notes
New Features
/systemtimetelemetry endpoint in thezetaclient.Bug Fixes
Tests
Chores