Fix rustchain-wallet dependency compatibility#5586
Conversation
|
Welcome to RustChain! Thanks for your first pull request. Before we review, please make sure:
Bounty tiers: Micro (1-10 RTC) | Standard (20-50) | Major (75-100) | Critical (100-150) A maintainer will review your PR soon. Thanks for contributing! |
github-actions
Bot
added
BCOS-L1
iamdinhthuan
left a comment
iamdinhthuan
left a comment
There was a problem hiding this comment.
Reviewed PR #5586 at head f28c4cb7d3a65d420dd5bf653affd278e6450ec6.
This is a focused rustchain-wallet dependency compatibility fix. Pinning sha2 back to 0.10, hmac to 0.12, and getrandom to 0.2 matches the APIs used by the current wallet code: getrandom::getrandom, pbkdf2_hmac::<Sha256/Sha512>, and HmacSha512::new_from_slice compile cleanly again on fresh resolves. The rustdoc comment edits also remove the bracketed slice notation that trips docs when warnings are denied.
Local validation in a PR worktree under rustchain-wallet/:
cargo test --all-targets
# lib tests: 53 passed; integration tests: 16 passed; examples: 0-test binaries OK
cargo clippy --all-targets -- -D warnings
RUSTDOCFLAGS='-D warnings' cargo doc --no-deps
git diff --check origin/main...HEAD -- rustchain-wallet
Live CI also shows the Rust-specific jobs passing on this head: Rustfmt, Clippy, Documentation, Security Audit, Test on Ubuntu/macOS/Windows, and Build on Ubuntu/macOS/Windows. The remaining repo-wide Python test failure is the existing broad-suite baseline, not a rustchain-wallet dependency failure.
No blocker from me on this focused compatibility fix.
jaxint
left a comment
jaxint
left a comment
There was a problem hiding this comment.
LGTM! Great work on this PR. 🚀
TJCurnutte
left a comment
TJCurnutte
left a comment
There was a problem hiding this comment.
Validated the rustchain-wallet dependency compatibility patch at f28c4cb7d3a65d420dd5bf653affd278e6450ec6 and this is mergeable from the wallet crate side.
Checks run:
git diff --check origin/main...HEAD -- rustchain-wallet/Cargo.toml rustchain-wallet/src/keys.rs rustchain-wallet/src/lib.rspassed.- Source probe confirmed the intended pins are present:
sha2 = "0.10",hmac = "0.12",getrandom = "0.2", withpbkdf2 = "0.12"unchanged. cargo test --manifest-path rustchain-wallet/Cargo.toml --all-targetspassed.RUSTDOCFLAGS='-D warnings' cargo doc --manifest-path rustchain-wallet/Cargo.toml --no-depspassed and generated the crate docs.cargo clippy --manifest-path rustchain-wallet/Cargo.toml --all-targets -- -D warningspassed.
The cargo resolve during validation selected the expected compatible stack (sha2 0.10.9, hmac 0.12.1, getrandom 0.2.17, pbkdf2 0.12.2), and the existing key derivation/address code compiles cleanly against it. The rustdoc wording changes also avoid slice-link style ambiguity under denied doc warnings.
kekehanshujun
left a comment
kekehanshujun
left a comment
There was a problem hiding this comment.
Requesting changes because the compatibility fix is incomplete for the crate's declared MSRV.
rustchain-wallet/Cargo.toml declares rust-version = 1.70. This PR downgrades sha2, hmac, and getrandom, but it leaves rand = 0.10. I checked crates.io: rand 0.10.1 declares rust_version = 1.85, so the package can still select a dependency that is incompatible with Rust 1.70. That means the branch can still fail for users/toolchains respecting the crate's declared MSRV.
Please either update the wallet crate's rust-version to the actual supported compiler or pin/downgrade the remaining MSRV-incompatible dependencies such as rand and add a lockfile/MSRV validation note.
iamdinhthuan
left a comment
iamdinhthuan
left a comment
There was a problem hiding this comment.
Updating my earlier approval after checking the crate against its declared MSRV.
rustchain-wallet/Cargo.toml declares rust-version = "1.70", but this head still resolves dependencies that Cargo/Rust 1.70 cannot use. In a fresh checkout of head f28c4cb7d3a65d420dd5bf653affd278e6450ec6:
cargo check --lockedwith the current toolchain finishes successfully.cargo +1.70.0 check --lockedfails during dependency resolution:
error: failed to select a version for `rand`.
...
the package `rustchain-wallet` depends on `rand`, with features: `getrandom` but `rand` does not have these features.
It has an optional dependency with that name, but that dependency uses the "dep:" syntax in the features table, so it does not have an implicit feature with that name.
cargo metadata --locked also shows several resolved packages above the crate's MSRV, including rand 0.10.1 / rand_core 0.10.1 requiring Rust 1.85, clap 4.6.1 requiring Rust 1.85, tokio 1.52.3 requiring Rust 1.71, and ed25519-dalek 2.2.0 requiring Rust 1.81.
So the PR does not yet restore compatibility for the crate as declared. Please either pin/downgrade the remaining direct dependency ranges to versions compatible with Rust 1.70, commit a lockfile/CI path that enforces the intended compatible resolution, or raise rust-version if the project no longer intends to support 1.70.
6 participants
Summary
rustchain-walletcrypto dependencies back to the digest 0.10-compatible stack used by the current codegetrandom::getrandom,pbkdf2_hmac::<Sha256/Sha512>, andHmacSha512::new_from_slicecompiling on fresh CI resolvesWhy
Fresh CI resolves currently pull
sha2 0.11,hmac 0.13, andgetrandom 0.4, which breaksrustchain-walletbuilds before unrelated PRs can get green checks. This is the same failure currently visible on PRs such as #4623.Validation
cargo test --all-targetscargo clippy --all-targets -- -D warnings$env:RUSTDOCFLAGS='-D warnings'; cargo doc --no-deps