fix: improve single-node.sh #4041
Conversation
rootulp
requested review from
rach-id and
evan-forbes
and removed request for
a team
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request involve modifications to the versioning and upgrade logic of the Celestia application. Key updates include enhanced logging for version upgrades in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 4
🧹 Outside diff range and nitpick comments (4)
scripts/upgrade-to-v3.sh (2)
12-16: Enhance binary verificationThe binary check could be more comprehensive by also verifying the version compatibility.
Consider enhancing the verification:
if ! [ -x "$(command -v celestia-appd)" ] then echo "celestia-appd could not be found. Please install the celestia-appd binary using 'make install' and make sure the PATH contains the directory where the binary exists. By default, go will install the binary under '~/go/bin'" exit 1 fi +# Verify minimum version requirement +VERSION=$(celestia-appd version 2>&1) +if ! echo "$VERSION" | grep -q "v2"; then + echo "Error: celestia-appd version must be v2.x.x for this upgrade" + exit 1 +fi
1-1: Ensure script permissionsThe script will need executable permissions to run.
Run the following command after adding the script:
chmod +x scripts/upgrade-to-v3.shscripts/single-node.sh (2)
81-81: LGTM! Consider adding error handlingThe new sed pattern correctly sets the initial app version to "1", fixing the upgrade path issue. The regex pattern is robust and will handle various formatting of the existing app_version value.
Consider adding error checking after the sed command:
-sed -i'.bak' 's/"app_version": *"[^"]*"/"app_version": "1"/' ${CELESTIA_APP_HOME}/config/genesis.json +if ! sed -i'.bak' 's/"app_version": *"[^"]*"/"app_version": "1"/' ${CELESTIA_APP_HOME}/config/genesis.json; then + echo "Error: Failed to set initial app version in genesis.json" + exit 1 +fi
Line range hint
83-93: Consider parameterizing tracing configurationThe tracing configuration is well-structured and includes clear user feedback. However, hardcoding these values might limit flexibility for different environments.
Consider making these values configurable through environment variables:
- trace_type="local" + trace_type="${CELESTIA_TRACE_TYPE:-local}" - trace_pull_address=":26661" + trace_pull_address="${CELESTIA_TRACE_PULL_ADDRESS:-:26661}" - trace_push_batch_size=1000 + trace_push_batch_size="${CELESTIA_TRACE_PUSH_BATCH_SIZE:-1000}"This would allow users to customize the tracing configuration without modifying the script.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
app/app.go(2 hunks)scripts/single-node.sh(1 hunks)scripts/upgrade-to-v3.sh(1 hunks)
🔇 Additional comments (3)
scripts/single-node.sh (1)
Line range hint 108-114: Verify upgrade height configuration
The --v2-upgrade-height 3 setting needs verification to ensure it aligns with the new versioning approach where we start from app version 1.
Let's check for any related upgrade configurations:
✅ Verification successful
The upgrade height setting of 3 in the script is correct and aligns with the versioning approach
The verification shows that:
- The
--v2-upgrade-heightflag is consistently used across different scripts with appropriate values - The codebase supports version-specific constants and upgrade mechanisms
- The setting of height 3 for local testnet is appropriate as it allows:
- Initial blocks with version 1
- Early upgrade testing without waiting long periods
- Consistent with the app's upgrade capability from version 1 to 2
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for upgrade-related configurations and height settings
rg -l "upgrade.*height|app.*version" --type-add 'proto:*.proto' --type proto --type go
# Check for any upgrade handlers that might need adjustment
ast-grep --pattern 'func ($x *Keeper) HandleUpgrade($y context.Context, $z Plan) {
$$$
}'
Length of output: 1595
Script:
#!/bin/bash
# Let's check the specific upgrade height configuration in the script and related files
rg -A 5 "v2-upgrade-height" scripts/
# Check app version constants and initialization
rg -A 5 "InitialVersion|AppVersion" pkg/appconsts/
Length of output: 1162
Script:
#!/bin/bash
# Let's check the versioning approach in the codebase
rg -A 3 "version.*=.*1" pkg/appconsts/
rg -A 3 "version.*=.*1" app/
# Check for any upgrade height related constants or configurations
rg -A 3 "UpgradeHeight.*=|DefaultUpgradeHeight"
Length of output: 4023
app/app.go (2)
462-462: LGTM: Added helpful debug logging.
The addition of debug logging for the current version is valuable for troubleshooting upgrade issues.
Line range hint 480-484: LGTM: Improved version upgrade logic.
The changes enhance the version upgrade process by:
- Adding informative logging for version transitions
- Ensuring version changes are strictly increasing
- Using SignalKeeper for coordinated upgrades
| CHAIN_ID="test" | ||
| KEY_NAME="validator" | ||
| KEYRING_BACKEND="test" | ||
| CELESTIA_APP_HOME="${HOME}/.celestia-app" | ||
| CELESTIA_APP_VERSION=$(celestia-appd version 2>&1) | ||
| FEES="500utia" | ||
| BROADCAST_MODE="block" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make environment variables configurable
The script uses hardcoded values which could limit its reusability across different environments.
Consider making these values configurable:
-CHAIN_ID="test"
-KEY_NAME="validator"
-KEYRING_BACKEND="test"
-FEES="500utia"
+CHAIN_ID="${CHAIN_ID:-test}"
+KEY_NAME="${KEY_NAME:-validator}"
+KEYRING_BACKEND="${KEYRING_BACKEND:-test}"
+FEES="${FEES:-500utia}"📝 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.
| CHAIN_ID="test" | |
| KEY_NAME="validator" | |
| KEYRING_BACKEND="test" | |
| CELESTIA_APP_HOME="${HOME}/.celestia-app" | |
| CELESTIA_APP_VERSION=$(celestia-appd version 2>&1) | |
| FEES="500utia" | |
| BROADCAST_MODE="block" | |
| CHAIN_ID="${CHAIN_ID:-test}" | |
| KEY_NAME="${KEY_NAME:-validator}" | |
| KEYRING_BACKEND="${KEYRING_BACKEND:-test}" | |
| CELESTIA_APP_HOME="${HOME}/.celestia-app" | |
| CELESTIA_APP_VERSION=$(celestia-appd version 2>&1) | |
| FEES="${FEES:-500utia}" | |
| BROADCAST_MODE="block" |
| echo "Querying for pending upgrade..." | ||
| celestia-appd query signal upgrade |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Verify upgrade status
The script queries for pending upgrade but doesn't verify the result or provide clear success/failure indication.
Add verification of the upgrade status:
echo "Querying for pending upgrade..."
-celestia-appd query signal upgrade
+UPGRADE_STATUS=$(celestia-appd query signal upgrade -o json)
+if echo "$UPGRADE_STATUS" | jq -e '.upgrade.plan.height' > /dev/null; then
+ echo "Success: Upgrade to v3 is pending at height $(echo "$UPGRADE_STATUS" | jq -r '.upgrade.plan.height')"
+else
+ echo "Error: No pending upgrade found"
+ exit 1
+fi📝 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.
| echo "Querying for pending upgrade..." | |
| celestia-appd query signal upgrade | |
| echo "Querying for pending upgrade..." | |
| UPGRADE_STATUS=$(celestia-appd query signal upgrade -o json) | |
| if echo "$UPGRADE_STATUS" | jq -e '.upgrade.plan.height' > /dev/null; then | |
| echo "Success: Upgrade to v3 is pending at height $(echo "$UPGRADE_STATUS" | jq -r '.upgrade.plan.height')" | |
| else | |
| echo "Error: No pending upgrade found" | |
| exit 1 | |
| fi |
scripts/upgrade-to-v3.sh
Outdated
| # Prerequisites: ensure ./single-node.sh is running in another terminal. | ||
| # Wait until block height is 3 for the node to upgrade from v1 -> v2. | ||
| # This script will upgrade the node from v2 -> v3. |
There was a problem hiding this comment.
Consider adding block height verification
The comment indicates waiting for block height 3 is required, but the script doesn't verify this condition. This could lead to upgrade failures if run too early.
Add block height verification before proceeding:
+# Verify current block height
+CURRENT_HEIGHT=$(celestia-appd query block | jq -r '.block.header.height')
+if [ "$CURRENT_HEIGHT" -lt "3" ]; then
+ echo "Error: Current block height ($CURRENT_HEIGHT) is less than required height (3)"
+ exit 1
+fiCommittable suggestion skipped: line range outside the PR's diff.
| echo "Submitting signal for v3..." | ||
| celestia-appd tx signal signal 3 \ | ||
| --keyring-backend=${KEYRING_BACKEND} \ | ||
| --home ${CELESTIA_APP_HOME} \ | ||
| --from ${KEY_NAME} \ | ||
| --fees ${FEES} \ | ||
| --chain-id ${CHAIN_ID} \ | ||
| --broadcast-mode ${BROADCAST_MODE} \ | ||
| --yes | ||
|
|
||
| echo "Querying the tally for v3..." | ||
| celestia-appd query signal tally 3 | ||
|
|
||
| echo "Submitting msg try upgrade..." | ||
| celestia-appd tx signal try-upgrade \ | ||
| --keyring-backend=${KEYRING_BACKEND} \ | ||
| --home ${CELESTIA_APP_HOME} \ | ||
| --from ${KEY_NAME} \ | ||
| --fees ${FEES} \ | ||
| --chain-id ${CHAIN_ID} \ | ||
| --broadcast-mode ${BROADCAST_MODE} \ | ||
| --yes |
There was a problem hiding this comment.
Add error handling between upgrade steps
The upgrade commands are executed sequentially without verifying the success of each step. This could lead to incomplete upgrades.
Consider adding verification between steps:
echo "Submitting signal for v3..."
celestia-appd tx signal signal 3 \
--keyring-backend=${KEYRING_BACKEND} \
--home ${CELESTIA_APP_HOME} \
--from ${KEY_NAME} \
--fees ${FEES} \
--chain-id ${CHAIN_ID} \
--broadcast-mode ${BROADCAST_MODE} \
--yes
+
+# Verify signal submission success
+sleep 5 # Wait for transaction to be processed
+SIGNAL_COUNT=$(celestia-appd query signal tally 3 -o json | jq '.count')
+if [ "$SIGNAL_COUNT" -eq "0" ]; then
+ echo "Error: Signal submission failed"
+ exit 1
+fiCommittable suggestion skipped: line range outside the PR's diff.
Co-authored-by: CHAMI Rachid <[email protected]>
celestia-bot
requested review from
a team and
staheri14
and removed request for
a team
Co-authored-by: CHAMI Rachid <[email protected]>
Co-authored-by: CHAMI Rachid <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
app/app.go(2 hunks)scripts/upgrade-to-v3.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/upgrade-to-v3.sh
🧰 Additional context used
🪛 GitHub Check: test / test-race
app/app.go
[failure] 480-480:
syntax error: unexpected ) at end of statement
[failure] 480-480:
syntax error: unexpected ) at end of statement
[failure] 480-480:
syntax error: unexpected ) at end of statement
[failure] 480-480:
syntax error: unexpected ) at end of statement
[failure] 480-480:
syntax error: unexpected ) at end of statement
🪛 GitHub Check: test / test-fuzz
app/app.go
[failure] 480-480:
syntax error: unexpected ) at end of statement
🪛 GitHub Check: test / test-short
app/app.go
[failure] 480-480:
syntax error: unexpected ) at end of statement
[failure] 480-480:
syntax error: unexpected ) at end of statement
[failure] 480-480:
syntax error: unexpected ) at end of statement
[failure] 480-480:
syntax error: unexpected ) at end of statement
[failure] 480-480:
syntax error: unexpected ) at end of statement
🪛 GitHub Check: test / test
app/app.go
[failure] 480-480:
syntax error: unexpected ) at end of statement
[failure] 480-480:
syntax error: unexpected ) at end of statement
[failure] 480-480:
syntax error: unexpected ) at end of statement
[failure] 480-480:
syntax error: unexpected ) at end of statement
[failure] 480-480:
syntax error: unexpected ) at end of statement
🪛 GitHub Check: lint / golangci-lint
app/app.go
[failure] 480-480:
syntax error: unexpected ) at end of statement) (typecheck)
[failure] 480-480:
syntax error: unexpected ) at end of statement) (typecheck)
[failure] 480-480:
syntax error: unexpected ) at end of statement) (typecheck)
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
rootulp
requested review from
ninabarbakadze
and removed request for
staheri14
Fixes a few issues I discovered with single-node.sh
single-node.shpreviously created a genesis file withappVersion: 3so even though it attempted to upgrade from v1 -> v2, that step doesn't make any sense because the node was already on v3. This PR fixes the issue by modifying the genesis file to useappVersion: 1upgrade-to-v3.shthat can be invoked to upgrade the single node from v2 -> v3. After running the command, the user will seeFLUPs