ci: overhaul CI #253
ci: overhaul CI #253
Conversation
📝 WalkthroughWalkthroughThis PR centralizes CI: adds YAML group and no-std configs, a Python CI orchestration script, new/updated GitHub Actions workflows (including sanitizers), reorganizes fuzz targets, adds cargo-deny, and removes five legacy contrib test scripts. CI now validates groups, runs grouped tests and no-std checks via the new script. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant GH as GitHub Actions
participant Runner as CI runner (ubuntu/macos/win)
participant Script as .github/scripts/ci_config.py
participant Cargo as cargo (metadata/test/check)
participant FS as Repository files (YAML, Cargo.toml)
GH->>Runner: trigger build (inputs: os, groups)
Runner->>FS: read `.github/ci-*.yml` and workspace files
Runner->>Script: run ci_config.py (verify-groups/run-no-std/run-group)
Script->>Cargo: call `cargo metadata` (workspace crates)
Script->>FS: load YAML group/no-std configs
alt verify groups
Script->>GH: emit groups JSON output & annotations
else run no-std / run-group
Script->>Cargo: run `cargo check/test` per crate with feature combos
Cargo-->>Script: pass/fail results
Script->>GH: fold logs, emit annotations, set outputs
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
xdustinface
force-pushed
the
ci/overhaul
branch
from
1364c77 to
faa4b34
Compare
Using `BlockHash::hash(&[0])` causes heap buffer overflow in sanitizer step in the CI overhaul #253 because it tries to X11 hash a 1 byte input.
Use underscore instead of colon or dots since they are not allowed in filenames in Windows. This is are some of the fixes required for Windows support which gets introduced/tested in the CI overhaul PR #253.
Use underscore instead of colon or dots since they are not allowed in filenames in Windows. This is are some of the fixes required for Windows support which gets introduced/tested in the CI overhaul PR #253.
Uses fully qualified syntax `<T as $crate::Hash>::method()` instead of relying on the `Hash` trait being in scope. This ensures `hash_newtype!` macros compile correctly when used with `--no-default-features` in CI overhaul PR #253.
Adds `license = "CC0-1.0"` to `fuzz/Cargo.toml` and `rpc-integration-test/Cargo.toml` to pass cargo-deny license checks added in the CI overhaul PR #253.
Use underscore instead of colon or dots since they are not allowed in filenames in Windows. This is are some of the fixes required for Windows support which gets introduced/tested in the CI overhaul PR dashpay#253.
Uses fully qualified syntax `<T as $crate::Hash>::method()` instead of relying on the `Hash` trait being in scope. This ensures `hash_newtype!` macros compile correctly when used with `--no-default-features` in CI overhaul PR dashpay#253.
Adds `license = "CC0-1.0"` to `fuzz/Cargo.toml` and `rpc-integration-test/Cargo.toml` to pass cargo-deny license checks added in the CI overhaul PR dashpay#253.
Fixes an issue encountered in the CI overhaul PR dashpay#253. Open lock file with read+write access and add test-only `read_pid` function that reads from the same file handle. This works on Windows where exclusive locks prevent reading through another file handle: ``` thread 'storage::lockfile::tests::test_lock_file_contains_pid' (8952) panicked at dash-spv\src\storage\lockfile.rs:72:59: Should read lock file: Os { code: 33, kind: Uncategorized, message: "The process cannot access the file because another process has locked a portion of the file." } ```
commit bf6389a Author: xdustinface <xdustinfacex@gmail.com> Date: Sat Dec 27 11:38:02 2025 +0100 fix: add license field to fuzz Cargo.toml template commit c6f8398 Author: xdustinface <xdustinfacex@gmail.com> Date: Fri Dec 12 11:52:08 2025 +1100 fix: add missing license fields for cargo-deny audit Adds `license = "CC0-1.0"` to `fuzz/Cargo.toml` and `rpc-integration-test/Cargo.toml` to pass cargo-deny license checks added in the CI overhaul PR dashpay#253.
Fixes an issue encountered in the CI overhaul PR dashpay#253. Open lock file with read+write access and add test-only `read_pid` function that reads from the same file handle. This works on Windows where exclusive locks prevent reading through another file handle: ``` thread 'storage::lockfile::tests::test_lock_file_contains_pid' (8952) panicked at dash-spv\src\storage\lockfile.rs:72:59: Should read lock file: Os { code: 33, kind: Uncategorized, message: "The process cannot access the file because another process has locked a portion of the file." } ```
Fixes an issue encountered in the CI overhaul PR #253. Open lock file with read+write access and add test-only `read_pid` function that reads from the same file handle. This works on Windows where exclusive locks prevent reading through another file handle: ``` thread 'storage::lockfile::tests::test_lock_file_contains_pid' (8952) panicked at dash-spv\src\storage\lockfile.rs:72:59: Should read lock file: Os { code: 33, kind: Uncategorized, message: "The process cannot access the file because another process has locked a portion of the file." } ```
Fixes an issue encountered in the CI overhaul PR dashpay#253. Open lock file with read+write access and add test-only `read_pid` function that reads from the same file handle. This works on Windows where exclusive locks prevent reading through another file handle: ``` thread 'storage::lockfile::tests::test_lock_file_contains_pid' (8952) panicked at dash-spv\src\storage\lockfile.rs:72:59: Should read lock file: Os { code: 33, kind: Uncategorized, message: "The process cannot access the file because another process has locked a portion of the file." } ```
Fixes an issue encountered in Windows support in CI overhaul PR dashpay#253.
This fixes a bunch of memory leaks in tests detected by sanitizer in CI overhaul PR dashpay#253. - Properly keep track of all create wallets and wallet infos and clean them up at the end. - Head allocate wallet infos like it would happen in real FFI usage and free them properly at the end.
This fixes a bunch of memory leaks in tests detected by sanitizer in CI overhaul PR dashpay#253. - Modified `set_error`, `set_success`, and `ffi_error_set` macro to free previous error messages before setting new ones - Added `FFIError::free_message` helper for test cleanup - Added cleanup in all relevalt tests
Fixes an issue encountered in Windows support in CI overhaul PR dashpay#253.
Uses one file handle for create/write/sync instead of separate write then reopen for sync, it just makes sense and it also avoids a race condition on Windows where the file may still be held briefly after closing which leads to test failures in the CI overhaul PR #253.
Uses one file handle for create/write/sync instead of separate write then reopen for sync, it just makes sense and it also avoids a race condition on Windows where the file may still be held briefly after closing which leads to test failures in the CI overhaul PR #253.
Uses one file handle for create/write/sync instead of separate write then reopen for sync, it just makes sense and it also avoids a race condition on Windows where the file may still be held briefly after closing which leads to test failures in the CI overhaul PR #253.
Uses one file handle for create/write/sync instead of separate write then reopen for sync, it just makes sense and it also avoids a race condition on Windows where the file may still be held briefly after closing which leads to Windows test failures in the CI overhaul PR #253.
Fixes an issue encountered in the CI overhaul PR dashpay#253. Open lock file with read+write access and add test-only `read_pid` function that reads from the same file handle. This works on Windows where exclusive locks prevent reading through another file handle: ``` thread 'storage::lockfile::tests::test_lock_file_contains_pid' (8952) panicked at dash-spv\src\storage\lockfile.rs:72:59: Should read lock file: Os { code: 33, kind: Uncategorized, message: "The process cannot access the file because another process has locked a portion of the file." } ```
Fix a bug in Windows discovered in CI overhaul PR dashpay#253 where `:9999` was being accepted as a valid peer address on Windows due to DNS resolution differences.
Bumps `rs-x11-hash` to `0.1.9` which now supports windows build. This is one part of the puzzle for the Windows support in the CI overhaul PR dashpay#253.
Fix a bug in Windows discovered in CI overhaul PR #253 where `:9999` was being accepted as a valid peer address on Windows due to DNS resolution differences.
Uses fully qualified syntax `<T as $crate::Hash>::method()` instead of relying on the `Hash` trait being in scope. This ensures `hash_newtype!` macros compile correctly when used with `--no-default-features` in CI overhaul PR #253.
* fix: add missing license fields for cargo-deny audit Adds `license = "CC0-1.0"` to `fuzz/Cargo.toml` and `rpc-integration-test/Cargo.toml` to pass cargo-deny license checks added in the CI overhaul PR #253. * fix: add license field to fuzz Cargo.toml template
) Fixes an issue encountered in the CI overhaul PR #253. Open lock file with read+write access and add test-only `read_pid` function that reads from the same file handle. This works on Windows where exclusive locks prevent reading through another file handle: ``` thread 'storage::lockfile::tests::test_lock_file_contains_pid' (8952) panicked at dash-spv\src\storage\lockfile.rs:72:59: Should read lock file: Os { code: 33, kind: Uncategorized, message: "The process cannot access the file because another process has locked a portion of the file." } ```
Fix a bug in Windows discovered in CI overhaul PR dashpay#253 where `:9999` was being accepted as a valid peer address on Windows due to DNS resolution differences.
Bumps `rs-x11-hash` to `0.1.9` which now supports windows build. This is one part of the puzzle for the Windows support in the CI overhaul PR dashpay#253.
Use underscore instead of colon or dots since they are not allowed in filenames in Windows. This is are some of the fixes required for Windows support which gets introduced/tested in the CI overhaul PR dashpay#253.
Fix a bug in Windows discovered in CI overhaul PR dashpay#253 where `:9999` was being accepted as a valid peer address on Windows due to DNS resolution differences.
Bumps `rs-x11-hash` to `0.1.9` which now supports windows build. This is one part of the puzzle for the Windows support in the CI overhaul PR dashpay#253.
Bumps `rs-x11-hash` to `0.1.9` which now supports windows build. This is one part of the puzzle for the Windows support in the CI overhaul PR #253.
Use underscore instead of colon or dots since they are not allowed in filenames in Windows. This is are some of the fixes required for Windows support which gets introduced/tested in the CI overhaul PR dashpay#253.
Fix a bug in Windows discovered in CI overhaul PR dashpay#253 where `:9999` was being accepted as a valid peer address on Windows due to DNS resolution differences.
Use underscore instead of colon or dots since they are not allowed in filenames in Windows. This is are some of the fixes required for Windows support which gets introduced/tested in the CI overhaul PR #253.
Fix a bug in Windows discovered in CI overhaul PR #253 where `:9999` was being accepted as a valid peer address on Windows due to DNS resolution differences.
xdustinface
force-pushed
the
ci/overhaul
branch
from
648485d to
e611a91
Compare
Use underscore instead of colon since its not allowed in filenames in Windows. This is are some of the fixes required for Windows support which gets introduced/tested in the CI overhaul PR #253.
* fix: reject empty hostname in peer address in FFI Fix a bug in Windows discovered in CI overhaul PR #253 where `:9999` was being accepted as a valid peer address on Windows due to DNS resolution differences. * Drop step 1
xdustinface
force-pushed
the
ci/overhaul
branch
from
e611a91 to
f2b6890
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (9)
deny.toml (1)
30-33: Reconsider allowing wildcard dependencies.Setting
wildcards = "allow"permits dependency specifications likefoo = "*"in Cargo.toml files, which is generally discouraged as it can lead to non-reproducible builds and unexpected breaking changes.Consider changing to
wildcards = "deny"or"warn"to enforce version pinning best practices.🔎 Proposed change to restrict wildcards
[bans] multiple-versions = "warn" -wildcards = "allow" +wildcards = "deny" highlight = "all"fuzz/fuzz-util.sh (1)
5-11: LGTM! Consider explicit error handling for missing directories.The function correctly lists dash targets before hashes as documented. The
2>/dev/nullsuppression means missing directories return empty results rather than failing, which may be acceptable for incremental adoption but could mask configuration errors.If stricter validation is desired, consider checking directory existence:
Optional: Add explicit directory checks
listAllTargetFiles() { pushd "$REPO_DIR/fuzz" > /dev/null || exit 1 + if [ ! -d "fuzz_targets/dash" ] && [ ! -d "fuzz_targets/hashes" ]; then + echo "Error: No fuzz target directories found" >&2 + exit 1 + fi # List dash targets first, then hashes find fuzz_targets/dash -type f -name "*.rs" 2>/dev/null find fuzz_targets/hashes -type f -name "*.rs" 2>/dev/null popd > /dev/null || exit 1 }.github/workflows/build-and-test.yml (1)
1-32: LGTM! Consider pinning pyyaml version for reproducibility.The reusable workflow is well-structured with proper input validation, minimal permissions, and appropriate caching strategy. The
fail-fast: falseensures all test groups run even if one fails.Recommended: Pin pyyaml version
- - run: pip install pyyaml + - run: pip install pyyaml==6.0.2This ensures consistent behavior across CI runs and prevents potential breaking changes from pyyaml updates.
.github/workflows/rust.yml (2)
55-55: Consider pinning the pyyaml dependency version.Using
pip install pyyamlwithout a version pin could lead to non-reproducible builds if a breaking change is released. Consider pinning to a specific version.🔎 Suggested fix
- - run: pip install pyyaml + - run: pip install pyyaml==6.0.2Also applies to: 99-99
103-117: Disabled integration tests retain outdated toolchain setup.The disabled
integrations_testsjob usesdtolnay/rust-toolchain@stablebut thematrix.rustvariable (stable) is defined but never used. When re-enabling, ensure the toolchain setup is consistent with the rest of the workflow..github/scripts/ci_config.py (3)
33-44: Specify encoding when opening files.The
open()call doesn't specify an encoding, which defaults to the system locale and could cause issues on non-UTF-8 systems (particularly Windows).🔎 Suggested fix
try: - with open(path) as f: + with open(path, encoding="utf-8") as f: content = yaml.safe_load(f)
67-72: Specify encoding when writing to GITHUB_OUTPUT.Similarly, specify UTF-8 encoding when appending to the output file.
🔎 Suggested fix
if output_file: - with open(output_file, "a") as f: + with open(output_file, "a", encoding="utf-8") as f: f.write(f"{name}={value}\n")
145-152: Consider adding timeouts to subprocess calls.The
subprocess.run()calls forcargo checkandcargo testdon't specify a timeout. Long-running or hung builds could block CI indefinitely. Consider adding a reasonable timeout.🔎 Suggested fix
- result = subprocess.run(cmd) + result = subprocess.run(cmd, timeout=600) # 10 minute timeout- result = subprocess.run(cmd) + result = subprocess.run(cmd, timeout=1800) # 30 minute timeout for testsNote: You'll need to wrap these in
try/except subprocess.TimeoutExpiredto handle timeouts gracefully.Also applies to: 185-192
.github/workflows/sanitizer.yml (1)
52-68: LGTM! TSAN configuration is correct.The Thread Sanitizer job correctly targets the async crate with concurrent code and uses appropriate TSAN options.
Optional: Consider adding cargo caching to speed up both sanitizer jobs.
While sanitizer builds are inherently slower, caching dependencies could reduce overall runtime. Example:
- uses: actions/checkout@v6 - uses: Swatinem/rust-cache@v2 with: key: sanitizer-${{ matrix.sanitizer }} - uses: dtolnay/rust-toolchain@nightlyThis can be applied to both
asanandtsanjobs if build times become a concern.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
.github/ci-groups.yml.github/ci-no-std.yml.github/scripts/ci_config.py.github/workflows/build-and-test.yml.github/workflows/fuzz.yml.github/workflows/pre-commit.yml.github/workflows/rust.yml.github/workflows/sanitizer.ymlcontrib/test.shdash/contrib/test.shdeny.tomlfuzz/Cargo.tomlfuzz/contrib/test.shfuzz/fuzz-util.shfuzz/generate-files.shhashes/contrib/test.shinternals/contrib/test.sh
💤 Files with no reviewable changes (5)
- dash/contrib/test.sh
- contrib/test.sh
- internals/contrib/test.sh
- hashes/contrib/test.sh
- fuzz/contrib/test.sh
🧰 Additional context used
🧠 Learnings (22)
📓 Common learnings
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T17:59:51.097Z
Learning: Run pre-PR checks: `cargo fmt`, `cargo clippy`, `cargo test` (workspace) before submitting pull requests
📚 Learning: 2025-12-22T17:59:51.097Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T17:59:51.097Z
Learning: Run pre-PR checks: `cargo fmt`, `cargo clippy`, `cargo test` (workspace) before submitting pull requests
Applied to files:
.github/workflows/pre-commit.yml.github/workflows/rust.ymlfuzz/generate-files.sh
📚 Learning: 2025-12-22T17:59:51.097Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T17:59:51.097Z
Learning: Applies to **/*.rs : Format code with `rustfmt` before commits; ensure `cargo fmt --all` is run
Applied to files:
.github/workflows/pre-commit.ymlfuzz/generate-files.sh
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/tests/**/*.rs : Organize tests into unit tests (in-module), integration tests (tests/ directory), real network tests (with live Dash Core nodes), and performance benchmarks
Applied to files:
.github/ci-groups.yml.github/workflows/fuzz.yml.github/ci-no-std.yml.github/workflows/rust.ymlfuzz/generate-files.shfuzz/Cargo.toml
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/tests/**/*.rs : Integration tests should gracefully handle unavailable Dash Core nodes at 127.0.0.1:9999 without failing the test suite
Applied to files:
.github/ci-groups.yml.github/workflows/rust.ymlfuzz/Cargo.toml
📚 Learning: 2025-12-30T22:00:57.000Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-30T22:00:57.000Z
Learning: Applies to **/*test*.rs : Test both mainnet and testnet configurations
Applied to files:
.github/ci-groups.yml.github/ci-no-std.yml.github/workflows/rust.yml
📚 Learning: 2025-12-19T00:07:22.904Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-19T00:07:22.904Z
Learning: Applies to key-wallet/**/tests/**/*.rs : Organize unit tests by functionality: separate test files for BIP32, mnemonics, addresses, derivation paths, and PSBT operations
Applied to files:
.github/ci-groups.yml.github/workflows/rust.yml
📚 Learning: 2025-12-19T00:07:22.904Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-19T00:07:22.904Z
Learning: Applies to key-wallet/**/tests/**/*.rs : Use deterministic testing with known test vectors and fixed seeds for reproducible results
Applied to files:
.github/ci-groups.yml.github/workflows/rust.yml
📚 Learning: 2025-12-22T17:59:51.097Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T17:59:51.097Z
Learning: Target feature work to `v**-dev` branches (development) and hotfixes/docs to `master` unless directed otherwise
Applied to files:
.github/workflows/fuzz.ymlfuzz/generate-files.sh
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Run 'cargo fmt --check' to verify formatting, 'cargo clippy --all-targets --all-features -- -D warnings' for linting, and 'cargo check --all-features' to verify all features compile
Applied to files:
.github/ci-no-std.ymldeny.toml
📚 Learning: 2025-08-21T04:45:50.436Z
Learnt from: QuantumExplorer
Repo: dashpay/rust-dashcore PR: 108
File: key-wallet/src/gap_limit.rs:291-295
Timestamp: 2025-08-21T04:45:50.436Z
Learning: The rust-dashcore project uses Rust 1.89 as evidenced by rust-version = "1.89" in dash-spv/Cargo.toml. Modern Rust features like Option::is_none_or (stabilized in 1.82) can be used safely.
Applied to files:
.github/ci-no-std.yml.github/workflows/rust.ymlfuzz/Cargo.toml
📚 Learning: 2025-12-30T22:00:57.000Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-30T22:00:57.000Z
Learning: Applies to **/*.rs : Use conditional compilation with feature flags for optional features
Applied to files:
.github/ci-no-std.yml
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/**/*.rs : Maintain minimum Rust version (MSRV) of 1.89 and use only compatible syntax and features
Applied to files:
.github/workflows/rust.ymlfuzz/generate-files.shfuzz/Cargo.toml
📚 Learning: 2025-12-22T17:59:51.097Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T17:59:51.097Z
Learning: Applies to **/*.rs : MSRV (Minimum Supported Rust Version) is 1.89; ensure compatibility with this version for all builds
Applied to files:
.github/workflows/rust.yml
📚 Learning: 2025-12-01T07:59:58.608Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-12-01T07:59:58.608Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Add cbindgen annotations for complex types in FFI functions
Applied to files:
fuzz/generate-files.shfuzz/Cargo.toml
📚 Learning: 2025-12-01T07:59:58.608Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-12-01T07:59:58.608Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Use `#[no_mangle] extern "C"` attribute when implementing new FFI functions in Rust
Applied to files:
fuzz/generate-files.shfuzz/Cargo.toml
📚 Learning: 2025-12-01T07:59:58.608Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-12-01T07:59:58.608Z
Learning: Applies to dash-spv-ffi/tests/unit/**/*.rs : Add corresponding unit tests in `tests/unit/` for each new FFI function
Applied to files:
fuzz/generate-files.shfuzz/Cargo.toml
📚 Learning: 2025-12-01T07:59:58.608Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-12-01T07:59:58.608Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Rust strings must be returned as `*const c_char` with caller responsibility to free using `dash_string_free`
Applied to files:
fuzz/generate-files.shfuzz/Cargo.toml
📚 Learning: 2025-12-22T17:59:51.097Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T17:59:51.097Z
Learning: Applies to **/*.rs : Run `cargo clippy --workspace --all-targets -- -D warnings` for linting; avoid warnings in CI
Applied to files:
deny.toml
📚 Learning: 2025-12-01T07:59:58.608Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-12-01T07:59:58.608Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Input strings in FFI functions are `*const c_char` (borrowed, not freed by C caller)
Applied to files:
fuzz/Cargo.toml
📚 Learning: 2025-06-26T15:47:37.438Z
Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 0
File: :0-0
Timestamp: 2025-06-26T15:47:37.438Z
Learning: Transaction IDs (txids) and block hashes in Dash are always 32 bytes and should be passed as fixed-size byte arrays in FFI interfaces, not as C strings, to avoid inefficiency and encoding issues.
Applied to files:
fuzz/Cargo.toml
📚 Learning: 2025-12-01T07:59:58.608Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-12-01T07:59:58.608Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : All FFI types must have corresponding `_destroy()` functions for explicit memory management
Applied to files:
fuzz/Cargo.toml
🧬 Code graph analysis (1)
fuzz/generate-files.sh (1)
fuzz/fuzz-util.sh (2)
listAllTargetFiles(5-11)listTargetNames(33-37)
🪛 Ruff (0.14.10)
.github/scripts/ci_config.py
25-25: Starting a process with a partial executable path
(S607)
145-145: Consider ["cargo", "check", "-p", crate_name, *flags] instead of concatenation
Replace with ["cargo", "check", "-p", crate_name, *flags]
(RUF005)
146-146: subprocess call: check for execution of untrusted input
(S603)
186-186: subprocess call: check for execution of untrusted input
(S603)
🔇 Additional comments (22)
deny.toml (2)
35-38: Sources configuration provides good security posture.The configuration properly restricts package sources to crates.io and denies unknown registries, which helps prevent supply chain attacks. Warning on git dependencies is a reasonable balance between flexibility and visibility.
13-28: License configuration looks good.The license allow-list is comprehensive and covers common permissive licenses used in the Rust ecosystem, including the primary licenses (MIT, Apache-2.0), variants (BSD-2-Clause, BSD-3-Clause), and alternatives (ISC, Unlicense, Zlib, CC0-1.0, MPL-2.0, Unicode-3.0, MITNFA) that appear across workspace dependencies. The
confidence-thresholdof 0.8 aligns with cargo-deny defaults and is appropriate for license detection..github/workflows/pre-commit.yml (6)
10-12: LGTM! Good addition of concurrency control.This standard pattern will prevent duplicate workflow runs when new commits are pushed to the same branch, improving CI efficiency.
21-21: LGTM! Windows support added as intended.Expanding the matrix to include Windows aligns with the PR objectives and ensures pre-commit hooks work consistently across all platforms.
29-29: LGTM! Python version update looks good.Python 3.12 is a good choice for modern pre-commit hooks. Ensure that all pre-commit hooks in the repository are compatible with Python 3.12.
39-39: LGTM! Improved cache key specificity.The more specific cache key (
pre-commit-rust-) provides better isolation from other Rust workflows and prevents cache conflicts.
44-44: LGTM! Comprehensive pre-commit configuration.The additional flags ensure thorough checking:
--all-files: Validates the entire codebase (appropriate for CI)--hook-stage push: Runs push-stage hooks for comprehensive validation--verbose: Provides detailed output for debugging failures
24-24: Theactions/checkout@v6action is stable and the latest available version (v6.0.1 released December 2, 2025). The notable changes include improved credential security through separate file handling, which requires no workflow modifications for standard git operations. No breaking changes impact this pre-commit workflow.fuzz/fuzz-util.sh (1)
13-16: LGTM! Filtering logic is correct.The grep pattern correctly excludes the three problematic targets using alternation. The partial match is appropriate since the target file paths contain these identifiers.
fuzz/Cargo.toml (2)
4-4: LGTM! Workspace version inheritance.Using
{ workspace = true }ensures version consistency across the workspace, which is a best practice for monorepos.
31-32: Bin definitions correctly align with fuzz utility filtering.All bin entries follow a consistent naming convention (dash_* and hashes_*) with matching target file paths. The targets deserialize_transaction, deserialize_prefilled_transaction, and deserialize_psbt are correctly defined in Cargo.toml but intentionally excluded from execution by fuzz-util.sh. All referenced target files exist.
.github/ci-no-std.yml (1)
1-16: Incorrect feature name in dashcore_hashes configuration.The configuration references
alloc serdefordashcore_hashes, but the crate only defines aserde-stdfeature, notserde. This will cause the CI check to fail. Changealloc serdetoalloc serde-std.Additionally, note that the YAML structure uses space-separated feature names (e.g.,
alloc serde-std), which will be passed tocargo check --no-default-features --features alloc serde-std, so the features must be valid for that crate.⛔ Skipped due to learnings
Learnt from: CR Repo: dashpay/rust-dashcore PR: 0 File: dash-spv/CLAUDE.md:0-0 Timestamp: 2025-12-16T09:03:55.811Z Learning: Run 'cargo fmt --check' to verify formatting, 'cargo clippy --all-targets --all-features -- -D warnings' for linting, and 'cargo check --all-features' to verify all features compileLearnt from: CR Repo: dashpay/rust-dashcore PR: 0 File: dash-spv/CLAUDE.md:0-0 Timestamp: 2025-12-16T09:03:55.811Z Learning: Applies to dash-spv/**/*.rs : Maintain minimum Rust version (MSRV) of 1.89 and use only compatible syntax and featuresLearnt from: CR Repo: dashpay/rust-dashcore PR: 0 File: dash-spv-ffi/CLAUDE.md:0-0 Timestamp: 2025-12-01T07:59:58.608Z Learning: Applies to dash-spv-ffi/src/**/*.rs : Rust strings must be returned as `*const c_char` with caller responsibility to free using `dash_string_free`Learnt from: QuantumExplorer Repo: dashpay/rust-dashcore PR: 108 File: key-wallet/src/gap_limit.rs:291-295 Timestamp: 2025-08-21T04:45:50.436Z Learning: The rust-dashcore project uses Rust 1.89 as evidenced by rust-version = "1.89" in dash-spv/Cargo.toml. Modern Rust features like Option::is_none_or (stabilized in 1.82) can be used safely.Learnt from: CR Repo: dashpay/rust-dashcore PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-12-30T22:00:57.000Z Learning: Applies to **/*test*.rs : Test both mainnet and testnet configurationsLearnt from: CR Repo: dashpay/rust-dashcore PR: 0 File: dash-spv/CLAUDE.md:0-0 Timestamp: 2025-12-16T09:03:55.811Z Learning: Applies to dash-spv/tests/**/*.rs : Organize tests into unit tests (in-module), integration tests (tests/ directory), real network tests (with live Dash Core nodes), and performance benchmarks.github/workflows/fuzz.yml (1)
75-78: LGTM on the simplified verification logic.The consolidated verification step cleanly replaces the prior two-step file inspection with a single pipeline that collects executed targets and diffs against the expected list from
listTargetNames..github/workflows/rust.yml (2)
26-34: LGTM on MSRV check job.The MSRV check correctly uses Rust 1.89 with
--workspace --all-features, aligning with the project's documented minimum supported Rust version.
60-90: Well-structured multi-platform test matrix.The test jobs cleanly consume the
groupsoutput fromverify-executionand delegate to the reusablebuild-and-test.ymlworkflow. The dependency chain ensures crates are verified before tests run.fuzz/generate-files.sh (2)
42-50: Intentional discrepancy between Cargo.toml targets and workflow matrix.The script uses
listAllTargetFiles()(line 42) for generating Cargo.toml bin entries, but the workflow matrix useslistTargetNames()(line 79) which filters throughlistTargetFiles(). This means Cargo.toml contains all fuzz target binaries, while CI only runs a subset (excludingdeserialize_transaction,deserialize_prefilled_transaction,deserialize_psbtper the relatedfuzz-util.shlogic).This appears intentional to allow local fuzzing of all targets while CI runs a curated subset. Consider adding a brief comment in the script to document this design choice for future maintainers.
Also applies to: 79-79
14-14: No action required. Rust edition 2024 is fully compatible with MSRV 1.89. Rust 2024 edition was introduced in Rust 1.85, and since the project's MSRV is 1.89, theedition = "2024"setting is supported..github/scripts/ci_config.py (3)
75-102: LGTM on verify_groups implementation.The function correctly validates that all workspace crates are assigned to test groups or explicitly excluded. The GitHub Actions output for the matrix is properly formatted as JSON.
105-162: Well-structured no-std checks with clear feature handling.The logic for handling
bare,no-std, single features, and space-separated multiple features is clear and handles edge cases appropriately. The error aggregation and reporting is comprehensive.
177-181: Good platform-specific handling for Windows.Correctly skipping
dash-fuzzon Windows due to honggfuzz incompatibility, with appropriate notice logging..github/workflows/sanitizer.yml (2)
3-18: LGTM! Well-configured trigger conditions.The path filters correctly include both FFI crates and their base dependencies, ensuring that changes to foundation crates trigger sanitizer checks for their FFI consumers.
20-25: LGTM! Proper permission and concurrency controls.Minimal read-only permissions and standard concurrency control to cancel redundant runs are correctly configured.
This is a general CI overhaul. ### Summary - Add `cargo-deny` security audit for vulnerability and license checking - Add MSRV (1.89) verification job - Add documentation build check - Add Sanitizer workflow with ASAN for FFI crates and TSAN async crates. - Add Windows to tests and pre-commit - Rewrite test workflow with grouped multi-platform matrix (Linux x86/ARM, macOS, Windows) - Add `verify-coverage` job that fails if any crate isn't assigned to a test group - Drop the complex segfault detection logic in the SPV part which was actually hiding test failures - Add enable concurrency control in `pre-commit.yml` to cancel redundant runs - Add `no-std` build checks to verify crates that claim to support it actually build. - Fix and improve fuzz workflow generation files ### Test Groups Tests are now organized in `.github/ci-groups.yml`. CI fails if a new crate is added without being assigned to a group. ### Jobs - 24 test jobs (6 groups (core, spv, ffi, wallet, rpc, tools) × 4 platforms (ubuntu arm, ubuntu x86_64, macos, windows)) - verify-coverage, security, msrv, docs build, sanitizer, no-std checks, pre-commit, 15 x fuzz
xdustinface
force-pushed
the
ci/overhaul
branch
from
f2b6890 to
287845a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
deny.toml (2)
30-33: Consider tightening the wildcard dependency policy.Setting
wildcards = "allow"permits wildcard version specifications, which can lead to non-reproducible builds and unexpected dependency updates. Consider changing to"warn"or"deny"to enforce more predictable builds.🔎 Proposed configuration change
[bans] multiple-versions = "warn" -wildcards = "allow" +wildcards = "warn" highlight = "all"
35-38: Consider stricter git source policy for enhanced security.Setting
unknown-git = "warn"allows dependencies from arbitrary git sources. For stronger supply chain security, consider changing to"deny", though this may require explicitly allowing specific git dependencies if needed during development.🔎 Proposed configuration change
[sources] unknown-registry = "deny" -unknown-git = "warn" +unknown-git = "deny" allow-registry = ["https://github.com/rust-lang/crates.io-index"]fuzz/fuzz-util.sh (1)
13-16: Document why specific fuzz targets are excluded from CI.The wrapper filters out three deserialize targets with the comment "don't work in CI". Consider documenting the specific reasons (e.g., excessive runtime, memory issues, platform incompatibilities) to help future maintainers understand whether these exclusions are still necessary.
📝 Suggested documentation improvement
listTargetFiles() { - # Exclude targets that don't work in CI + # Exclude targets that don't work in CI due to [REASON: timeout/memory/etc] + # See issue #XXX for details listAllTargetFiles | grep -v 'deserialize_transaction\|deserialize_prefilled_transaction\|deserialize_psbt' }.github/workflows/rust.yml (1)
103-117: Track re-enablement of integration tests.The integration tests are temporarily disabled with
if: false. While this is acceptable for the PR, ensure there's a tracking issue or TODO to re-enable these tests once the blocking issues are resolved.Do you want me to open a tracking issue for re-enabling the integration tests?
.github/scripts/ci_config.py (2)
67-72: Consider handling newlines in multi-line output values.The current implementation writes values directly, but if
valuecontains newlines, this could break GitHub Actions output parsing. For single-line JSON output this works, but consider using the heredoc syntax for robustness.🔎 Proposed fix using heredoc for multi-line safety
def github_output(name: str, value: str): """Write a GitHub Actions output variable.""" output_file = os.environ.get("GITHUB_OUTPUT") if output_file: with open(output_file, "a") as f: - f.write(f"{name}={value}\n") + if "\n" in value: + import uuid + delimiter = f"EOF_{uuid.uuid4().hex[:8]}" + f.write(f"{name}<<{delimiter}\n{value}\n{delimiter}\n") + else: + f.write(f"{name}={value}\n")
143-152: Use unpacking syntax instead of list concatenation.The static analyzer suggests using unpacking for cleaner list construction.
🔎 Proposed fix
- cmd = ["cargo", "check", "-p", crate_name] + flags + cmd = ["cargo", "check", "-p", crate_name, *flags]
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
.github/ci-groups.yml.github/ci-no-std.yml.github/scripts/ci_config.py.github/workflows/build-and-test.yml.github/workflows/fuzz.yml.github/workflows/pre-commit.yml.github/workflows/rust.yml.github/workflows/sanitizer.ymlcontrib/test.shdash/contrib/test.shdeny.tomlfuzz/Cargo.tomlfuzz/contrib/test.shfuzz/fuzz-util.shfuzz/generate-files.shhashes/contrib/test.shinternals/contrib/test.sh
💤 Files with no reviewable changes (5)
- internals/contrib/test.sh
- dash/contrib/test.sh
- hashes/contrib/test.sh
- contrib/test.sh
- fuzz/contrib/test.sh
🚧 Files skipped from review as they are similar to previous changes (3)
- .github/ci-groups.yml
- .github/workflows/sanitizer.yml
- .github/workflows/fuzz.yml
🧰 Additional context used
🧠 Learnings (22)
📓 Common learnings
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/tests/**/*.rs : Organize tests into unit tests (in-module), integration tests (tests/ directory), real network tests (with live Dash Core nodes), and performance benchmarks
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T17:59:51.097Z
Learning: Run pre-PR checks: `cargo fmt`, `cargo clippy`, `cargo test` (workspace) before submitting pull requests
📚 Learning: 2025-12-22T17:59:51.097Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T17:59:51.097Z
Learning: Applies to **/*.rs : Run `cargo clippy --workspace --all-targets -- -D warnings` for linting; avoid warnings in CI
Applied to files:
deny.toml
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Run 'cargo fmt --check' to verify formatting, 'cargo clippy --all-targets --all-features -- -D warnings' for linting, and 'cargo check --all-features' to verify all features compile
Applied to files:
deny.toml.github/ci-no-std.yml
📚 Learning: 2025-08-21T04:45:50.436Z
Learnt from: QuantumExplorer
Repo: dashpay/rust-dashcore PR: 108
File: key-wallet/src/gap_limit.rs:291-295
Timestamp: 2025-08-21T04:45:50.436Z
Learning: The rust-dashcore project uses Rust 1.89 as evidenced by rust-version = "1.89" in dash-spv/Cargo.toml. Modern Rust features like Option::is_none_or (stabilized in 1.82) can be used safely.
Applied to files:
.github/ci-no-std.yml.github/workflows/rust.ymlfuzz/Cargo.toml
📚 Learning: 2025-12-30T22:00:57.000Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-30T22:00:57.000Z
Learning: Applies to **/*test*.rs : Test both mainnet and testnet configurations
Applied to files:
.github/ci-no-std.yml.github/workflows/rust.yml
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/tests/**/*.rs : Organize tests into unit tests (in-module), integration tests (tests/ directory), real network tests (with live Dash Core nodes), and performance benchmarks
Applied to files:
.github/ci-no-std.yml.github/workflows/rust.ymlfuzz/Cargo.tomlfuzz/generate-files.sh
📚 Learning: 2025-12-30T22:00:57.000Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-30T22:00:57.000Z
Learning: Applies to **/*.rs : Use conditional compilation with feature flags for optional features
Applied to files:
.github/ci-no-std.yml
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/**/*.rs : Maintain minimum Rust version (MSRV) of 1.89 and use only compatible syntax and features
Applied to files:
.github/workflows/rust.ymlfuzz/Cargo.tomlfuzz/generate-files.sh
📚 Learning: 2025-12-22T17:59:51.097Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T17:59:51.097Z
Learning: Run pre-PR checks: `cargo fmt`, `cargo clippy`, `cargo test` (workspace) before submitting pull requests
Applied to files:
.github/workflows/rust.yml.github/workflows/pre-commit.ymlfuzz/generate-files.sh
📚 Learning: 2025-12-22T17:59:51.097Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T17:59:51.097Z
Learning: Applies to **/*.rs : MSRV (Minimum Supported Rust Version) is 1.89; ensure compatibility with this version for all builds
Applied to files:
.github/workflows/rust.yml
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/tests/**/*.rs : Integration tests should gracefully handle unavailable Dash Core nodes at 127.0.0.1:9999 without failing the test suite
Applied to files:
.github/workflows/rust.ymlfuzz/Cargo.toml
📚 Learning: 2025-12-19T00:07:22.904Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-19T00:07:22.904Z
Learning: Applies to key-wallet/**/tests/**/*.rs : Use deterministic testing with known test vectors and fixed seeds for reproducible results
Applied to files:
.github/workflows/rust.yml
📚 Learning: 2025-12-19T00:07:22.904Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-19T00:07:22.904Z
Learning: Applies to key-wallet/**/tests/**/*.rs : Organize unit tests by functionality: separate test files for BIP32, mnemonics, addresses, derivation paths, and PSBT operations
Applied to files:
.github/workflows/rust.yml
📚 Learning: 2025-12-22T17:59:51.097Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T17:59:51.097Z
Learning: Applies to **/*.rs : Format code with `rustfmt` before commits; ensure `cargo fmt --all` is run
Applied to files:
.github/workflows/pre-commit.ymlfuzz/generate-files.sh
📚 Learning: 2025-12-01T07:59:58.608Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-12-01T07:59:58.608Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Add cbindgen annotations for complex types in FFI functions
Applied to files:
fuzz/Cargo.tomlfuzz/generate-files.sh
📚 Learning: 2025-12-01T07:59:58.608Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-12-01T07:59:58.608Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Use `#[no_mangle] extern "C"` attribute when implementing new FFI functions in Rust
Applied to files:
fuzz/Cargo.tomlfuzz/generate-files.sh
📚 Learning: 2025-12-01T07:59:58.608Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-12-01T07:59:58.608Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Rust strings must be returned as `*const c_char` with caller responsibility to free using `dash_string_free`
Applied to files:
fuzz/Cargo.tomlfuzz/generate-files.sh
📚 Learning: 2025-12-01T07:59:58.608Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-12-01T07:59:58.608Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Input strings in FFI functions are `*const c_char` (borrowed, not freed by C caller)
Applied to files:
fuzz/Cargo.toml
📚 Learning: 2025-12-01T07:59:58.608Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-12-01T07:59:58.608Z
Learning: Applies to dash-spv-ffi/tests/unit/**/*.rs : Add corresponding unit tests in `tests/unit/` for each new FFI function
Applied to files:
fuzz/Cargo.tomlfuzz/generate-files.sh
📚 Learning: 2025-06-26T15:47:37.438Z
Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 0
File: :0-0
Timestamp: 2025-06-26T15:47:37.438Z
Learning: Transaction IDs (txids) and block hashes in Dash are always 32 bytes and should be passed as fixed-size byte arrays in FFI interfaces, not as C strings, to avoid inefficiency and encoding issues.
Applied to files:
fuzz/Cargo.toml
📚 Learning: 2025-12-01T07:59:58.608Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-12-01T07:59:58.608Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : All FFI types must have corresponding `_destroy()` functions for explicit memory management
Applied to files:
fuzz/Cargo.toml
📚 Learning: 2025-12-22T17:59:51.097Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T17:59:51.097Z
Learning: Target feature work to `v**-dev` branches (development) and hotfixes/docs to `master` unless directed otherwise
Applied to files:
fuzz/generate-files.sh
🧬 Code graph analysis (1)
fuzz/generate-files.sh (1)
fuzz/fuzz-util.sh (2)
listAllTargetFiles(5-11)listTargetNames(33-37)
🪛 Ruff (0.14.10)
.github/scripts/ci_config.py
25-25: Starting a process with a partial executable path
(S607)
145-145: Consider ["cargo", "check", "-p", crate_name, *flags] instead of concatenation
Replace with ["cargo", "check", "-p", crate_name, *flags]
(RUF005)
146-146: subprocess call: check for execution of untrusted input
(S603)
186-186: subprocess call: check for execution of untrusted input
(S603)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: fuzz (dash_deser_net_msg)
- GitHub Check: fuzz (hashes_sha512_256)
- GitHub Check: Pre-commit (windows-latest)
- GitHub Check: fuzz (dash_deserialize_block)
- GitHub Check: fuzz (hashes_sha256)
- GitHub Check: fuzz (hashes_sha1)
- GitHub Check: fuzz (hashes_cbor)
- GitHub Check: Pre-commit (macos-latest)
- GitHub Check: fuzz (dash_deserialize_witness)
- GitHub Check: fuzz (dash_script_bytes_to_asm_fmt)
- GitHub Check: fuzz (hashes_sha512)
- GitHub Check: fuzz (dash_deserialize_address)
- GitHub Check: fuzz (dash_outpoint_string)
🔇 Additional comments (30)
deny.toml (2)
13-28: License policy looks good.The allow-list appropriately includes permissive and weakly copyleft licenses while excluding strong copyleft licenses like GPL, which aligns well with maintaining a permissively-licensed library.
8-11: No action needed. The configuration is correct—serde_cbor is confirmed to be used only in fuzz targets (hashes/fuzz/fuzz_targets/cbor.rsandfuzz/fuzz_targets/hashes/cbor.rs). Ignoring RUSTSEC-2021-0127 is justified since the unmaintained dependency does not affect production code..github/workflows/pre-commit.yml (3)
10-12: LGTM! Concurrency control improves CI efficiency.The concurrency configuration correctly groups runs by workflow and ref, canceling redundant runs when new commits are pushed to the same branch. This aligns with the PR objectives and is a best practice.
21-21: No changes needed. The addition of Windows to the test matrix is properly supported.The pre-commit hooks in
.pre-commit-config.yamlare all cross-platform (standard checkers, actionlint, typos, cargo fmt, and Python scripts). The Python scripts already include Windows-specific handling—contrib/run_clippy.pyexplicitly excludes thedash-fuzzcrate on Windows since honggfuzz is not supported on that platform. The shell scripts in the repository are not part of the pre-commit hooks and do not affect this workflow.
29-29: No action needed — Python 3.12 is correctly configured and available on all platforms.The workflow already uses
actions/setup-python@v6, which is GitHub's recommended approach for reliable Python version management across all platforms (ubuntu-latest, macos-latest, windows-latest). This ensures Python 3.12 is consistently available. The reference to a ci_config.py dependency appears to be unfounded, as no such file exists in the codebase..github/workflows/build-and-test.yml (2)
3-11: LGTM! Clean reusable workflow design.The workflow inputs are well-defined with clear types and requirements. The groups input is designed to accept JSON strings, which enables flexible matrix configuration from calling workflows.
25-32: ci_config.py is properly implemented with correct error handling.The workflow correctly delegates to the
run-groupsubcommand which validates the group argument, skips OS-incompatible crates (dash-fuzz on Windows), and reports test failures with clear GitHub Actions error annotations. The YAML loading has proper error handling for missing files and parse errors. The pyyaml installation in the workflow is necessary and correctly used.fuzz/fuzz-util.sh (1)
5-11: LGTM! Clear separation of target enumeration.The refactored
listAllTargetFiles()provides a clean internal listing with explicit ordering (dash targets first, then hashes). The error suppression with2>/dev/nullappropriately handles cases where target directories might not exist..github/workflows/rust.yml (7)
2-2: LGTM! Manual workflow triggering improves flexibility.The addition of
workflow_dispatchallows manual CI runs for testing and debugging, which is a valuable improvement.
26-34: LGTM! MSRV check correctly enforces Rust 1.89 compatibility.The MSRV Check job correctly uses Rust 1.89 and validates the entire workspace with all features. This matches the project's documented MSRV and ensures compatibility.
Based on learnings, the project MSRV is 1.89.
36-46: LGTM! Documentation job enforces quality standards.Building documentation with
RUSTDOCFLAGS: -D warningsensures high-quality docs by treating warnings as errors. The flags--workspace --all-features --no-depsappropriately check all project documentation without building dependencies.
48-58: LGTM! Excellent coverage verification pattern.The verify-execution job ensures all workspace crates are assigned to test groups and provides the groups matrix for downstream jobs. This enforces the coverage requirement and centralizes group validation, which is a clean orchestration pattern.
60-90: LGTM! Clean multi-platform test orchestration.The four test jobs (Ubuntu x86_64, Ubuntu ARM, macOS, Windows) all use the reusable
build-and-test.ymlworkflow with a consistent interface. The dependency onverify-executionensures they all use the same validated groups. This design reduces duplication and improves maintainability.
92-101: LGTM! No-std validation ensures embedded compatibility.The no-std checks job validates that crates claiming no-std support actually build without the standard library. This is essential for embedded and constrained environments and aligns with the PR objectives.
19-24: cargo-deny configuration is properly in place.The repository has a
deny.tomlfile at the root with comprehensive configuration covering advisories, licenses, bans, and sources. The security audit job will execute with appropriate policy checks, including an advisory ignore list for known unmaintained dependencies in fuzz targets only..github/ci-no-std.yml (1)
6-16: Configuration verified. All crate names (dashcore_hashes,dashcore-private,dash-network) match workspace members, specified features exist in their respectiveCargo.tomlfiles, and thebarekeyword is correctly handled byci_config.pyto run with--no-default-featuresonly..github/scripts/ci_config.py (5)
22-30: LGTM!Clean implementation using
subprocess.runwithcheck=Truefor automatic exception handling on failure. The--no-depsflag is appropriate for workspace metadata retrieval.
33-44: LGTM!Robust YAML loading with proper error handling for both missing files and malformed YAML. The
Nonecoalescing to empty dict on line 38 handles empty YAML files correctly.
75-102: LGTM!The verify-groups logic correctly:
- Fetches workspace crates from cargo metadata
- Loads group assignments from YAML
- Computes unassigned crates via set difference
- Handles null/empty group lists gracefully with
or []- Outputs the groups list as JSON for the matrix
This enforces that new crates must be assigned to a group or explicitly excluded.
165-202: LGTM!The
run_group_testsfunction correctly:
- Validates the group exists before running
- Handles Windows-specific skip for dash-fuzz (honggfuzz limitation)
- Uses
--all-featuresfor comprehensive testing- Aggregates and reports failures clearly
The hardcoded "windows-latest" matches the workflow matrix OS values.
205-241: LGTM!Clean CLI structure using argparse with subcommands. The command dispatch pattern on lines 231-237 is idiomatic and maintainable.
fuzz/generate-files.sh (5)
42-50: LGTM!Correctly uses
listAllTargetFilesto enumerate all fuzz target files and generates corresponding[[bin]]entries with properly formatted names viatargetFileToName.
64-69: LGTM!Good additions: explicit read-only permissions and concurrency control to cancel redundant runs. This aligns with the PR objective of enabling concurrency control.
84-92: LGTM!Updated to use latest action versions (checkout@v6, upload-artifact@v6) and switched from custom matrix to
dtolnay/rust-toolchain@stablewith explicit toolchain version 1.89.
106-114: LGTM!The verification step correctly:
- Downloads all execution artifacts
- Concatenates and sorts executed targets
- Compares against expected targets from
listTargetNamesThis ensures all fuzz targets in the matrix were actually executed.
10-28: No issue. The generatedCargo.tomlusesedition = "2024", which is fully compatible with the project's MSRV of 1.89 (Rust 2024 edition requires minimum 1.85).fuzz/Cargo.toml (4)
1-7: Note: This is a generated file.This file is generated by
fuzz/generate-files.sh. Any manual edits will be overwritten. Changes should be made to the generator script instead.The workspace version inheritance on line 4 is a good practice for consistency.
9-16: LGTM! Appropriate fuzzing profile settings.
lto = falseandcodegen-units = 16: Faster compile times for fuzzing iterationsdebug = 1: Minimal debug info for crash diagnosticspanic = "unwind": Required for honggfuzz to catch panics properlyunexpected_cfgslint withcfg(fuzzing): Suppresses warnings for the fuzzing cfg flag
74-100: LGTM!New hash fuzzing targets (sha1, sha256, sha512) provide good coverage alongside existing hash targets (json, cbor, ripemd160, sha512_256).
30-72: All fuzz target source files referenced infuzz/Cargo.tomlexist and are properly linked to their source implementations underfuzz_targets/dash/.
ZocoLini
left a comment
ZocoLini
left a comment
There was a problem hiding this comment.
Not an expert in CI/CD configurations but everything looks fine
This is a general CI overhaul.
Summary
cargo-denysecurity audit for vulnerability and license checkingverify-coveragejob that fails if any crate isn't assigned to a test grouppre-commit.ymlto cancel redundant runsno-stdbuild checks to verify crates that claim to support it actually build.Test Groups
Tests are now organized in
.github/ci-groups.yml. CI fails if a new crate is added without being assigned to a group.Jobs
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.