fix: block sync on mocha on main #3846
Conversation
|
Moving to draft. Discussion w/ @cmwaters :
|
rootulp
requested review from
cmwaters and
staheri14
and removed request for
a team
WalkthroughWalkthroughThe pull request introduces significant changes to the Changes
Assessment against linked issues
Possibly related issues
Possibly related PRs
Suggested labels
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
Files selected for processing (7)
- app/app.go (3 hunks)
- app/app_test.go (3 hunks)
- go.mod (1 hunks)
- scripts/arabica-block-sync.sh (1 hunks)
- scripts/arabica.sh (1 hunks)
- scripts/mocha-block-sync.sh (1 hunks)
- scripts/mocha.sh (1 hunks)
Additional context used
Shellcheck
scripts/arabica-block-sync.sh
[warning] 24-24: In POSIX sh, read -p is undefined.
(SC3045)
scripts/mocha-block-sync.sh
[warning] 24-24: In POSIX sh, read -p is undefined.
(SC3045)
Additional comments not posted (6)
scripts/arabica.sh (1)
60-60: Verify the purpose and implications of the--force-no-bbrflag.The new
--force-no-bbrflag suggests disabling the Block-Based Reconciliation (BBR) feature during the startup ofcelestia-appd. While this might speed up the startup process, it's important to consider the potential implications:
- Disabling BBR could lead to state inconsistencies if the local state differs significantly from the network state.
- It might affect how the application handles block synchronization or reconciliation with the network.
Please provide more context on the reasoning behind this change and ensure that disabling BBR aligns with the desired behavior and doesn't introduce any unintended consequences.
scripts/mocha.sh (1)
60-60: LGTM, but verify the impact of the--force-no-bbrflag.The change to include the
--force-no-bbrflag in thecelestia-appdstartup command is approved.However, it's important to verify the impact of this flag on the application's behavior and performance, especially in the context of block processing and upgrade mechanisms.
To verify the impact, consider the following:
- Conduct thorough testing to ensure that the application can successfully sync blocks and handle upgrades with the
--force-no-bbrflag enabled.- Monitor the application's performance metrics, such as block processing time and resource utilization, to identify any potential regressions or improvements introduced by the flag.
- Document the observed behavior and any notable findings in the PR description or relevant documentation.
By verifying the impact of the
--force-no-bbrflag, we can ensure that the change achieves the desired outcome of addressing block synchronization issues on the Mocha testnet without introducing unintended side effects.app/app_test.go (1)
59-112: LGTM!The new test function
TestInitChainis well-structured and enhances the test coverage for the application by validating the initialization process. It follows good practices and covers important scenarios.Some key points:
- The test cases are clearly defined with descriptive names and expected outcomes.
- The
assertpackage is used for validations.- Each test case is run in a separate subtest for better isolation.
- The test cases cover panicking when consensus parameters are not set and not panicking when a genesis state without an application version is provided.
Great job on adding this comprehensive test function!
go.mod (1)
255-255: Approve Tendermint version update, but verify changelog.The Tendermint dependency has been updated from
v1.40.0-tm-v0.34.29tov1.41.0-tm-v0.34.29. This is a minor version update, which typically includes bug fixes, performance improvements, and backward-compatible features.Please verify the Tendermint changelog for any breaking changes or important updates that may affect the codebase. Pay special attention to any modifications in the Tendermint APIs or behaviors used by the Celestia app.
app/app.go (2)
561-575: LGTM!The
setDefaultAppVersionfunction is correctly implemented and handles the case where the app version is not explicitly set in the genesis file. The panics are appropriate as the function expects the consensus params and version to be set.
544-544: LGTM!The changes to
InitChainare correctly implemented and ensure that the app version is properly set and stored by calling the newsetDefaultAppVersionfunction. Setting the initial app version in the consensus params and the app version in the store for versions other than v1 is appropriate.
|
|
||
| # Ask the user for confirmation before deleting the existing celestia-app home | ||
| # directory. | ||
| read -p "Are you sure you want to delete: $CELESTIA_APP_HOME? [y/n] " response |
There was a problem hiding this comment.
Use a POSIX-compatible alternative for the read -p command.
Shellcheck has flagged a warning for the read -p command being undefined in POSIX sh.
To ensure compatibility with POSIX sh, consider using the following alternative:
-read -p "Are you sure you want to delete: $CELESTIA_APP_HOME? [y/n] " response
+printf "Are you sure you want to delete: $CELESTIA_APP_HOME? [y/n] "
+read responseThis alternative uses printf to display the prompt and read to capture the user's response, which is POSIX-compatible.
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.
| read -p "Are you sure you want to delete: $CELESTIA_APP_HOME? [y/n] " response | |
| printf "Are you sure you want to delete: $CELESTIA_APP_HOME? [y/n] " | |
| read response |
Tools
Shellcheck
[warning] 24-24: In POSIX sh, read -p is undefined.
(SC3045)
| #!/bin/sh | ||
|
|
||
| # This script starts a consensus node on Mocha and block syncs from genesis to | ||
| # the tip of the chain. This is expected to take a few weeks. | ||
|
|
||
| # Stop script execution if an error is encountered | ||
| set -o errexit | ||
| # Stop script execution if an undefined variable is used | ||
| set -o nounset | ||
|
|
||
| CHAIN_ID="mocha-4" | ||
| NODE_NAME="node-name" | ||
| SEEDS="[email protected]:26656,258f523c96efde50d5fe0a9faeea8a3e83be22ca@seed.mocha-4.celestia.aviaone.com:20279,5d0bf034d6e6a8b5ee31a2f42f753f1107b3a00e@celestia-testnet-seed.itrocket.net:11656,7da0fb48d6ef0823bc9770c0c8068dd7c89ed4ee@celest-test-seed.theamsolutions.info:443" | ||
|
|
||
| CELESTIA_APP_HOME="${HOME}/.celestia-app" | ||
| CELESTIA_APP_VERSION=$(celestia-appd version 2>&1) | ||
|
|
||
| echo "celestia-app home: ${CELESTIA_APP_HOME}" | ||
| echo "celestia-app version: ${CELESTIA_APP_VERSION}" | ||
| echo "" | ||
|
|
||
| # Ask the user for confirmation before deleting the existing celestia-app home | ||
| # directory. | ||
| read -p "Are you sure you want to delete: $CELESTIA_APP_HOME? [y/n] " response | ||
|
|
||
| # Check the user's response | ||
| if [ "$response" != "y" ]; then | ||
| # Exit if the user did not respond with "y" | ||
| echo "You must delete $CELESTIA_APP_HOME to continue." | ||
| exit 1 | ||
| fi | ||
|
|
||
| echo "Deleting $CELESTIA_APP_HOME..." | ||
| rm -r "$CELESTIA_APP_HOME" | ||
|
|
||
| echo "Initializing config files..." | ||
| celestia-appd init ${NODE_NAME} --chain-id ${CHAIN_ID} > /dev/null 2>&1 # Hide output to reduce terminal noise | ||
|
|
||
| echo "Settings seeds in config.toml..." | ||
| sed -i.bak -e "s/^seeds *=.*/seeds = \"$SEEDS\"/" $CELESTIA_APP_HOME/config/config.toml | ||
|
|
||
| echo "Downloading genesis file..." | ||
| celestia-appd download-genesis ${CHAIN_ID} > /dev/null 2>&1 # Hide output to reduce terminal noise | ||
|
|
||
| echo "Starting celestia-appd..." | ||
| celestia-appd start --v2-upgrade-height 2585031 --force-no-bbr |
There was a problem hiding this comment.
LGTM, but address the Shellcheck warning.
The script is well-structured and follows best practices. The code changes are approved.
However, the Shellcheck warning for the usage of read -p in POSIX sh is valid and should be addressed.
To address the Shellcheck warning, you can use the following alternative that is compatible with POSIX sh:
-read -p "Are you sure you want to delete: $CELESTIA_APP_HOME? [y/n] " response
+printf "Are you sure you want to delete: $CELESTIA_APP_HOME? [y/n] "
+read responseCommittable 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.
| #!/bin/sh | |
| # This script starts a consensus node on Mocha and block syncs from genesis to | |
| # the tip of the chain. This is expected to take a few weeks. | |
| # Stop script execution if an error is encountered | |
| set -o errexit | |
| # Stop script execution if an undefined variable is used | |
| set -o nounset | |
| CHAIN_ID="mocha-4" | |
| NODE_NAME="node-name" | |
| SEEDS="[email protected]:26656,258f523c96efde50d5fe0a9faeea8a3e83be22ca@seed.mocha-4.celestia.aviaone.com:20279,5d0bf034d6e6a8b5ee31a2f42f753f1107b3a00e@celestia-testnet-seed.itrocket.net:11656,7da0fb48d6ef0823bc9770c0c8068dd7c89ed4ee@celest-test-seed.theamsolutions.info:443" | |
| CELESTIA_APP_HOME="${HOME}/.celestia-app" | |
| CELESTIA_APP_VERSION=$(celestia-appd version 2>&1) | |
| echo "celestia-app home: ${CELESTIA_APP_HOME}" | |
| echo "celestia-app version: ${CELESTIA_APP_VERSION}" | |
| echo "" | |
| # Ask the user for confirmation before deleting the existing celestia-app home | |
| # directory. | |
| read -p "Are you sure you want to delete: $CELESTIA_APP_HOME? [y/n] " response | |
| # Check the user's response | |
| if [ "$response" != "y" ]; then | |
| # Exit if the user did not respond with "y" | |
| echo "You must delete $CELESTIA_APP_HOME to continue." | |
| exit 1 | |
| fi | |
| echo "Deleting $CELESTIA_APP_HOME..." | |
| rm -r "$CELESTIA_APP_HOME" | |
| echo "Initializing config files..." | |
| celestia-appd init ${NODE_NAME} --chain-id ${CHAIN_ID} > /dev/null 2>&1 # Hide output to reduce terminal noise | |
| echo "Settings seeds in config.toml..." | |
| sed -i.bak -e "s/^seeds *=.*/seeds = \"$SEEDS\"/" $CELESTIA_APP_HOME/config/config.toml | |
| echo "Downloading genesis file..." | |
| celestia-appd download-genesis ${CHAIN_ID} > /dev/null 2>&1 # Hide output to reduce terminal noise | |
| echo "Starting celestia-appd..." | |
| celestia-appd start --v2-upgrade-height 2585031 --force-no-bbr | |
| #!/bin/sh | |
| # This script starts a consensus node on Mocha and block syncs from genesis to | |
| # the tip of the chain. This is expected to take a few weeks. | |
| # Stop script execution if an error is encountered | |
| set -o errexit | |
| # Stop script execution if an undefined variable is used | |
| set -o nounset | |
| CHAIN_ID="mocha-4" | |
| NODE_NAME="node-name" | |
| SEEDS="[email protected]:26656,258f523c96efde50d5fe0a9faeea8a3e83be22ca@seed.mocha-4.celestia.aviaone.com:20279,5d0bf034d6e6a8b5ee31a2f42f753f1107b3a00e@celestia-testnet-seed.itrocket.net:11656,7da0fb48d6ef0823bc9770c0c8068dd7c89ed4ee@celest-test-seed.theamsolutions.info:443" | |
| CELESTIA_APP_HOME="${HOME}/.celestia-app" | |
| CELESTIA_APP_VERSION=$(celestia-appd version 2>&1) | |
| echo "celestia-app home: ${CELESTIA_APP_HOME}" | |
| echo "celestia-app version: ${CELESTIA_APP_VERSION}" | |
| echo "" | |
| # Ask the user for confirmation before deleting the existing celestia-app home | |
| # directory. | |
| printf "Are you sure you want to delete: $CELESTIA_APP_HOME? [y/n] " | |
| read response | |
| # Check the user's response | |
| if [ "$response" != "y" ]; then | |
| # Exit if the user did not respond with "y" | |
| echo "You must delete $CELESTIA_APP_HOME to continue." | |
| exit 1 | |
| fi | |
| echo "Deleting $CELESTIA_APP_HOME..." | |
| rm -r "$CELESTIA_APP_HOME" | |
| echo "Initializing config files..." | |
| celestia-appd init ${NODE_NAME} --chain-id ${CHAIN_ID} > /dev/null 2>&1 # Hide output to reduce terminal noise | |
| echo "Settings seeds in config.toml..." | |
| sed -i.bak -e "s/^seeds *=.*/seeds = \"$SEEDS\"/" $CELESTIA_APP_HOME/config/config.toml | |
| echo "Downloading genesis file..." | |
| celestia-appd download-genesis ${CHAIN_ID} > /dev/null 2>&1 # Hide output to reduce terminal noise | |
| echo "Starting celestia-appd..." | |
| celestia-appd start --v2-upgrade-height 2585031 --force-no-bbr |
Tools
Shellcheck
[warning] 24-24: In POSIX sh, read -p is undefined.
(SC3045)
Closes #3840
Forward port of #3843
Testing
I can state sync and block sync on Arabica and Mocha