Skip to content

[#2273 Item C] Add non-root key paths fallback#2305

Closed
sheerai wants to merge 3 commits into
Scottcjn:mainfrom
sheerai:fix-2273-item-c
Closed

[#2273 Item C] Add non-root key paths fallback#2305
sheerai wants to merge 3 commits into
Scottcjn:mainfrom
sheerai:fix-2273-item-c

Conversation

@sheerai
Copy link
Copy Markdown

@sheerai sheerai commented Apr 19, 2026

Fixes part of #2273

Claiming Item C. The LocalKeypair now successfully falls back to $RC_P2P_PRIVKEY_PATH and ~/.rustchain/p2p_identity.pem if /etc/rustchain is unwritable, allowing non-root execution.

UPDATE: I have also completed Item B. The get_pubkey method now validates not_before and not_after timestamps with the requested ±5 minute clock skew tolerance

GitHub Username: sheerai
RTC Wallet: RTCf3e03dba442d0140b78cf9b76068921e1badcd6b

@github-actions
Copy link
Copy Markdown
Contributor

Welcome to RustChain! Thanks for your first pull request.

Before we review, please make sure:

  • Your PR has a BCOS-L1 or BCOS-L2 label
  • New code files include an SPDX license header
  • You've tested your changes against the live node

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 github-actions Bot added BCOS-L1 Beacon Certified Open Source tier BCOS-L1 (required for non-doc PRs) node Node server related size/S PR: 11-50 lines labels Apr 19, 2026
@github-actions github-actions Bot added size/M PR: 51-200 lines and removed size/S PR: 11-50 lines labels Apr 19, 2026
Copy link
Copy Markdown
Contributor

@FlintLeng FlintLeng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

Non-root execution support + cert validation. ✅

Item C: Non-root key paths fallback

  • Falls back from /etc/rustchain → $RC_P2P_PRIVKEY_PATH → ~/.rustchain/p2p_identity.pem
  • Correct priority chain: system → env → home directory
  • Allows non-root execution without sacrificing security

Item B: Certificate clock skew validation

  • Validates not_before/not_after with ±5 min tolerance
  • Important for distributed systems with clock drift
  • Reasonable tolerance value

Concerns

  • 58 additions, 73 deletions — net reduction is good (cleaner code)
  • Should document the fallback path order in README or CONTRIBUTING.md

Good quality contribution. Recommended merge. ✅

Copy link
Copy Markdown

@fengqiankun6-sudo fengqiankun6-sudo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Key changes: (1) Improved P2P identity path resolution with RC_P2P_PRIVKEY_PATH env var and multi-candidate search - much more robust; (2) Simplifies expiry check using timedelta objects instead of raw timestamp arithmetic - cleaner and more readable. Good incremental improvement to the key management system.

Copy link
Copy Markdown
Contributor

@FlintLeng FlintLeng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review — PR #2305

Review: ✅ Good approach.

Summary:

  • Well-scoped change addressing the described issue
  • Code is clean and follows project conventions
  • No obvious issues found

Bounty: Claiming #2782 | 2 RTC
Wallet: RTC019e78d600fb3131c29d7ba80aba8fe644be426e

@Scottcjn
Copy link
Copy Markdown
Owner

Closing as superseded. @MichaelSovereign's #2296 landed Items A+B+C on Apr 19 and was paid 70 RTC (pid 1251, confirmed). Your PR was submitted Apr 21 — first-to-merge took the bounty. No hard feelings — your #2308 deadlock fix is independent and just got paid 12 RTC (pid 1269). Keep shipping.

@Scottcjn Scottcjn closed this Apr 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

BCOS-L1 Beacon Certified Open Source tier BCOS-L1 (required for non-doc PRs) node Node server related size/M PR: 51-200 lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants