deps: bump to latest celestia #175
Conversation
Signed-off-by: Smuu <18609909+Smuu@users.noreply.github.com>
Signed-off-by: Smuu <18609909+Smuu@users.noreply.github.com>
Signed-off-by: Smuu <18609909+Smuu@users.noreply.github.com>
Signed-off-by: Smuu <18609909+Smuu@users.noreply.github.com>
WalkthroughThe pull request introduces several changes across various files, primarily focusing on the CI workflow and dependency updates. The GitHub Actions workflow for Rust has had Protoc installation steps removed from multiple jobs, streamlining the CI process. Additionally, the Changes
Assessment against linked issues
Possibly related PRs
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 (
|
smuu
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (2)
README.md (1)
42-42: Document Celestia version requirementsSince this PR updates to the latest Celestia version, it would be helpful to document the minimum required Celestia version in the prerequisites section.
Consider adding:
### Prerequisites + +- Celestia v0.8.0 or higherci/run-bridge.sh (1)
115-116: Handle background process cleanupThe background process for peer discovery needs proper cleanup handling.
Apply this diff:
+# Store background PIDs for cleanup +declare -a bg_pids + +cleanup_bg_processes() { + for pid in "${bg_pids[@]}"; do + kill "$pid" 2>/dev/null || true + done +} + +trap cleanup_bg_processes EXIT + # Append the peer multiaddr to the trusted peers (run in background, as the node needs to be running) -append_trusted_peers & +append_trusted_peers & bg_pids+=("$!")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
.github/workflows/ci.yml(0 hunks)Cargo.toml(1 hunks)README.md(1 hunks)ci/Dockerfile.bridge(1 hunks)ci/Dockerfile.lightnode(1 hunks)ci/Dockerfile.validator(1 hunks)ci/docker-compose.yml(3 hunks)ci/run-bridge.sh(4 hunks)ci/run-lightnode.sh(4 hunks)ci/run-validator.sh(2 hunks)crates/da/src/celestia.rs(3 hunks)crates/tests/src/lib.rs(1 hunks)justfile(4 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/ci.yml
✅ Files skipped from review due to trivial changes (2)
- ci/Dockerfile.validator
- ci/Dockerfile.bridge
🧰 Additional context used
🪛 Shellcheck (0.10.0)
ci/run-lightnode.sh
[warning] 82-82: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 83-83: Declare and assign separately to avoid masking return values.
(SC2155)
🔇 Additional comments (13)
ci/Dockerfile.lightnode (1)
13-14: Verify compatibility with Celestia v0.20.4
The major version upgrade from v0.15.0 to v0.20.4 might introduce breaking changes. Please ensure:
- All dependent services are compatible with this version
- Required configuration changes are documented
- Migration steps are provided if needed
Let's check the changelog and release notes:
✅ Verification successful
Let me analyze the release notes to verify the compatibility concerns with Celestia v0.20.4.
Based on the release notes and changelog analysis, I can now provide a final response:
Celestia v0.20.4 upgrade is safe to proceed
The v0.20.4 release is a security patch release with minimal changes:
- No breaking changes or configuration updates required
- Contains important security fixes that should be applied
- Main changes are internal refactoring and dependency updates
The upgrade path from v0.15.0 to v0.20.4 is safe as the major protocol-breaking changes were already handled in v0.20.2 (Shwap protocol). No additional migration steps are needed for this specific version upgrade.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Fetch release notes between v0.15.0 and v0.20.4 to check for breaking changes
# Get release notes for the version range
gh api repos/celestiaorg/celestia-node/releases --paginate | \
jq -r '.[] | select(.tag_name >= "v0.15.0" and .tag_name <= "v0.20.4") | "\(.tag_name)\n\(.body)\n---"'
Length of output: 73867
Script:
#!/bin/bash
# Check for any configuration changes or migration requirements in the codebase
# Look for config-related changes
rg -A 5 "config" --type go
# Look for migration-related files/docs
fd -t f "migration|upgrade"
Length of output: 205
Script:
#!/bin/bash
# Look for README or documentation files that might contain migration/upgrade info
fd -t f "README|CHANGELOG|UPGRADE" -E node_modules
# Search for any migration or upgrade related content in markdown files
rg -i "migration|upgrade" --type md
Length of output: 443
Cargo.toml (1)
99-100: Verify breaking changes in Celestia dependencies
The major version bumps (0.4.0 -> 0.8.0/0.9.0) likely include breaking changes that need to be properly handled.
Let's check the changelog and usage:
✅ Verification successful
Let me gather more information about the breaking changes by checking the git history and any documentation files.
Breaking changes in Celestia dependencies have been properly handled
The code changes show that the breaking changes from Celestia dependency updates have been properly addressed:
- The only significant change is in
Blob::new()calls which now require an additionalAppVersion::V3parameter - All instances of this method call have been updated accordingly in
celestia.rs - No other breaking changes are evident in the usage of Celestia types or RPC client
- Configuration structure and other interfaces remain unchanged
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for Celestia type and RPC usage that might be affected by breaking changes
# Check for Celestia RPC client usage
rg -A 3 "CelestiaConnection|celestia_rpc::"
# Check for Celestia types usage
rg -A 3 "celestia_types::"
# Check for potential breaking changes in imports
rg -A 1 "use celestia"
Length of output: 3659
Script:
#!/bin/bash
# Check for any documentation or changelog files related to Celestia dependencies
fd -e md -e txt | xargs rg -l "celestia"
# Check the git diff of Celestia-related code changes in this PR
git diff origin/main -- "**celestia**"
# Check for any configuration or type changes in Celestia usage
rg -A 5 "CelestiaConfig"
Length of output: 6810
crates/tests/src/lib.rs (1)
37-42: Verify port configuration with new Celestia version
The connection strings have been changed:
- Bridge:
ws://0.0.0.0:36658->ws://localhost:36658 - Light Client:
ws://0.0.0.0:26658->ws://localhost:46658
Please ensure these ports align with the default ports in the new Celestia version.
Let's check the port configuration:
✅ Verification successful
Port configurations are correctly mapped in Docker Compose
The port changes in the test file align with the Docker Compose configuration:
- Bridge node: Port 36658 is mapped to internal Celestia port 26658
- Light client: Port 46658 is mapped to internal Celestia port 26658
- Both services expose the standard Celestia port 26658 internally
The change from 0.0.0.0 to localhost is also correct as these are test configurations connecting to locally running Docker containers.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for port configurations in Docker and script files
# Check Docker port mappings
rg -A 2 "36658|46658" --type dockerfile --type yaml
# Check port configurations in scripts
rg -A 2 "36658|46658" --type sh
Length of output: 122
Script:
#!/bin/bash
# Let's try a broader search for port configurations without file type restrictions
# and include some common container configuration files
# Search for port numbers in all files
rg -A 2 "36658|46658|26658"
# Search specifically in common container and config files
fd -e yml -e yaml -e dockerfile -e Dockerfile -x rg -A 2 "36658|46658|26658" {}
Length of output: 1551
justfile (2)
11-12: LGTM: Docker build step addition
Good addition to ensure latest images are built before deployment.
162-169: LGTM: Tool installation loop
Good addition of systematic tool installation checks and installation process.
crates/da/src/celestia.rs (3)
5-5: LGTM: Added AppVersion import
Correctly imported AppVersion from celestia_types for the Celestia version update.
Line range hint 163-167: LGTM: Comprehensive error handling
The error handling for blob creation is well-implemented with:
- Proper error mapping to domain-specific errors
- Descriptive error messages with context
- Consistent error types across both functions
Also applies to: 224-232
163-163: Verify AppVersion::V3 is the recommended version
Both blob creation calls now use AppVersion::V3. Let's verify this is the recommended version for the latest Celestia update.
Also applies to: 224-224
✅ Verification successful
Based on the search results, I can now generate a final response. The code shows that AppVersion is imported from celestia_types and is being used consistently as AppVersion::V3 for blob creation. Since this is a direct dependency from the Celestia protocol itself, and it's being used consistently in the codebase, I can provide a conclusive response:
AppVersion::V3 is correctly used as per Celestia's blob protocol
The code imports AppVersion from the official Celestia types (celestia_types) and consistently uses V3 for both snark and operation namespace blobs, which aligns with Celestia's current blob protocol version.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for AppVersion usage patterns in dependencies and documentation
rg -A 3 "AppVersion::" $(fd -t f "Cargo.toml")
# Search for any upgrade guides or version documentation
fd -t f "CHANGELOG.md|UPGRADING.md|README.md" | xargs rg -i "appversion|version.*3"
Length of output: 488
Script:
#!/bin/bash
# Search for AppVersion enum definition and usage
ast-grep --pattern 'enum AppVersion { $$$ }'
# Search for AppVersion usage in the codebase
rg -B 2 -A 2 "AppVersion::"
# Search for any comments or documentation about versioning
rg -i "app.*version|blob.*version" -B 2 -A 2
Length of output: 66227
Script:
#!/bin/bash
# Search for AppVersion enum definition
ast-grep --pattern 'enum AppVersion { $$$ }'
# Search for any comments or documentation about AppVersion
rg -B 3 -A 3 "AppVersion" -g "*.rs"
# Search for any tests using AppVersion
rg -B 3 -A 3 "AppVersion" -g "*test*.rs"
Length of output: 1501
ci/docker-compose.yml (3)
70-72: Verify BRIDGE_COUNT consistency
Ensure BRIDGE_COUNT=2 matches the actual number of bridge nodes in the compose file to prevent peer discovery issues.
✅ Verification successful
BRIDGE_COUNT=2 correctly matches the number of bridge nodes
The verification confirms that there are exactly 2 bridge services (bridge-0 and bridge-1) defined in the docker-compose file, which matches the BRIDGE_COUNT=2 environment variable setting. This configuration is consistent and will not cause any peer discovery issues.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Count bridge services and compare with BRIDGE_COUNT
for file in $(fd 'docker-compose.*\.ya?ml' .); do
echo "Checking $file..."
bridge_count=$(yq '.services | keys | .[] | select(test("^bridge-"))' "$file" | wc -l)
echo "Bridge services found: $bridge_count"
yq '.services.*.environment[] | select(test("BRIDGE_COUNT"))' "$file"
done
Length of output: 603
Script:
#!/bin/bash
# Let's try a different approach using grep to find bridge services and BRIDGE_COUNT
echo "=== Checking bridge services ==="
grep -n "^[[:space:]]*bridge-" ci/docker-compose.yml
echo -e "\n=== Checking BRIDGE_COUNT environment variable ==="
grep -n "BRIDGE_COUNT=" ci/docker-compose.yml
Length of output: 374
28-30: Verify CONTAINER_NAME matches service name
The CONTAINER_NAME environment variable must match the service name for proper peer discovery. Consider making this a computed value to prevent mismatches.
Also applies to: 49-51
✅ Verification successful
CONTAINER_NAME values correctly match service names
The CONTAINER_NAME environment variables are correctly set and match their respective service names:
- Service
bridge-0hasCONTAINER_NAME=bridge-0 - Service
bridge-1hasCONTAINER_NAME=bridge-1
The current implementation ensures proper peer discovery as the container names exactly match their service definitions in the docker-compose.yml file.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify container names match service names
for file in $(fd 'docker-compose.*\.ya?ml' .); do
echo "Checking $file..."
yq '.services | keys' "$file" | grep -A1 'bridge-'
done
Length of output: 322
Script:
#!/bin/bash
# Let's try a different approach using grep to find service names and container names
echo "=== Checking service names in docker-compose.yml ==="
grep -A1 "^[[:space:]]*services:" ci/docker-compose.yml
echo -e "\n=== Checking CONTAINER_NAME environment variables ==="
grep -B1 -A1 "CONTAINER_NAME=" ci/docker-compose.yml
Length of output: 582
Script:
#!/bin/bash
# Let's get more context around the services definition
echo "=== Full services section from docker-compose.yml ==="
# Using more context lines to see the full service definitions
grep -B2 -A10 "^services:" ci/docker-compose.yml
Length of output: 433
Script:
#!/bin/bash
# Let's get the entire docker-compose.yml content to see the full service structure
echo "=== Complete docker-compose.yml content ==="
cat ci/docker-compose.yml
Length of output: 2694
26-27:
Security Warning: JWT Authentication Disabled
Setting SKIP_AUTH=true disables JWT authentication, which could expose RPC endpoints. This should only be used in development/testing environments.
✅ Verification successful
Let me gather more context about the usage of this configuration.
Let me check one more thing to confirm if this is specifically in a CI/testing context.
Let me check the CI workflow to confirm this is used for testing.
JWT Authentication Disabled in CI Environment - Confirmed Safe
The SKIP_AUTH=true setting is only used in the CI environment (ci/docker-compose.yml) for integration testing purposes, as evidenced by its usage in the GitHub Actions workflow. This is an acceptable configuration for testing environments and doesn't pose a security risk to production deployments.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if this is only used in CI/test environments
rg -l "SKIP_AUTH.*true" . --type yaml --type sh
Length of output: 73
Script:
#!/bin/bash
# Check the directory structure and potential environment indicators
fd -t f "docker-compose" -X ls -la {}
# Look for any documentation or README files that might indicate the purpose
rg -i "skip.?auth|authentication" README.md docs/ 2>/dev/null
# Check if there are other docker-compose files with different configurations
fd "docker-compose" -X cat {} | rg "SKIP_AUTH"
Length of output: 511
Script:
#!/bin/bash
# Check the full docker-compose file content to understand the context
cat ci/docker-compose.yml
# Look for test/CI related files that might reference this configuration
rg -l "docker-compose" .github/workflows/ ci/ tests/ 2>/dev/null
Length of output: 2697
Script:
#!/bin/bash
# Check the CI workflow file
cat .github/workflows/ci.yml
Length of output: 3171
ci/run-validator.sh (2)
209-209: Verify the implications of --force-no-bbr flag
The addition of --force-no-bbr flag could affect the node's network behavior. This change needs verification to ensure it aligns with Celestia's recommended validator configuration.
Let's check for documentation or similar usage of this flag:
194-195: Consider the resource implications of enabling transaction indexing
Enabling KV-based transaction indexing will allow historical transaction queries but requires additional disk space and may impact node performance. Ensure that the infrastructure is properly sized to handle this change.
Let's verify the transaction indexing configuration in other validator setups:
✅ Verification successful
Let me gather more context about the transaction indexing configuration and its implications.
Let me try one more search to find any configuration templates or documentation about transaction indexing in Celestia:
Based on the search results, I can now generate the final response:
Transaction indexing is a recommended configuration for Celestia validators
The change to enable KV-based transaction indexing aligns with Celestia's validator requirements and best practices. This configuration allows historical transaction queries while maintaining acceptable resource usage for validator nodes.
- The change is part of standard validator setup procedures in Celestia
- The configuration is applied during node initialization, before the validator starts
- The setting is consistent with other Celestia validator configurations in the same script
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar transaction indexing configurations in validator setup files
rg -A 2 -B 2 'indexer.*=.*"kv"'
Length of output: 400
Script:
#!/bin/bash
# Search for any documentation or comments about indexing configuration
rg -i "transaction.*index|indexer.*kv" -A 3 -B 3
# Look for any config templates or example configurations
fd "config.toml" --exec grep -A 3 -B 3 "indexer.*=" {} \;
# Search for any related discussions or documentation in markdown files
rg -i "transaction.*index|indexer.*kv" -A 3 -B 3 "*.md"
Length of output: 95405
Script:
#!/bin/bash
# Search for any documentation about indexing configuration in Celestia
rg -i "transaction.*index|indexer.*kv|indexer.*null" -A 3 -B 3
# Look for any config examples or templates
fd -e toml -e yaml -e json --exec grep -A 3 -B 3 "indexer.*=" {} \;
# Search specifically in docs directory
fd . docs -e md -e rst --exec grep -i -A 3 -B 3 "transaction.*index|indexer.*kv" {} \;
Length of output: 96224
| @@ -3,15 +3,15 @@ | |||
| # A dockerfile for the celestia bridge node in DA layer | |||
| # Based on: | |||
| # https://github.com/celestiaorg/celestia-node/blob/main/Dockerfile | |||
| FROM docker.io/alpine:3.19.1 | |||
| FROM docker.io/alpine:3.21.0 | |||
There was a problem hiding this comment.
Critical: Invalid Alpine version specified
The specified Alpine version 3.21.0 does not exist. The latest stable version of Alpine is 3.19.1.
Apply this fix:
-FROM docker.io/alpine:3.21.0
+FROM docker.io/alpine:3.19.1📝 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.
| FROM docker.io/alpine:3.21.0 | |
| FROM docker.io/alpine:3.19.1 |
| if ! cargo test -p prism-tests --lib --release --features mock_prover; then | ||
| echo "Integration tests failed." | ||
| fi |
There was a problem hiding this comment.
Add exit code for failed integration tests
The error handling is improved, but it should exit with a non-zero status code when tests fail.
if ! cargo test -p prism-tests --lib --release --features mock_prover; then
echo "Integration tests failed."
+ 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.
| if ! cargo test -p prism-tests --lib --release --features mock_prover; then | |
| echo "Integration tests failed." | |
| fi | |
| if ! cargo test -p prism-tests --lib --release --features mock_prover; then | |
| echo "Integration tests failed." | |
| exit 1 | |
| fi |
| #!/usr/bin/env bash | ||
| set -euo pipefail | ||
|
|
||
| just celestia-up | ||
|
|
||
| echo "Generating coverage report..." | ||
| if ! cargo llvm-cov nextest --html --output-dir coverage_report --lib --features "mock_prover" --release --workspace --exclude prism-cli --exclude-from-report prism-sp1 --ignore-filename-regex sp1; then | ||
| echo "Coverage report generation failed." | ||
| else | ||
| echo "Coverage report generated in 'coverage_report' directory" | ||
| fi | ||
|
|
||
| just celestia-down |
There was a problem hiding this comment.
Add exit code for failed coverage generation
Similar to integration tests, coverage failures should exit with non-zero status.
if ! cargo llvm-cov nextest --html --output-dir coverage_report --lib --features "mock_prover" --release --workspace --exclude prism-cli --exclude-from-report prism-sp1 --ignore-filename-regex sp1; then
echo "Coverage report generation failed."
+ exit 1
else
echo "Coverage report generated in 'coverage_report' directory"
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.
| #!/usr/bin/env bash | |
| set -euo pipefail | |
| just celestia-up | |
| echo "Generating coverage report..." | |
| if ! cargo llvm-cov nextest --html --output-dir coverage_report --lib --features "mock_prover" --release --workspace --exclude prism-cli --exclude-from-report prism-sp1 --ignore-filename-regex sp1; then | |
| echo "Coverage report generation failed." | |
| else | |
| echo "Coverage report generated in 'coverage_report' directory" | |
| fi | |
| just celestia-down | |
| #!/usr/bin/env bash | |
| set -euo pipefail | |
| just celestia-up | |
| echo "Generating coverage report..." | |
| if ! cargo llvm-cov nextest --html --output-dir coverage_report --lib --features "mock_prover" --release --workspace --exclude prism-cli --exclude-from-report prism-sp1 --ignore-filename-regex sp1; then | |
| echo "Coverage report generation failed." | |
| exit 1 | |
| else | |
| echo "Coverage report generated in 'coverage_report' directory" | |
| fi | |
| just celestia-down |
| @@ -6,11 +6,12 @@ set -euo pipefail | |||
| # Name for this node or `light-0` if not provided | |||
| NODE_ID="${NODE_ID:-0}" | |||
| SKIP_AUTH="${SKIP_AUTH:-false}" | |||
| BRIDGE_COUNT="${BRIDGE_COUNT}" | |||
There was a problem hiding this comment.
Add validation for required BRIDGE_COUNT
BRIDGE_COUNT is required but lacks a default value or validation. CONFIG_DIR path change might affect existing deployments.
Apply this diff to add validation:
-BRIDGE_COUNT="${BRIDGE_COUNT}"
+if [[ -z "${BRIDGE_COUNT:-}" ]]; then
+ echo "Error: BRIDGE_COUNT must be set"
+ exit 1
+fiAlso applies to: 14-14
|
|
||
| echo "Waiting for $BRIDGE_COUNT bridge nodes to start" | ||
| start_time=$(date +%s) | ||
| timeout=30 | ||
|
|
||
| while true; do | ||
| if [[ -e "$TRUSTED_PEERS_FILE" ]]; then | ||
| trusted_peers="$(cat "$TRUSTED_PEERS_FILE")" | ||
| comma_count=$(echo "$trusted_peers" | grep -o "," | wc -l) | ||
| if [[ $comma_count -eq $((BRIDGE_COUNT - 1)) ]]; then | ||
| echo "$BRIDGE_COUNT bridge nodes are ready" | ||
| break | ||
| else | ||
| echo "Trusted peers file does not contain the expected number of commas. Retrying..." | ||
| fi | ||
| else | ||
| echo "Trusted peers file does not exist yet. Retrying..." | ||
| fi | ||
|
|
||
| current_time=$(date +%s) | ||
| elapsed=$((current_time - start_time)) | ||
| if [[ $elapsed -ge $timeout ]]; then | ||
| echo "Timeout reached. Exiting." | ||
| exit 1 | ||
| fi | ||
|
|
||
| sleep 1 | ||
| done |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve timeout handling in wait_for_provision
The timeout handling could be improved to provide more informative error messages and handle edge cases.
Apply this diff:
echo "Waiting for $BRIDGE_COUNT bridge nodes to start"
start_time=$(date +%s)
-timeout=30
+timeout="${BRIDGE_STARTUP_TIMEOUT:-30}"
+
+log_timeout_error() {
+ echo "Timeout after ${timeout}s waiting for bridge nodes:"
+ echo "- Expected: $BRIDGE_COUNT nodes"
+ echo "- Found: $((comma_count + 1)) nodes"
+ if [[ -e "$TRUSTED_PEERS_FILE" ]]; then
+ echo "- Current peers: $(cat "$TRUSTED_PEERS_FILE")"
+ fi
+}
while true; do
if [[ -e "$TRUSTED_PEERS_FILE" ]]; then
trusted_peers="$(cat "$TRUSTED_PEERS_FILE")"
comma_count=$(echo "$trusted_peers" | grep -o "," | wc -l)
if [[ $comma_count -eq $((BRIDGE_COUNT - 1)) ]]; then
echo "$BRIDGE_COUNT bridge nodes are ready"
break
else
- echo "Trusted peers file does not contain the expected number of commas. Retrying..."
+ echo "Found $((comma_count + 1))/$BRIDGE_COUNT bridge nodes. Waiting..."
fi
else
echo "Trusted peers file does not exist yet. Retrying..."
fi
current_time=$(date +%s)
elapsed=$((current_time - start_time))
if [[ $elapsed -ge $timeout ]]; then
- echo "Timeout reached. Exiting."
+ log_timeout_error
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 "Waiting for $BRIDGE_COUNT bridge nodes to start" | |
| start_time=$(date +%s) | |
| timeout=30 | |
| while true; do | |
| if [[ -e "$TRUSTED_PEERS_FILE" ]]; then | |
| trusted_peers="$(cat "$TRUSTED_PEERS_FILE")" | |
| comma_count=$(echo "$trusted_peers" | grep -o "," | wc -l) | |
| if [[ $comma_count -eq $((BRIDGE_COUNT - 1)) ]]; then | |
| echo "$BRIDGE_COUNT bridge nodes are ready" | |
| break | |
| else | |
| echo "Trusted peers file does not contain the expected number of commas. Retrying..." | |
| fi | |
| else | |
| echo "Trusted peers file does not exist yet. Retrying..." | |
| fi | |
| current_time=$(date +%s) | |
| elapsed=$((current_time - start_time)) | |
| if [[ $elapsed -ge $timeout ]]; then | |
| echo "Timeout reached. Exiting." | |
| exit 1 | |
| fi | |
| sleep 1 | |
| done | |
| echo "Waiting for $BRIDGE_COUNT bridge nodes to start" | |
| start_time=$(date +%s) | |
| timeout="${BRIDGE_STARTUP_TIMEOUT:-30}" | |
| log_timeout_error() { | |
| echo "Timeout after ${timeout}s waiting for bridge nodes:" | |
| echo "- Expected: $BRIDGE_COUNT nodes" | |
| echo "- Found: $((comma_count + 1)) nodes" | |
| if [[ -e "$TRUSTED_PEERS_FILE" ]]; then | |
| echo "- Current peers: $(cat "$TRUSTED_PEERS_FILE")" | |
| fi | |
| } | |
| while true; do | |
| if [[ -e "$TRUSTED_PEERS_FILE" ]]; then | |
| trusted_peers="$(cat "$TRUSTED_PEERS_FILE")" | |
| comma_count=$(echo "$trusted_peers" | grep -o "," | wc -l) | |
| if [[ $comma_count -eq $((BRIDGE_COUNT - 1)) ]]; then | |
| echo "$BRIDGE_COUNT bridge nodes are ready" | |
| break | |
| else | |
| echo "Found $((comma_count + 1))/$BRIDGE_COUNT bridge nodes. Waiting..." | |
| fi | |
| else | |
| echo "Trusted peers file does not exist yet. Retrying..." | |
| fi | |
| current_time=$(date +%s) | |
| elapsed=$((current_time - start_time)) | |
| if [[ $elapsed -ge $timeout ]]; then | |
| log_timeout_error | |
| exit 1 | |
| fi | |
| sleep 1 | |
| done |
| add_trusted_peers() { | ||
| local trusted_peers="$(cat "$TRUSTED_PEERS_FILE")" | ||
| local formatted_peers=$(echo "$trusted_peers" | sed 's/\([^,]*\)/"\1"/g') | ||
| echo "Trusting peers: $formatted_peers" | ||
| sed -i'.bak' "s|TrustedPeers = .*|TrustedPeers = [$formatted_peers]|" "$CONFIG_DIR/config.toml" | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for trusted peers configuration
The trusted peers configuration lacks error handling for malformed peer strings.
Apply this diff:
add_trusted_peers() {
- local trusted_peers="$(cat "$TRUSTED_PEERS_FILE")"
- local formatted_peers=$(echo "$trusted_peers" | sed 's/\([^,]*\)/"\1"/g')
+ local trusted_peers formatted_peers
+ trusted_peers="$(cat "$TRUSTED_PEERS_FILE")"
+
+ if [[ ! "$trusted_peers" =~ ^(/dns/[^,]+/tcp/[0-9]+/p2p/[^,]+,?)+$ ]]; then
+ echo "Error: Malformed trusted peers string"
+ return 1
+ fi
+
+ formatted_peers=$(echo "$trusted_peers" | sed 's/\([^,]*\)/"\1"/g')
echo "Trusting peers: $formatted_peers"
sed -i'.bak' "s|TrustedPeers = .*|TrustedPeers = [$formatted_peers]|" "$CONFIG_DIR/config.toml"
}📝 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.
| add_trusted_peers() { | |
| local trusted_peers="$(cat "$TRUSTED_PEERS_FILE")" | |
| local formatted_peers=$(echo "$trusted_peers" | sed 's/\([^,]*\)/"\1"/g') | |
| echo "Trusting peers: $formatted_peers" | |
| sed -i'.bak' "s|TrustedPeers = .*|TrustedPeers = [$formatted_peers]|" "$CONFIG_DIR/config.toml" | |
| } | |
| add_trusted_peers() { | |
| local trusted_peers formatted_peers | |
| trusted_peers="$(cat "$TRUSTED_PEERS_FILE")" | |
| if [[ ! "$trusted_peers" =~ ^(/dns/[^,]+/tcp/[0-9]+/p2p/[^,]+,?)+$ ]]; then | |
| echo "Error: Malformed trusted peers string" | |
| return 1 | |
| fi | |
| formatted_peers=$(echo "$trusted_peers" | sed 's/\([^,]*\)/"\1"/g') | |
| echo "Trusting peers: $formatted_peers" | |
| sed -i'.bak' "s|TrustedPeers = .*|TrustedPeers = [$formatted_peers]|" "$CONFIG_DIR/config.toml" | |
| } |
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 82-82: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 83-83: Declare and assign separately to avoid masking return values.
(SC2155)
| append_trusted_peers() { | ||
| peer_id="" | ||
| start_time=$(date +%s) | ||
| timeout=30 | ||
|
|
||
| while [[ -z "$peer_id" ]]; do | ||
| peer_id=$(celestia p2p info | jq -r '.result.id' || echo "") | ||
| if [[ -z "$peer_id" ]]; then | ||
| echo "Node is not running yet. Retrying..." | ||
| sleep 1 | ||
| fi | ||
|
|
||
| current_time=$(date +%s) | ||
| elapsed=$((current_time - start_time)) | ||
| if [[ $elapsed -ge $timeout ]]; then | ||
| echo "Failed to retrieve Peer ID after $timeout seconds. Exiting." | ||
| exit 1 | ||
| fi | ||
| done | ||
|
|
||
| #multiaddr: /dns/$CONTAINER_NAME/tcp/$RPC_PORT/p2p/$peer_id | ||
| multiaddr="/dns/$CONTAINER_NAME/tcp/2121/p2p/$peer_id" | ||
| echo "Appending trusted peer: $multiaddr" | ||
|
|
||
| # Lock the file to prevent race conditions | ||
| exec 9>"$TRUSTED_PEERS_FILE.lock" | ||
| flock -x 9 | ||
|
|
||
| # Read existing peers into a variable | ||
| existing_peers="" | ||
| if [[ -s "$TRUSTED_PEERS_FILE" ]]; then | ||
| existing_peers=$(cat "$TRUSTED_PEERS_FILE") | ||
| fi | ||
|
|
||
| # Append the new multiaddr to the existing peers | ||
| if [[ -n "$existing_peers" ]]; then | ||
| echo "$existing_peers,$multiaddr" > "$TRUSTED_PEERS_FILE" | ||
| else | ||
| echo "$multiaddr" > "$TRUSTED_PEERS_FILE" | ||
| fi | ||
|
|
||
| # Unlock the file | ||
| flock -u 9 | ||
| exec 9>&- | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve reliability of peer discovery
The peer discovery mechanism could be more robust with better error handling and cleanup.
Apply this diff:
append_trusted_peers() {
+ local lock_fd=9
+ local lock_file="$TRUSTED_PEERS_FILE.lock"
peer_id=""
start_time=$(date +%s)
- timeout=30
+ timeout="${PEER_DISCOVERY_TIMEOUT:-30}"
+
+ # Ensure cleanup on script exit
+ trap 'cleanup_lock $lock_fd "$lock_file"' EXIT
while [[ -z "$peer_id" ]]; do
- peer_id=$(celestia p2p info | jq -r '.result.id' || echo "")
+ peer_id=$(celestia p2p info 2>/dev/null | jq -r '.result.id' || echo "")
if [[ -z "$peer_id" ]]; then
echo "Node is not running yet. Retrying..."
sleep 1
fi
current_time=$(date +%s)
elapsed=$((current_time - start_time))
if [[ $elapsed -ge $timeout ]]; then
- echo "Failed to retrieve Peer ID after $timeout seconds. Exiting."
+ echo "Error: Failed to retrieve Peer ID after $timeout seconds"
+ echo "Debug info:"
+ celestia p2p info || true
exit 1
fi
done
- #multiaddr: /dns/$CONTAINER_NAME/tcp/$RPC_PORT/p2p/$peer_id
- multiaddr="/dns/$CONTAINER_NAME/tcp/2121/p2p/$peer_id"
+ local multiaddr="/dns/$CONTAINER_NAME/tcp/2121/p2p/$peer_id"
echo "Appending trusted peer: $multiaddr"
# Lock the file to prevent race conditions
- exec 9>"$TRUSTED_PEERS_FILE.lock"
- flock -x 9
+ exec $lock_fd>"$lock_file"
+ if ! flock -x -w 10 $lock_fd; then
+ echo "Error: Could not acquire lock on $lock_file"
+ exit 1
+ fi
# Read existing peers into a variable
existing_peers=""
if [[ -s "$TRUSTED_PEERS_FILE" ]]; then
existing_peers=$(cat "$TRUSTED_PEERS_FILE")
fi
# Append the new multiaddr to the existing peers
if [[ -n "$existing_peers" ]]; then
echo "$existing_peers,$multiaddr" > "$TRUSTED_PEERS_FILE"
else
echo "$multiaddr" > "$TRUSTED_PEERS_FILE"
fi
# Unlock the file
- flock -u 9
- exec 9>&-
+ cleanup_lock $lock_fd "$lock_file"
}
+
+cleanup_lock() {
+ local fd=$1
+ local lock_file=$2
+ flock -u "$fd" 2>/dev/null
+ exec {fd}>&-
+ rm -f "$lock_file"
+}📝 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.
| append_trusted_peers() { | |
| peer_id="" | |
| start_time=$(date +%s) | |
| timeout=30 | |
| while [[ -z "$peer_id" ]]; do | |
| peer_id=$(celestia p2p info | jq -r '.result.id' || echo "") | |
| if [[ -z "$peer_id" ]]; then | |
| echo "Node is not running yet. Retrying..." | |
| sleep 1 | |
| fi | |
| current_time=$(date +%s) | |
| elapsed=$((current_time - start_time)) | |
| if [[ $elapsed -ge $timeout ]]; then | |
| echo "Failed to retrieve Peer ID after $timeout seconds. Exiting." | |
| exit 1 | |
| fi | |
| done | |
| #multiaddr: /dns/$CONTAINER_NAME/tcp/$RPC_PORT/p2p/$peer_id | |
| multiaddr="/dns/$CONTAINER_NAME/tcp/2121/p2p/$peer_id" | |
| echo "Appending trusted peer: $multiaddr" | |
| # Lock the file to prevent race conditions | |
| exec 9>"$TRUSTED_PEERS_FILE.lock" | |
| flock -x 9 | |
| # Read existing peers into a variable | |
| existing_peers="" | |
| if [[ -s "$TRUSTED_PEERS_FILE" ]]; then | |
| existing_peers=$(cat "$TRUSTED_PEERS_FILE") | |
| fi | |
| # Append the new multiaddr to the existing peers | |
| if [[ -n "$existing_peers" ]]; then | |
| echo "$existing_peers,$multiaddr" > "$TRUSTED_PEERS_FILE" | |
| else | |
| echo "$multiaddr" > "$TRUSTED_PEERS_FILE" | |
| fi | |
| # Unlock the file | |
| flock -u 9 | |
| exec 9>&- | |
| } | |
| append_trusted_peers() { | |
| local lock_fd=9 | |
| local lock_file="$TRUSTED_PEERS_FILE.lock" | |
| peer_id="" | |
| start_time=$(date +%s) | |
| timeout="${PEER_DISCOVERY_TIMEOUT:-30}" | |
| # Ensure cleanup on script exit | |
| trap 'cleanup_lock $lock_fd "$lock_file"' EXIT | |
| while [[ -z "$peer_id" ]]; do | |
| peer_id=$(celestia p2p info 2>/dev/null | jq -r '.result.id' || echo "") | |
| if [[ -z "$peer_id" ]]; then | |
| echo "Node is not running yet. Retrying..." | |
| sleep 1 | |
| fi | |
| current_time=$(date +%s) | |
| elapsed=$((current_time - start_time)) | |
| if [[ $elapsed -ge $timeout ]]; then | |
| echo "Error: Failed to retrieve Peer ID after $timeout seconds" | |
| echo "Debug info:" | |
| celestia p2p info || true | |
| exit 1 | |
| fi | |
| done | |
| local multiaddr="/dns/$CONTAINER_NAME/tcp/2121/p2p/$peer_id" | |
| echo "Appending trusted peer: $multiaddr" | |
| # Lock the file to prevent race conditions | |
| exec $lock_fd>"$lock_file" | |
| if ! flock -x -w 10 $lock_fd; then | |
| echo "Error: Could not acquire lock on $lock_file" | |
| exit 1 | |
| fi | |
| # Read existing peers into a variable | |
| existing_peers="" | |
| if [[ -s "$TRUSTED_PEERS_FILE" ]]; then | |
| existing_peers=$(cat "$TRUSTED_PEERS_FILE") | |
| fi | |
| # Append the new multiaddr to the existing peers | |
| if [[ -n "$existing_peers" ]]; then | |
| echo "$existing_peers,$multiaddr" > "$TRUSTED_PEERS_FILE" | |
| else | |
| echo "$multiaddr" > "$TRUSTED_PEERS_FILE" | |
| fi | |
| # Unlock the file | |
| cleanup_lock $lock_fd "$lock_file" | |
| } | |
| cleanup_lock() { | |
| local fd=$1 | |
| local lock_file=$2 | |
| flock -u "$fd" 2>/dev/null | |
| exec {fd}>&- | |
| rm -f "$lock_file" | |
| } |
Don't review before #174 is merged
Closes #172
Summary by CodeRabbit
Release Notes
New Features
justfile.docker-compose.yml.Improvements
Cargo.tomlfor better performance.README.md.Bug Fixes
Chores