Conversation
Refactor client version checks to use conditional assignments for better error handling.
properly handles unset variables
📝 WalkthroughWalkthroughBumps EthPillar version and hardens status reporting; adds README section about switching to the fork; improves Nethermind asset selection/download flow; and tightens MEV-Boost version parsing and tag selection logic. Changes touch scripts and docs to make version/asset detection and messaging more robust. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
… extraction/sorting
…oost update fixes
Clarify instructions for updating EthPillar.
Refactor client version checks to use conditional assignments for better error handling.
properly handles unset variables
…1.10.1 instead of 1.10)
Updated README to reflect version change to 5.2.6 and improvements.
Correct grammatical errors in README note.
Updated version for MEV_Boost version fix.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@update_mevboost.sh`:
- Around line 20-33: The getCurrentVersion function wrongly treats stderr output
as evidence the binary exists because INSTALLED captures stderr via `2>&1`,
making the `-n $INSTALLED` branch always true; fix by first checking binary
existence or mev-boost exit status before using its output—e.g., use `command -v
mev-boost` or run mev-boost and test `$?` to decide whether to set
VERSION="Client not installed." and only then capture and parse output into
VERSION with the existing sed fallback logic; update references in
getCurrentVersion (INSTALLED, VERSION) accordingly.
🧹 Nitpick comments (2)
README.md (1)
18-38: Hardcoded version string will go stale.Line 36 hardcodes "5.2.7" — this will need manual updates with every future release. Consider referencing the version dynamically (e.g., linking to the releases page) or adding a note to update this line on each bump.
Also, the install one-liner (line 61) and manual clone (line 123) still point at
coincashew/EthPillar. This is presumably intentional (install from upstream, then switch remote), but it may confuse readers of this fork's README. A brief clarification note near those commands would help.update_mevboost.sh (1)
88-88: Minor:sed 's/^v//'strips the leadingvhere, but${TAG#v}is applied again on lines 55, 64, 67.Not a bug — stripping an already-stripped prefix is a no-op — but it's slightly confusing. Consider either keeping the
vinTAGand relying on${TAG#v}everywhere, or stripping it once here and removing the${TAG#v}expansions downstream.
| function getCurrentVersion(){ | ||
| INSTALLED=$(mev-boost --version 2>&1) | ||
| #Find version in format #.#.# | ||
| if [[ $INSTALLED ]] ; then | ||
| INSTALLED=$(mev-boost --version 2>&1) # capture stderr too, just in case | ||
| if [[ -n $INSTALLED ]] ; then | ||
| # shellcheck disable=SC2001 | ||
| VERSION=$(echo "$INSTALLED" | sed 's/.*\s\([0-9]*\.[0-9]*\).*/\1/') | ||
| else | ||
| VERSION="Client not installed." | ||
| fi | ||
| # Extract major.minor or major.minor.patch, optional leading 'v', ignore suffix/commit info | ||
| # Patch part is optional to handle versions like 1.11 (no patch) | ||
| VERSION=$(echo "$INSTALLED" | sed 's/.*v\?\([0-9]\+\.[0-9]\+\(\.[0-9]\+\)\?\).*/\1/') | ||
| # Fallback if sed fails or no match | ||
| if [[ -z $VERSION || $VERSION == "$INSTALLED" ]]; then | ||
| VERSION="unknown" | ||
| fi | ||
| else | ||
| VERSION="Client not installed." | ||
| fi |
There was a problem hiding this comment.
"Client not installed." branch is unreachable due to stderr capture.
On line 21, 2>&1 redirects stderr into $INSTALLED. When mev-boost is not found on the system, bash emits a "command not found" message to stderr, so $INSTALLED will be non-empty. This means line 22's -n test always succeeds, the else branch (line 31–32) is never reached, and a missing binary reports "unknown" instead of "Client not installed."
Proposed fix: check the exit code or binary existence first
function getCurrentVersion(){
- INSTALLED=$(mev-boost --version 2>&1) # capture stderr too, just in case
- if [[ -n $INSTALLED ]] ; then
+ if ! command -v mev-boost &>/dev/null; then
+ VERSION="Client not installed."
+ return
+ fi
+ INSTALLED=$(mev-boost --version 2>&1)
+ if [[ -n $INSTALLED ]] ; then
# shellcheck disable=SC2001
# Extract major.minor or major.minor.patch, optional leading 'v', ignore suffix/commit info
# Patch part is optional to handle versions like 1.11 (no patch)
VERSION=$(echo "$INSTALLED" | sed 's/.*v\?\([0-9]\+\.[0-9]\+\(\.[0-9]\+\)\?\).*/\1/')
# Fallback if sed fails or no match
if [[ -z $VERSION || $VERSION == "$INSTALLED" ]]; then
VERSION="unknown"
fi
else
VERSION="Client not installed."
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.
| function getCurrentVersion(){ | |
| INSTALLED=$(mev-boost --version 2>&1) | |
| #Find version in format #.#.# | |
| if [[ $INSTALLED ]] ; then | |
| INSTALLED=$(mev-boost --version 2>&1) # capture stderr too, just in case | |
| if [[ -n $INSTALLED ]] ; then | |
| # shellcheck disable=SC2001 | |
| VERSION=$(echo "$INSTALLED" | sed 's/.*\s\([0-9]*\.[0-9]*\).*/\1/') | |
| else | |
| VERSION="Client not installed." | |
| fi | |
| # Extract major.minor or major.minor.patch, optional leading 'v', ignore suffix/commit info | |
| # Patch part is optional to handle versions like 1.11 (no patch) | |
| VERSION=$(echo "$INSTALLED" | sed 's/.*v\?\([0-9]\+\.[0-9]\+\(\.[0-9]\+\)\?\).*/\1/') | |
| # Fallback if sed fails or no match | |
| if [[ -z $VERSION || $VERSION == "$INSTALLED" ]]; then | |
| VERSION="unknown" | |
| fi | |
| else | |
| VERSION="Client not installed." | |
| fi | |
| function getCurrentVersion(){ | |
| if ! command -v mev-boost &>/dev/null; then | |
| VERSION="Client not installed." | |
| return | |
| fi | |
| INSTALLED=$(mev-boost --version 2>&1) | |
| if [[ -n $INSTALLED ]] ; then | |
| # shellcheck disable=SC2001 | |
| # Extract major.minor or major.minor.patch, optional leading 'v', ignore suffix/commit info | |
| # Patch part is optional to handle versions like 1.11 (no patch) | |
| VERSION=$(echo "$INSTALLED" | sed 's/.*v\?\([0-9]\+\.[0-9]\+\(\.[0-9]\+\)\?\).*/\1/') | |
| # Fallback if sed fails or no match | |
| if [[ -z $VERSION || $VERSION == "$INSTALLED" ]]; then | |
| VERSION="unknown" | |
| fi | |
| else | |
| VERSION="Client not installed." | |
| fi | |
| } |
🤖 Prompt for AI Agents
In `@update_mevboost.sh` around lines 20 - 33, The getCurrentVersion function
wrongly treats stderr output as evidence the binary exists because INSTALLED
captures stderr via `2>&1`, making the `-n $INSTALLED` branch always true; fix
by first checking binary existence or mev-boost exit status before using its
output—e.g., use `command -v mev-boost` or run mev-boost and test `$?` to decide
whether to set VERSION="Client not installed." and only then capture and parse
output into VERSION with the existing sed fallback logic; update references in
getCurrentVersion (INSTALLED, VERSION) accordingly.
3 participants
In "Failover Staking Node" mode there is no validator client. In this mode, when you try to view the software versions you get the following crash:
/usr/local/bin/ethpillar: line 680: _VC: unbound variable
This PR fixes this by adding a missing check that _VC is set, and if not setting the value to 'Validator client: Not Installed'
Summary by CodeRabbit
Bug Fixes
Chores
Documentation