feat: use golang 1.25.0#1927
Conversation
WalkthroughUpdated CI workflows and Go/Nix tooling pins; introduced gomodOverlay/gomod2nix wiring and go_1_25; multiple Nix overlays and packages adjusted (Apple SDK, rocksdb, golangci-lint); Go module and gomod2nix hashes refreshed; TypeScript/test tooling and pytest markers added; minor repo and script tweaks. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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 |
1270626 to
ccc4fd3
Compare
d78ab09 to
94eb5dc
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
♻️ Duplicate comments (1)
nix/default.nix (1)
33-45: Duplicate gomodOverlay pattern.This is the same pattern as in
flake.nix. As noted there, consider extracting to a shared module.
🧹 Nitpick comments (5)
integration_tests/contracts/tsconfig.json (1)
1-11: TS compiler options are sensible for Hardhat; consider stricter settings laterThis tsconfig is reasonable for a Hardhat/Node test harness (ES2020 + CommonJS,
esModuleInterop,skipLibCheck). Once things stabilize, you might optionally:
- Flip
"strict"totrueto catch more type issues.- Add explicit
"exclude"fornode_modules,artifacts,cacheto keep the TS project surface small.These are nice-to-have and not blockers for this PR.
.github/workflows/lint.yml (1)
26-26: Update actions/checkout to v4.The linting workflow uses
actions/checkout@v3, which the static analysis tool flags as too old for modern GitHub Actions. Consider updating tov4for consistency with other workflows in this PR (e.g., dependabot-update-all.yml already uses v4).- uses: actions/checkout@v3 + - uses: actions/checkout@v4 with: fetch-depth: 0go.mod (1)
3-3: Verify codebase compatibility with Go 1.25 behavioral changes.Go 1.25 introduces a fix for a compiler bug (present since 1.21) that could cause programs using a result before checking the error to now correctly panic. Review the codebase for patterns like:
f, err := os.Open("file") name := f.Name() // This will now panic if err != nil if err != nil { ... }Additionally, go1.25.5 (released 2025-12-02) includes two security fixes to the crypto/x509 package. Consider updating the toolchain directive to
go 1.25.5to include the latest security patches.flake.nix (1)
95-99: Consider extracting the duplicated gomodOverlay pattern.The gomodOverlay logic defined here (lines 32-43) is duplicated in
nix/default.nix(lines 33-45). If both files need this pattern, consider extracting it to a shared module (e.g.,nix/gomod-overlay.nix) to reduce maintenance burden and ensure consistency.nix/default.nix (1)
13-14: Inconsistent overlay parameter naming:finalvspkgs.The overlay uses
finalfor the first parameter but the body referencespkgs(the second parameter, which isprev/super). This is semantically confusing sincefinaltypically refers to the final fixed-point andpkgs/prevrefers to the previous overlay result.For clarity, consider using consistent naming:
- (final: pkgs: { - go = final.go_1_25; + (final: prev: { + go = final.go_1_25; go-ethereum = pkgs.callPackage ./go-ethereum.nix { - buildGoModule = pkgs.buildGoModule; + buildGoModule = final.buildGoModule; };Alternatively, keep the
_: pkgs:pattern used elsewhere in this file ifprevisn't needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
flake.lockis excluded by!**/*.lockgo.sumis excluded by!**/*.sumintegration_tests/contracts/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (27)
.github/workflows/build.yml(4 hunks).github/workflows/codeql-analysis.yml(1 hunks).github/workflows/dependabot-update-all.yml(1 hunks).github/workflows/dependencies-review.yml(1 hunks).github/workflows/lint.yml(3 hunks).github/workflows/release.yml(2 hunks).github/workflows/sims.yml(6 hunks).github/workflows/test.yml(4 hunks).gitignore(1 hunks)Makefile(1 hunks)default.nix(3 hunks)flake.nix(3 hunks)go.mod(2 hunks)gomod2nix.toml(1 hunks)integration_tests/contracts/package.json(1 hunks)integration_tests/contracts/tsconfig.json(1 hunks)integration_tests/pytest.ini(1 hunks)integration_tests/test_basic.py(1 hunks)nix/build_overlay.nix(1 hunks)nix/default.nix(3 hunks)nix/go-ethereum.nix(2 hunks)nix/golangci-lint.nix(1 hunks)nix/hermes.nix(2 hunks)nix/rocksdb.nix(3 hunks)nix/sources.json(2 hunks)nix/testenv.nix(2 hunks)scripts/run-integration-tests(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.9)
.github/workflows/lint.yml
26-26: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
⏰ 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). (200)
- GitHub Check: build (macos-latest)
- GitHub Check: unittest
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (macos-14)
- GitHub Check: gomod2nix
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (mint)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (evm)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: Lint python
- GitHub Check: Run golangci-lint
- GitHub Check: build (macos-latest)
- GitHub Check: unittest
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (macos-14)
- GitHub Check: gomod2nix
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (mint)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (evm)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: Lint python
- GitHub Check: Run golangci-lint
- GitHub Check: build (macos-latest)
- GitHub Check: unittest
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (macos-14)
- GitHub Check: gomod2nix
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (mint)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (evm)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: Lint python
- GitHub Check: Run golangci-lint
- GitHub Check: build (macos-latest)
- GitHub Check: unittest
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (macos-14)
- GitHub Check: gomod2nix
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (mint)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (evm)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: Lint python
- GitHub Check: Run golangci-lint
- GitHub Check: build (macos-latest)
- GitHub Check: unittest
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (macos-14)
- GitHub Check: gomod2nix
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (mint)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (evm)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: Lint python
- GitHub Check: Run golangci-lint
- GitHub Check: build (macos-latest)
- GitHub Check: unittest
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (macos-14)
- GitHub Check: gomod2nix
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (mint)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (evm)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: Lint python
- GitHub Check: Run golangci-lint
- GitHub Check: build (macos-latest)
- GitHub Check: unittest
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (macos-14)
- GitHub Check: gomod2nix
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (mint)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (evm)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: Lint python
- GitHub Check: Run golangci-lint
- GitHub Check: build (macos-latest)
- GitHub Check: unittest
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (macos-14)
- GitHub Check: gomod2nix
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (mint)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (evm)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: Lint python
- GitHub Check: Run golangci-lint
- GitHub Check: build (macos-latest)
- GitHub Check: unittest
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (macos-14)
- GitHub Check: gomod2nix
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (mint)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (evm)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: Lint python
- GitHub Check: Run golangci-lint
- GitHub Check: build (macos-latest)
- GitHub Check: unittest
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (macos-14)
- GitHub Check: gomod2nix
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (mint)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (evm)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: Lint python
- GitHub Check: Run golangci-lint
🔇 Additional comments (34)
integration_tests/test_basic.py (1)
114-121: Verify that the new gas expectation reflects actual execution with Go 1.25.0.The expected gas usage was updated to 564724. While this change is plausible given the Go version upgrade mentioned in the PR, there is no evidence that these tests were executed with Go 1.25.0 to validate the new value.
Please confirm:
- Was this test executed with Go 1.25.0, and does it consistently produce a
gasUsedof 564724?- What caused the gas increase? (e.g., compiled bytecode differences, runtime changes, ethermint version updates)
- Only one other hard-coded
exp_gas_usedwas found in the test suite (test_pruned_node.py:49); are there any other integration tests that may need similar updates?integration_tests/pytest.ini (1)
4-17: Pytest marker definitions are clear and consistent with integration test usageRegistering these markers avoids
PytestUnknownMarkWarningand gives a good taxonomy for selecting subsets (ibc, gov, upgrade, gas, etc.). This looks correct and well-scoped.scripts/run-integration-tests (1)
10-10: Switch tonpm ciimproves determinism; ensure lockfile is present and maintainedUsing
npm cihere is good for reproducible, clean installs, especially under CI. Just make sureintegration_tests/contractshas a committed, up‑to‑date lockfile (e.g.,package-lock.json), otherwise this step will now fail wherenpm installpreviously succeeded.integration_tests/contracts/package.json (1)
12-20: Hardhat + TypeScript tooling additions align with the existing typechain workflowAdding
hardhatplusts-node/typescriptis consistent with thenpx hardhat typechainscript and the new TS config. From a config perspective this looks correct; just ensure:
- The Node version used in
integration_tests/shell.nixmatches the supported range for this Hardhat + TypeScript combo.- The lockfile in this directory is regenerated so CI’s
npm cipicks up these exact versions..github/workflows/dependencies-review.yml (2)
18-18: Version bump looks good.The go-version upgrade from 1.24 to 1.25 is straightforward and properly configured.
39-45: Hardcoded vulnerability exception GO-2025-3443 is justified and properly tracked.This exception references a real CometBFT vulnerability (network stalling via malicious peers) that was fixed in v0.38.17 and v1.0.1. The fix was applied in PR #1736 (Dec 19, 2024), and the codebase currently uses v0.38.19, which contains the patch. The exception is documented in both the CHANGELOG and the workflow comment. No action required.
.github/workflows/codeql-analysis.yml (1)
47-47: Go version upgrade looks good.The version constraint from
^1.24.0to^1.25.0is appropriately updated and allows patch releases..gitignore (1)
28-28: Git ignore pattern added.The
chain-main-*pattern correctly excludes chain-main build artifacts from version control, aligning with Nix build tooling changes elsewhere in the PR..github/workflows/build.yml (1)
25-25: Nixpkgs channel upgrade applied consistently.All four jobs (build, unittest, gomod2nix, contracts) are updated from
nixos-22.11tonixos-25.11consistently.Also applies to: 58-58, 108-108, 142-142
.github/workflows/release.yml (1)
19-19: Nixpkgs channel upgrade applied to release workflows.Both release jobs (Linux and macOS) properly updated from
nixos-22.11tonixos-25.11.Also applies to: 60-60
.github/workflows/lint.yml (1)
25-25: Go and Nixpkgs versions properly updated.Go version upgraded to 1.25.0, and nixpkgs channel updated to nixos-25.11 across all three linting jobs (golangci, lint-python, lint-nix).
Also applies to: 29-29, 77-77, 103-103
.github/workflows/dependabot-update-all.yml (1)
29-29: Go version upgrade looks good.The go-version is properly updated from
1.24to1.25for dependency updates.nix/sources.json (2)
101-110: Nixpkgs source pinning updated to release-25.11.The nixpkgs entry is properly updated with:
- Branch:
release-25.11- New revision:
f27d6578053dfba71ffe5156a8274a552340b730- Updated SHA256 hash and tarball URL
Ensure these values match the official release-25.11 branch upstream before merging.
118-122: Poetry2nix entry updated.Poetry2nix is updated to revision
ce2369db77f45688172384bbeb962bc6c2ea6f94with matching SHA256 hash. This appears consistent with the broader tooling updates.nix/rocksdb.nix (2)
14-14: Formatting looks good.The inline comment referencing the nixpkgs issue for the jemalloc condition is helpful for maintainability.
103-123: Clean reformatting of shell script blocks.The preInstall and postFixup blocks are now properly formatted with cleaner multi-line string handling. The logic remains unchanged.
.github/workflows/test.yml (2)
40-40: Nix channel update aligns with the Go toolchain upgrade.The nixos-25.11 channel is consistent with the PR's goal of modernizing the build environment.
67-67: Go 1.25 for coverage tooling.The coverage tool update to
go_1_25aligns with the rest of the toolchain upgrade. Ensure integration tests pass to validate compatibility.go.mod (1)
306-306: Ethermint dependency update looks consistent.The ethermint fork version is updated to a newer pseudo-version and is synchronized with the gomod2nix.toml entry.
gomod2nix.toml (1)
327-329: Ethermint mapping correctly synchronized with go.mod.The version and hash updates are consistent with the replace directive in go.mod, ensuring the Nix build will use the correct ethermint fork version.
.github/workflows/sims.yml (2)
111-113: Consistent Go 1.25 upgrade across simulation tests.The
^1.25.0semver constraint allows automatic patch updates, which is good for picking up security fixes while maintaining compatibility.
132-134: Runsim installation job updated.The Go version is consistently updated to 1.25.0.
nix/hermes.nix (1)
10-10: LGTM! Darwin SDK migration looks correct.The switch to
apple-sdk_15consolidates the individual framework dependencies (Security, SystemConfiguration) into the unified SDK approach used in nixpkgs 25.11. Retainingdarwin.libiconvseparately is appropriate since it's not part of the SDK.Also applies to: 48-51
nix/go-ethereum.nix (1)
6-6: LGTM! Consistent Darwin SDK update.The migration to
apple-sdk_15aligns with the nixpkgs 25.11 changes and follows the same pattern as other Nix files in this PR. The comment documenting the rationale is appreciated.Also applies to: 59-63
nix/testenv.nix (2)
7-30: LGTM! Proper rpds-py Rust build override.The custom override scope correctly handles
rpds-pysource builds by adding Cargo dependencies and Rust tooling. The conditional check!(old.src.isWheel or false)ensures this only applies to non-wheel sources.Verify the cargo vendor hash is still valid if
rpds-pyversion changes in the future. A version bump would require updating the hash on line 18.
61-74: LGTM! License format normalization for pyproject.toml.The postPatch scripts handle the PEP 639 license format migration by removing
license-filesand converting the license string to the object format.default.nix (2)
5-5: LGTM! Explicit Go toolchain wiring.Adding
goto inputs and inheriting it in the derivation ensures the build uses the explicitly configuredgo_1_25from the overlay rather than relying on implicit resolution.Also applies to: 43-43
60-60: LGTM! Required for gomod2nix configuration.The
modRoot = "."setting aligns with the updated gomod overlay configuration per the AI summary.nix/build_overlay.nix (1)
41-76: LGTM! Reasonable workaround for Darwin solc builds.The Darwin-specific workaround using pre-built macOS binaries is a pragmatic solution for the nixpkgs 25.11 SDK compatibility issues. The fallback to
super.solc-static-versionson Linux maintains expected behavior. The two versions defined (solc_0_6_8andsolc_0_8_21) are exactly those referenced in the project's nix files (contracts/shell.nixandnix/gen-binding-shell.nix), so the workaround is complete and sufficient.flake.nix (2)
32-43: LGTM on the gomodOverlay pattern with modRoot workaround.The
modRoot = "."override addresses the gomod2nix builder issue where a null modRoot causes build failures. This approach correctly exposesbuildGoApplication,mkGoEnv, andmkVendorEnvwhile applying the necessary fix.
3-3: Be aware of known Go 1.25 compatibility issues on macOS.The
release-25.11branch andgo_1_25package are both available in nixpkgs. However, Go 1.25 has known linking failures on macOS (macOS 15.6+) and CGO behavior changes (CGO_ENABLED=1). Test thoroughly on macOS if that's a supported platform.Likely an incorrect or invalid review comment.
nix/default.nix (3)
19-30: Good approach for chain-maind Go version compatibility.The mapping of
go_1_23togo_1_25viabuildPackagesoverride maintains compatibility with chain-main's expected interface while upgrading the actual toolchain. ThemodRoot = "."fix with the linked PR reference provides good traceability.
85-85: Correct update frombuildGo123ModuletobuildGoModule.Using the generic
buildGoModuleis appropriate here since the Go version is now set at the overlay level viago = final.go_1_25. This avoids hardcoding Go version in individual package builds.
67-69: apple-sdk_15 is available in nixpkgs 25.11.Verified that
apple-sdk_15is defined in all-packages.nix ascallPackage ../by-name/ap/apple-sdk/package.nix { darwinSdkMajorVersion = "15"; }. This consolidation of Darwin framework dependencies toapple-sdk_15follows the correct pattern for nixpkgs 25.11.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
nix/build_overlay.nix (1)
31-36: Use--replace-failfor safer builds.The previous review suggested using
--replace-failto ensure the build fails if the patterns aren't found, rather than silently succeeding. This hasn't been addressed yet.postPatch = (old.postPatch or "") + '' substituteInPlace src/net/lookup_unix.go \ - --replace 'open("/etc/protocols")' 'open("${final.iana-etc}/etc/protocols")' + --replace-fail 'open("/etc/protocols")' 'open("${final.iana-etc}/etc/protocols")' substituteInPlace src/net/port_unix.go \ - --replace 'open("/etc/services")' 'open("${final.iana-etc}/etc/services")' + --replace-fail 'open("/etc/services")' 'open("${final.iana-etc}/etc/services")' '';
🧹 Nitpick comments (1)
nix/build_overlay.nix (1)
46-59: Consider extracting common derivation logic to reduce duplication.Both
solc_0_6_8andsolc_0_8_21share nearly identical structure. A helper function would improve maintainability if more versions are needed.# Example refactor using a helper function: solc-static-versions = if final.stdenv.isDarwin then let mkSolc = version: sha256: final.stdenv.mkDerivation { pname = "solc"; inherit version; src = final.fetchurl { url = "https://github.com/ethereum/solidity/releases/download/v${version}/solc-macos"; inherit sha256; }; dontUnpack = true; installPhase = '' mkdir -p $out/bin cp $src $out/bin/solc-${version} chmod +x $out/bin/solc-${version} ''; }; in { solc_0_6_8 = mkSolc "0.6.8" "sha256-W0qCSwXC9nb1a6pRKPeczT9RZ+1RpVMVCD/vl8xEaEc="; solc_0_8_21 = mkSolc "0.8.21" "sha256-mzXHa30o2kCPW9XhBfr0XsCXS0+7VdPN3bZ4pJG3PU0="; } else super.solc-static-versions;Also applies to: 60-73
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.github/workflows/codeql-analysis.yml(1 hunks).github/workflows/dependabot-update-all.yml(1 hunks).github/workflows/dependencies-review.yml(1 hunks).github/workflows/lint.yml(3 hunks).github/workflows/sims.yml(6 hunks)Makefile(2 hunks)go.mod(2 hunks)nix/build_overlay.nix(1 hunks)nix/golangci-lint.nix(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- go.mod
- .github/workflows/codeql-analysis.yml
- .github/workflows/dependabot-update-all.yml
- .github/workflows/dependencies-review.yml
- Makefile
- .github/workflows/sims.yml
- nix/golangci-lint.nix
🧰 Additional context used
🪛 actionlint (1.7.9)
.github/workflows/lint.yml
26-26: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
⏰ 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). (8)
- GitHub Check: Run golangci-lint
- GitHub Check: build (ubuntu-latest)
- GitHub Check: unittest
- GitHub Check: Lint python
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-14)
- GitHub Check: gomod2nix
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (2)
.github/workflows/lint.yml (1)
25-25: Go version is only updated in the golangci job; nixos-25.11 is consistent across all three lint jobs.The go-version setup (1.25.5) appears only in the golangci job (line 25) via
actions/setup-go, which is correct since only that job compiles and lints Go code. The lint-python and lint-nix jobs use nix-shell environments instead and do not require explicit Go setup in the workflow.The nixos-25.11 channel is consistently applied across all three jobs (lines 29, 77, 103). The nix overlay in
nix/default.nixalready specifiesgo = final.go_1_25, andnix/build_overlay.nixdefines the Go 1.25.5 build itself, ensuring consistency via the nix package definitions rather than workflow duplication.Likely an incorrect or invalid review comment.
nix/build_overlay.nix (1)
12-21: Go 1.25.5 update looks good.The version has been updated to 1.25.5 with the corresponding hash, and the comments clearly explain why
go_1_24is used as the base (Windows platform support). The patch filtering foriana-etc-1.17.patchis a reasonable approach.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
nix/build_overlay.nix (2)
32-35: Apply--replace-failas suggested in the previous review.The past review comment suggested using
--replace-failin thesubstituteInPlacecalls to ensure build failure if the expected patterns are not found. This recommendation has not been implemented.Apply this diff to add
--replace-fail:postPatch = (old.postPatch or "") + '' substituteInPlace src/net/lookup_unix.go \ - --replace 'open("/etc/protocols")' 'open("${final.iana-etc}/etc/protocols")' + --replace-fail 'open("/etc/protocols")' 'open("${final.iana-etc}/etc/protocols")' substituteInPlace src/net/port_unix.go \ - --replace 'open("/etc/services")' 'open("${final.iana-etc}/etc/services")' + --replace-fail 'open("/etc/services")' 'open("${final.iana-etc}/etc/services")' '';
46-81: Document the Rosetta 2 requirement for Apple Silicon as suggested in the previous review.The solc binaries for versions 0.6.8 and 0.8.21 are x86_64-only (Apple Silicon support was added in v0.8.24). On Apple Silicon Macs, these binaries will run under Rosetta 2 translation. As noted in the previous review, this requirement should be documented.
Apply this diff to add clarifying documentation:
# solc-static-versions is broken on Darwin in nixpkgs 25.11 due to legacy SDK usage - # On Darwin: use pre-built macOS binaries. On Linux: use the original nixpkgs derivations. + # On Darwin: use pre-built macOS binaries (x86_64; requires Rosetta 2 on Apple Silicon for v0.6.8 and v0.8.21). + # On Linux: use the original nixpkgs derivations. solc-static-versions =
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
nix/build_overlay.nix(1 hunks)
⏰ 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). (20)
- GitHub Check: gomod2nix
- GitHub Check: build (macos-latest)
- GitHub Check: unittest
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (macos-14)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (evm)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (mint)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (gov)
- GitHub Check: Run golangci-lint
- GitHub Check: semgrep-cloud-platform/scan
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
nix/build_overlay.nix (1)
50-68: Add comment documenting Rosetta 2 requirement for Apple Silicon.The solc-macos binaries for v0.6.8 and v0.8.21 are x86_64-only; official Apple Silicon (arm64) binaries were not released until v0.8.24. On Apple Silicon Macs, these binaries run under Rosetta 2 translation. A brief comment would help future maintainers understand this platform constraint.
# solc-static-versions is broken on Darwin in nixpkgs 25.11 due to legacy SDK usage # On Darwin: use pre-built macOS binaries. On Linux: use the original nixpkgs derivations. + # Note: These macOS binaries are x86_64-only. On Apple Silicon, Rosetta 2 is required. solc-static-versions =
🧹 Nitpick comments (1)
nix/build_overlay.nix (1)
35-40: Consider using--replace-failfor safer substitutions.Using
--replacesilently succeeds even if the pattern isn't found. If Go source files change in future patch releases, the build would succeed but produce incorrect binaries that still reference/etc/protocolsand/etc/services.postPatch = (old.postPatch or "") + '' substituteInPlace src/net/lookup_unix.go \ - --replace 'open("/etc/protocols")' 'open("${final.iana-etc}/etc/protocols")' + --replace-fail 'open("/etc/protocols")' 'open("${final.iana-etc}/etc/protocols")' substituteInPlace src/net/port_unix.go \ - --replace 'open("/etc/services")' 'open("${final.iana-etc}/etc/services")' + --replace-fail 'open("/etc/services")' 'open("${final.iana-etc}/etc/services")' '';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
nix/build_overlay.nix(1 hunks)
⏰ 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). (20)
- GitHub Check: unittest
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (macos-latest)
- GitHub Check: gomod2nix
- GitHub Check: build (macos-14)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (evm)
- GitHub Check: integration_tests (mint)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: Run golangci-lint
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (1)
nix/build_overlay.nix (1)
12-15: LGTM!The backwards compatibility alias for
windows.threads→pthreadsis correctly implemented and well-documented.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
nix/rocksdb.nix (1)
24-24: Consider using the finalAttrs pattern instead of rec.The change from
stdenv.mkDerivation (finalAttrs: { ... })tostdenv.mkDerivation rec { ... }moves away from the modern nixpkgs pattern. ThefinalAttrspattern is preferred because it:
- Provides better error messages for infinite recursion
- Makes the derivation more maintainable
- Follows current nixpkgs conventions
Revert to the finalAttrs pattern:
-stdenv.mkDerivation rec { +stdenv.mkDerivation (finalAttrs: {And update references to use
finalAttrs.versionwhere needed.nix/build_overlay.nix (1)
77-104: Consider extracting a helper function to reduce duplication.The
solc_0_6_8andsolc_0_8_21derivations follow an identical pattern. Consider extracting a helper function to reduce duplication:let mkSolcMacos = version: sha256: final.stdenv.mkDerivation { pname = "solc"; inherit version; src = final.fetchurl { url = "https://github.com/ethereum/solidity/releases/download/v${version}/solc-macos"; inherit sha256; }; dontUnpack = true; installPhase = '' mkdir -p $out/bin cp $src $out/bin/solc-${version} chmod +x $out/bin/solc-${version} ''; }; in { solc_0_6_8 = mkSolcMacos "0.6.8" "sha256-W0qCSwXC9nb1a6pRKPeczT9RZ+1RpVMVCD/vl8xEaEc="; solc_0_8_21 = mkSolcMacos "0.8.21" "sha256-mzXHa30o2kCPW9XhBfr0XsCXS0+7VdPN3bZ4pJG3PU0="; }Note: An existing review comment already covers the Rosetta 2 requirement for these x86_64-only binaries on Apple Silicon.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
nix/build_overlay.nix(1 hunks)nix/rocksdb.nix(5 hunks)
⏰ 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). (20)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (evm)
- GitHub Check: integration_tests (mint)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: build (macos-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (macos-14)
- GitHub Check: unittest
- GitHub Check: gomod2nix
- GitHub Check: Run golangci-lint
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (11)
nix/rocksdb.nix (7)
8-8: LGTM: pkg-config correctly added for dependency resolution.The addition of
pkg-configto imports andnativeBuildInputsis appropriate for the MinGW CMake configuration added later in the file (lines 93-96).Also applies to: 38-38
19-19: LGTM: enableLite parameter properly implemented.The new
enableLiteparameter with sensible default (false) provides optional ROCKSDB_LITE support without breaking existing behavior.Also applies to: 89-89
59-70: LGTM: Improved compiler-specific flag handling.The restructure properly separates GNU and Clang compiler flags, improving clarity and maintainability for multi-compiler builds.
89-102: LGTM: Comprehensive CMake configuration improvements.The additions handle:
- ROCKSDB_LITE support via
enableLite- Platform-specific warning handling for MinGW
- Proper pkg-config integration for MinGW/LZ4 resolution
The comment clearly explains the pkg-config requirement for MinGW builds.
113-113: LGTM: Dynamic version extraction improves maintainability.Using
lib.versions.major versioninstead of hardcoded version numbers makes the derivation more maintainable for future version updates.Also applies to: 129-129
135-135: LGTM: Consistent version interpolation.The changelog URL now uses
v${version}, consistent with therevfield change and improving maintainability.
30-32: LGTM: Improved maintainability with variable interpolation.Using
pnameforrepoandv${version}forrevmakes future version updates easier. Both variables are correctly defined and the sha256 hash format is valid.nix/build_overlay.nix (4)
22-36: Patch filtering and manual substitutions look correct.The patch filtering properly excludes
iana-etc-1.17.patchfor Go 1.25, and the manualpostPatchsubstitutions correctly rewrite the iana-etc paths. This approach is valid when upstream patches don't apply cleanly to newer versions.Note: An existing review comment already covers updating the Go version to 1.25.5 and using
--replace-fail.
37-51: LGTM: Cross-compilation threading dependencies handled correctly.The
depsTargetTargetlogic safely handles MinGW cross-compilation scenarios wherethreadsmay not be available intargetPackages. The fallback towindows.pthreadsand null-checks prevent evaluation errors.
52-58: LGTM: Windows platform support properly added.Extending
meta.platformswith Windows variants enables cross-compilation support. The use oflib.uniqueensures no duplicate entries.
60-68: LGTM: iana-etc platform support aligned with Go.The
iana-etcoverride is necessary because it's referenced in the Go 1.25 postPatch substitutions (lines 33, 35). Extending its platform support to match Go's Windows support ensures consistency.
48de370 to
9a029b6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
nix/build_overlay.nix (3)
17-17: Consider updating to the latest Go 1.25 patch version.Go 1.25.0 was released in August 2025, but more recent patch versions may include important bug fixes and security updates. Verify that 1.25.0 is the intended version or update to the latest 1.25.x patch.
What is the latest Go 1.25 patch version as of December 2025?
31-36: Consider using--replace-failfor safer substitutions.Using
--replace-failinstead of--replaceensures the build fails if the target strings are not found (e.g., if Go's source changes in a future version), rather than silently succeeding with no substitution.postPatch = (old.postPatch or "") + '' substituteInPlace src/net/lookup_unix.go \ - --replace 'open("/etc/protocols")' 'open("${final.iana-etc}/etc/protocols")' + --replace-fail 'open("/etc/protocols")' 'open("${final.iana-etc}/etc/protocols")' substituteInPlace src/net/port_unix.go \ - --replace 'open("/etc/services")' 'open("${final.iana-etc}/etc/services")' + --replace-fail 'open("/etc/services")' 'open("${final.iana-etc}/etc/services")' '';
72-107: Document Rosetta 2 requirement for Apple Silicon Macs.The solc-macos binaries for v0.6.8 and v0.8.21 are x86_64-only; official Apple Silicon (arm64) binaries were not released until v0.8.24. On Apple Silicon Macs, these binaries will run under Rosetta 2 translation. Consider adding a comment to clarify this requirement:
# solc-static-versions is broken on Darwin in nixpkgs 25.11 due to legacy SDK usage # On Darwin: use pre-built macOS binaries. On Linux: use the original nixpkgs derivations. + # Note: solc-macos binaries for v0.6.8 and v0.8.21 are x86_64 only and require Rosetta 2 + # on Apple Silicon Macs (arm64 binaries not available until v0.8.24). solc-static-versions =Additionally, consider adding
metaattributes for consistency:meta = { description = "Solidity compiler"; platforms = [ "x86_64-darwin" "aarch64-darwin" ]; # aarch64 via Rosetta 2 };nix/rocksdb.nix (1)
48-48: lz4 duplication remains unresolved.
lz4appears in bothpropagatedBuildInputs(line 48) andbuildInputs(line 51), which is redundant. This issue was previously flagged in an earlier review. Typically,lz4should only be inpropagatedBuildInputsif downstream packages need to link against it, or only inbuildInputsif it's a private build-time dependency.Based on learnings, this duplication was flagged in a previous review and should be addressed.
Also applies to: 51-51
🧹 Nitpick comments (1)
nix/build_overlay.nix (1)
77-104: Consider extracting a helper function if more versions are needed.The two solc derivations follow an identical pattern. If additional versions are required in the future, consider extracting a helper function:
mkSolcDarwin = { version, sha256 }: final.stdenv.mkDerivation { pname = "solc"; inherit version; src = final.fetchurl { url = "https://github.com/ethereum/solidity/releases/download/v${version}/solc-macos"; inherit sha256; }; dontUnpack = true; installPhase = '' mkdir -p $out/bin cp $src $out/bin/solc-${version} chmod +x $out/bin/solc-${version} ''; };For the current two versions, the existing approach is acceptable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
nix/build_overlay.nix(1 hunks)nix/bundle-win-exe.nix(1 hunks)nix/rocksdb.nix(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- nix/bundle-win-exe.nix
⏰ 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). (20)
- GitHub Check: integration_tests (gov)
- GitHub Check: unittest
- GitHub Check: Run golangci-lint
- GitHub Check: build (macos-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: gomod2nix
- GitHub Check: build (macos-14)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (mint)
- GitHub Check: integration_tests (evm)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (7)
nix/build_overlay.nix (2)
37-58: Well-structured cross-compilation handling.The
depsTargetTargetlogic provides a robust fallback chain for MinGW cross builds, handling cases where the threads package may not be exposed intargetPackages. The platform extension for Windows variants aligns with the PR's cross-platform support goals.
60-68: LGTM!The
iana-etcplatform extension is consistent with thego_1_25changes and ensures the dependency is available for Windows cross-compilation targets.nix/rocksdb.nix (5)
24-24: Verify the derivation pattern change from finalAttrs to rec.The derivation signature changed from
stdenv.mkDerivation (finalAttrs: { ... })tostdenv.mkDerivation rec { ... }. ThefinalAttrspattern is generally preferred in modern nixpkgs for accessing final attribute values and composability. Confirm that this change to therecpattern is intentional and doesn't impact derivation overrides or attribute finalization.
109-113: Good addition: postPatch fixes memcpy type safety.The explicit
static_castconversions in thememcpycall improve compiler compatibility and address potential type safety warnings. This is a sensible fix for C++ strict aliasing requirements.
60-72: LGTM: Improved compiler flags organization.The restructured
NIX_CFLAGS_COMPILEwith separate conditional blocks for GCC and Clang is clearer and more maintainable than a single list.
182-182: LGTM: Improved version handling.The use of
lib.versions.majorfor extracting the major version number ininstall_name_toolcommands (lines 182, 198) and the consistentv${version}in the changelog URL (line 204) are good improvements for version string handling.Also applies to: 198-198, 204-204
19-19: Verify public API documentation for enableLite parameter.The
enableLiteparameter correctly maps to RocksDB's legitimateROCKSDB_LITEcompile flag used to reduce binary size for lightweight deployments. However, confirm that this parameter is intentionally exposed as public API and includes documentation for derivation consumers on its purpose and when to use it.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
nix/build_overlay.nix (1)
78-113: Add a comment documenting Rosetta 2 requirement for Apple Silicon.The solc-macos binaries for v0.6.8 and v0.8.21 are x86_64-only; official Apple Silicon (arm64) binaries were not released until v0.8.24. On Apple Silicon Macs, these binaries will run under Rosetta 2 translation.
Consider adding a comment clarifying this requirement:
# solc-static-versions is broken on Darwin in nixpkgs 25.11 due to legacy SDK usage # On Darwin: use pre-built macOS binaries. On Linux: use the original nixpkgs derivations. + # NOTE: The macOS binaries for solc <0.8.24 are x86_64-only and require Rosetta 2 on Apple Silicon. solc-static-versions =nix/rocksdb.nix (2)
43-54: Clarify lz4 duplication between propagatedBuildInputs and buildInputs.
lz4appears in bothpropagatedBuildInputs(line 49) andbuildInputs(line 52) whenwithLz4is true. Typically, if a dependency needs to be propagated to consumers, it should only be inpropagatedBuildInputs. Having it in both is redundant.Consider removing lz4 from
buildInputs:buildInputs = - lib.optional withLz4 lz4 - ++ lib.optional enableJemalloc jemalloc + lib.optional enableJemalloc jemalloc ++ lib.optional stdenv.hostPlatform.isMinGW windows.pthreads;
28-28: Critical logic error: MinGW LZ4 handling code is unreachable.Line 28 defines
withLz4 = !stdenv.hostPlatform.isMinGW, which setswithLz4to false when building on MinGW. However, thepreConfigureblock on line 113 requires bothstdenv.hostPlatform.isMinGWandwithLz4to be true. This condition can never be satisfied, making the entire MinGW LZ4 import library generation code (lines 113-172) unreachable.To enable LZ4 on MinGW with the custom import library handling:
- withLz4 = !stdenv.hostPlatform.isMinGW; + withLz4 = true;Or, if LZ4 should genuinely be disabled on MinGW, remove the entire
preConfigureblock (lines 113-172) and the MinGW-specific cmake flags (lines 95-101) as they become dead code.Also applies to: 113-172
🧹 Nitpick comments (1)
nix/rocksdb.nix (1)
143-143: Consider dynamically determining the LZ4 version.The LZ4 version is hardcoded as
1.10.0in the generated pkgconfig file. If the lz4 package version changes, this could become stale. Consider using${lz4.version}if available:- Version: 1.10.0 + Version: ${lz4.version or "1.10.0"}
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
nix/build_overlay.nix(1 hunks)nix/rocksdb.nix(3 hunks)
⏰ 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). (20)
- GitHub Check: Run golangci-lint
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (mint)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (evm)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: gomod2nix
- GitHub Check: build (macos-14)
- GitHub Check: build (macos-latest)
- GitHub Check: unittest
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (5)
nix/build_overlay.nix (2)
12-62: LGTM! Well-structured Go 1.25 override with proper cross-compilation support.The override approach to inherit Windows platform support from go_1_24 is appropriate, and the depsTargetTarget fallback logic for MinGW LLVM cross builds (lines 40-51) handles edge cases gracefully. The patch filtering and manual iana-etc substitutions are correctly implemented.
63-74: LGTM!Platform extension for Windows variants is consistent with the go_1_25 override pattern.
nix/rocksdb.nix (3)
61-73: LGTM!Compiler-specific warning suppressions are appropriately organized for GCC and Clang toolchains.
107-111: LGTM!The
std::memcpycast fix addresses strict aliasing requirements correctly.
174-195: LGTM!Platform-specific post-processing for tools and shared libraries is correctly implemented for both Darwin (install_name_tool) and Linux (patchelf).
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
nix/rocksdb.nix (1)
48-49: lz4 duplication persists between propagatedBuildInputs and buildInputs.
lz4appears in bothpropagatedBuildInputs(line 49) andbuildInputs(line 52). This was flagged in a previous review and remains unresolved. Since RocksDB links against lz4 and downstream consumers need it, keeping it inpropagatedBuildInputsis correct—the occurrence inbuildInputsis redundant.Apply this diff to remove the redundant entry:
buildInputs = - lib.optional withLz4 lz4 - ++ lib.optional enableJemalloc jemalloc + lib.optional enableJemalloc jemalloc ++ lib.optional stdenv.hostPlatform.isMinGW windows.pthreads;Also applies to: 52-54
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/build.yml(6 hunks).github/workflows/gosec.yml(1 hunks).github/workflows/linkchecker.yml(1 hunks).github/workflows/lint.yml(5 hunks).github/workflows/proto.yml(3 hunks).github/workflows/release.yml(2 hunks).github/workflows/semgrep.yml(1 hunks).github/workflows/sims.yml(6 hunks).github/workflows/test.yml(6 hunks)nix/rocksdb.nix(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- .github/workflows/lint.yml
- .github/workflows/release.yml
- .github/workflows/build.yml
🧰 Additional context used
🪛 actionlint (1.7.9)
.github/workflows/sims.yml
111-111: the runner of "actions/setup-go@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
⏰ 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). (20)
- GitHub Check: integration_tests (mint)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (evm)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-14)
- GitHub Check: unittest
- GitHub Check: gomod2nix
- GitHub Check: Run golangci-lint
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (18)
.github/workflows/semgrep.yml (1)
23-23: LGTM! Checkout action upgrade is correct.Upgrading to actions/checkout@v4 is appropriate and aligns with the repository-wide workflow modernization.
.github/workflows/linkchecker.yml (1)
14-14: LGTM! Checkout action upgrade is correct..github/workflows/gosec.yml (1)
19-19: LGTM! Checkout action upgrade is correct..github/workflows/proto.yml (1)
16-16: LGTM! Consistent checkout action upgrades across all jobs.All three jobs (lint, breakage, protogen) have been correctly updated to use actions/checkout@v4.
Also applies to: 27-27, 38-38
.github/workflows/sims.yml (2)
99-99: LGTM! Checkout action upgrades are consistent.All checkout actions have been correctly updated to v4 across all jobs.
Also applies to: 107-107, 163-163, 171-171, 219-219, 227-227, 275-275, 283-283, 331-331, 339-339
113-113: Go 1.25.0 has been released and is suitable for production use.Go 1.25 was officially released on August 12, 2025, and Go 1.25.5 was released on December 2, 2025 with additional security and bug fixes, demonstrating stable maintenance. The version specifier ^1.25.0 is appropriate for production workflows.
.github/workflows/test.yml (3)
28-28: LGTM! Checkout action upgrades are consistent.All checkout actions have been correctly updated to v4.
Also applies to: 85-85, 116-116
67-67: LGTM! Go toolchain reference updated correctly.The update from go_1_22 to go_1_25 aligns with the PR objective.
40-40: The nixos-25.11 channel is available and stable.The channel has been officially released and will receive security updates through June 2026. However, this is a major version jump spanning three years and multiple release cycles. Review the 25.11 release notes for breaking changes—notably GCC 14, kernel updates, and removed packages—before merging this change to ensure system compatibility.
Also applies to: 97-97, 121-121
nix/rocksdb.nix (9)
8-8: LGTM: pkg-config addition supports MinGW LZ4 discovery.Adding pkg-config as a native build input aligns with the MinGW-specific CMake flags that require pkg-config for LZ4 discovery (line 99).
Also applies to: 40-40
19-19: New public API: enableLite parameter.The new
enableLiteparameter exposes RocksDB's LITE build variant. The defaultfalsemaintains backward compatibility.
28-28: Excellent fix: withLz4 = true resolves the critical logic error.Setting
withLz4 = truefixes the previous critical issue where the preConfigure block (line 113) required bothisMinGWandwithLz4to be true, which was impossible whenwithLz4 = !stdenv.hostPlatform.isMinGW.
61-73: LGTM: Compiler-specific flag separation.The restructured
NIX_CFLAGS_COMPILEproperly separates GNU and Clang toolchain flags, including necessary GCC 12 compatibility warnings.
84-84: LGTM: cmakeFlags align with new features.The updated cmakeFlags correctly gate LZ4, ROCKSDB_LITE, and shared library options based on the new parameters, and appropriately handle MinGW-specific pkg-config discovery requirements.
Also applies to: 92-102
107-111: LGTM: postPatch addresses nontrivial-memcall warning.The explicit
void*casts in the memcpy call avoid C++ undefined behavior warnings when copying viathispointer. This aligns with the-Wno-nontrivial-memcallflag (line 71).
174-183: LGTM: Platform-specific preInstall handling.The preInstall correctly handles tool installation and applies platform-specific dylib/rpath patching for Darwin and Linux shared library builds.
186-195: LGTM: postFixup properly handles pkg-config and Darwin dylib references.The postFixup correctly patches pkg-config paths and updates Darwin install_name references using
lib.versions.majorfor version-specific dylib paths.
200-200: LGTM: Changelog URL uses standard format.The changelog URL correctly uses
v${version}format, consistent with the rec pattern change.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/lint.yml (1)
23-29: Consider upgrading to setup-go@v6 and using patch-flexible version syntax.Go 1.25.0 exists and was released in August 2025, with the latest patch being go1.25.5 released on 2025-12-02. The latest version of setup-go is v6, while this workflow uses v5.
Additionally, pinning to the exact version
'1.25.0'means the workflow won't automatically receive patch releases containing security fixes and bug fixes. Consider using'1.25.x'or enablingcheck-latest: trueto automatically get the latest patch release.Option 1 (Recommended): Upgrade to setup-go@v6 with flexible patch version
- - uses: actions/setup-go@v5 + - uses: actions/setup-go@v6 with: - go-version: '1.25.0' + go-version: '1.25.x'Option 2: Keep v5 but use flexible patch version
- uses: actions/setup-go@v5 with: - go-version: '1.25.0' + go-version: '1.25.x'Option 3: Enable check-latest for automatic patch updates
- uses: actions/setup-go@v5 with: go-version: '1.25.0' + check-latest: true
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/lint.yml(5 hunks).github/workflows/sims.yml(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/sims.yml
⏰ 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). (20)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (mint)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (evm)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: Run golangci-lint
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-14)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: unittest
- GitHub Check: gomod2nix
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (2)
.github/workflows/lint.yml (2)
65-77: LGTM! Consistent updates across lint-python job.NixOS 25.11 "Xantusia" is publicly available, and the checkout action has been correctly updated to v4. The changes are consistent with the updates in the golangci job.
92-103: LGTM! Consistent updates across lint-nix job.The checkout action and nixpkgs channel updates are consistent with the rest of the workflow. All three jobs now use the same versions, which is good for maintainability.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
nix/rocksdb.nix (2)
48-54: lz4 appears in bothpropagatedBuildInputsandbuildInputs.This redundancy was already flagged in a previous review. Typically, including a package in
propagatedBuildInputsmakes it available both at build time and to consumers, so thebuildInputsentry is redundant.
143-143: Hardcoded lz4 version1.10.0in generated pkg-config file.This was already flagged in a previous review. The version should reference the actual lz4 package version dynamically rather than being hardcoded.
🧹 Nitpick comments (1)
nix/rocksdb.nix (1)
94-102: Minor: Empty line inside the MinGW conditional block.There's a stray empty line at line 100 inside the conditional. Consider removing it for consistency.
++ lib.optionals stdenv.hostPlatform.isMinGW [ # Let pkg-config drive LZ4 discovery on MinGW; the CMake package file # can point at a stub import library, while pkg-config resolves to the # full import library output. "-DCMAKE_REQUIRE_FIND_PACKAGE_PkgConfig=ON" - ]
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
nix/rocksdb.nix(4 hunks)
⏰ 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). (17)
- GitHub Check: Run golangci-lint
- GitHub Check: integration_tests (mint)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (gov)
- GitHub Check: build (macos-latest)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (evm)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: gomod2nix
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (4)
nix/rocksdb.nix (4)
107-111: LGTM!The explicit casts in
std::memcpyaddress potential compiler warnings about type-safety when copying to/fromthispointer. This is a valid fix for strict type checking.
61-73: LGTM!The compiler-specific warning flags are appropriately separated for GCC and Clang. The
-faligned-allocationflag for Clang ensures proper support for over-aligned types.
178-195: Verifyinstall_name_toolprefix consistency for cross-compilation.Line 179 uses
${stdenv.cc.bintools.targetPrefix}install_name_toolwhile line 193 uses${stdenv.cc.targetPrefix}install_name_tool. These prefixes could differ in cross-compilation scenarios. Consider using the same prefix for consistency.+ lib.optionalString stdenv.isDarwin '' - ls -1 $tools/bin/* | xargs -I{} ${stdenv.cc.bintools.targetPrefix}install_name_tool -change "@rpath/librocksdb.${lib.versions.major finalAttrs.version}.dylib" $out/lib/librocksdb.dylib {} + ls -1 $tools/bin/* | xargs -I{} ${stdenv.cc.targetPrefix}install_name_tool -change "@rpath/librocksdb.${lib.versions.major finalAttrs.version}.dylib" $out/lib/librocksdb.dylib {} ''
28-29: Confirm intended behavior forwithLz4on MinGW.The current definition
withLz4 = !stdenv.hostPlatform.isMinGWdisables LZ4 on MinGW. However, the extensivepreConfigureblock (lines 113-172) is specifically designed to handle LZ4 import library generation on MinGW. These appear contradictory.If LZ4 should be supported on MinGW (using the preConfigure workaround), consider:
- withLz4 = !stdenv.hostPlatform.isMinGW; + withLz4 = true;If LZ4 should genuinely be disabled on MinGW, the preConfigure block and related cmake flags (lines 95-100) should be removed as dead code.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
nix/rocksdb.nix (1)
113-172: Hardcoded LZ4 version should be dynamic.Line 143 hardcodes version "1.10.0" in the generated pkg-config file. Use
lz4.versionto ensure the version field reflects the actual lz4 package version.Apply this diff:
- Version: 1.10.0 + Version: ${lz4.version}Note: This entire
preConfigureblock is currently unreachable due to the logic issue at line 28 (flagged separately).Based on learnings from past review comments about the hardcoded version.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
nix/rocksdb.nix(4 hunks)
⏰ 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). (20)
- GitHub Check: Run golangci-lint
- GitHub Check: integration_tests (ica)
- GitHub Check: unittest
- GitHub Check: integration_tests (gas)
- GitHub Check: gomod2nix
- GitHub Check: build (macos-latest)
- GitHub Check: integration_tests (evm)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (mint)
- GitHub Check: build (macos-14)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (6)
nix/rocksdb.nix (6)
8-8: LGTM: pkg-config and enableLite additions are appropriate.The pkg-config addition supports MinGW LZ4 discovery (referenced in cmake flags at lines 96-99), and the enableLite parameter provides control over ROCKSDB_LITE compilation.
Also applies to: 19-19, 40-40
61-73: LGTM: Toolchain-specific compiler flags are well-structured.The restructured
NIX_CFLAGS_COMPILEappropriately differentiates between GNU and Clang toolchains, applying relevant warning suppressions for each.
75-102: LGTM: cmake flags are properly structured.The updated cmake flags correctly integrate the new
enableLiteparameter and handle MinGW-specific LZ4 discovery via pkg-config.
107-111: LGTM: memcpy fix is appropriate.The explicit casts in the memcpy call address potential type-safety issues when copying from 'this'.
174-183: LGTM: Tool installation and platform-specific binary patching are correct.The preInstall properly handles tool extraction and applies appropriate dylib/rpath fixes for Darwin and Linux respectively.
186-195: LGTM: postFixup correctly handles pkg-config and dylib references.The path substitutions and Darwin-specific install_name_tool adjustments properly finalize the package.
2 participants
👮🏻👮🏻👮🏻 !!!! REFERENCE THE PROBLEM YOUR ARE SOLVING IN THE PR TITLE AND DESCRIBE YOUR SOLUTION HERE !!!! DO NOT FORGET !!!! 👮🏻👮🏻👮🏻
PR Checklist:
make)make test)go fmt)golangci-lint run)go list -json -m all | nancy sleuth)Thank you for your code, it's appreciated! :)
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.