Problem: testground test case not fast enough#1495
Problem: testground test case not fast enough#1495yihuang merged 4 commits intocrypto-org-chain:mainfrom
Conversation
Solution: - patch cronosd temporarily to disable nonce checking - optimise the load generator
yihuang
requested review from
calvinaco and
mmsqe
and removed request for
a team
|
Warning Rate limit exceeded@yihuang has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 28 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThis update primarily focuses on version upgrades, code refactoring, and functionality enhancements across various files. Key areas of improvement include updating dependencies in Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App
participant TxHandler as TransactionHandler
participant EthAccount as EthereumAccountManager
User ->> App: Initiate Transfer
App ->> TxHandler: Process Transaction
TxHandler ->> EthAccount: Validate Account
EthAccount -->> TxHandler: Account Validated
TxHandler -->> App: Transaction Processed
App -->> User: Transfer Complete
Poem
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 as PR comments)
Additionally, you can add CodeRabbit Configration 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: 3
Outside diff range and nitpick comments (3)
testground/benchmark/benchmark/utils.py (2)
Line range hint
47-48: Use modern string formatting inwait_for_port.Replace the
formatcall with an f-string for better readability and performance.- "Waited too long for the port {} on host {} to start accepting " - "connections.".format(port, host) + f"Waited too long for the port {port} on host {host} to start accepting connections."
Line range hint
54-54: Refactor loop control variable inwait_for_block.The loop variable
iis unused within the loop body. Consider renaming it to_ito indicate it is intentionally unused.- for i in range(timeout): + for _i in range(timeout):testground/benchmark/benchmark/main.py (1)
Line range hint
73-76: Improve timeout handling in subprocess management.Replace the existing try-except block with
contextlib.suppressfor handling theTimeoutExpiredexception from subprocesses. This results in cleaner and more readable code.- try: - proc.wait(5) - except subprocess.TimeoutExpired: - pass + with contextlib.suppress(subprocess.TimeoutExpired): + proc.wait(5)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
Files selected for processing (12)
- go.mod (1 hunks)
- gomod2nix.toml (1 hunks)
- nix/testground-cronosd.patch (1 hunks)
- nix/testground-image.nix (1 hunks)
- testground/benchmark/benchmark/context.py (2 hunks)
- testground/benchmark/benchmark/main.py (3 hunks)
- testground/benchmark/benchmark/params.py (1 hunks)
- testground/benchmark/benchmark/peer.py (3 hunks)
- testground/benchmark/benchmark/sendtx.py (1 hunks)
- testground/benchmark/benchmark/sync.py (1 hunks)
- testground/benchmark/benchmark/utils.py (2 hunks)
- testground/benchmark/compositions/local.toml (1 hunks)
Additional context used
Ruff
testground/benchmark/benchmark/sendtx.py
14-14: Loop control variable
inot used within loop bodyRename unused
ito_i(B007)
testground/benchmark/benchmark/utils.py
47-48: Use f-string instead of
formatcallConvert to f-string
(UP032)
54-54: Loop control variable
inot used within loop bodyRename unused
ito_i(B007)
testground/benchmark/benchmark/main.py
33-33: Use context handler for opening files
(SIM115)
73-76: Use
contextlib.suppress(subprocess.TimeoutExpired)instead oftry-except-passReplace with
contextlib.suppress(subprocess.TimeoutExpired)(SIM105)
Additional comments not posted (16)
nix/testground-cronosd.patch (1)
9-9: Verify the security implications of enablingUnsafeUnorderedTx.Adding
UnsafeUnorderedTx: truepotentially affects the transaction handling logic, which could have security implications. Please ensure this change aligns with the application's security model and is thoroughly tested.nix/testground-image.nix (1)
3-7: Ensure the patch is applied correctly in the Docker image configuration.The
patched-cronosddefinition applies thetestground-cronosd.patch. Verify that the patch is correctly integrated and does not introduce any issues in the Docker image.testground/benchmark/compositions/local.toml (1)
16-18: Verify the appropriateness of the test configuration changes.The configuration for
num_accountsandnum_txshas been updated. Please verify that these changes align with the intended test scenarios and do not introduce any performance or scalability issues.testground/benchmark/benchmark/main.py (4)
3-6: Add new imports for concurrency, file path handling, and Ethereum web3 interactions.These additions support the new functionalities introduced in the file, specifically for handling multiple transactions concurrently, managing file paths more robustly, and interacting with Ethereum. Ensure these libraries are included in the project's dependencies.
11-12: Introduction of new functions from sendtx module.The introduction of
fund_test_accountsandsendtxfunctions from thesendtxmodule aligns with the PR's objective to optimize the load generator. This change should improve the performance of the test cases by enabling more efficient management of transactions.
[APROVED]
27-34: Refactor node startup process to use thePathclass for log file management.Using
Pathfrom thepathlibmodule for handling file paths is a more modern and robust approach compared to traditional string-based paths. This change enhances the readability and maintainability of the code.Tools
Ruff
33-33: Use context handler for opening files
(SIM115)
40-60: Enhanced transaction handling and concurrency using ThreadPoolExecutor.This block introduces concurrent transaction submissions using a thread pool, which is crucial for performance testing. However, ensure that the exception handling within the thread pool's tasks is robust enough to not crash the entire application.
testground/benchmark/benchmark/context.py (2)
7-7: Rename global sequence constant for clarity.Renaming
LEADER_GLOBAL_SEQUENCEtoLEADER_SEQUENCEsimplifies the terminology and aligns with the other naming conventions in the project. This change should make the codebase easier to understand for new contributors.
108-114: Add properties to determine leadership roles.The addition of
is_fullnode_leaderandis_validator_leaderproperties helps in determining the specific roles of nodes in the network, which is crucial for managing different behaviors during tests. This change enhances the control flow and decision-making in test scenarios.testground/benchmark/benchmark/params.py (1)
110-115: Add properties for managing accounts and transactions.The addition of
num_accountsandnum_txsproperties supports the PR's objective to optimize the load generator by allowing dynamic configuration of the number of accounts and transactions. This is crucial for scalability and flexibility in performance testing.testground/benchmark/benchmark/peer.py (2)
33-39: Refactor peer packet creation process.The changes in peer packet creation, including the introduction of
GenesisAccountinstances, enhance the clarity and modularity of the code. This refactoring helps in managing network configurations more effectively.
77-94: Update network configurations and toml patches.The extensive modifications to the
config.tomlandapp.tomlfiles, including settings for persistent peers, mempool, and indexer configurations, are critical for the correct operation of the network under test conditions. Ensure these configurations are thoroughly tested to prevent runtime issues.testground/benchmark/benchmark/sync.py (1)
73-75: Improve error handling in synchronization service.Adding a check for a
Noneresponse in the_requestfunction is a defensive programming practice that helps in handling unexpected disconnections more gracefully. This change enhances the robustness of the network synchronization services.go.mod (2)
279-279: Confirm the correctness of replace directives.The replace directives have been modified to point to specific versions or local paths. Please confirm that these changes are intentional and correctly align with the project's dependency management strategy.
279-279: Verify the updated dependency for potential issues.The dependency
github.com/evmos/etherminthas been updated to a specific commit. Please ensure that this version has been tested thoroughly for stability and does not introduce any known vulnerabilities.gomod2nix.toml (1)
265-266: Dependency Update ApprovedThe version and hash for
github.com/evmos/etherminthave been updated. This change is consistent with the PR's objectives to optimize a test case by updating dependencies.However, ensure that this new version does not introduce any known vulnerabilities.
2 participants
Solution:
Benchmark Results
Can record
360tps on a local m3 mac laptop, 3 validators + 5 full nodes + load generators all running in local docker containers in the same Linux VM,63blocks, each has around 1k transactions,2.9savg block time.EDIT: by allocate all 16 cpus to the VM, it can reach 849 tps (2292 transactions per block and 2.7 block time on avg).
👮🏻👮🏻👮🏻 !!!! REFERENCE THE PROBLEM YOUR ARE SOLVING IN THE PR TITLE AND DESCRIBE YOUR SOLUTION HERE !!!! DO NOT FORGET !!!! 👮🏻👮🏻👮🏻
PR Checklist:
make)make test)go fmt)golangci-lint run)go list -json -m all | nancy sleuth)Thank you for your code, it's appreciated! :)
Summary by CodeRabbit
Dependencies
github.com/evmos/ethermintto improve performance and stability.Refactor
config.tomlandapp.toml.New Features
Bug Fixes
Configuration