chore(ci): migrate docker build back into repo and remove docker buil… #6798
Conversation
…d section from ReadMe
WalkthroughThe changes update and streamline the Docker build and deployment configurations. GitHub Actions workflows have been modified by adjusting input options, branch build tags, and output settings while removing unnecessary environment variables and repository path references. The documentation now points to the latest Docker image and removes self-build instructions. Several legacy Dockerfiles and scripts for the Minotari Base Node and Tor have been deleted. In their place, new and updated Dockerfiles and configuration files for Monero, Tor, XMRig, and Tari applications have been added along with a new startup script that orchestrates application launch within Docker. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant WS as GitHub Workflow
participant DB as Docker Build Process
U->>WS: Trigger workflow_dispatch
WS->>WS: Read build_items input (without monerod and xmrig)
WS->>WS: Set branch build outputs (tag_alias → latest-testing, build_items → all)
WS->>DB: Execute Docker build steps
DB-->>WS: Return build status
WS->>U: Notify build completion
sequenceDiagram
participant C as Docker Container
participant S as start_tari_app.sh
participant FS as File System
C->>S: Start container & invoke startup script
S->>S: Display configuration variables
S->>S: Wait for Tor if WAIT_FOR_TOR is set
S->>FS: Check and create TARI_BASE directory if missing
S->>S: Change directory to TARI_BASE
S->>C: Execute Tari application command (APP_EXEC)
C-->>S: Application runs in container
Poem
🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (2)
.github/workflows/build_dockers_workflow.yml (2)
17-17:⚠️ Potential issueUpdate the Rust toolchain version.
The default toolchain is set to
nightly-2024-07-07which is outdated. Consider updating to a more recent nightly version or using a stable release.- default: nightly-2024-07-07 + default: nightly-2025-02-01
81-81: 🛠️ Refactor suggestionImplement error handling for matrix setup.
The TODO comment indicates missing error handling. Consider implementing checks for:
- Invalid JSON files
- Empty matrix selection
- Invalid build item names
🧹 Nitpick comments (7)
.github/workflows/build_dockers_workflow.yml (1)
216-216: Enable Docker provenance for supply chain security.The
provenanceflag is set to false. Consider enabling it to improve supply chain security by providing verifiable build metadata.- provenance: false + provenance: truebuildtools/docker_rig/start_tari_app.sh (2)
31-33: Add error handling for directory creation.The
mkdiroperation should include error handling to ensure the directory is created successfully.-if [[ ! -d "${TARI_BASE}" ]]; then - mkdir -p "${TARI_BASE}" -fi +if [[ ! -d "${TARI_BASE}" ]]; then + mkdir -p "${TARI_BASE}" || { + echo "Error: Failed to create directory ${TARI_BASE}" + exit 1 + } +fi
1-40: Add signal handling for graceful shutdown.The script should handle signals to ensure graceful shutdown of the application.
#!/bin/bash + +# Handle signals +cleanup() { + echo "Shutting down ${APP_NAME}..." + kill -TERM "$child" 2>/dev/null +} + +trap cleanup SIGTERM SIGINT + # # Docker Start Script for Tari applications🧰 Tools
🪛 Shellcheck (0.10.0)
[error] 23-23: Argument mixes string and array. Use * or separate argument.
(SC2145)
[error] 38-38: Argument mixes string and array. Use * or separate argument.
(SC2145)
[error] 39-39: Double quote array expansions to avoid re-splitting elements.
(SC2068)
buildtools/docker_rig/xmrig.Dockerfile (1)
47-48: Add HEALTHCHECK instruction.Consider adding a health check to monitor the XMRig process status.
COPY --from=builder /xmrig/build/xmrig /usr/local/bin/ + +# Add health check +HEALTHCHECK --interval=30s --timeout=5s --start-period=30s --retries=3 \ + CMD pgrep xmrig || exit 1buildtools/docker_rig/monerod.Dockerfile (1)
54-55: Simplify SHA256 environment variable assignment.The current implementation uses two lines to set the SHA256 environment variable. This can be simplified.
-ENV MONERO_SHA256=${MONERO_SHA256:-$MONERO_AMD64_SHA256} -ENV MONERO_SHA256=${MONERO_SHA256:-$MONERO_ARM64_SHA256} +ENV MONERO_SHA256=${MONERO_SHA256:-${MONERO_AMD64_SHA256:-$MONERO_ARM64_SHA256}}buildtools/docker_rig/tarilabs.Dockerfile (1)
161-162: Add HEALTHCHECK instruction.Consider adding a health check to monitor the application status.
ENTRYPOINT [ "start_tari_app.sh" ] CMD [ "--non-interactive-mode" ] + +# Add health check +HEALTHCHECK --interval=30s --timeout=5s --start-period=30s --retries=3 \ + CMD pgrep -f "$APP_EXEC" || exit 1.github/workflows/build_dockers.yml (1)
47-48: Verify the build configuration changes.The following changes have been made to the Docker build workflow:
- Commented out monerod and xmrig from build options
- Changed tag alias from latest-weekly to latest-testing
- Changed build_items from minotari_all to all
Please confirm if these changes align with the intended migration strategy.
Consider documenting these changes in the repository's documentation to help other developers understand the new build process.
Also applies to: 94-98
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.github/workflows/build_dockers.yml(2 hunks).github/workflows/build_dockers_workflow.yml(6 hunks)README.md(1 hunks)buildtools/docker/base_node.Dockerfile(0 hunks)buildtools/docker/start.sh(0 hunks)buildtools/docker/torrc(0 hunks)buildtools/docker_rig/3rdparty.json(1 hunks)buildtools/docker_rig/monerod.Dockerfile(1 hunks)buildtools/docker_rig/start_tari_app.sh(1 hunks)buildtools/docker_rig/tarilabs.Dockerfile(1 hunks)buildtools/docker_rig/tarisuite.json(1 hunks)buildtools/docker_rig/tor.Dockerfile(1 hunks)buildtools/docker_rig/xmrig.Dockerfile(1 hunks)
💤 Files with no reviewable changes (3)
- buildtools/docker/start.sh
- buildtools/docker/base_node.Dockerfile
- buildtools/docker/torrc
✅ Files skipped from review due to trivial changes (2)
- buildtools/docker_rig/3rdparty.json
- buildtools/docker_rig/tarisuite.json
🧰 Additional context used
🪛 Shellcheck (0.10.0)
buildtools/docker_rig/start_tari_app.sh
[error] 23-23: Argument mixes string and array. Use * or separate argument.
(SC2145)
[error] 38-38: Argument mixes string and array. Use * or separate argument.
(SC2145)
[error] 39-39: Double quote array expansions to avoid re-splitting elements.
(SC2068)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: machete
- GitHub Check: Cucumber tests / FFI
- GitHub Check: cargo check with stable
- GitHub Check: Cucumber tests / Base Layer
- GitHub Check: clippy
🔇 Additional comments (8)
README.md (1)
347-347: LGTM! Docker image reference updated correctly.The Docker image reference has been updated to use the latest-nextnet tag, which aligns with the PR objectives and provides a more maintainable approach.
buildtools/docker_rig/tor.Dockerfile (5)
11-15: LGTM! Well-structured package installation.The package installation is properly configured with:
- Minimum version constraint for Tor
- No-cache flag to reduce image size
- Helpful reference link to the Alpine package repository
17-20: LGTM! Good use of environment variables for versioning.The environment variables provide excellent transparency for debugging and maintenance by exposing:
- Dockerfile version
- Build architecture
- Alpine version
- Tor version
22-27: LGTM! Well-configured network and storage setup.The configuration properly:
- Exposes and documents both required Tor ports
- Defines persistent storage volumes for configuration and data
52-54: LGTM! Secure runtime configuration.The runtime configuration follows security best practices:
- Runs as non-root
toruser- Provides flexibility with missing config files
3-9:⚠️ Potential issueFix incorrect Alpine version.
Alpine Linux version 3.21 does not exist. The latest stable version is 3.19.
Apply this diff to fix the version:
-ARG ALPINE_VERSION=3.21 +ARG ALPINE_VERSION=3.19❌ Incorrect review comment
🌐 Web query:
What is the latest version of Alpine Linux?💡 Result:
The latest stable release of Alpine Linux as of February 2025 is version 3.21.2, released on January 8, 2025[1][4]. This release remains actively supported with security updates until November 1, 2026[1].
Key Details
- Release cycle: Alpine follows a rolling release model where new stable versions are created from development snapshots ("edge") approximately every six months[1].
- Current supported versions:
- 3.21.x (supported until Nov 2026)
- 3.20.x (supported until Apr 2026)
- 3.19.x (supported until Nov 2025)
- 3.18.x (supported until May 2025)[1]
To check your Alpine version:
cat /etc/alpine-releaseThis command will output your specific installed version[1][3]. For comprehensive system details including kernel version and architecture:
hostnamectl uname -rAlpine continues to emphasize security through position-independent executables and a hardened toolchain while maintaining its signature small footprint (~5MB base image)[4][5].
Citations:
- 1: https://endoflife.date/alpine
- 2: https://www.alpinelinux.org
- 3: https://linuxhandbook.com/check-linux-version/
- 4: https://en.wikipedia.org/wiki/Alpine_Linux
- 5: https://hub.docker.com/_/alpine
Incorrect Alpine version review comment
The latest available Alpine Linux stable release as of February 2025 is version 3.21.x (specifically 3.21.2). This confirms that using
ARG ALPINE_VERSION=3.21is appropriate per the current release cycle. No change is needed.Likely an incorrect or invalid review comment.
buildtools/docker_rig/xmrig.Dockerfile (1)
3-4: Verify the version downgrades.The Alpine version has been downgraded from 3.21 to 3.19, and XMRig from 6.22.2 to 6.21.3. Please confirm if these downgrades are intentional and if there are any known issues with the newer versions.
Also applies to: 12-13
✅ Verification successful
🌐 Web query:
What are the latest stable versions of Alpine Linux and XMRig? Are there any known issues with Alpine 3.21 or XMRig 6.22.2?💡 Result:
The latest stable version of Alpine Linux is 3.21.2 (released January 8, 2025)[1][9], while XMRig is at v6.22.2 (released November 3, 2024)[6][7].
Alpine Linux 3.21 Known Issues
- Filesystem Separation Limitation: Systems with separate
/and/usrpartitions are no longer supported[5].- Custom Kernel Requirements: Users must enable
CONFIG_FW_LOADER_COMPRESS_ZSTD=ydue to Zstd compression inlinux-firmware[5].- GNOME Software Compatibility:
gnome-softwareis excluded due to incompatibility with Alpine’sapkpackage format[4].- LoongArch64 Support: Initial support for this architecture is experimental but functional[8].
XMRig v6.22.2 Known Issues
- Thread Configuration on Intel CPUs: Requires fixes for thread count detection on Intel Core Ultra CPUs[6][7].
- Stability Risks:
- Overclocking instability or insufficient power supply can cause crashes[3].
- First-gen AMD Ryzen/EPYC CPUs may require BIOS-level
Opcachedisabling or MSR mods inconfig.json[3].- Windows scheduler bugs (post-v1903) may cause hashrate drops; unpinning CPU threads via
"rx": [-1,...]is a workaround[3].- Algorithm-Specific Requirements: Some algorithms (e.g., GhostRider) require specific CPU cache sizes and lack proxy support[10].
Both projects remain actively maintained, with Alpine emphasizing security and lightweight design[1][4], while XMRig focuses on multi-algorithm mining efficiency[2][10].
Citations:
- 1: https://endoflife.date/alpine
- 2: https://slashdot.org/software/p/XMRig/
- 3: https://xmrig.com/docs/miner/randomx-stability-troubleshooting-guide
- 4: https://www.theregister.com/2024/12/17/alpine_linux_321/
- 5: https://9to5linux.com/alpine-linux-3-21-released-with-linux-kernel-6-12-lts-initial-loongarch64-support
- 6: https://github.com/xmrig/xmrig/releases
- 7: https://archlinux.org/packages/extra/x86_64/xmrig/
- 8: https://wiki.alpinelinux.org/wiki/Release_Notes_for_Alpine_3.21.0
- 9: https://www.versio.io/product-release-end-of-life-eol-Alpine%20Linux-Alpine%20Linux.html
- 10: https://www.miner.download/en/xmrig
Action Required: Confirming Intentional Version Downgrades
Based on the latest stable releases and their known issues:
- Alpine Linux: The latest stable version is 3.21.2; however, Alpine 3.21 has reported challenges (e.g., filesystem separation limitations, kernel configuration requirements for Zstd, and GNOME software compatibility issues). The decision to revert to Alpine 3.19 may be aimed at avoiding these issues.
- XMRig: The latest version is v6.22.2, but it comes with known concerns such as thread configuration problems on some Intel CPUs and other stability risks. Downgrading to v6.21.3 could be a precautionary measure for improved reliability.
It appears the downgrades in the Dockerfile are consistent with concerns over the newer versions’ issues. Please confirm if these intentional adjustments did not overlook any dependency that might require the latest versions elsewhere.
buildtools/docker_rig/monerod.Dockerfile (1)
117-119: Verify health check endpoint security.The health check uses an HTTP endpoint. Ensure this endpoint is properly secured and doesn't expose sensitive information.
❌ Verification inconclusive
🌐 Web query:
What security considerations should be taken into account when exposing Monero daemon's HTTP endpoints?💡 Result:
When exposing Monero daemon (
monerod) HTTP endpoints, several critical security considerations must be addressed to mitigate risks such as unauthorized access, data leakage, and potential exploitation:
1. Authentication and Authorization
- Enable RPC authentication using
--rpc-loginwith strong credentials[7]. Without this, attackers can freely interact with exposed endpoints.- Restrict RPC commands by limiting unnecessary endpoints (e.g., avoid exposing administrative commands like
start_mining)[6][7].- Use TLS encryption (
--rpc-ssl) to protect credentials and data in transit[6].
2. Network Exposure Risks
- Avoid public exposure: Bind
monerodto localhost (--rpc-bind-ip 127.0.0.1) unless remote access is essential[4][7].- Firewall rules: Block external access to port 18081 (default RPC port) unless explicitly required[2][8].
- Monitor traffic: Use tools like
tcpdumpor intrusion detection systems (IDS) to detect suspicious connections (e.g., unexpected IPs accessing seed-related endpoints)[1][5].
3. Container Hardening (If Deployed in Docker)
- Disable inter-container communication via Docker’s
"icc": falsesetting[3].- Run containers as non-root users and apply
--no-new-privilegesto prevent privilege escalation[3][5].- Restrict environment variables: Avoid leaking credentials or API keys via metrics/logging[2][8].
4. Data Leakage Vulnerabilities
- Sensitive information exposure: Endpoints like
/get_transaction_poolor/get_infocan leak node metadata (e.g., blockchain height, peer IPs)[2][7]. Restrict access to trusted users.- Wallet integration risks: While
moneroddoesn’t handle private keys[4], compromised clients (e.g., maliciousmonero-wallet-cli) could exfiltrate seeds via HTTP requests[1].
5. Software Integrity
- Verify binary hashes: Ensure downloaded
monerodbinaries match official releases to avoid tampered versions[1].- Regular updates: Patch vulnerabilities promptly by tracking Monero’s GitHub repository[3][9].
6. Attack Surface Reduction
- Disable unused services: Avoid enabling
--restricted-rpcunless necessary for lightweight clients[7].- Limit bootstrap nodes: Use
set_bootstrap_daemononly with trusted nodes to prevent man-in-the-middle attacks[7].
7. Monitoring and Logging
- Audit logs: Review
bitmonero.logfor unusual activity (e.g., unexpected RPC calls)[4].- Detect cryptojacking: Monitor CPU/memory usage for unauthorized mining activity[5][8].
By addressing these considerations—through authentication controls, network segmentation, container hardening, and vigilant monitoring—administrators can securely expose Monero daemon HTTP endpoints while minimizing attack vectors[1][3][5][7].
Citations:
- 1: https://serhack.me/articles/cli-binaries-compromised-monero-analysis/
- 2: https://www.trendmicro.com/vinfo/us/security/news/virtualization-and-cloud/observability-exposed-exploring-risks-in-cloud-native-metrics
- 3: https://github.com/monero-project/meta/blob/master/SERVER_SETUP_HARDENING.md
- 4: https://docs.getmonero.org/interacting/monerod-reference/
- 5: https://www.trendmicro.com/vinfo/us/security/news/virtualization-and-cloud/malicious-docker-hub-container-images-cryptocurrency-mining
- 6: https://github.com/cirocosta/go-monero
- 7: https://web.getmonero.org/resources/developer-guides/daemon-rpc.html
- 8: https://eudl.eu/pdf/10.4108/eai.24-3-2022.2318925
- 9: https://prisonprofessors.com/masterclass-catalog/digital-economy/lesson-18-monero-xmr-prioritizing-privacy-and-security/
- 10: https://www.getmonero.org/resources/developer-guides/wallet-rpc.html
Health Check Endpoint Security – Verify Container RPC Configuration
The Dockerfile’s health check is currently using an unsecured HTTP call (i.e.
curl --fail http://localhost:18089/get_info). This is acceptable if the Monero daemon is configured to bind solely to localhost and is not exposed outside the container. However, as highlighted in recent research, consider the following to ensure endpoint security:
- Local Binding: Confirm that Monero’s RPC (and any related HTTP endpoints) are configured with
--rpc-bind-ip=127.0.0.1(or an equivalent restriction) so that they are not inadvertently accessible from an external network.- Authentication: Verify that, if any RPC endpoint exposure is required (or if there’s any risk of spillover), RPC authentication (such as using
--rpc-login) is enabled to prevent unauthorized access.- Network Controls: Ensure that Docker networking and any external firewall rules prevent unintended public access to the port in use (here, port 18089).
- Additional Hardening: Evaluate whether additional measures (e.g., TLS encryption) make sense in your deployment context—especially if sensitive node metadata might be exposed on the
/get_infoendpoint.Please review the overall container and monerod configuration to confirm that these security measures are in place. If they are, the current health check is sufficient; if not, adjustments will be required to align with best practices.
| # Tell Docker to periodically run curl as a way of checking that Tor is runnning OK, | ||
| # and is able to build a circuit. Link goes to a Tor Project page, which checks that | ||
| # client is accessing it through Tor network and not directly. It gives false negatives | ||
| # sometimes, so we should allow several retries. | ||
| # | ||
| # --socks5-hostname parameter is very important - it tells curl to ask proxy (Tor) for DNS lookup, | ||
| # instead of doing it on its own - the behavior that torrc file above explicitly prohibits, | ||
| # because it opens a possibility for a traffic correlation attack. | ||
| # | ||
| # --location flag is added just in case Tor Project changes the location of the page and puts a redirect at | ||
| # the previos location, so curl can follow that redirect. | ||
| # | ||
| # grep gets the output of curl and looks for first occurence of the string 'Congratulations', | ||
| # exits with 0 if found and 1 otherwise. Nothing is printed to stdout during this command. | ||
|
|
||
| # Currently this healthcheck is problematic and may need very long timeout periods to operate. The endpoint we're using | ||
| # isn't controlled by us and sometimes takes up to 45seconds to return, othertimes it gets denied by cloudflare. | ||
| # Additionally the HEALTHCHECK emits docker events that launchpad doesn't currently handle well, and will cause the | ||
| # container to show as offline. | ||
| # HEALTHCHECK --interval=120s --timeout=30s --start-period=60s --retries=5 \ | ||
| # CMD curl --silent --location --socks5-hostname localhost:9050 https://check.torproject.org/?lang=en_US | \ | ||
| # grep -qm1 Congratulations |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider implementing a more reliable healthcheck mechanism.
The current healthcheck implementation has several issues:
- Long response times from the external endpoint
- Potential Cloudflare denials
- Docker event handling problems with launchpad
Consider implementing a local healthcheck that verifies Tor's functionality without external dependencies. Here's a suggested implementation:
-# HEALTHCHECK --interval=120s --timeout=30s --start-period=60s --retries=5 \
-# CMD curl --silent --location --socks5-hostname localhost:9050 https://check.torproject.org/?lang=en_US | \
-# grep -qm1 Congratulations
+HEALTHCHECK --interval=30s --timeout=10s --start-period=60s --retries=3 \
+ CMD nc -z localhost 9050 && nc -z localhost 9051 || exit 1This approach:
- Checks if both required ports are listening
- Avoids external dependencies
- Has faster response times
- Is more reliable
📝 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.
| # Tell Docker to periodically run curl as a way of checking that Tor is runnning OK, | |
| # and is able to build a circuit. Link goes to a Tor Project page, which checks that | |
| # client is accessing it through Tor network and not directly. It gives false negatives | |
| # sometimes, so we should allow several retries. | |
| # | |
| # --socks5-hostname parameter is very important - it tells curl to ask proxy (Tor) for DNS lookup, | |
| # instead of doing it on its own - the behavior that torrc file above explicitly prohibits, | |
| # because it opens a possibility for a traffic correlation attack. | |
| # | |
| # --location flag is added just in case Tor Project changes the location of the page and puts a redirect at | |
| # the previos location, so curl can follow that redirect. | |
| # | |
| # grep gets the output of curl and looks for first occurence of the string 'Congratulations', | |
| # exits with 0 if found and 1 otherwise. Nothing is printed to stdout during this command. | |
| # Currently this healthcheck is problematic and may need very long timeout periods to operate. The endpoint we're using | |
| # isn't controlled by us and sometimes takes up to 45seconds to return, othertimes it gets denied by cloudflare. | |
| # Additionally the HEALTHCHECK emits docker events that launchpad doesn't currently handle well, and will cause the | |
| # container to show as offline. | |
| # HEALTHCHECK --interval=120s --timeout=30s --start-period=60s --retries=5 \ | |
| # CMD curl --silent --location --socks5-hostname localhost:9050 https://check.torproject.org/?lang=en_US | \ | |
| # grep -qm1 Congratulations | |
| # Tell Docker to periodically run curl as a way of checking that Tor is runnning OK, | |
| # and is able to build a circuit. Link goes to a Tor Project page, which checks that | |
| # client is accessing it through Tor network and not directly. It gives false negatives | |
| # sometimes, so we should allow several retries. | |
| # | |
| # --socks5-hostname parameter is very important - it tells curl to ask proxy (Tor) for DNS lookup, | |
| # instead of doing it on its own - the behavior that torrc file above explicitly prohibits, | |
| # because it opens a possibility for a traffic correlation attack. | |
| # | |
| # --location flag is added just in case Tor Project changes the location of the page and puts a redirect at | |
| # the previos location, so curl can follow that redirect. | |
| # | |
| # grep gets the output of curl and looks for first occurence of the string 'Congratulations', | |
| # exits with 0 if found and 1 otherwise. Nothing is printed to stdout during this command. | |
| # Currently this healthcheck is problematic and may need very long timeout periods to operate. The endpoint we're using | |
| # isn't controlled by us and sometimes takes up to 45seconds to return, othertimes it gets denied by cloudflare. | |
| # Additionally the HEALTHCHECK emits docker events that launchpad doesn't currently handle well, and will cause the | |
| # container to show as offline. | |
| HEALTHCHECK --interval=30s --timeout=10s --start-period=60s --retries=3 \ | |
| CMD nc -z localhost 9050 && nc -z localhost 9051 || exit 1 |
| echo "WAIT_FOR_TOR: $WAIT_FOR_TOR" | ||
| echo "base folder (in container): ${TARI_BASE}" | ||
| echo "config file (in container): ${TARI_CONFIG}" | ||
| echo "arguments (in container): ${@}" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix shellcheck warnings about array expansion.
The script has issues with array expansion that could lead to word splitting.
-echo "arguments (in container): ${@}"
+echo "arguments (in container): $*"
-echo " > command: ${APP_EXEC} --config ${TARI_CONFIG} --base-path ${TARI_BASE} ${@} <"
-exec ${APP_EXEC} --config ${TARI_CONFIG} --base-path ${TARI_BASE} ${@} || exit 1
+echo " > command: ${APP_EXEC} --config ${TARI_CONFIG} --base-path ${TARI_BASE} $* <"
+exec "${APP_EXEC}" --config "${TARI_CONFIG}" --base-path "${TARI_BASE}" "$@" || exit 1Also applies to: 38-39
🧰 Tools
🪛 Shellcheck (0.10.0)
[error] 23-23: Argument mixes string and array. Use * or separate argument.
(SC2145)
| APP_NAME=${APP_NAME:-base_node} | ||
| APP_EXEC=${APP_EXEC:-tari_base_node} | ||
| WAIT_FOR_TOR=${WAIT_FOR_TOR:-0} | ||
| TARI_BASE=${TARI_BASE:-/var/tari/$APP_NAME} | ||
| TARI_CONFIG=${TARI_CONFIG:-/var/tari/config/config.toml} | ||
| USER_ID=${USER_ID:-1000} | ||
| GROUP_ID=${GROUP_ID:-1000} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for required environment variables.
The script should validate that required environment variables are set and contain valid values.
+# Validate required environment variables
+for var in APP_NAME APP_EXEC; do
+ if [[ -z "${!var}" ]]; then
+ echo "Error: ${var} environment variable is required"
+ exit 1
+ fi
+done
+
APP_NAME=${APP_NAME:-base_node}
APP_EXEC=${APP_EXEC:-tari_base_node}📝 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.
| APP_NAME=${APP_NAME:-base_node} | |
| APP_EXEC=${APP_EXEC:-tari_base_node} | |
| WAIT_FOR_TOR=${WAIT_FOR_TOR:-0} | |
| TARI_BASE=${TARI_BASE:-/var/tari/$APP_NAME} | |
| TARI_CONFIG=${TARI_CONFIG:-/var/tari/config/config.toml} | |
| USER_ID=${USER_ID:-1000} | |
| GROUP_ID=${GROUP_ID:-1000} | |
| # Validate required environment variables | |
| for var in APP_NAME APP_EXEC; do | |
| if [[ -z "${!var}" ]]; then | |
| echo "Error: ${var} environment variable is required" | |
| exit 1 | |
| fi | |
| done | |
| APP_NAME=${APP_NAME:-base_node} | |
| APP_EXEC=${APP_EXEC:-tari_base_node} | |
| WAIT_FOR_TOR=${WAIT_FOR_TOR:-0} | |
| TARI_BASE=${TARI_BASE:-/var/tari/$APP_NAME} | |
| TARI_CONFIG=${TARI_CONFIG:-/var/tari/config/config.toml} | |
| USER_ID=${USER_ID:-1000} | |
| GROUP_ID=${GROUP_ID:-1000} |
|
|
||
| ARG XMRIG_VERSION | ||
| ARG VERSION=1.0.1 | ||
| COPY --from=builder /xmrig/build/xmrig /usr/local/bin/ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Set explicit permissions for the binary.
Set appropriate permissions for the XMRig binary to ensure secure execution.
COPY --from=builder /xmrig/build/xmrig /usr/local/bin/
+RUN chmod 755 /usr/local/bin/xmrig📝 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.
| COPY --from=builder /xmrig/build/xmrig /usr/local/bin/ | |
| COPY --from=builder /xmrig/build/xmrig /usr/local/bin/ | |
| RUN chmod 755 /usr/local/bin/xmrig |
| RUN apt-get update && \ | ||
| apt-get install --no-install-recommends --assume-yes \ | ||
| apt-transport-https \ | ||
| bash \ | ||
| ca-certificates \ | ||
| curl \ | ||
| gpg \ | ||
| iputils-ping \ | ||
| less \ | ||
| libreadline8 \ | ||
| libreadline-dev \ | ||
| libsqlite3-0 \ | ||
| openssl \ | ||
| procps \ | ||
| lsof && \ | ||
| # Docker image reduction | ||
| apt-get clean all && \ | ||
| apt-get autoremove --assume-yes && \ | ||
| rm -rf /var/lib/apt/lists/* /tmp/* /var/tmp/* |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Pin package versions for reproducible builds.
The apt-get install command doesn't specify versions for the installed packages. This could lead to inconsistent builds.
RUN apt-get update && \
apt-get install --no-install-recommends --assume-yes \
- apt-transport-https \
- bash \
- ca-certificates \
- curl \
+ apt-transport-https=2.6.1 \
+ bash=5.2.15-2+b2 \
+ ca-certificates=20230311 \
+ curl=7.88.1-10+deb12u5 \📝 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.
| RUN apt-get update && \ | |
| apt-get install --no-install-recommends --assume-yes \ | |
| apt-transport-https \ | |
| bash \ | |
| ca-certificates \ | |
| curl \ | |
| gpg \ | |
| iputils-ping \ | |
| less \ | |
| libreadline8 \ | |
| libreadline-dev \ | |
| libsqlite3-0 \ | |
| openssl \ | |
| procps \ | |
| lsof && \ | |
| # Docker image reduction | |
| apt-get clean all && \ | |
| apt-get autoremove --assume-yes && \ | |
| rm -rf /var/lib/apt/lists/* /tmp/* /var/tmp/* | |
| RUN apt-get update && \ | |
| apt-get install --no-install-recommends --assume-yes \ | |
| apt-transport-https=2.6.1 \ | |
| bash=5.2.15-2+b2 \ | |
| ca-certificates=20230311 \ | |
| curl=7.88.1-10+deb12u5 \ | |
| gpg \ | |
| iputils-ping \ | |
| less \ | |
| libreadline8 \ | |
| libreadline-dev \ | |
| libsqlite3-0 \ | |
| openssl \ | |
| procps \ | |
| lsof && \ | |
| # Docker image reduction | |
| apt-get clean all && \ | |
| apt-get autoremove --assume-yes && \ | |
| rm -rf /var/lib/apt/lists/* /tmp/* /var/tmp/* |
| #ENV RUSTFLAGS="-C target_cpu=$ARCH" | ||
| #ENV ROARING_ARCH=$ARCH |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Document the reason for commented build flags.
The RUSTFLAGS and ROARING_ARCH environment variables are commented out. Please document why these optimizations are disabled.
🏁 Script executed:
#!/bin/bash
# Check if these flags are used elsewhere in the codebase
rg "RUSTFLAGS.*target_cpu"
rg "ROARING_ARCH"Length of output: 478
Action: Document the Disabled Build Flags in tarilabs.Dockerfile
- The Dockerfile has the following commented-out lines:
#ENV RUSTFLAGS="-C target_cpu=$ARCH"#ENV ROARING_ARCH=$ARCH
- In other parts of the repository:
- buildtools/build-notes.md exports
RUSTFLAGS='-C target_cpu=generic'andROARING_ARCH=generic. - scripts/cross_compile_tooling.sh also has these exports commented out.
- buildtools/build-notes.md exports
- Recommendation: Update the Dockerfile (and possibly accompanying documentation) to explain why these build flags are disabled. Clarify whether the Docker image is intended to use the generic defaults defined elsewhere (as in the build-notes) or if there’s another reason for omitting these optimizations in this context.
Test Results (CI) 3 files 129 suites 36m 0s ⏱️ Results for commit eb9b250. |
Description
Migrate docker build back into repo
Remove old docker build section from ReadMe
Disabled xmrig and monerod docker builds, as they failing
Motivation and Context
Easier to maintain docker images
How Has This Been Tested?
Builds in local fork
Summary by CodeRabbit
New Features
Documentation
Chores