Skip to content

fix: avoid key persistence during miner dry-run#6856

Open
vicentsmith470-web wants to merge 1 commit into
Scottcjn:mainfrom
vicentsmith470-web:vicentsmith/fix-dry-run-key-persistence
Open

fix: avoid key persistence during miner dry-run#6856
vicentsmith470-web wants to merge 1 commit into
Scottcjn:mainfrom
vicentsmith470-web:vicentsmith/fix-dry-run-key-persistence

Conversation

@vicentsmith470-web
Copy link
Copy Markdown
Contributor

Summary

Fixes #6854 by making --dry-run use an ephemeral in-memory miner signing key instead of creating/updating ~/.rustchain/miner_key.json before the preflight output.

What changed

  • Added a persist_key constructor option to LocalMiner.
  • Normal mining keeps the existing persisted get_or_create_keypair() behavior.
  • CLI dry-run now constructs LocalMiner(..., persist_key=False), which uses generate_keypair() without saving a keystore.
  • Kept the existing Linux miner implementation intact: hardware probes, fingerprint checks, node health probe, attestation/enrollment/mining flow, Warthog sidecar wiring, and executable file mode are preserved.
  • Added focused tests for ephemeral key use and CLI dry-run vs normal-mode persistence behavior.

Validation

  • python -m py_compile miners/linux/rustchain_linux_miner.py
  • python -m pytest -q tests/test_linux_miner_identity.py tests/test_linux_miner_cli_help.py tests/test_linux_miner_network_retry.py tests/test_miner_hardware_probes.py tests/test_miner_balance_endpoints.py -> 19 passed
  • git diff --check

Bounty / payout

This is a focused bug fix for #6854. If accepted as a RustChain contribution bounty, please reserve payout to github:vicentsmith470-web until wallet linking/claim instructions are available. I can provide a public RTC wallet address if required.

@github-actions github-actions Bot added BCOS-L1 Beacon Certified Open Source tier BCOS-L1 (required for non-doc PRs) tests Test suite changes size/M PR: 51-200 lines labels Jun 4, 2026
@vicentsmith470-web vicentsmith470-web force-pushed the vicentsmith/fix-dry-run-key-persistence branch from 7180f25 to 3821251 Compare June 4, 2026 21:10
@vicentsmith470-web
Copy link
Copy Markdown
Contributor Author

CI follow-up: updated the pinned Linux miner SHA256 in miners/checksums.sha256 and setup_miner.py after the first CI run correctly caught the changed miner artifact hash.

Additional local validation now passing:

  • python -m pytest -q tests/test_install_miner_checksums.py tests/test_setup_miner_downloads.py tests/test_linux_miner_identity.py tests/test_linux_miner_cli_help.py tests/test_linux_miner_network_retry.py tests/test_miner_hardware_probes.py tests/test_miner_balance_endpoints.py -> 24 passed

Copy link
Copy Markdown
Contributor

@zqleslie zqleslie left a comment

Choose a reason for hiding this comment

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

Good focused fix. The persist_key parameter correctly isolates dry-run key generation from the persistent keystore. Two observations:

  1. Line 867 (indentation fix): The original had inconsistent indentation on verbose and show_payload (extra indent). This PR normalizes it — nice catch, but worth noting the diff shows both real logic change and whitespace cleanup in the same commit. Consider separating the whitespace-only changes into their own commit for easier review.

  2. Line 21 — generate_keypair fallback on ImportError: When miner_crypto is unavailable, generate_keypair gets set to None. However, when persist_key=False and CRYPTO_AVAILABLE is False, the __init__ at line 224 checks if CRYPTO_AVAILABLE: which is already False, so it skips the entire key generation block. The None assignment is unreachable in practice but could be confusing — consider a comment or assert to document why the fallback is harmless here.

  3. Test coverage is solid: The three new tests (test_local_miner_can_use_ephemeral_keypair_without_persisting, test_main_dry_run_disables_key_persistence, test_main_normal_mode_keeps_key_persistence) cover the critical paths well. The monkeypatch approach correctly validates that get_or_create_keypair is never called when persist_key=False.

Verdict: This is a clean, minimal fix with appropriate test coverage. The behavioral change (ephemeral vs persisted keys in dry-run) is exactly what issue #6854 requested.

I received RTC compensation for this review.

Copy link
Copy Markdown
Contributor

@jaxint jaxint left a comment

Choose a reason for hiding this comment

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

Excellent contribution to RustChain!

Copy link
Copy Markdown
Contributor

@jaxint jaxint left a comment

Choose a reason for hiding this comment

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

Great contribution! This looks good to me. 👍

@Scottcjn
Copy link
Copy Markdown
Owner

Scottcjn commented Jun 5, 2026

This is the fix we want for #6854 (ephemeral dry-run key, no functionality removed — preferred over the full-file-replacement #6855 which was closed). It now conflicts with main after #6839 (attestation signing) updated the linux miner + miners/checksums.sha256. Please rebase and regenerate the manifest:

git fetch origin && git rebase origin/main
cd miners && sha256sum linux/rustchain_linux_miner.py linux/fingerprint_checks.py macos/*.py > checksums.sha256
git commit -am 'rebase + regen checksums' && git push --force-with-lease

Once it's mergeable I'll land it. 🦞

@vicentsmith470-web vicentsmith470-web force-pushed the vicentsmith/fix-dry-run-key-persistence branch from 3821251 to 61ece10 Compare June 5, 2026 01:38
@vicentsmith470-web
Copy link
Copy Markdown
Contributor Author

Rebased on current origin/main after #6839 and regenerated the Linux miner SHA256 in both:

  • miners/checksums.sha256
  • setup_miner.py

The final Linux miner artifact hash is:

ca2931797a7a76fb7c9e61d69abdf75c57dab204ec77c2c5e9f23f323af63f9d

Validation after rebase:

python -m py_compile miners/linux/rustchain_linux_miner.py setup_miner.py
python -m pytest -q tests/test_install_miner_checksums.py tests/test_setup_miner_downloads.py tests/test_linux_miner_identity.py tests/test_linux_miner_cli_help.py tests/test_linux_miner_network_retry.py tests/test_miner_hardware_probes.py tests/test_miner_balance_endpoints.py
24 passed
git diff --check

No scope change beyond resolving the rebase/checksum conflict.

# Conflicts:
#	miners/checksums.sha256
#	setup_miner.py

# Conflicts:
#	miners/checksums.sha256
@vicentsmith470-web vicentsmith470-web force-pushed the vicentsmith/fix-dry-run-key-persistence branch from 61ece10 to 48bbc49 Compare June 5, 2026 01:44
@vicentsmith470-web
Copy link
Copy Markdown
Contributor Author

Follow-up: main advanced again while I was rebasing, so I rebased once more on the current base (61e1c180).

Resolved the remaining checksum manifest conflict with the final file hashes:

ca2931797a7a76fb7c9e61d69abdf75c57dab204ec77c2c5e9f23f323af63f9d  linux/rustchain_linux_miner.py
a8976c4c54ef8ae36e86dc2799ccc4335b93fa3850aa246a665abe71ee325f12  linux/fingerprint_checks.py

Validation rerun:

python -m pytest -q tests/test_install_miner_checksums.py tests/test_setup_miner_downloads.py tests/test_linux_miner_identity.py tests/test_linux_miner_cli_help.py tests/test_linux_miner_network_retry.py tests/test_miner_hardware_probes.py tests/test_miner_balance_endpoints.py
24 passed
git diff --check

Copy link
Copy Markdown
Contributor

@jaxint jaxint left a comment

Choose a reason for hiding this comment

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

Reviewed for RTC bounty

Copy link
Copy Markdown
Contributor

@jaxint jaxint left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! 🎉

Copy link
Copy Markdown
Contributor

@jaxint jaxint left a comment

Choose a reason for hiding this comment

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

Great work! 👍

Copy link
Copy Markdown
Contributor

@JesusMP22 JesusMP22 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: Avoid key persistence during miner dry-run

Summary: Prevents sensitive key material from being persisted to disk during miner dry-run operations, reducing the attack surface.

What I like:

  • Security best practice: minimize key persistence
  • Dry-run mode should never write sensitive data to disk

Suggestions:

  1. Verify that no key material ends up in logs during dry-run either
  2. Consider using memory-only storage for dry-run keys (e.g., tmpfs)
  3. Add a test that verifies no key files are created during dry-run
  4. Document which operations are safe in dry-run vs require real execution

Security considerations:

  • ✅ Positive security impact: reduces key exposure
  • Consider whether swap files could still leak key material
  • Ensure dry-run mode is clearly distinguishable in monitoring

Verdict: ✅ Good security improvement. Minimizing key persistence is always worthwhile.

Copy link
Copy Markdown
Contributor

@jaxint jaxint left a comment

Choose a reason for hiding this comment

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

Great work! Thanks for contributing.

Copy link
Copy Markdown
Contributor

@jaxint jaxint left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! 🎉

Copy link
Copy Markdown
Contributor

@jaxint jaxint left a comment

Choose a reason for hiding this comment

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

Thanks for this contribution! Great work.

Copy link
Copy Markdown
Contributor

@jaxint jaxint left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

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) size/M PR: 51-200 lines tests Test suite changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Dry-run miner creates local keypair despite no-state-change messaging

5 participants