Problem: cometbft rpc is more efficient to broadcast tx #1651
Conversation
yihuang
requested review from
devashishdxt and
leejw51crypto
and removed request for
a team
WalkthroughThe changes encompass updates to the Changes
Possibly related PRs
Suggested reviewers
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Signed-off-by: yihuang <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1651 +/- ##
=======================================
Coverage 16.64% 16.64%
=======================================
Files 72 72
Lines 6212 6212
=======================================
Hits 1034 1034
Misses 5055 5055
Partials 123 123 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (6)
testground/benchmark/benchmark/peer.py (1)
Line range hint
153-153: Consider updating minimum-gas-prices to use DEFAULT_DENOMWhile the changes to use
DEFAULT_DENOMare generally good, there's a potential inconsistency in thedefault_app_patchdictionary. Theminimum-gas-pricesis still set to "0basecro", which may not be consistent with the newDEFAULT_DENOM.Consider updating this line to use
DEFAULT_DENOM:"minimum-gas-prices": f"0{DEFAULT_DENOM}",This will ensure that the minimum gas price denomination is consistent with the rest of the changes.
CHANGELOG.md (1)
Line range hint
57-196: Comprehensive changelog with multiple significant releases.The changelog covers several major and minor releases, including v1.1.0-rc0, v1.0.7, v1.0.6, and earlier versions. Key points:
- Multiple state machine breaking changes across versions.
- Significant improvements in performance, especially with memiavl and versiondb.
- Integration of new modules and features like ICA and IBC fee middleware.
- Numerous bug fixes addressing issues in JSON-RPC, EVM compatibility, and IBC functionality.
- Regular updates to dependencies like Cosmos SDK, IBC-go, and Ethermint.
The frequent releases and ongoing improvements indicate active development and maintenance of the project. However, the number of breaking changes suggests that users need to be cautious when upgrading between versions.
Consider implementing a more robust upgrade path for users, possibly including:
- Detailed upgrade guides for each breaking change.
- Automated upgrade scripts where possible.
- A longer-term roadmap to stabilize the state machine and reduce the frequency of breaking changes.
🧰 Tools
🪛 Markdownlint
17-17: null
Emphasis used instead of a heading(MD036, no-emphasis-as-heading)
testground/benchmark/benchmark/cosmostx.py (4)
81-81: Avoid using reserved keywords with trailing underscores.The field name
from_in line 81 uses a trailing underscore to avoid clashing with the reserved keywordfrom. While this is acceptable, it may lead to confusion. Consider using a more descriptive field name likefrom_addressfor clarity.Apply this diff if you decide to rename the field:
- from_ = Field("bytes", 5) + from_address = Field("bytes", 5)
85-86: Consider adding documentation for the empty classExtensionOptionsEthereumTx.The class
ExtensionOptionsEthereumTxis currently empty. Adding a docstring can clarify its purpose and future implementation plans, which improves maintainability and readability.Example:
class ExtensionOptionsEthereumTx(ProtoEntity): + """Placeholder for Ethereum transaction extension options.""" pass
1-3: Group import statements together.The import statements in lines 1 and 3 are separated by a blank line. Grouping them together improves code readability and adheres to PEP 8 style guidelines.
Apply this diff to group the imports:
-from typing import Optional - -from cprotobuf import Field, ProtoEntity +from typing import Optional +from cprotobuf import Field, ProtoEntity
18-86: Consider adding unit tests for the new classes.The classes defined in this file are crucial for handling transaction-related data. Adding unit tests ensures that they work as expected and helps prevent future regressions.
Do you want assistance in generating unit tests for these classes or opening a GitHub issue to track this task?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
testground/benchmark/poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
- CHANGELOG.md (1 hunks)
- testground/benchmark/benchmark/cosmostx.py (1 hunks)
- testground/benchmark/benchmark/peer.py (1 hunks)
- testground/benchmark/benchmark/transaction.py (4 hunks)
- testground/benchmark/benchmark/utils.py (1 hunks)
- testground/benchmark/overlay.nix (1 hunks)
- testground/benchmark/pyproject.toml (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- testground/benchmark/benchmark/utils.py
🧰 Additional context used
🔇 Additional comments (15)
testground/benchmark/pyproject.toml (2)
22-22: Minor formatting adjustmentThe space before
jsonmerge = "^1.9.2"has been removed, aligning it with other dependencies in the file. This change improves consistency in the file formatting.
23-23: New dependency added: cprotobufThe addition of
cprotobuf = "^0.1.11"as a new dependency is noted. This library is likely being introduced to handle Protocol Buffers in Python, which aligns with the PR objective of improving efficiency in transaction broadcasting.However, there are a few points to consider:
- The version constraint
^0.1.11allows for compatible updates but may include breaking changes as it's a pre-1.0 version. Consider if a stricter version constraint is necessary for stability.- It's important to ensure that this dependency is actually used in the project code. If it's not yet utilized, it might be premature to add it to the dependencies.
To confirm the usage of this new dependency, please run the following script:
If the script doesn't return any results, consider removing this dependency until it's actually needed in the code.
✅ Verification successful
Adjusting Search for
cprotobufUsageThe previous attempt to search for
cprotobufusage encountered an issue becausergdoes not recognizepythonas a valid file type. To accurately verify the usage ofcprotobuf, please run the following corrected script:
Dependency
cprotobufis correctly utilizedThe
cprotobufdependency is imported in the following files:
testground/benchmark/benchmark/cosmostx.pyintegration_tests/test_streamer.pyintegration_tests/ibc_utils.pyThis confirms that the dependency is in use and necessary for the project.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for imports or usage of cprotobuf in Python files rg --type python 'import\s+cprotobuf|from\s+cprotobuf\s+import'Length of output: 96
Script:
# #!/bin/bash # Search for imports or usage of cprotobuf in Python files rg -g "*.py" 'import\s+cprotobuf|from\s+cprotobuf\s+import'Length of output: 311
testground/benchmark/overlay.nix (1)
13-13: Addition of cprotobuf to buildSystems looks appropriate.The addition of
cprotobuf = [ "setuptools" ];to thebuildSystemsattribute set is consistent with the existing structure and purpose of this configuration. This change indicates that thecprotobufpackage will use thesetuptoolsbuild system, which is a common choice for Python packages.A few points to consider:
- This change aligns with the PR objectives of improving efficiency, as it may be related to the integration of new protocol buffer entities mentioned in the AI-generated summary.
- The modification is minimal and follows the established pattern in the file, which suggests it's a low-risk change.
- It's worth verifying that the
cprotobufpackage is indeed compatible withsetuptoolsand that this configuration is necessary for the project.To ensure this change is justified and correctly implemented, please run the following verification script:
This script will help confirm the necessity of adding
cprotobufto thebuildSystemsand ensure it's being used in the project as expected.testground/benchmark/benchmark/peer.py (5)
14-14: LGTM: Import of DEFAULT_DENOMThe addition of
DEFAULT_DENOMto the import list fromutilsis appropriate and consistent with its usage throughout the file. This change centralizes the denomination definition, which is a good practice for maintainability.
Line range hint
20-22: LGTM: Updated balance definitions using DEFAULT_DENOMThe changes to
VAL_INITIAL_AMOUNT,VAL_STAKED_AMOUNT, andACC_INITIAL_AMOUNTto useDEFAULT_DENOMinstead of the hardcoded "basecro" are appropriate. This improves maintainability and consistency throughout the file while preserving the existing amount values.
Line range hint
47-51: LGTM: Updated init command with DEFAULT_DENOMThe use of
DEFAULT_DENOMas thedefault_denomparameter in theinitcommand call is appropriate. This change ensures consistency with the globally defined denomination and improves maintainability.
Line range hint
127-131: LGTM: Updated genesis configuration with DEFAULT_DENOMThe use of
DEFAULT_DENOMin theevmparameters of the genesis patch is appropriate. This change ensures consistency with the globally defined denomination and improves maintainability of the genesis configuration.
Line range hint
1-238: Overall assessment: Good changes with one minor suggestionThe changes in this file consistently introduce and use the
DEFAULT_DENOMconstant, which improves maintainability and consistency. The modifications to balance definitions, node initialization, and genesis configuration all align well with this goal.However, there's one minor suggestion to consider:
- Update the
minimum-gas-pricesin thedefault_app_patchto useDEFAULT_DENOMfor complete consistency.Once this small change is addressed, the modifications will fully achieve the goal of centralizing and standardizing the denomination usage throughout the file.
CHANGELOG.md (3)
15-15: New feature added: Cosmos broadcast RPC benchmark.A new benchmark for using Cosmos broadcast RPC has been added. This is likely an improvement to measure and potentially optimize the performance of RPC calls.
Line range hint
3-13: Multiple bug fixes and improvements in the unreleased version.The unreleased version includes several bug fixes and improvements:
- Fixed running single validator benchmark locally.
- Fixed an issue where the node couldn't shut down by signal.
- Improved test transaction generation in parallel for single node setups.
- Enhanced load generator with backoff retry on errors.
- Added abort operation in PrepareProposal function.
These changes appear to address stability issues and improve testing capabilities.
🧰 Tools
🪛 Markdownlint
17-17: null
Emphasis used instead of a heading(MD036, no-emphasis-as-heading)
Line range hint
17-55: Significant updates in v1.1.0-rc1 release.This release candidate includes several important changes:
- State machine breaking change in memiavl's initial version logic.
- Multiple improvements, including changes to block-stm parallelism, Ethermint updates, and IBC channel upgrade support.
- Several bug fixes, including query fixes and updates to Ethermint and Cosmos SDK.
These changes seem to focus on performance improvements and bug fixes. The state machine breaking change should be carefully considered during upgrades.
To verify the impact of the state machine breaking change, we should check for any migration scripts or upgrade handlers. Run the following:
🧰 Tools
🪛 Markdownlint
17-17: null
Emphasis used instead of a heading(MD036, no-emphasis-as-heading)
testground/benchmark/benchmark/transaction.py (4)
2-2: Imports updated appropriately to support new functionalityThe added imports for
base64,HexBytes, and modulescosmostx,DEFAULT_DENOM, andLOCAL_RPCare necessary and correctly included to support the new transaction construction and broadcasting logic.Also applies to: 13-13, 15-15, 17-17
69-71: Transaction signing and Cosmos transaction building are correctly implementedThe modifications in the
_do_jobfunction to sign transactions and build Cosmos transactions usingbuild_cosmos_txare appropriate. The use ofHexBytes(acct.address)ensures that the sender's address is correctly formatted as bytes.
Line range hint
145-161: Confirm the correct usage ofbroadcast_tx_asyncin JSON-RPC callIn the
async_sendtxfunction, the JSON-RPC methodbroadcast_tx_asyncis used with parameters{ "tx": raw }. Verify that this matches the expected format for the CometBFT (Tendermint) RPC API and that therawtransaction is correctly encoded as a base64 string. Incorrect parameters or encoding may result in transaction broadcast failures.To confirm the correct usage, you can run the following script:
#!/bin/bash # Description: Verify that the JSON-RPC call to broadcast transactions is correctly formed. # Test: Search for all usages of 'broadcast_tx_async' to ensure consistency. rg --type python -A 5 '"broadcast_tx_async"' # Additionally, compare the parameters with the official CometBFT RPC documentation.
126-138: Verify the correctness of fee calculation and denominationEnsure that the fee calculation
fee = tx["gas"] * tx["gasPrice"]accurately represents the transaction fee within the Cosmos SDK context. Also, confirm thatDEFAULT_DENOMcorresponds to the correct denomination used by the network for fee payments and that no unit conversions are necessary.To verify the fee calculation and denomination, run the following script:
✅ Verification successful
Fee Calculation and Denomination Verified
The fee calculation
fee = tx["gas"] * tx["gasPrice"]is correctly implemented, andDEFAULT_DENOMis consistently set to"basecro", aligning with the network's fee denomination. No unit conversions are necessary.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that DEFAULT_DENOM is correctly used and fee calculations are consistent. # Test 1: Check where DEFAULT_DENOM is defined and its usage. rg --type python 'DEFAULT_DENOM' # Test 2: Ensure fee calculation patterns are consistent across the codebase. rg --type python 'fee\s*=\s*tx\["gas"\]\s*\*\s*tx\["gasPrice"\]'Length of output: 171
Script:
#!/bin/bash # Description: Verify that DEFAULT_DENOM is correctly used and fee calculations are consistent. # Test 1: Check where DEFAULT_DENOM is defined and its usage. rg --type py 'DEFAULT_DENOM' # Test 2: Ensure fee calculation patterns are consistent across the codebase. rg --type py 'fee\s*=\s*tx\["gas"\]\s*\*\s*tx\["gasPrice"\]'Length of output: 1073
Signed-off-by: yihuang <[email protected]>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: yihuang <[email protected]>
👮🏻👮🏻👮🏻 !!!! 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
New Features
Bug Fixes
Improvements
Documentation