feat: build versioning#1260
Merged
Merged
Conversation
Member
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
📝 WalkthroughWalkthroughAdds compile-time Git capture via a new Cargo build script and a runtime, once-set SemVer with vendor build metadata; binaries initialize it early and codepaths (types, p2p, logs, tests) consume it for handshake/user-agent defaults and observability. Changes
Sequence Diagram(s)sequenceDiagram
participant Git as Git Repo
participant BuildScript as Build Script
participant Cargo as Cargo/Compiler
participant Binary as Binary (main)
participant Types as types::node_version
participant P2P as p2p (handshake)
Git->>BuildScript: git rev-parse / read HEAD, refs, packed-refs, tags
BuildScript->>Git: rev-parse --short=7 HEAD\ndescribe --exact-match --tags HEAD\ndiff-index --quiet HEAD --
BuildScript->>Cargo: emit cargo:rerun-if-changed entries\nemit env GIT_SHA, GIT_HAS_TAG, GIT_DIRTY
Cargo->>Binary: compile/link with env vars
Binary->>Types: init_version(CARGO_PKG_VERSION, GIT_SHA, GIT_HAS_TAG, GIT_DIRTY)
Types-->>Binary: store global SemVer (vendor metadata, optional .GIT_SHA/.dirty)
P2P->>Types: call get_version() when creating handshake/user_agent
Types-->>P2P: return SemVer used in handshake and defaults
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
6e82574 to
95451c5
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/macros/integer-tagged/Cargo.toml (1)
15-16: 🧹 Nitpick | 🔵 TrivialConsider using workspace dependency for
quotefor consistency.
synuses workspace reference whilequoteis hardcoded. The rootCargo.tomldefinesquote = "1.0"in workspace dependencies.🔧 Suggested change
[dependencies] syn = { workspace = true, features = ["full", "derive", "extra-traits"] } -quote = "1.0" +quote.workspace = true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/macros/integer-tagged/Cargo.toml` around lines 15 - 16, The crate's Cargo.toml mixes a workspace dependency for syn with a hardcoded version for quote; update the quote dependency to use the workspace reference (like syn does) so both use the workspace-managed version—modify the quote entry in crates/macros/integer-tagged/Cargo.toml to reference the workspace dependency instead of the literal "1.0" string to ensure consistency with syn.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/api-server/Cargo.toml`:
- Around line 2-4: Update the crate Cargo.toml so its edition matches the
workspace: replace the explicit edition = "2021" with edition.workspace = true
(or set edition = "2024" to match the workspace) in the irys-api-server
Cargo.toml; ensure the edition field for this crate (currently edition = "2021")
is removed or changed and, if you intentionally diverge, add a brief comment
explaining why.
In `@crates/chain/build.rs`:
- Around line 28-33: The current tags_dir iteration only uses std::fs::read_dir
which visits immediate children and misses nested tag files; replace the
non-recursive loop over entries (the block using tags_dir and entries.flatten())
with a recursive directory walk (e.g., use walkdir::WalkDir or a small recursive
helper) to traverse all files under tags_dir and call
println!("cargo:rerun-if-changed={}", path.display()) for every discovered file
so nested tag paths like refs/tags/v1/1.0.0 are tracked for rebuilds.
---
Outside diff comments:
In `@crates/macros/integer-tagged/Cargo.toml`:
- Around line 15-16: The crate's Cargo.toml mixes a workspace dependency for syn
with a hardcoded version for quote; update the quote dependency to use the
workspace reference (like syn does) so both use the workspace-managed
version—modify the quote entry in crates/macros/integer-tagged/Cargo.toml to
reference the workspace dependency instead of the literal "1.0" string to ensure
consistency with syn.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 60917e61-a804-4bcf-ad16-07f60ab2dfaf
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (38)
Cargo.tomlcrates/actors/Cargo.tomlcrates/api-client/Cargo.tomlcrates/api-server/Cargo.tomlcrates/c/Cargo.tomlcrates/chain-tests/Cargo.tomlcrates/chain/Cargo.tomlcrates/chain/build.rscrates/chain/src/main.rscrates/cli/Cargo.tomlcrates/config/Cargo.tomlcrates/database/Cargo.tomlcrates/domain/Cargo.tomlcrates/efficient-sampling/Cargo.tomlcrates/irys-reth/Cargo.tomlcrates/macros/diag-slow/Cargo.tomlcrates/macros/integer-tagged/Cargo.tomlcrates/p2p/Cargo.tomlcrates/p2p/src/server.rscrates/packing-worker/Cargo.tomlcrates/packing/Cargo.tomlcrates/price-oracle/Cargo.tomlcrates/reth-node-bridge/Cargo.tomlcrates/reward-curve/Cargo.tomlcrates/storage/Cargo.tomlcrates/tooling/multiversion-tests/Cargo.tomlcrates/tui/Cargo.tomlcrates/types/Cargo.tomlcrates/types/src/lib.rscrates/types/src/node_version.rscrates/types/src/version.rscrates/utils/debug-utils/Cargo.tomlcrates/utils/nextest-monitor/Cargo.tomlcrates/utils/testing-utils/Cargo.tomlcrates/utils/utils/Cargo.tomlcrates/vdf/Cargo.tomldesign/docs/build-versioning.mdxtask/Cargo.toml
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/types/src/node_version.rs`:
- Around line 40-52: Add a debug-only assertion in init_build_version to catch
misconfiguration where an untagged build has an empty git_sha: when has_tag is
false assert that git_sha is not empty (e.g., using debug_assert!) before
computing meta so developers get a clear failure in debug builds; reference
BUILD_VERSION and init_build_version so the check is placed alongside the
existing logic that constructs meta and sets version.build (this won’t affect
release behavior).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4229cb69-6008-41bd-b0c3-2d716a4d4aac
📒 Files selected for processing (2)
crates/chain/src/main.rscrates/types/src/node_version.rs
Contributor
|
Benchmark results: https://irys-xyz.github.io/irys/dev/bench/1260%2Fmerge/index.html |
JesseTheRobot
commented
Mar 30, 2026
JesseTheRobot
left a comment
Member
Author
There was a problem hiding this comment.
@coderabbitai full review
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/types/src/node_version.rs`:
- Around line 45-55: The current initialization silently ignores
BUILD_VERSION.set failure; change the BUILD_VERSION.set call in
init_build_version (the block constructing Version with Version::parse, meta,
and semver::BuildMetadata) to handle the Result and fail loudly in debug builds
instead of discarding it — e.g. replace the `let _ = BUILD_VERSION.set({...});`
pattern with logic that calls BUILD_VERSION.set(...) and on Err either panic (in
debug/assertion builds) or log and continue in release, so a late
init_build_version() call surfaces an error instead of silently dropping the SHA
metadata.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a579ecd2-584e-4e70-b96b-e6d1bd13a893
📒 Files selected for processing (1)
crates/types/src/node_version.rs
34175cf to
d39a59a
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/types/src/node_version.rs`:
- Around line 73-81: The test build_version_matches_cargo_pkg_version currently
assumes CARGO_PKG_VERSION has no prerelease (it asserts v.pre.is_empty());
update that assertion to compare prerelease fields exactly so prerelease package
versions pass: change the final assertion in the test for
build_version_matches_cargo_pkg_version to assert equality between v.pre and
pkg.pre (i.e., ensure build_version()'s pre field equals
Version::parse(env!("CARGO_PKG_VERSION")).unwrap().pre), leaving the other
major/minor/patch assertions unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5ffa433e-3654-48e1-b703-89243229a7ad
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
crates/chain/src/main.rscrates/types/src/node_version.rs
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/types/src/version.rs (1)
141-176: 🧹 Nitpick | 🔵 TrivialAdd a regression test that build metadata does not affect handshake signatures.
These defaults now make
+irys-rs[.sha]the normal path. Interop depends onencode_version_for_signing()andencode_version_rlp()continuing to hash onlymajor.minor.patch, so a focused V1/V2 test that varies onlyversion.buildwould lock down that invariant.Suggested coverage
Add one V1 case and one V2 case where two otherwise identical requests differ only in
version.build, then assert theirsignature_hash()values are equal.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/types/src/version.rs` around lines 141 - 176, Add regression tests for HandshakeRequestV1 and HandshakeRequestV2 that verify build metadata does not affect handshake signatures: create two HandshakeRequestV1 instances (and two HandshakeRequestV2 instances) that are identical except for Version.build, call encode_version_for_signing()/encode_version_rlp() indirectly by computing signature_hash() on each request (or by invoking the same signing-hash path used in production), and assert the pairwise signature_hash() values are equal; target the types HandshakeRequestV1, HandshakeRequestV2 and the signature_hash() method so the test guards that only major.minor.patch are used in the hash.
♻️ Duplicate comments (2)
crates/types/src/node_version.rs (1)
73-80:⚠️ Potential issue | 🟡 MinorKeep this baseline test valid for prerelease workspace versions.
build_version()preservespkg.pre, soassert!(v.pre.is_empty())will fail on-rc/-betaversions even when the implementation is correct.✅ Minimal fix
fn build_version_matches_cargo_pkg_version() { let v = build_version(); let pkg = Version::parse(env!("CARGO_PKG_VERSION")).unwrap(); assert_eq!(v.major, pkg.major); assert_eq!(v.minor, pkg.minor); assert_eq!(v.patch, pkg.patch); - assert!(v.pre.is_empty()); + assert_eq!(v.pre, pkg.pre); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/types/src/node_version.rs` around lines 73 - 80, The test build_version_matches_cargo_pkg_version incorrectly assumes no prerelease; update it to compare the prerelease component produced by build_version() to the package prerelease so prerelease workspace versions pass: locate the test function build_version_matches_cargo_pkg_version and replace the final assertion (currently assert!(v.pre.is_empty())) with an assertion that v.pre equals the parsed package prerelease (i.e., compare v.pre to pkg.pre or assert_eq!(v.pre, pkg.pre)) so the test validates prerelease parity as well as major/minor/patch.crates/chain/build.rs (1)
26-33:⚠️ Potential issue | 🟠 MajorTrack loose tag creation, not just the files that already exist.
This only registers tag refs present when
build.rsruns. If the same commit is built once untagged and later tagged, an incremental rebuild can miss the new loose ref because neitherpacked-refsnor any previously tracked file changed, and the binary can keep advertisingGIT_HAS_TAG=false. That breaks the tag-change guarantee described indesign/docs/build-versioning.mdLine 38-39.🛠️ Possible fix
+fn track_ref_tree(path: &std::path::Path) { + println!("cargo:rerun-if-changed={}", path.display()); + if let Ok(entries) = std::fs::read_dir(path) { + for entry in entries.flatten() { + let path = entry.path(); + if path.is_dir() { + track_ref_tree(&path); + } else { + println!("cargo:rerun-if-changed={}", path.display()); + } + } + } +} + fn main() { // ... if let Some(base) = git_output(&["rev-parse", "--git-common-dir"]) { let packed_refs = format!("{base}/packed-refs"); println!("cargo:rerun-if-changed={packed_refs}"); - // Track individual loose tag files — directory-level rerun-if-changed - // doesn't reliably detect new files on all filesystems. - let tags_dir = format!("{base}/refs/tags"); - if let Ok(entries) = std::fs::read_dir(&tags_dir) { - for entry in entries.flatten() { - println!("cargo:rerun-if-changed={}", entry.path().display()); - } + let tags_dir = std::path::PathBuf::from(format!("{base}/refs/tags")); + if tags_dir.exists() { + track_ref_tree(&tags_dir); } } // ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/chain/build.rs` around lines 26 - 33, The current loop only emits cargo:rerun-if-changed for existing loose tag files (variable tags_dir in build.rs) so new tag files created after the initial run are not detected; emit a rerun trigger for the tags directory itself (and also ensure packed-refs is tracked) in addition to the per-file prints so changes to directory contents (new/removed loose refs) cause rebuilds; update build.rs to print cargo:rerun-if-changed for tags_dir (the refs/tags directory) and for the packed-refs file alongside the existing per-entry println! calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@crates/types/src/version.rs`:
- Around line 141-176: Add regression tests for HandshakeRequestV1 and
HandshakeRequestV2 that verify build metadata does not affect handshake
signatures: create two HandshakeRequestV1 instances (and two HandshakeRequestV2
instances) that are identical except for Version.build, call
encode_version_for_signing()/encode_version_rlp() indirectly by computing
signature_hash() on each request (or by invoking the same signing-hash path used
in production), and assert the pairwise signature_hash() values are equal;
target the types HandshakeRequestV1, HandshakeRequestV2 and the signature_hash()
method so the test guards that only major.minor.patch are used in the hash.
---
Duplicate comments:
In `@crates/chain/build.rs`:
- Around line 26-33: The current loop only emits cargo:rerun-if-changed for
existing loose tag files (variable tags_dir in build.rs) so new tag files
created after the initial run are not detected; emit a rerun trigger for the
tags directory itself (and also ensure packed-refs is tracked) in addition to
the per-file prints so changes to directory contents (new/removed loose refs)
cause rebuilds; update build.rs to print cargo:rerun-if-changed for tags_dir
(the refs/tags directory) and for the packed-refs file alongside the existing
per-entry println! calls.
In `@crates/types/src/node_version.rs`:
- Around line 73-80: The test build_version_matches_cargo_pkg_version
incorrectly assumes no prerelease; update it to compare the prerelease component
produced by build_version() to the package prerelease so prerelease workspace
versions pass: locate the test function build_version_matches_cargo_pkg_version
and replace the final assertion (currently assert!(v.pre.is_empty())) with an
assertion that v.pre equals the parsed package prerelease (i.e., compare v.pre
to pkg.pre or assert_eq!(v.pre, pkg.pre)) so the test validates prerelease
parity as well as major/minor/patch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 770f87c2-f6c4-4870-9ec1-4472f4f1d337
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
crates/chain/build.rscrates/chain/src/main.rscrates/p2p/src/server.rscrates/types/src/lib.rscrates/types/src/node_version.rscrates/types/src/version.rsdesign/docs/build-versioning.md
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/chain/build.rs`:
- Around line 44-48: The current git dirty check uses unwrap_or(false) which
hides command failures; change the code around the Command::new(...) call and
the is_dirty assignment to detect Err(e) and surface it via a cargo warning
(println!("cargo:warning=...")) instead of silently treating it as clean—i.e.,
match the result of .status(), on Ok(s) set is_dirty = !s.success(), and on
Err(e) emit a cargo warning containing e (and optionally conservatively set
is_dirty = true).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 97378a7b-7ba6-44f1-8e93-618b23062c7a
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
crates/chain/build.rscrates/chain/src/main.rscrates/p2p/src/server.rscrates/types/src/lib.rscrates/types/src/node_version.rscrates/types/src/version.rsdesign/docs/build-versioning.md
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/chain/build.rs`:
- Around line 4-7: Replace the hard abort when Git metadata is missing in
build.rs by treating missing git output as absent metadata: change the code that
calls git_output for "rev-parse --git-dir" and "rev-parse --git-common-dir"
(symbols: git_output, git_dir, common_dir) to not call expect/unwrap; instead,
capture errors and set git_dir/common_dir (or equivalent env fallback variables)
to empty strings or emit fallback env vars so
init_build_version()/crates::types::node_version::build_version() can detect an
empty SHA and apply its vendor-only fallback; ensure no panic/expect remains so
builds without a .git directory proceed.
In `@crates/types/src/version.rs`:
- Around line 141-145: The HandshakeRequestV1 default and other
handshake-related code are freezing full build_version() (including +build
metadata), making signature_hash() spoofable; update HandshakeRequestV1 (and the
similar defaults at the other locations) to use only MAJOR.MINOR.PATCH (strip
pre-release and build metadata from crate::build_version()) for fields sent in
signed handshake requests, and alternatively if you intend to sign metadata
instead, modify encode_version_for_signing() and encode_version_rlp() to
explicitly include SemVer pre-release/build components so signature_hash()
covers them—choose one approach and apply it consistently to HandshakeRequestV1,
encode_version_for_signing(), encode_version_rlp(), and any other handshake
defaults referenced.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4ea8fc3c-ffff-48d9-9078-d70f204b8508
📒 Files selected for processing (4)
crates/chain/build.rscrates/p2p/src/server.rscrates/types/src/node_version.rscrates/types/src/version.rs
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/chain/build.rs (1)
4-7:⚠️ Potential issue | 🟠 MajorGit checkout requirement breaks vendored/archive builds.
The
expect()on line 6 turns any source tree without.gitinto a hard build failure. This breaks:
- Docker builds that
COPYsource without VCS metadata- Vendored tarballs /
cargo vendorworkflows- CI caches that strip
.gitfor spaceConsider emitting fallback env vars when git is unavailable instead of aborting:
🔧 Suggested fallback approach
fn main() { - // If we're not in a git repo at all, fail the build — we require git metadata. - let git_dir = git_output(&["rev-parse", "--git-dir"]) - .expect("not inside a git repository — irys-chain requires git metadata to build"); - let common_dir = git_output(&["rev-parse", "--git-common-dir"]).unwrap_or(git_dir.clone()); + // Attempt to locate git metadata; fall back gracefully for vendored builds. + let Some(git_dir) = git_output(&["rev-parse", "--git-dir"]) else { + println!("cargo:warning=not inside a git repository — version will lack git metadata"); + println!("cargo:rustc-env=GIT_SHA="); + println!("cargo:rustc-env=GIT_HAS_TAG=true"); + println!("cargo:rustc-env=GIT_DIRTY=false"); + return; + }; + let common_dir = git_output(&["rev-parse", "--git-common-dir"]).unwrap_or(git_dir.clone());This keeps git metadata optional while
init_build_version()already handles empty SHA via its vendor-only fallback path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/chain/build.rs` around lines 4 - 7, The build script currently hard-fails when git metadata is missing because git_output(...).expect(...) sets git_dir via a panic; replace this with a non-panicking fallback so builds without .git succeed: change the git_dir assignment (the git_output call that currently uses expect) to use unwrap_or_else / unwrap_or and fall back to environment vars (e.g., read IRYS_GIT_DIR or IRYS_GIT_SHA if present) or to an empty string/None, and do the same for common_dir (the git_output for --git-common-dir) so it uses the git_dir clone or a safe fallback; keep init_build_version() as-is since it already supports empty SHA. Ensure no panic is introduced and add a brief debug/warn log when falling back to indicate git metadata is absent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@crates/chain/build.rs`:
- Around line 4-7: The build script currently hard-fails when git metadata is
missing because git_output(...).expect(...) sets git_dir via a panic; replace
this with a non-panicking fallback so builds without .git succeed: change the
git_dir assignment (the git_output call that currently uses expect) to use
unwrap_or_else / unwrap_or and fall back to environment vars (e.g., read
IRYS_GIT_DIR or IRYS_GIT_SHA if present) or to an empty string/None, and do the
same for common_dir (the git_output for --git-common-dir) so it uses the git_dir
clone or a safe fallback; keep init_build_version() as-is since it already
supports empty SHA. Ensure no panic is introduced and add a brief debug/warn log
when falling back to indicate git metadata is absent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8f4b3b7f-6fd4-4c37-af0e-5f4974f6262b
📒 Files selected for processing (3)
crates/chain/build.rscrates/types/src/node_version.rscrates/types/src/version.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/chain/src/main.rs`:
- Around line 81-87: The log currently calls irys_types::build_version() twice;
instead, call irys_types::build_version() once into the local variable version
(already present) and reuse that variable in the info! macro rather than calling
irys_types::build_version() again—update the info! invocation that formats
"starting irys node v{}, mode: {:?}" to use version (and keep node_version =
%version and &config.node_mode as-is) so the function is not duplicated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 14569485-d25e-43bc-9eda-7bf03ac84aad
📒 Files selected for processing (2)
crates/chain/build.rscrates/chain/src/main.rs
DanMacDonald
approved these changes
Mar 31, 2026
DanMacDonald
left a comment
Collaborator
There was a problem hiding this comment.
yeah I think this is fine
8580db8 to
d18ce23
Compare
Member
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/types/src/version.rs (1)
211-229:⚠️ Potential issue | 🟠 MajorChanging the V2 signature preimage without a protocol bump will strand mixed-version peers.
verify_signature()now authenticates[major, minor, patch, pre, build]while the request still advertises the sameProtocolVersion::V2. Any older V2 implementation that still hashes the legacy 3-field version will now fail authentication, and there is no negotiation path because the protocol number is unchanged. Please either gate this behind a new protocol version or accept both hashes during the rollout window.Also applies to: 482-510
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/types/src/version.rs` around lines 211 - 229, The signature preimage change in verify_signature() now authenticates five fields [major, minor, patch, pre, build] while the request still uses ProtocolVersion::V2, which will break older V2 peers; either introduce a new protocol variant (e.g., ProtocolVersion::V3) and gate the new preimage code paths (update places that call encode_version_rlp_v2 and encode_version_rlp_v2_length to a v3 variant and only use the new hash when protocol == ProtocolVersion::V3), or temporarily accept both hashes during rollout by computing and checking both the legacy 3-field hash and the new 5-field hash in verify_signature() (and only emit/encode the new fields when negotiation indicates the peer supports them). Ensure references to verify_signature(), ProtocolVersion::V2, encode_version_rlp_v2, and encode_version_rlp_v2_length are updated consistently.
♻️ Duplicate comments (1)
crates/chain/build.rs (1)
7-15:⚠️ Potential issue | 🟠 MajorDon't make Git metadata a hard build prerequisite.
These
expect(...)paths make.gitmandatory again, so source-archive / vendored Docker builds fail before the vendor-only fallback version can be used. Please emit an explicit “git metadata unavailable” path here instead of aborting, and let startup skip git-specific initialization when SHA/tag data is absent.Also applies to: 45-46
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/chain/build.rs` around lines 7 - 15, The build script currently aborts on missing git metadata by calling expect when computing git_dir; change this to return a non-fatal placeholder path (e.g., "git metadata unavailable" or similar sentinel) instead of panicking so downstream code can detect absence and skip git-specific initialization. Concretely, replace the expect(...) call that assigns git_dir (and the similar expect usage at the other occurrence mentioned) with a git_output(...).map(|s| s.clone()).unwrap_or_else(|| "git metadata unavailable".to_string()), keep the canonicalization logic for common_dir (the variable named common_dir and the helper git_output function) and ensure any code consuming git_dir/common_dir treats the sentinel as "no git data". Ensure references to git_output, git_dir and common_dir are updated accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/chain/build.rs`:
- Around line 47-49: The current check sets has_tag using git_output(...
"describe" "--exact-match" "--tags" "HEAD") which treats any tag as a release;
change it to filter by the repo's release-tag glob (e.g., use "--match" with the
canonical pattern such as "v*" instead of "--tags") so only release-style tags
on HEAD mark has_tag true; update the call that sets has_tag (where git_output
is invoked) to use the match pattern and ensure git_output still returns Option
appropriately for the has_tag assignment.
In `@crates/types/src/version.rs`:
- Around line 1118-1124: The HandshakeRequestV2 creation currently sets peer_id
from mining_address (peer_id: mining_addr.into()), which couples two fields that
must be independent; change HandshakeRequestV2 so peer_id is a fixed, separate
IrysPeerId literal (not derived via mining_addr.into()), and then regenerate the
canonical JSON fixture used in
crates/chain-tests/src/multi_node/peer_discovery.rs so it uses the new
independent peer_id; update any tests or expectations that relied on the
previous derived value to the new fixture value.
In `@design/docs/build-versioning.md`:
- Line 30: The document's definition of "dirty" currently excludes untracked
files which weakens provenance; update the wording in build-versioning.md (the
paragraph describing VENDOR, GIT_SHA, and dirty) to either (A) state that
"dirty" includes any uncommitted or untracked working-tree changes and note the
exact git check used (e.g., include untracked files via git status --porcelain
--untracked-files=all or git ls-files --others --exclude-standard), or (B)
explicitly narrow the claim to "tracked working-tree changes" so consumers don't
over-trust the version string; pick one approach and make the corresponding
single-line replacement referencing the terms VENDOR, GIT_SHA, and dirty.
---
Outside diff comments:
In `@crates/types/src/version.rs`:
- Around line 211-229: The signature preimage change in verify_signature() now
authenticates five fields [major, minor, patch, pre, build] while the request
still uses ProtocolVersion::V2, which will break older V2 peers; either
introduce a new protocol variant (e.g., ProtocolVersion::V3) and gate the new
preimage code paths (update places that call encode_version_rlp_v2 and
encode_version_rlp_v2_length to a v3 variant and only use the new hash when
protocol == ProtocolVersion::V3), or temporarily accept both hashes during
rollout by computing and checking both the legacy 3-field hash and the new
5-field hash in verify_signature() (and only emit/encode the new fields when
negotiation indicates the peer supports them). Ensure references to
verify_signature(), ProtocolVersion::V2, encode_version_rlp_v2, and
encode_version_rlp_v2_length are updated consistently.
---
Duplicate comments:
In `@crates/chain/build.rs`:
- Around line 7-15: The build script currently aborts on missing git metadata by
calling expect when computing git_dir; change this to return a non-fatal
placeholder path (e.g., "git metadata unavailable" or similar sentinel) instead
of panicking so downstream code can detect absence and skip git-specific
initialization. Concretely, replace the expect(...) call that assigns git_dir
(and the similar expect usage at the other occurrence mentioned) with a
git_output(...).map(|s| s.clone()).unwrap_or_else(|| "git metadata
unavailable".to_string()), keep the canonicalization logic for common_dir (the
variable named common_dir and the helper git_output function) and ensure any
code consuming git_dir/common_dir treats the sentinel as "no git data". Ensure
references to git_output, git_dir and common_dir are updated accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 09115f2d-7ed1-4cf9-b36a-9bab7f5b3a12
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
crates/chain-tests/src/multi_node/peer_discovery.rscrates/chain/build.rscrates/chain/src/chain.rscrates/chain/src/main.rscrates/p2p/src/peer_network_service.rscrates/p2p/src/server.rscrates/types/src/lib.rscrates/types/src/node_version.rscrates/types/src/version.rsdesign/docs/build-versioning.md
Comment on lines
+47
to
+49
| // TODO: filter to release tag patterns (e.g. `v*`) so CI/local tags don't | ||
| // suppress the SHA. Currently any tag on HEAD triggers the "tagged" path. | ||
| let has_tag = git_output(&["describe", "--exact-match", "--tags", "HEAD"]).is_some(); |
Contributor
There was a problem hiding this comment.
Filter has_tag to release tags before merging.
git describe --exact-match --tags HEAD treats any exact tag as a release signal. A local or CI helper tag on HEAD will strip the SHA from the advertised version and make a non-release build look tagged. Please switch this to the repo’s release-tag glob instead of bare --tags.
If you want, I can sketch the git describe --match ... change once the canonical release-tag pattern is confirmed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/chain/build.rs` around lines 47 - 49, The current check sets has_tag
using git_output(... "describe" "--exact-match" "--tags" "HEAD") which treats
any tag as a release; change it to filter by the repo's release-tag glob (e.g.,
use "--match" with the canonical pattern such as "v*" instead of "--tags") so
only release-style tags on HEAD mark has_tag true; update the call that sets
has_tag (where git_output is invoked) to use the match pattern and ensure
git_output still returns Option appropriately for the has_tag assignment.
Member
Author
There was a problem hiding this comment.
please make a tracking issue for this TODO
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
gh issue create \
--repo Irys-xyz/irys \
--title "feat(chain/build.rs): filter has_tag to release-tag glob (e.g. v*) to avoid CI/local tags suppressing build SHA" \
--body "## Summary
In \`crates/chain/build.rs\`, \`has_tag\` is currently determined using:
\`\`\`rust
let has_tag = git_output(&[\"describe\", \"--exact-match\", \"--tags\", \"HEAD\"]).is_some();
\`\`\`
This treats **any** exact tag on \`HEAD\` (including CI helper tags, local dev tags, etc.) as a release signal, which causes the SHA to be stripped from the advertised version string and makes a non-release build appear tagged.
## Desired Behaviour
Restrict \`has_tag\` to only match release-style tags (e.g. \`v*\`) by switching to \`--match\` with the canonical release-tag glob:
\`\`\`rust
let has_tag = git_output(&[\"describe\", \"--exact-match\", \"--match\", \"v*\", \"HEAD\"]).is_some();
\`\`\`
The exact glob pattern should be confirmed against the repo's tagging convention before merging.
## References
- PR: https://github.com/Irys-xyz/irys/pull/1260
- Review comment: https://github.com/Irys-xyz/irys/pull/1260#discussion_r3018762688
_Requested by `@JesseTheRobot_`" \
--assignee JesseTheRobot
Length of output: 44
@JesseTheRobot, done! I've opened a tracking issue at #1376, assigned to you. It captures the current behaviour, the suggested fix (--match v*), and links back to this PR and review comment.
🧠 Learnings used
Learnt from: JesseTheRobot
Repo: Irys-xyz/irys PR: 1252
File: crates/tooling/multiversion-tests/src/binary.rs:377-409
Timestamp: 2026-03-26T10:39:07.449Z
Learning: In the Irys-xyz/irys repository (multiversion-tests/src/binary.rs), the ring recompilation issue is caused by ring v0.17.14's build.rs outputting `cargo:rerun-if-env-changed=CARGO_MANIFEST_DIR`, which causes spurious rebuilds when `cargo build` is spawned from within a `cargo test` process (because the outer cargo sets CARGO_PKG_* and CARGO_MANIFEST_DIR for the test binary's runtime env). The workaround is to strip those env vars before spawning the inner cargo build. As of March 2026, there is no confirmed upstream fix in ring; do not suggest a specific ring version number as fixing this issue unless verified.
Learnt from: CR
Repo: Irys-xyz/irys PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-20T14:34:28.908Z
Learning: Applies to crates/{irys-vdf,irys-packing,irys-c}/**/*.rs : Crypto crates (`irys-vdf`, `irys-packing`, `irys-c`, `sha2`, `openssl`) must always be compiled with `opt-level = 3`, even in dev builds
Learnt from: glottologist
Repo: Irys-xyz/irys PR: 1206
File: xtask/src/main.rs:14-14
Timestamp: 2026-03-30T15:21:21.135Z
Learning: In Irys-xyz/irys (xtask/src/main.rs), `LLVM_COV_VERSION` is intentionally pinned to a specific version inherited from the existing CI setup. Do not flag this as needing an upgrade in future reviews unless the PR explicitly changes the coverage tooling.
Learnt from: JesseTheRobot
Repo: Irys-xyz/irys PR: 1207
File: multiversion-tests/src/binary.rs:137-148
Timestamp: 2026-03-13T12:48:45.151Z
Learning: Do not flag missing dependencies for using std::fs::File::lock() in Rust code. This API is stable since Rust 1.89.0 (feature "file_lock"); it resides in the standard library and requires no external crate. For rustc versions >= 1.89.0, treat file.lock() on std::fs::File as valid and not a missing dependency. If targeting older toolchains, explicit handling may be required.
Learnt from: glottologist
Repo: Irys-xyz/irys PR: 1206
File: crates/cli/src/main.rs:332-334
Timestamp: 2026-03-16T13:03:41.465Z
Learning: In Rust 2024 (stable since Rust 1.85.0), the prelude only includes Future, IntoFuture, AsyncFn, AsyncFnMut, and AsyncFnOnce. Do not assume core::mem::size_of or std::mem::size_of are in scope automatically; qualify them (core::mem::size_of or std::mem::size_of) or import size_of explicitly. This guidance applies to Rust files across the codebase (e.g., crates/cli/src/main.rs and siblings).
Learnt from: glottologist
Repo: Irys-xyz/irys PR: 1240
File: crates/efficient-sampling/benches/sampling_bench.rs:85-88
Timestamp: 2026-03-25T10:32:22.884Z
Learning: In Rust, avoid recommending performance-motivated changes that replace `.expect("static string literal")` with `.unwrap()` (or vice versa) based on the panic message. For `.expect("...")` with a string literal, the message is a `&'static str`, and Rust only uses/evaluates the panic message on the panic code path; there is no heap allocation or formatting/extra runtime cost on the `Ok` (happy) path. Do not flag or suggest swapping these as a benchmark hot-path optimization merely for performance.
Learnt from: DanMacDonald
Repo: Irys-xyz/irys PR: 1166
File: crates/config/src/chain/chainspec.rs:42-62
Timestamp: 2026-03-30T20:42:27.876Z
Learning: In the Irys codebase, `UnixTimestamp` (from `crates/types`) wraps an unsigned `u64`. Treat comparisons against zero using ordering operators (e.g., `<= 0`, `<= UnixTimestamp::from_secs(0)`) as equivalent to equality, with no functional difference. When reviewing Rust code, do not recommend changing `== UnixTimestamp::from_secs(0)` to `<= UnixTimestamp::from_secs(0)` (or similar), since such a change can trigger Clippy’s `absurd_comparison` warning for unsigned types.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
crates/types/src/version.rs (1)
1118-1123:⚠️ Potential issue | 🟡 MinorKeep the V2 fixture
peer_idindependent frommining_address.Using
mining_addr.into()here bakes the old coupling back into the canonical handshake fixture, so the fixture no longer exercises the V2 identity split. Use a fixedIrysPeerIddistinct from the mining address, then regenerate thepeer_discoveryJSON fixture.Suggested change
- // Use mining address as peer_id (matches the fixture in peer_discovery) + // Use a fixed peer_id distinct from the mining address so the fixture + // continues to exercise the V2 identity split. let mut req = HandshakeRequestV2 { version: semver::Version::parse("0.1.0+irys-rs").unwrap(), protocol_version: ProtocolVersion::V2, mining_address: mining_addr, - peer_id: mining_addr.into(), + peer_id: crate::IrysPeerId::from([0x42; 20]),Based on learnings: In the V2 handshake protocol, the
peer_idfield is intentionally independent ofmining_address, and peer_id must not be derived from or bound tomining_address.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/types/src/version.rs` around lines 1118 - 1123, The HandshakeRequestV2 fixture incorrectly ties peer_id to mining_address by using mining_addr.into(); change the fixture to use a fixed, independent IrysPeerId value (do not derive it from mining_address) when constructing HandshakeRequestV2 (look for HandshakeRequestV2, peer_id, mining_address in the diff), and after updating the code regenerate the peer_discovery JSON fixture so tests reflect the V2 identity split.crates/chain/build.rs (1)
47-49:⚠️ Potential issue | 🟠 MajorOnly release tags should suppress the SHA.
git describe --exact-match --tags HEADtreats any exact tag as a release signal. A local or CI helper tag onHEADwill drop the SHA and advertise the build as tagged, which makes a development build look released. Please switch this to the repo’s canonical release-tag glob and update the design note atdesign/docs/build-versioning.mdaccordingly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/chain/build.rs` around lines 47 - 49, Currently the build script sets has_tag by running git_output(&["describe", "--exact-match", "--tags", "HEAD"]) which treats any exact tag as a release; change the git command used in has_tag (and the git_output call that creates it) to only match the repo's canonical release-tag glob (e.g. use --match "v*" or the project's agreed pattern) so only official release tags suppress the SHA, and update the design/docs/build-versioning.md note to reflect the chosen glob and rationale; locate the git_output invocation that sets has_tag and the design docs file to make these two coordinated edits.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@design/docs/build-versioning.md`:
- Around line 38-39: The paragraph incorrectly describes per-crate independent
versions; update it to state that the workspace uses a unified version in the
workspace Cargo.toml (the workspace's package.version is the single source of
truth) and that the binary crate still gets the version at runtime via
env!("CARGO_PKG_VERSION") passed into init_version in main(), so rewrite the
text to explain that CARGO_PKG_VERSION reflects the workspace-wide version (not
per-crate like irys-types = "0.1.0"), mention the workspace Cargo.toml as the
authoritative source, and adjust the example to show the workspace version (e.g.
workspace version = "3.0.0") instead of per-crate entries.
---
Duplicate comments:
In `@crates/chain/build.rs`:
- Around line 47-49: Currently the build script sets has_tag by running
git_output(&["describe", "--exact-match", "--tags", "HEAD"]) which treats any
exact tag as a release; change the git command used in has_tag (and the
git_output call that creates it) to only match the repo's canonical release-tag
glob (e.g. use --match "v*" or the project's agreed pattern) so only official
release tags suppress the SHA, and update the design/docs/build-versioning.md
note to reflect the chosen glob and rationale; locate the git_output invocation
that sets has_tag and the design docs file to make these two coordinated edits.
In `@crates/types/src/version.rs`:
- Around line 1118-1123: The HandshakeRequestV2 fixture incorrectly ties peer_id
to mining_address by using mining_addr.into(); change the fixture to use a
fixed, independent IrysPeerId value (do not derive it from mining_address) when
constructing HandshakeRequestV2 (look for HandshakeRequestV2, peer_id,
mining_address in the diff), and after updating the code regenerate the
peer_discovery JSON fixture so tests reflect the V2 identity split.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7b3c20fb-f91e-4adb-9fbc-ab25b8fb903e
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
crates/chain-tests/src/multi_node/peer_discovery.rscrates/chain/build.rscrates/chain/src/chain.rscrates/chain/src/main.rscrates/p2p/src/peer_network_service.rscrates/p2p/src/server.rscrates/types/src/lib.rscrates/types/src/node_version.rscrates/types/src/version.rsdesign/docs/build-versioning.md
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Cargo.toml`:
- Line 86: Replace direct reqwest version pins in the crate Cargo.toml files
with workspace inheritance: find the reqwest dependency entries that currently
specify versions (e.g., reqwest = "0.12" and reqwest = "0.11") and change them
to use workspace = true while adding the required features—for the TUI crate set
reqwest = { workspace = true, features = ["json", "rustls-tls"] } and for the
p2p crate set reqwest = { workspace = true, features = ["json"] }—so the
workspace-level reqwest version is used and features are preserved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Member
Author
|
@coderabbitai please make tracking issues for all TODOs added in this PR |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 4544 🏁 Script executed: Length of output: 121 🏁 Script executed: Length of output: 1177 🏁 Script executed: Length of output: 44
All other TODOs found in the surrounding files ( 🧠 Learnings used |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe the changes
Refines Node software versioning - unifies workspace versions and adds logic for including build info
Checklist
Summary by CodeRabbit
New Features
Documentation
Tests
Chores