ci: fix cargo caches and add celestia integration test #124
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe pull request introduces significant updates to the CI workflow, dependency management, and Docker configurations for a Celestia blockchain application. Key changes include renaming and restructuring CI jobs, enhancing caching mechanisms, and adding new jobs for integration testing and unused dependency checks. Additionally, several Dockerfiles for different components of the Celestia stack have been created, along with scripts for managing nodes. Dependency versions in Changes
Possibly related PRs
Poem
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 37
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
Files selected for processing (14)
- .github/workflows/ci.yml (3 hunks)
- Cargo.toml (1 hunks)
- ci/Dockerfile.bridge (1 hunks)
- ci/Dockerfile.lightnode (1 hunks)
- ci/Dockerfile.validator (1 hunks)
- ci/credentials/.gitignore (1 hunks)
- ci/docker-compose.yml (1 hunks)
- ci/run-bridge.sh (1 hunks)
- ci/run-lightnode.sh (1 hunks)
- ci/run-validator.sh (1 hunks)
- crates/prism/src/da/celestia.rs (5 hunks)
- crates/prism/src/node_types/sequencer.rs (2 hunks)
- crates/prism/tests/integration_tests.rs (4 hunks)
- justfile (1 hunks)
Files not reviewed due to content moderation or server errors (5)
- ci/credentials/.gitignore
- ci/Dockerfile.validator
- ci/run-bridge.sh
- Cargo.toml
- crates/prism/src/node_types/sequencer.rs
Additional context used
actionlint
.github/workflows/ci.yml
66-66: job "integration_test" needs job "test" which does not exist in this workflow
(job-needs)
Hadolint
ci/Dockerfile.bridge
[warning] 10-10: Pin versions in apk add. Instead of
apk add <package>useapk add <package>=<version>(DL3018)
[warning] 20-20: Use arguments JSON notation for CMD and ENTRYPOINT arguments
(DL3025)
ci/Dockerfile.lightnode
[warning] 10-10: Pin versions in apk add. Instead of
apk add <package>useapk add <package>=<version>(DL3018)
[warning] 20-20: Use arguments JSON notation for CMD and ENTRYPOINT arguments
(DL3025)
ci/Dockerfile.validator
[warning] 10-10: Pin versions in apk add. Instead of
apk add <package>useapk add <package>=<version>(DL3018)
[warning] 20-20: Use arguments JSON notation for CMD and ENTRYPOINT arguments
(DL3025)
Shellcheck
ci/run-bridge.sh
[info] 72-72: Double quote to prevent globbing and word splitting.
(SC2086)
ci/run-lightnode.sh
[info] 72-72: Double quote to prevent globbing and word splitting.
(SC2086)
Additional comments not posted (12)
ci/docker-compose.yml (2)
2-15: Thevalidatorservice configuration looks good.
- Using a custom
validatorimage built fromDockerfile.validatoris a good practice for reproducibility.- Environment variables
BRIDGE_COUNTandLIGHT_COUNTallow customizing the number of bridge and light nodes.- Mounting
credentialsandgenesisvolumes enables persisting and sharing data between services.
86-98: The volumes configuration looks good.
- Using named volumes
credentialsandgenesisis a good practice for data persistence and sharing between services.- The
credentialsvolume is appropriately configured to persist data on the host machine in the "./credentials" directory.- The
genesisvolume is using tmpfs for ephemeral storage, which is suitable for temporary data that doesn't need to persist across container restarts.ci/Dockerfile.lightnode (2)
6-6: Good use of a specific base image version.Specifying the exact version of the base image helps ensure reproducible builds. Well done!
13-14: Efficient use of multi-stage builds.Copying only the necessary binaries from a specific version of the
celestia-nodeimage is an efficient way to keep the final image size small. It also helps with reproducibility. Good job!.github/workflows/ci.yml (2)
Line range hint
12-64: Theunit-testjob looks good!The job is well-structured and covers the essential aspects:
- Running on the latest Ubuntu
- Testing against nightly Rust
- Caching Cargo dependencies
- Excluding integration tests
Nice work on setting this up! The caching, in particular, should help speed up the builds.
140-162: Theunused-depsjob looks solid.Checking for unused dependencies is a good practice to keep the codebase clean. Using the nightly toolchain and running with
--all-features --all-targetsensures a thorough check.Good thinking adding this job! It'll help prevent cruft from accumulating over time.
crates/prism/tests/integration_tests.rs (1)
114-114: Verify the impact of reducing the number of new accountsReducing the range for generating new accounts from
1..=10to1..=3may limit the test's ability to simulate a wider variety of scenarios. Ensure that this change aligns with the test objectives and doesn't compromise the robustness of the testing.ci/run-validator.sh (3)
178-178: Confirm Keyring Backend UsageUsing
--keyring-backend="test"is suitable for development but may pose security risks if used in production environments.Please ensure that this script is intended for testing purposes only. If deploying to production, consider using a secure keyring backend.
196-199: Review Reducedtimeout_commitValueModifying
timeout_committo"1s"can affect the stability of the network. Setting it too low may lead to errors or unexpected behavior.Ensure that the new timeout value is appropriate for your testing environment. If issues arise, consider adjusting it to a higher value.
201-201: Validateapp_versionDowngrade to "1"Changing the
app_versionfrom"2"to"1"ingenesis.jsonmay have implications on compatibility and functionality.Confirm that this version change is intentional and compatible with the rest of the Celestia application. Run the following script to check for any potential issues:
This will help identify if there are other configurations that expect
app_versionto be"2".crates/prism/src/da/celestia.rs (2)
148-148: Confirm thatTxConfig::default()meets transaction requirementsThe code now uses
TxConfig::default()in theblob_submitmethod. Please verify that the default transaction configuration aligns with the required gas prices and other parameters for successful blob submission in your network environment.
217-217: Confirm thatTxConfig::default()is appropriate for operation submissionsSimilarly, the
submit_operationsmethod now usesTxConfig::default(). Ensure that this default configuration is suitable for submitting operations and that it satisfies all necessary transaction settings.
distractedm1nd
force-pushed
the
new-ci
branch
from
4792725 to
a41f298
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
Files selected for processing (3)
- .github/workflows/ci.yml (3 hunks)
- crates/nova/Cargo.toml (0 hunks)
- crates/prism/Cargo.toml (0 hunks)
Files not reviewed due to no reviewable changes (2)
- crates/nova/Cargo.toml
- crates/prism/Cargo.toml
Additional comments not posted (4)
.github/workflows/ci.yml (4)
Line range hint
12-64: Theunit-testjob changes look good!The updates to the
unit-testjob are positive changes that make the CI workflow more efficient and maintainable:
- Renaming the job to
unit-testclarifies its purpose.- Simplifying the matrix to one Rust version should speed up CI.
- Using the
arduino/setup-protoc@v2action for Protoc installation is more robust.- Adding Cargo caching with
actions/cache@v3will reduce build times.- Focusing this job on unit tests and moving integration tests out simplifies the job's responsibilities.
Great work on these enhancements! The
unit-testjob looks solid.
66-138: The newintegration-testjob looks great!Adding a separate job for running integration tests is an excellent practice. It keeps the CI workflow modular and allows for a dedicated setup.
Some highlights of this job:
- Using Docker Compose to define the test environment ensures consistency across runs.
- Enabling caching for the validator and bridge node images will speed up builds.
- Waiting for the bridge node to be ready before running tests is crucial for reliable results.
- Scoping this job to only run integration tests keeps it focused and efficient.
Fantastic work on implementing this new job! It's a valuable addition to the CI workflow.
140-162: The newunused-depsjob is a great addition!Checking for unused dependencies is an important step in maintaining a clean and efficient codebase. This new job does it well:
- Using the
dtolnay/rust-toolchainaction ensures a reliable Rust installation.- Caching the
cargo-udepsinstallation withtaiki-e/cache-cargo-install-actionwill speed up runs.- Running
cargo-udepswith--all-features --all-targetsperforms a thorough check.Great job on adding this valuable check to the CI workflow! It will help keep the dependencies tidy.
190-190: The updated Clippy command looks good!Running Clippy with
--all --all-targetsensures a comprehensive lint check across the entire codebase.Enabling warnings as errors with
-D warningsis a strict but effective way to maintain code quality. It prevents any warnings from slipping through.Great job on making the Clippy check more thorough!
distractedm1nd
force-pushed
the
new-ci
branch
from
9dd0606 to
4e2521a
Compare
distractedm1nd
force-pushed
the
new-ci
branch
from
4e2521a to
fae1a1f
Compare
distractedm1nd
force-pushed
the
new-ci
branch
from
fae1a1f to
e083dba
Compare
There was a problem hiding this comment.
Actionable comments posted: 18
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
Files selected for processing (19)
- .github/workflows/ci.yml (3 hunks)
- Cargo.toml (1 hunks)
- ci/Dockerfile.bridge (1 hunks)
- ci/Dockerfile.lightnode (1 hunks)
- ci/Dockerfile.validator (1 hunks)
- ci/credentials/.gitignore (1 hunks)
- ci/docker-compose.yml (1 hunks)
- ci/run-bridge.sh (1 hunks)
- ci/run-lightnode.sh (1 hunks)
- ci/run-validator.sh (1 hunks)
- crates/errors/Cargo.toml (0 hunks)
- crates/nova/Cargo.toml (0 hunks)
- crates/prism/Cargo.toml (0 hunks)
- crates/prism/src/da/celestia.rs (5 hunks)
- crates/prism/src/main.rs (2 hunks)
- crates/prism/src/node_types/lightclient.rs (1 hunks)
- crates/prism/src/node_types/sequencer.rs (2 hunks)
- crates/prism/tests/integration_tests.rs (1 hunks)
- justfile (1 hunks)
Files not reviewed due to no reviewable changes (3)
- crates/errors/Cargo.toml
- crates/nova/Cargo.toml
- crates/prism/Cargo.toml
Additional context used
Hadolint
ci/Dockerfile.bridge
[warning] 10-10: Pin versions in apk add. Instead of
apk add <package>useapk add <package>=<version>(DL3018)
[warning] 20-20: Use arguments JSON notation for CMD and ENTRYPOINT arguments
(DL3025)
ci/Dockerfile.lightnode
[warning] 10-10: Pin versions in apk add. Instead of
apk add <package>useapk add <package>=<version>(DL3018)
[warning] 20-20: Use arguments JSON notation for CMD and ENTRYPOINT arguments
(DL3025)
ci/Dockerfile.validator
[warning] 10-10: Pin versions in apk add. Instead of
apk add <package>useapk add <package>=<version>(DL3018)
[warning] 20-20: Use arguments JSON notation for CMD and ENTRYPOINT arguments
(DL3025)
Shellcheck
ci/run-bridge.sh
[info] 72-72: Double quote to prevent globbing and word splitting.
(SC2086)
ci/run-lightnode.sh
[info] 72-72: Double quote to prevent globbing and word splitting.
(SC2086)
Additional comments not posted (48)
ci/credentials/.gitignore (1)
1-2: Excellent addition for securing credentials!This
.gitignorefile is well-structured and serves an important purpose:
- The
/*pattern ensures that all files and directories withinci/credentialsare ignored by Git.- The
!/.gitignoreexception allows this.gitignorefile itself to be tracked.This setup effectively prevents any sensitive information or credentials from being accidentally committed to the repository while ensuring that the ignore rules are shared with all contributors.
ci/Dockerfile.validator (1)
17-18: LGTM! Clear port exposure with descriptive comment.The port exposure is well-defined and the comment clearly describes the purpose of each port. This is good practice for container configuration.
ci/Dockerfile.bridge (5)
1-8: LGTM: Base image and environment setup look good.The use of a specific Alpine version (3.19.1) ensures reproducibility, and setting the CELESTIA_HOME environment variable is a good practice for application-specific configurations.
10-10: Consider pinning package versions for better reproducibility.While the package installation looks correct, it's recommended to pin the versions of the installed packages. This ensures consistent builds across different environments and times.
As suggested in a previous review, you can update the line as follows:
-RUN apk update && apk add --no-cache bash jq +RUN apk update && apk add --no-cache bash=5.2.15-r1 jq=1.6-r2Please verify the exact versions available in the Alpine 3.19.1 repository before applying this change.
Tools
Hadolint
[warning] 10-10: Pin versions in apk add. Instead of
apk add <package>useapk add <package>=<version>(DL3018)
12-14: LGTM: Binary copying is correctly implemented.The use of a specific version (v0.15.0) of the celestia-node image for copying the binaries ensures reproducibility. The COPY instructions are correctly implemented.
16-18: LGTM: Entrypoint script setup and port exposure are correct.The entrypoint script is correctly copied and renamed, and the necessary ports (2121 and 26658) are exposed for the application to communicate.
20-20: Consider using JSON notation for the CMD instruction.While the current CMD instruction will work, it's recommended to use JSON notation for clarity and consistency with Docker best practices.
As suggested in a previous review, you can update the line as follows:
-CMD /opt/entrypoint.sh +CMD ["/opt/entrypoint.sh"]This change improves readability and ensures consistent behavior across different shell environments.
Tools
Hadolint
[warning] 20-20: Use arguments JSON notation for CMD and ENTRYPOINT arguments
(DL3025)
ci/Dockerfile.lightnode (4)
1-8: LGTM: Good use of specific Alpine version and environment variable.The use of a specific Alpine version (3.19.1) ensures reproducibility, and setting the
CELESTIA_HOMEenvironment variable is a good practice for specifying the home directory.
12-14: LGTM: Good practice in copying specific binaries.The use of a specific version (v0.15.0) of the Celestia Node image for copying the binaries ensures reproducibility. Copying only the necessary binaries (celestia and cel-key) helps keep the image size small, which is a good Docker practice.
16-18: LGTM: Proper script setup and port exposure.Copying the
run-lightnode.shscript to/opt/entrypoint.shis a good practice for setting up the container's entry point. Exposing ports 2121 and 26658 is necessary for the Celestia node to communicate properly.
20-20: Use JSON notation for CMD.While the current
CMDinstruction will work, it's recommended to use the JSON notation to avoid potential shell string munging issues and align with Docker best practices.Please update the line to use JSON notation:
-CMD /opt/entrypoint.sh +CMD ["/opt/entrypoint.sh"]Likely invalid or redundant comment.
Tools
Hadolint
[warning] 20-20: Use arguments JSON notation for CMD and ENTRYPOINT arguments
(DL3025)
ci/docker-compose.yml (6)
1-15: LGTM: Validator service configuration is well-structuredThe validator service is properly configured with:
- A custom image built from
Dockerfile.validator- Environment variables to set the number of bridge and light nodes
- Appropriate volume mounts for credentials and genesis
This setup provides flexibility in node provisioning and consistent data management across services.
16-32: LGTM: Bridge-0 service configuration is appropriate, with a security reminderThe bridge-0 service is well-configured with:
- A custom image built from
Dockerfile.bridge- Correct NODE_ID setting
- Proper port exposure and volume mounts
Note: The
SKIP_AUTH=truesetting disables JWT authentication. Ensure this is intentional and only used in non-production environments.
33-49: LGTM: Bridge-1 service configuration is consistent with bridge-0The bridge-1 service maintains consistency with bridge-0 while appropriately differentiating:
- NODE_ID is correctly set to 1
- A different port (36658) is exposed, allowing multiple bridge nodes to run simultaneously
This configuration enables proper scaling of bridge nodes in the system.
51-66: LGTM: Light-0 service configuration is well-structuredThe light-0 service is properly configured with:
- A custom image built from
Dockerfile.lightnode- Correct NODE_ID setting
- Appropriate port exposure (46658) and volume mounts
This configuration aligns well with the overall architecture of the system.
86-98: LGTM: Volumes configuration is well-designedThe volumes configuration is appropriate and well-thought-out:
- The
credentialsvolume uses a local driver with bind mount, ensuring persistence of node credentials.- The
genesisvolume uses a tmpfs driver, which is suitable for temporary data that doesn't need to persist across restarts.This setup provides a good balance between data persistence and performance.
1-98: Overall, the docker-compose.yml file is well-structured and comprehensiveThis configuration file provides a robust setup for a blockchain application with multiple node types (validator, bridge, and light nodes). Key strengths include:
- Consistent use of custom images across services
- Proper differentiation of node types and IDs
- Appropriate port exposures for each service
- Well-designed volume configuration for both persistent and temporary data
The file structure promotes scalability and maintainability. Just ensure that the
SKIP_AUTHsetting is used judiciously, as mentioned in previous comments.Cargo.toml (1)
53-54: Dependency versions updated: Verify compatibility and review changelogsThe update of
celestia-rpcandcelestia-typesfrom version 0.2.0 to 0.4.0 is a good practice to keep dependencies current. However, this version jump might introduce significant changes.To ensure smooth integration:
- Review the changelogs for both
celestia-rpcandcelestia-typesto understand the changes between versions 0.2.0 and 0.4.0.- Verify that the updated versions are compatible with your current implementation.
- Run your test suite to catch any potential breaking changes.
Here's a script to help verify the impact of these changes:
This script will help identify areas of the codebase that might be affected by the dependency updates.
crates/prism/src/node_types/lightclient.rs (2)
Line range hint
1-11: Imports are consistent with the changesThe necessary import for
ed25519_dalek::VerifyingKeyis already present, which is consistent with the constructor parameter type change. No additional import changes are required.
44-44: Approve constructor parameter type changeThe change from
Option<String>toOption<VerifyingKey>for thesequencer_pubkeyparameter is a good improvement. It enhances type safety, simplifies the constructor, and potentially improves performance by moving the conversion logic outside this function.To ensure this change doesn't introduce any issues, please verify that all code calling this constructor has been updated to provide a
VerifyingKeyinstead of aString. Run the following script to check for any remaining usage of the old signature:If this script returns any results, those locations need to be updated to use the new
VerifyingKeytype..github/workflows/ci.yml (5)
Line range hint
12-65: LGTM: Improved job structure and caching mechanismThe changes to the
unit-testjob (previouslybuild) are well-implemented:
- The job renaming provides better clarity.
- The Redis service configuration is correctly defined.
- Protoc installation now uses the recommended action, addressing a previous review comment.
- The new caching mechanism for Cargo should significantly improve CI performance.
These changes should result in a more efficient and maintainable CI workflow.
66-143: LGTM: Well-structured integration test jobThe new
integration-testjob is a valuable addition to the CI workflow:
- The job dependency on
unit-testis correct, addressing the issue raised in a previous review.- The Docker Compose stack setup is comprehensive and well-structured.
- The integration test command is correctly specified.
This job will enhance the overall testing coverage of the project.
145-180: LGTM: Useful addition for dependency managementThe new
unused-depsjob is a valuable addition to the CI workflow:
- It uses
cargo-udepsto check for unused dependencies, which will help maintain a clean and efficient codebase.- The job is well-structured and consistent with other jobs in the workflow.
- The caching mechanism for Cargo is properly implemented, which should improve job performance.
This job will contribute to better dependency management in the project.
182-224: LGTM: Enhanced clippy jobThe modifications to the
clippyjob improve its effectiveness:
- Using the nightly Rust version aligns with the other jobs in the workflow.
- The Protoc installation now uses the recommended action, consistent with other jobs.
- The clippy command has been enhanced to include all targets and treat warnings as errors, which will help maintain high code quality.
These changes will contribute to more thorough code linting and consistency across the project.
Line range hint
1-224: Excellent improvements to the CI workflowThe changes to this CI workflow file are comprehensive and well-implemented:
- Job structure has been improved with clear naming and purpose.
- Caching mechanisms have been consistently implemented across all jobs, which should significantly improve CI performance.
- New jobs (
integration-testandunused-deps) have been added to enhance testing coverage and dependency management.- Previous review comments have been addressed, particularly regarding Protoc installation and job dependencies.
- The
clippyjob has been enhanced for more thorough code linting.These changes will result in a more efficient, comprehensive, and maintainable CI process. Great work on improving the overall quality of the project's continuous integration!
justfile (7)
5-6: Previous comment still applies: Remove unnecessary shebang line and set shell in Justfile
9-9: Previous comment still applies: Consistent use of 'docker-compose' vs 'docker compose'Also applies to: 12-12, 21-21, 43-43, 55-55, 58-58
15-15: Previous comment still applies: Clarify the timeout value with units
21-23: Previous comment still applies: Optimize log parsing for service readiness checks
33-33: Previous comment still applies: Ensure proper syntax for Boolean evaluations in Bash
62-63: Previous comment still applies: Remove unnecessary shebang line and set shell in Justfile
65-65: Previous comment still applies: Avoid calling 'just' within recipes; use dependencies insteadci/run-lightnode.sh (6)
13-13: EnsureCELESTIA_HOMEis definedThe variable
CELESTIA_HOMEmight not be set, which could lead to issues when settingCONFIG_DIR. Please ensure thatCELESTIA_HOMEis defined before it's used.
26-26: Correct the conditional expression in thewhileloopThe condition in the
whileloop may not function as intended due to the use of parentheses within[[ ]]. Consider revising the condition to avoid potential syntax errors.
35-35: Avoid hardcoding passwords in scriptsHardcoding the password
"password"can pose security risks, even in test environments. It's advisable to avoid embedding plaintext passwords in scripts.
48-48: Handle special characters insedsubstitutionUsing
$genesis_hashdirectly in thesedcommand may cause issues if the hash contains special characters. Consider using an alternative delimiter or properly escaping the variable.
68-68: Avoid using fixedsleepfor synchronizationUsing
sleep 10for synchronization might not be reliable due to variable startup times. It's better to implement a more robust method to check when the validator is ready.
72-72: Quote variable expansions to prevent globbing and word splittingThe variable
$SKIP_AUTHshould be wrapped in double quotes to prevent unexpected behavior if it contains spaces or special characters.Tools
Shellcheck
[info] 72-72: Double quote to prevent globbing and word splitting.
(SC2086)
ci/run-bridge.sh (1)
2-2: Verify licensing and attribution for borrowed codeThe comment indicates that this code is taken from
eigerco/lumina. Please ensure that any licensing requirements are met and appropriate attribution is provided, if necessary.crates/prism/src/main.rs (2)
11-11: ImportingVerifyingKeyis appropriateThe addition of
use ed25519_dalek::VerifyingKey;is necessary for handling the verification key in the code.
19-20: Importing base64 engine for decodingThe inclusion of
use base64::{engine::general_purpose::STANDARD as engine, Engine as _};is appropriate for decoding the base64-encoded verifying key from the configuration.crates/prism/src/da/celestia.rs (6)
8-8: ImportTxConfigfor transaction configurationsThe addition of
TxConfigin the import statement is appropriate for managing transaction settings in blob submissions.
105-119: Enhanced handling of missing blobsThe updated code now gracefully handles the case when no blobs are found at a given height by returning
Ok(None). This improves the robustness of theget_finalized_epochmethod.
110-115: Update error message to reflect actual deserialization formatThe error message still mentions "marshalling blob from height {} to epoch json", but deserialization is performed using
bincode, not JSON. This could lead to confusion during debugging.
155-168: Graceful handling of absent operation blobsThe code now correctly handles cases where no operation blobs are found by returning an empty vector. This prevents potential errors when there are no operations at a given height.
240-240: Implement proper error handling for height updatesThe
TODOcomment indicates that error handling forheight_update_tx.send(height)is still pending. Ignoring potential errors can lead to missed height updates or silent failures. Please handle theSendErrorto ensure reliability.
148-148: Verify thatTxConfig::default()meets transaction requirementsReplacing
GasPrice::from(-1.0)withTxConfig::default()changes how transaction configurations are set during blob submissions. Ensure that the default configuration aligns with the necessary parameters, such as gas prices or fees, required for successful transactions. If the defaults are insufficient, consider specifying the needed settings explicitly.Run the following script to inspect the default parameters of
TxConfig:Also applies to: 217-217
crates/prism/src/node_types/sequencer.rs (1)
18-25: Imports are appropriately addedThe imported modules support the new error handling and functionality introduced in the changes.
distractedm1nd
force-pushed
the
new-ci
branch
from
2ed5ee9 to
b262d58
Compare
There was a problem hiding this comment.
Actionable comments posted: 19
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
Files selected for processing (19)
- .github/workflows/ci.yml (3 hunks)
- Cargo.toml (1 hunks)
- ci/Dockerfile.bridge (1 hunks)
- ci/Dockerfile.lightnode (1 hunks)
- ci/Dockerfile.validator (1 hunks)
- ci/credentials/.gitignore (1 hunks)
- ci/docker-compose.yml (1 hunks)
- ci/run-bridge.sh (1 hunks)
- ci/run-lightnode.sh (1 hunks)
- ci/run-validator.sh (1 hunks)
- crates/errors/Cargo.toml (0 hunks)
- crates/nova/Cargo.toml (0 hunks)
- crates/prism/Cargo.toml (0 hunks)
- crates/prism/src/da/celestia.rs (5 hunks)
- crates/prism/src/main.rs (2 hunks)
- crates/prism/src/node_types/lightclient.rs (1 hunks)
- crates/prism/src/node_types/sequencer.rs (2 hunks)
- crates/prism/tests/integration_tests.rs (1 hunks)
- justfile (1 hunks)
Files not reviewed due to no reviewable changes (3)
- crates/errors/Cargo.toml
- crates/nova/Cargo.toml
- crates/prism/Cargo.toml
Additional context used
Hadolint
ci/Dockerfile.bridge
[warning] 10-10: Pin versions in apk add. Instead of
apk add <package>useapk add <package>=<version>(DL3018)
[warning] 20-20: Use arguments JSON notation for CMD and ENTRYPOINT arguments
(DL3025)
ci/Dockerfile.lightnode
[warning] 10-10: Pin versions in apk add. Instead of
apk add <package>useapk add <package>=<version>(DL3018)
[warning] 20-20: Use arguments JSON notation for CMD and ENTRYPOINT arguments
(DL3025)
ci/Dockerfile.validator
[warning] 10-10: Pin versions in apk add. Instead of
apk add <package>useapk add <package>=<version>(DL3018)
[warning] 20-20: Use arguments JSON notation for CMD and ENTRYPOINT arguments
(DL3025)
Shellcheck
ci/run-bridge.sh
[info] 72-72: Double quote to prevent globbing and word splitting.
(SC2086)
ci/run-lightnode.sh
[info] 72-72: Double quote to prevent globbing and word splitting.
(SC2086)
Additional comments not posted (34)
ci/credentials/.gitignore (1)
1-2: Excellent addition for securing credentials!This
.gitignorefile is well-structured to prevent accidental commits of sensitive information:
/*ignores all files and directories inci/credentials/.!/.gitignoreensures this file itself is tracked.This setup effectively secures potential credentials while maintaining the ignore rules.
ci/Dockerfile.validator (3)
17-18: LGTM! Clear port exposition with helpful comments.The exposition of specific ports (26656, 26657, 1317, and 9090) for p2p, rpc, and Prometheus is well-documented and follows good Docker practices.
6-8: Consider using a non-root user for improved security.While setting
CELESTIA_HOME=/rootworks, it's generally recommended to run containers with a non-root user for better security. Consider creating a dedicated user and setting their home directory asCELESTIA_HOME.Here's a suggested modification:
FROM docker.io/alpine:3.19.1 -ENV CELESTIA_HOME=/root +RUN adduser -D celestia +ENV CELESTIA_HOME=/home/celestia +USER celestiaThis change creates a non-root user 'celestia' and sets their home directory as
CELESTIA_HOME.Likely invalid or redundant comment.
12-15: Set execute permissions for the entrypoint script.The binary copy and script addition look good. Using a specific version of the celestia-appd binary ensures reproducibility.
For completeness, consider explicitly setting execute permissions for the entrypoint script:
COPY ./run-validator.sh /opt/entrypoint.sh +RUN chmod +x /opt/entrypoint.shThis ensures the script is executable, although it's likely already set by default.
Likely invalid or redundant comment.
ci/Dockerfile.bridge (5)
1-8: LGTM: Good base image selection and environment setupThe use of a specific Alpine version (3.19.1) is a good practice for reproducibility. Setting the CELESTIA_HOME environment variable is appropriate for configuration.
12-14: LGTM: Proper binary copying from a specific versionThe use of a specific version (v0.15.0) of the celestia-node image for copying binaries ensures reproducibility. Copying only the necessary binaries (celestia and cel-key) helps keep the image size optimized.
16-18: LGTM: Clear entrypoint setup and port exposureThe setup of the entrypoint script and the exposure of specific ports (2121 and 26658) are well-defined. This clearly documents the container's configuration and network requirements.
10-10: Consider pinning package versions for reproducibilityWhile the package installation is correct, it's recommended to pin the versions of the installed packages to ensure consistent builds across different environments and times.
Consider updating the line as follows:
-RUN apk update && apk add --no-cache bash jq +RUN apk update && apk add --no-cache bash=5.2.15-r5 jq=1.6-r3Note: Please verify the exact versions available in the Alpine 3.19.1 repositories and adjust accordingly.
Tools
Hadolint
[warning] 10-10: Pin versions in apk add. Instead of
apk add <package>useapk add <package>=<version>(DL3018)
20-20: Use JSON notation for CMD instructionWhile the current CMD instruction works, it's recommended to use JSON notation for clarity and consistency with Docker best practices.
Consider updating the line as follows:
-CMD /opt/entrypoint.sh +CMD ["/opt/entrypoint.sh"]This change improves readability and ensures proper parsing of the command.
Tools
Hadolint
[warning] 20-20: Use arguments JSON notation for CMD and ENTRYPOINT arguments
(DL3025)
ci/Dockerfile.lightnode (4)
6-8: LGTM: Good practices for base image and environment setup.Using a specific version of Alpine (3.19.1) is good for reproducibility. Setting the
CELESTIA_HOMEenvironment variable is likely necessary for the Celestia node configuration.
10-10: Consider pinning package versions for reproducibility.While using
--no-cacheis good practice to keep the image size small, it's recommended to pin package versions for reproducibility. This issue was previously raised but hasn't been addressed yet.As suggested in a previous review, consider updating the line to pin package versions:
-RUN apk update && apk add --no-cache bash jq +RUN apk update && apk add --no-cache bash=5.2.15-r1 jq=1.6-r2Note: Please use the specific versions that are appropriate for your use case and compatible with Alpine 3.19.1.
Tools
Hadolint
[warning] 10-10: Pin versions in apk add. Instead of
apk add <package>useapk add <package>=<version>(DL3018)
13-14: LGTM: Good practices for binary copying.Using a specific version (v0.15.0) of the Celestia Node image for copying binaries ensures reproducibility. Copying only the necessary binaries ('celestia' and 'cel-key') helps keep the image size small.
20-20: Use JSON array notation for CMD instruction.While the current
CMDinstruction will work, it's recommended to use the JSON array notation to improve signal handling and avoid potential issues with shell string parsing.As suggested in a previous review, please update the line to use JSON array notation:
-CMD /opt/entrypoint.sh +CMD ["/opt/entrypoint.sh"]This change ensures that the entrypoint script is executed directly, rather than being launched via a shell, which can lead to better signal handling in containerized environments.
Tools
Hadolint
[warning] 20-20: Use arguments JSON notation for CMD and ENTRYPOINT arguments
(DL3025)
ci/docker-compose.yml (5)
1-15: LGTM: Validator service configuration is well-structuredThe validator service is correctly configured with:
- A custom image built from
Dockerfile.validator- Environment variables to flexibly set the number of bridge and light nodes
- Appropriate volume mounts for credentials and genesis
This setup aligns well with the PR objectives and provides a good foundation for the blockchain application.
16-32: Configuration looks good, but note the existing security concernThe bridge-0 service configuration is appropriate, with correct image, NODE_ID, port exposure, and volume mounts. However, as noted in a previous review comment, the
SKIP_AUTH=truesetting is a potential security risk if this is not a dev/test environment.Please ensure that JWT authentication is enabled in production deployments.
51-66: Light node configuration is appropriate, but note the existing security concernThe light-0 service configuration is suitable for a light node, with the correct image, NODE_ID, port exposure, and volume mounts. However, as noted in a previous review comment, the
SKIP_AUTH=truesetting is a potential security risk if this is not a dev/test environment.Please ensure that JWT authentication is enabled in production deployments for all nodes, including light nodes.
86-98: LGTM: Volumes configuration is well-structuredThe volumes configuration is appropriate and follows best practices:
- The
credentialsvolume uses a local driver with a bind mount, ensuring that sensitive data persists across container restarts.- The
genesisvolume uses a tmpfs driver, which is suitable for temporary data that doesn't need to persist.This setup provides a good balance between data persistence and security for different types of information used by the blockchain nodes.
68-85: Remove commented-out bridge-1 service configurationAs noted in a previous review comment, this commented-out configuration for another bridge-1 service is redundant and should be removed. Keeping unused code, even in comments, can lead to confusion and maintenance issues over time.
Please remove this entire commented-out section to improve code clarity and maintainability.
Cargo.toml (1)
53-54: Dependency versions updated for celestia-rpc and celestia-typesThe versions for
celestia-rpcandcelestia-typeshave been updated from "0.2.0" to "0.4.0". This update aligns with the PR objective of improving the Celestia integration.To ensure these updates don't introduce any conflicts or breaking changes, let's verify the changelog or release notes for these dependencies:
crates/prism/src/node_types/lightclient.rs (1)
44-44: Approve the type change and verify its impact.The change from
Option<String>toOption<VerifyingKey>for thesequencer_pubkeyparameter is a good improvement. It provides type safety and eliminates the need for string-to-key conversion within the method. This aligns well with the usage in theverify_signaturemethod (line 86).To ensure this change doesn't break existing code, please run the following script to check for any calls to
LightClient::newthat might need updating:If any occurrences are found, please update them to provide a
VerifyingKeyinstead of aString.Verification successful
Verified the
LightClient::newcalls useOption<VerifyingKey>.All instances of
LightClient::newin the codebase are passingOption<VerifyingKey>for thesequencer_pubkeyparameter, ensuring type consistency and preventing runtime errors.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all occurrences of LightClient::new to verify if they need updating # Search for LightClient::new calls echo "Searching for LightClient::new calls:" rg --type rust "LightClient::new\s*\(" -A 3 # Search for potential string arguments that might need conversion echo "\nSearching for potential string arguments that might need conversion:" rg --type rust "LightClient::new\s*\([^)]*String" -A 3Length of output: 938
.github/workflows/ci.yml (5)
12-12: Job renaming and test command update look good.The renaming of the job from
buildtounit-testbetter reflects its purpose. The updated test command correctly excludes integration tests, which is a good separation of concerns.Also applies to: 63-64
42-53: Excellent addition of Cargo caching.The new caching mechanism for Cargo using
actions/cache@v3is a great improvement. This should significantly reduce CI time by reusing previously downloaded dependencies.
145-180: Excellent addition of unused dependencies check.The new
unused-depsjob is a valuable addition to the CI pipeline. It helps maintain a clean and efficient codebase by identifying unused dependencies. The job is well-structured, using appropriate caching and the required nightly toolchain.
182-224: Improved Clippy job with stricter checks.The
clippyjob has been significantly improved:
- The use of the nightly Rust version is now consistent with other jobs.
- The Protoc installation has been updated to use the
arduino/setup-protoc@v2action, addressing a previous inconsistency.- Cargo caching has been added, which should improve job performance.
- The clippy command now runs on all targets and treats warnings as errors, which is excellent for maintaining code quality.
These changes make the job more effective and consistent with the rest of the workflow.
Line range hint
1-224: Excellent overhaul of the CI workflow.This PR significantly improves the CI workflow:
- Job restructuring: Clear separation of unit tests, integration tests, and code quality checks.
- Consistent use of the nightly Rust toolchain across jobs.
- Implementation of effective caching mechanisms for Cargo dependencies.
- Addition of valuable checks like unused dependency detection.
- Improved Clippy job with stricter checks.
These changes should result in a more efficient, comprehensive, and maintainable CI pipeline. Great work on addressing previous inconsistencies and enhancing the overall quality of the workflow.
justfile (3)
1-2: File header and variable definition look goodThe
DOCKER_COMPOSE_FILEis clearly defined and sets the stage for consistent Docker operations.
52-53: Confirmation message is appropriateThe message confirming that the Celestia stack is up and running provides clear feedback to the user.
58-59: Logs command is correctly configuredThe
celestia-logsrecipe correctly tails the logs of the services, which is useful for real-time monitoring.ci/run-lightnode.sh (3)
1-79: Overall, the script is well-structured and effectiveThe script efficiently initializes and runs a Celestia light node with appropriate configurations and functions. Error handling and use of best practices enhance its maintainability.
Tools
Shellcheck
[info] 72-72: Double quote to prevent globbing and word splitting.
(SC2086)
41-49: Ensuresedhandles special characters ingenesis_hashIf
$genesis_hashcontains characters like slashes or other regex special characters, thesedsubstitution might fail. Using an alternative delimiter or escaping the variable can prevent this issue.Apply this diff to use an alternative delimiter:
echo "Trusting a genesis: $genesis_hash" - sed -i'.bak' "s/TrustedHash = .*/TrustedHash = $genesis_hash/" "$CONFIG_DIR/config.toml" + sed -i'.bak' "s|TrustedHash = .*|TrustedHash = $genesis_hash|" "$CONFIG_DIR/config.toml"Likely invalid or redundant comment.
56-79: Verify thatcelestiacommand-line tool is availableBefore running the main operations, ensure that the
celestiaCLI tool is installed and accessible to prevent runtime errors.Run the following script to check for the
celestiacommand:Tools
Shellcheck
[info] 72-72: Double quote to prevent globbing and word splitting.
(SC2086)
crates/prism/src/main.rs (1)
46-62: Previous review comment is still applicable regarding error handling insequencer_pubkeyinitialization.The prior feedback on improving error handling and readability for the
sequencer_pubkeyremains valid. Refactoring using the?operator to propagate errors will enhance code clarity and ensure proper error propagation.crates/prism/src/da/celestia.rs (1)
148-148: EnsureTxConfig::default()is appropriate for blob submissionsThe change replaces the previous transaction configuration with
TxConfig::default(). Please verify that the default transaction settings, such as gas prices and fees, are suitable for submitting blobs in your context.Also applies to: 217-217
crates/prism/src/node_types/sequencer.rs (1)
54-58: Review the error handling inself.da.start()The previous review comment is still applicable. The error handling for
self.da.start()could be improved to preserve the original error context instead of converting the error to a string. This ensures that valuable debugging information is not lost.
Based on
verify-hashchain-circuitbranchSummary by CodeRabbit
Summary by CodeRabbit
Release Notes
New Features
docker-compose.ymlfile for managing multiple services within the blockchain application.run-bridge.sh,run-lightnode.sh,run-validator.sh) to facilitate the initialization and management of nodes.justfilefor managing the Docker environment, including setup, teardown, and logging.Bug Fixes
Documentation
.gitignorefile to manage credential files in the CI directory.