Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: improve single-node.sh #4041

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
+58 −1
Open

fix: improve single-node.sh #4041

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d2f02b0
fix: improve single-node.sh
rootulp Nov 20, 2024
b6470c7
Merge branch 'main' into rp/improve-single-node
rootulp Nov 20, 2024
7df6ccb
Update scripts/upgrade-to-v3.sh
rootulp Nov 20, 2024
03d8410
Update app/app.go
rootulp Nov 20, 2024
8e9dd1f
Update app/app.go
rootulp Nov 20, 2024
586e7f7
Update app/app.go
rootulp Nov 21, 2024
434f6ba
fix: golangci-lint
rootulp Nov 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions app/app.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -459,6 +459,7 @@ func (app *App) EndBlocker(ctx sdk.Context, req abci.RequestEndBlock) abci.Respo
res := app.manager.EndBlock(ctx, req)
currentVersion := app.AppVersion()
// For v1 only we upgrade using an agreed upon height known ahead of time
app.BaseApp.Logger().Debug("current version", "version", currentVersion)
if currentVersion == v1 {
// check that we are at the height before the upgrade
if req.Height == app.upgradeHeightV2-1 {
Expand All @@ -476,6 +477,7 @@ func (app *App) EndBlocker(ctx sdk.Context, req abci.RequestEndBlock) abci.Respo
} else if shouldUpgrade, newVersion := app.SignalKeeper.ShouldUpgrade(ctx); shouldUpgrade {
// Version changes must be increasing. Downgrades are not permitted
if newVersion > currentVersion {
app.BaseApp.Logger().Info("upgrading app version", "current version", currentVersion, "new version", newVersion)
app.SetAppVersion(ctx, newVersion)
app.SignalKeeper.ResetTally(ctx)
}
Expand Down
2 changes: 1 addition & 1 deletion scripts/single-node.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ createGenesis() {
sed -i'.bak' 's#"604800s"#"60s"#g' "${CELESTIA_APP_HOME}"/config/genesis.json

# Override the genesis to use app version 1 and then upgrade to app version 2 later.
sed -i'.bak' 's#"app_version": "2"#"app_version": "1"#g' "${CELESTIA_APP_HOME}"/config/genesis.json
sed -i'.bak' 's/"app_version": *"[^"]*"/"app_version": "1"/' ${CELESTIA_APP_HOME}/config/genesis.json

trace_type="local"
sed -i.bak -e "s/^trace_type *=.*/trace_type = \"$trace_type\"/" ${CELESTIA_APP_HOME}/config/config.toml
Expand Down
55 changes: 55 additions & 0 deletions scripts/upgrade-to-v3.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
#!/bin/sh

# This script will upgrade the node from v2 -> v3.
# Prerequisites: ensure ./single-node.sh is running in another terminal.
# Wait until block height is 3 for the node to upgrade from v1 -> v2.

# Stop script execution if an error is encountered
set -o errexit
# Stop script execution if an undefined variable is used
set -o nounset

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

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"
Comment on lines +18 to +24
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
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 "celestia-app home: ${CELESTIA_APP_HOME}"
echo "celestia-app version: ${CELESTIA_APP_VERSION}"
echo ""


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
Comment on lines +31 to +52
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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
+fi

Committable suggestion skipped: line range outside the PR's diff.


echo "Querying for pending upgrade..."
celestia-appd query signal upgrade
Comment on lines +54 to +55
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
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

Loading