Problem: node can't shutdown by signal #1647
Conversation
Signed-off-by: yihuang <[email protected]>
Signed-off-by: yihuang <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1647 +/- ##
=======================================
Coverage 34.94% 34.94%
=======================================
Files 123 123
Lines 11803 11803
=======================================
Hits 4124 4124
Misses 7265 7265
Partials 414 414 |
yihuang
requested review from
calvinaco and
devashishdxt
and removed request for
a team
WalkthroughThe pull request includes 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
CHANGELOG.md (2)
5-7: Correct grammar in bug fix descriptionThere's a minor grammatical issue in the bug fix description.
Consider changing:
-* (cli)[#1647](https://github.com/crypto-org-chain/cronos/pull/1647) Fix node can't shutdown by signal. +* (cli)[#1647](https://github.com/crypto-org-chain/cronos/pull/1647) Fix node can't shut down by signal.This change uses the correct verb form "shut down" instead of the noun "shutdown".
🧰 Tools
🪛 LanguageTool
[grammar] ~7-~7: The word ‘shutdown’ is a noun. Did you mean the verb “shut down”?
Context: ...-chain/cronos/pull/1647) Fix node can't shutdown by signal. ### Improvements * [#1645]...(SHUTDOWN)
Line range hint
13-64: Significant state machine breaking change and comprehensive improvementsThe v1.4.0-rc1 release includes a significant state machine breaking change in the memiavl module, along with numerous improvements and bug fixes.
The state machine breaking change in the memiavl module is crucial and may require careful consideration during upgrades. Ensure that all node operators are aware of this change and its implications.
The improvements and bug fixes address various aspects of the system, including:
- Performance optimizations (e.g., changes to block-stm parallelism)
- API enhancements (e.g., avoiding unnecessary block results in header-related API calls)
- Bug fixes for account handling, transaction validation, and other critical areas
These changes appear to significantly enhance the system's functionality and reliability. However, due to the extensive nature of the changes, thorough testing is recommended before deployment to production environments.
🧰 Tools
🪛 LanguageTool
[grammar] ~7-~7: The word ‘shutdown’ is a noun. Did you mean the verb “shut down”?
Context: ...-chain/cronos/pull/1647) Fix node can't shutdown by signal. ### Improvements * [#1645]...(SHUTDOWN)
go.mod (1)
276-276: Add a comment explaining theethermintreplacementIncluding a comment to explain why
github.com/evmos/ethermintis replaced withgithub.com/crypto-org-chain/etherminthelps future maintainers understand the rationale behind this change and maintain the codebase more effectively.Apply this diff to add the explanatory comment:
+ // Replace ethermint with a custom version to fix node shutdown issue github.com/evmos/ethermint => github.com/crypto-org-chain/ethermint v0.6.1-0.20241017130935-816389c76eac
📜 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 (3)
- CHANGELOG.md (1 hunks)
- go.mod (1 hunks)
- gomod2nix.toml (1 hunks)
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.md
[grammar] ~7-~7: The word ‘shutdown’ is a noun. Did you mean the verb “shut down”?
Context: ...-chain/cronos/pull/1647) Fix node can't shutdown by signal. ### Improvements * [#1645]...(SHUTDOWN)
🔇 Additional comments (2)
gomod2nix.toml (1)
265-266: Version update and module replacement for ethermintThe
github.com/evmos/ethermintmodule has been updated to a newer commit (v0.6.1-0.20241017130935-816389c76eac) and is now replaced withgithub.com/crypto-org-chain/ethermint.Please ensure that the changes in the forked repository (
github.com/crypto-org-chain/ethermint) are reviewed and aligned with the project's requirements. Run the following script to compare the differences between the original and forked repositories:CHANGELOG.md (1)
8-11: LGTM: Improvements enhance system functionalityThe improvements listed here seem to enhance the system's functionality by optimizing test transaction generation and improving the load generator's error handling.
|
test_local_statesync seems flaky now |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
app/app.go (1)
1457-1462: Improved shutdown logging, but consider enhancing error handlingThe changes to the
Closemethod improve the logging behavior during application shutdown. The separation of message construction and error handling provides clearer and more consistent logging. However, there's room for a minor improvement in error handling.Consider modifying the error handling to provide more detailed information when multiple errors occur. Here's a suggested improvement:
- err := stderrors.Join(errs...) - msg := "Application gracefully shutdown" - if err == nil { - app.Logger().Info(msg) - } else { - app.Logger().Error(msg, "error", err) - } + err := stderrors.Join(errs...) + if err == nil { + app.Logger().Info("Application gracefully shutdown") + } else { + app.Logger().Error("Application shutdown with errors", "error", err) + // Log individual errors for more detailed debugging + for i, e := range errs { + if e != nil { + app.Logger().Error(fmt.Sprintf("Shutdown error %d", i+1), "error", e) + } + } + }This modification will provide more detailed error information when multiple errors occur during shutdown, aiding in debugging and troubleshooting.
Signed-off-by: mmsqe <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
integration_tests/test_basic.py (3)
380-383: LGTM! Consider extracting port configuration to a separate function.The JSON-RPC configuration looks good. It correctly uses dynamic port allocation to avoid conflicts.
For improved readability and maintainability, consider extracting the port configuration to a separate function:
def get_json_rpc_config(base_port): return { "address": f"127.0.0.1:{ports.evmrpc_port(base_port)}", "ws-address": f"127.0.0.1:{ports.evmrpc_ws_port(base_port)}", } # Usage cluster.edit_app_cfg( Path(home) / "config/app.toml", base_port, { "json-rpc": get_json_rpc_config(base_port), # ... other configurations ... }, )This refactoring would make the code more modular and easier to maintain.
384-386: LGTM! Consider adding a comment explaining versiondb.The addition of the versiondb configuration is appropriate for this test.
To improve code clarity, consider adding a brief comment explaining the purpose of enabling versiondb in this context:
"versiondb": { "enable": True, # Enable versiondb for efficient state management during statesync },This comment would help other developers understand the significance of this configuration.
Line range hint
329-452: LGTM! Consider enhancing error handling in the subprocess call.The
test_local_statesyncfunction is a well-structured and comprehensive test for local state synchronization. It covers all the necessary steps from setting up a new node to verifying the synced state.To improve error handling and resource management, consider using a context manager for the subprocess call:
import contextlib # ... (rest of the code) with contextlib.closing(subprocess.Popen( [cronos.chain_binary, "start", "--home", home], stdout=subprocess.PIPE, stderr=subprocess.STDOUT, )) as process: try: wait_for_port(ports.rpc_port(base_port)) # check the node sync normally wait_for_new_blocks(cli, 2) # check grpc works print("distribution", cli.distribution_community(height=height)) with pytest.raises(Exception) as exc_info: cli.distribution_community(height=height - 1) assert "collections: not found" in exc_info.value.args[0] finally: process.terminate() process.wait(timeout=10)This approach ensures that the subprocess is properly terminated even if an exception occurs during the test execution.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- integration_tests/test_basic.py (1 hunks)
🧰 Additional context used
🔇 Additional comments (1)
integration_tests/test_basic.py (1)
Line range hint
1-452: Overall, the changes in this file are well-implemented and valuable.The addition of the
test_local_statesyncfunction significantly improves the test coverage for state synchronization in the Cronos blockchain. The test is comprehensive and well-structured, covering all necessary steps of the statesync process.The suggestions provided in the review comments aim to further enhance the code quality by:
- Improving modularity with the extraction of the JSON-RPC configuration.
- Adding clarifying comments for the versiondb configuration.
- Enhancing error handling in the subprocess call.
These improvements will contribute to better maintainability and robustness of the test suite.
github-merge-queue
bot
removed this pull request from the merge queue due to a conflict with the base branch
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
CHANGELOG.md (1)
5-7: Correct grammar in bug fix description.There's a minor grammatical issue in the bug fix description.
Consider changing:
-* (cli)[#1647](https://github.com/crypto-org-chain/cronos/pull/1647) Fix node can't shutdown by signal. +* (cli)[#1647](https://github.com/crypto-org-chain/cronos/pull/1647) Fix node can't shut down by signal.This change uses the correct verb form "shut down" instead of the noun "shutdown".
🧰 Tools
🪛 LanguageTool
[grammar] ~7-~7: The word ‘shutdown’ is a noun. Did you mean the verb “shut down”?
Context: ...-chain/cronos/pull/1647) Fix node can't shutdown by signal. ### Improvements * [#1645]...(SHUTDOWN)
📜 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 (3)
- CHANGELOG.md (1 hunks)
- go.mod (1 hunks)
- gomod2nix.toml (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- go.mod
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.md
[grammar] ~7-~7: The word ‘shutdown’ is a noun. Did you mean the verb “shut down”?
Context: ...-chain/cronos/pull/1647) Fix node can't shutdown by signal. ### Improvements * [#1645]...(SHUTDOWN)
🔇 Additional comments (6)
gomod2nix.toml (2)
Line range hint
1-266: Overall dependency updates look goodThe changes in this file primarily consist of version updates for various dependencies. These updates likely include bug fixes, security patches, and potentially new features. Some key points to note:
- Most changes are minor version updates or commit hash changes, suggesting they are likely compatible with the existing codebase.
- Several modules are marked as replaced (e.g.,
github.com/ethereum/go-ethereum,github.com/syndtr/goleveldb), indicating the use of custom forks or versions. Ensure these custom versions are maintained and kept up-to-date with their upstream counterparts.- There are no new dependencies added or existing ones removed, which is good for maintaining stability.
These updates should improve the overall stability and security of the project. However, it's recommended to thoroughly test the application to ensure these updates don't introduce any unexpected behavior or compatibility issues.
To verify the impact of these dependency updates, run the following command to check for any breaking changes or significant updates in the most critical dependencies:
#!/bin/bash # Check for major version changes or significant updates in key dependencies grep -E "github.com/(cosmos|ethereum|tendermint|cometbft)/" gomod2nix.toml | while read -r line; do if [[ $line =~ version[[:space:]]*=[[:space:]]*\"([^\"]*)\" ]]; then version="${BASH_REMATCH[1]}" module=$(echo "$line" | awk -F'"' '{print $2}') echo "Checking $module at version $version" gh repo view "$module" --json url -q .url | xargs -I {} gh api {}/releases/tags/"$version" fi done
265-266: Ethermint module updated to latest versionThe
github.com/evmos/ethermintmodule has been updated to the latest version (v0.6.1-0.20241017130935-816389c76eac). This update likely includes new features, improvements, or bug fixes.Note that this module is replaced with
github.com/crypto-org-chain/ethermint, indicating the use of a custom fork. Ensure that this fork is up-to-date with the latest changes from the original repository and that it doesn't introduce any incompatibilities with other dependencies.To verify the changes in this update, you can run the following command:
CHANGELOG.md (4)
Line range hint
8-12: LGTM: Improvements look good.The improvements in this section appear to enhance performance and functionality:
- Generating test transactions in parallel even on a single node.
- Implementing a load generator with error retry and backoff.
- Adding an abort option in the PrepareProposal process.
These changes should contribute to better system efficiency and reliability.
🧰 Tools
🪛 LanguageTool
[grammar] ~7-~7: The word ‘shutdown’ is a noun. Did you mean the verb “shut down”?
Context: ...-chain/cronos/pull/1647) Fix node can't shutdown by signal. ### Improvements * [#1645]...(SHUTDOWN)
[uncategorized] ~11-~11: Possible missing comma found.
Context: ...-chain/cronos/pull/1645) Gen test tx in parallel even in single node. * (testground)[#16...(AI_HYDRA_LEO_MISSING_COMMA)
Line range hint
21-28: LGTM: Improvements and bug fixes look comprehensive.The improvements and bug fixes in this section address various aspects of the system:
- Changes to block state machine parallelism.
- Updates to the Ethermint module for better API performance.
- Enhancements to PebbleDB support.
- Multiple bug fixes for critical issues like query failures, multisig account problems, and unsupported sign modes.
These changes should significantly improve the system's stability and performance.
🧰 Tools
🪛 LanguageTool
[grammar] ~7-~7: The word ‘shutdown’ is a noun. Did you mean the verb “shut down”?
Context: ...-chain/cronos/pull/1647) Fix node can't shutdown by signal. ### Improvements * [#1645]...(SHUTDOWN)
[uncategorized] ~11-~11: Possible missing comma found.
Context: ...-chain/cronos/pull/1645) Gen test tx in parallel even in single node. * (testground)[#16...(AI_HYDRA_LEO_MISSING_COMMA)
Line range hint
30-34: Noted: Previous version entriesThe changelog includes entries for older versions (v1.4.0-rc0, v1.3.0-rc2, v1.3.0-rc1, and earlier). These entries provide valuable context for the project's development history and demonstrate ongoing improvements and bug fixes across multiple releases.
🧰 Tools
🪛 LanguageTool
[grammar] ~7-~7: The word ‘shutdown’ is a noun. Did you mean the verb “shut down”?
Context: ...-chain/cronos/pull/1647) Fix node can't shutdown by signal. ### Improvements * [#1645]...(SHUTDOWN)
[uncategorized] ~11-~11: Possible missing comma found.
Context: ...-chain/cronos/pull/1645) Gen test tx in parallel even in single node. * (testground)[#16...(AI_HYDRA_LEO_MISSING_COMMA)
Line range hint
16-19: Important: State Machine Breaking ChangeThe change in the memiavl module alters the initial version logic to ensure compatibility with iavl 1.2.0. This is a state machine breaking change and requires careful consideration during the upgrade process.
To verify the impact of this change, you can run the following script:
🧰 Tools
🪛 LanguageTool
[grammar] ~7-~7: The word ‘shutdown’ is a noun. Did you mean the verb “shut down”?
Context: ...-chain/cronos/pull/1647) Fix node can't shutdown by signal. ### Improvements * [#1645]...(SHUTDOWN)
[uncategorized] ~11-~11: Possible missing comma found.
Context: ...-chain/cronos/pull/1645) Gen test tx in parallel even in single node. * (testground)[#16...(AI_HYDRA_LEO_MISSING_COMMA)
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
👮🏻👮🏻👮🏻 !!!! 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
Release Notes
Bug Fixes
Improvements
Documentation
Tests