feat: reth backup scripts#61
Conversation
|
Claude finished @randygrok's task —— View job PR Review: Reth Backup ScriptsI've completed a comprehensive review of the reth backup scripts. Here's my detailed feedback: Todo List:
🔍 Overall AssessmentThis is a well-structured backup solution with good separation of concerns and comprehensive documentation. The scripts demonstrate solid bash practices and thoughtful architecture. ✅ Strengths
|
Summary of ChangesHello @randygrok, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request delivers a robust and flexible solution for backing up the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a set of scripts for creating hot backups of a reth node's database, supporting both local and Docker-based environments. The scripts are well-structured and the documentation is comprehensive. My review focuses on improving the Dockerfile for better reproducibility and smaller image size, enhancing the robustness of the backup script by not silencing critical errors, and pointing out a security concern in the documentation's example commands.
| @@ -0,0 +1,24 @@ | |||
| FROM ghcr.io/evstack/ev-reth:latest | |||
There was a problem hiding this comment.
Using the :latest tag for a base image can lead to non-reproducible builds, as it can change unexpectedly. It's a best practice to pin to a specific version tag or a digest to ensure build consistency and avoid breaking changes.
| FROM ghcr.io/evstack/ev-reth:latest | ||
|
|
||
| ARG LIBMDBX_REPO=https://github.com/erthink/libmdbx.git | ||
| ARG LIBMDBX_REF=master |
There was a problem hiding this comment.
Using the master branch for dependencies can cause non-reproducible builds as the branch head moves. It's better to use a specific git tag or commit hash for LIBMDBX_REF to ensure that the same version of the dependency is always used.
| RUN set -eux; \ | ||
| apt-get update; \ | ||
| DEBIAN_FRONTEND=noninteractive apt-get install -y --no-install-recommends \ | ||
| build-essential \ | ||
| ca-certificates \ | ||
| cmake \ | ||
| git \ | ||
| jq \ | ||
| ; \ | ||
| rm -rf /var/lib/apt/lists/* | ||
|
|
||
| RUN set -eux; \ | ||
| git clone --depth 1 --branch "${LIBMDBX_REF}" "${LIBMDBX_REPO}" /tmp/libmdbx; \ | ||
| cmake -S /tmp/libmdbx -B /tmp/libmdbx/build -DCMAKE_BUILD_TYPE=Release; \ | ||
| cmake --build /tmp/libmdbx/build --target mdbx_copy mdbx_dump mdbx_chk; \ | ||
| install -m 0755 /tmp/libmdbx/build/mdbx_copy /usr/local/bin/mdbx_copy; \ | ||
| install -m 0755 /tmp/libmdbx/build/mdbx_dump /usr/local/bin/mdbx_dump; \ | ||
| install -m 0755 /tmp/libmdbx/build/mdbx_chk /usr/local/bin/mdbx_chk; \ | ||
| rm -rf /tmp/libmdbx |
There was a problem hiding this comment.
The current Dockerfile includes build-time dependencies like build-essential, cmake, and git in the final image, which significantly increases its size and potential attack surface. Using a multi-stage build would create a smaller, more secure final image. The first stage would build the libmdbx tools, and the final stage would copy only the necessary binaries from the builder stage.
Here is an example of how it could be structured:
# Builder stage
FROM ghcr.io/evstack/ev-reth:latest as builder
ARG LIBMDBX_REPO=https://github.com/erthink/libmdbx.git
ARG LIBMDBX_REF=master
RUN set -eux; \
apt-get update; \
DEBIAN_FRONTEND=noninteractive apt-get install -y --no-install-recommends \
build-essential \
ca-certificates \
cmake \
git \
; \
rm -rf /var/lib/apt/lists/*
RUN set -eux; \
git clone --depth 1 --branch "${LIBMDBX_REF}" "${LIBMDBX_REPO}" /tmp/libmdbx; \
cmake -S /tmp/libmdbx -B /tmp/libmdbx/build -DCMAKE_BUILD_TYPE=Release; \
cmake --build /tmp/libmdbx/build --target mdbx_copy mdbx_dump mdbx_chk;
# Final stage
FROM ghcr.io/evstack/ev-reth:latest
RUN set -eux; \
apt-get update; \
DEBIAN_FRONTEND=noninteractive apt-get install -y --no-install-recommends \
jq \
; \
rm -rf /var/lib/apt/lists/*
COPY --from=builder /tmp/libmdbx/build/mdbx_copy /usr/local/bin/mdbx_copy
COPY --from=builder /tmp/libmdbx/build/mdbx_dump /usr/local/bin/mdbx_dump
COPY --from=builder /tmp/libmdbx/build/mdbx_chk /usr/local/bin/mdbx_chk| docker run -d \ | ||
| --name evolveevm-ev-node-evm-single-1 \ | ||
| --network evolveevm_evolve-network \ | ||
| -p 7676:7676 -p 7331:7331 \ | ||
| -v evolveevm_evm-single-data:/root/.evm-single/ \ | ||
| -e EVM_ENGINE_URL=http://ev-reth:8551 \ | ||
| -e EVM_ETH_URL=http://ev-reth:8545 \ | ||
| -e EVM_JWT_SECRET=f747494bb0fb338a0d71f5f9fe5b5034c17cc988c229b59fd71e005ee692e9bf \ | ||
| -e EVM_GENESIS_HASH=0x2b8bbb1ea1e04f9c9809b4b278a8687806edc061a356c7dbc491930d8e922503 \ | ||
| -e EVM_BLOCK_TIME=1s \ | ||
| -e EVM_SIGNER_PASSPHRASE=secret \ | ||
| -e DA_ADDRESS=http://local-da:7980 \ | ||
| ghcr.io/evstack/ev-node-evm-single:main \ | ||
| start --evnode.clear_cache |
There was a problem hiding this comment.
The docker run command in this example contains hardcoded sensitive values like EVM_JWT_SECRET and EVM_SIGNER_PASSPHRASE. While this is for documentation, it's a security risk if copied into production environments as it encourages bad practice. It would be better to demonstrate using environment variables from a file (e.g., --env-file .env) or another secret management practice.
| $exec_remote "touch '$REMOTE_TMP/db/mdbx.lck'" | ||
|
|
||
| echo "Copying static_files..." | ||
| $exec_remote "if [ -d '${DATADIR}/static_files' ]; then cp -a '${DATADIR}/static_files/.' '$REMOTE_TMP/static_files/' 2>/dev/null || true; fi" |
There was a problem hiding this comment.
The command to copy static_files uses 2>/dev/null || true, which completely suppresses any errors during the copy process. If some files fail to copy (e.g., due to permissions), the backup might be incomplete without any warning. It would be safer to remove 2>/dev/null to allow errors to be visible, while || true can still prevent the script from exiting if a partial copy is acceptable.
| $exec_remote "if [ -d '${DATADIR}/static_files' ]; then cp -a '${DATADIR}/static_files/.' '$REMOTE_TMP/static_files/' 2>/dev/null || true; fi" | |
| $exec_remote "if [ -d '${DATADIR}/static_files' ]; then cp -a '${DATADIR}/static_files/.' '$REMOTE_TMP/static_files/' || true; fi" |
|
|
||
| if remote_path_exists "${REMOTE_TMP}/static_files"; then | ||
| mkdir -p "$HOST_DEST/static_files" | ||
| $copy_from_remote "${REMOTE_TMP}/static_files/." "$HOST_DEST/static_files/" || true |
There was a problem hiding this comment.
Using || true here suppresses the exit code from the $copy_from_remote command (which is docker cp in docker mode). If the copy from the container to the host fails, the script will continue as if successful, leading to an incomplete or empty backup. This is a critical step, and it's safer to let the script fail if this copy operation fails.
| $copy_from_remote "${REMOTE_TMP}/static_files/." "$HOST_DEST/static_files/" || true | |
| $copy_from_remote "${REMOTE_TMP}/static_files/." "$HOST_DEST/static_files/" |
3 participants
Description
Type of Change
Related Issues
Fixes #(issue)
Checklist
Testing
Additional Notes