Skip to content

Fix install dry-run side effects#6348

Open
Thanhdn1984 wants to merge 2 commits into
Scottcjn:mainfrom
Thanhdn1984:fix-install-dry-run-no-side-effects
Open

Fix install dry-run side effects#6348
Thanhdn1984 wants to merge 2 commits into
Scottcjn:mainfrom
Thanhdn1984:fix-install-dry-run-no-side-effects

Conversation

@Thanhdn1984
Copy link
Copy Markdown
Contributor

Fixes #6346.\n\nThis moves the dry-run handling to immediately after argument parsing, so bash install.sh --dry-run exits before GPU/Python checks, pip installs, wallet prompts, mkdir/downloads, or other side effects.\n\nValidation:\n- bash -n install.sh\n- timeout 5 bash install.sh --dry-run (prints preview and exits without prompting)

@github-actions
Copy link
Copy Markdown
Contributor

Welcome to RustChain! Thanks for your first pull request.

Before we review, please make sure:

  • Non-doc PRs have a BCOS-L1 or BCOS-L2 label
  • Doc-only PRs are exempt from BCOS tier labels when they only touch docs/**, *.md, or common image/PDF files
  • 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 tests Test suite changes size/M PR: 51-200 lines labels May 26, 2026
Copy link
Copy Markdown

@MolhamHamwi MolhamHamwi left a comment

Choose a reason for hiding this comment

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

I reviewed install.sh in this PR and ran bash -n install.sh plus bash install.sh --dry-run locally on macOS; the early dry-run path now exits cleanly before the installer reaches prompts/downloads.

Two technical observations:

  1. The dry-run fix duplicates the OS-to-miner/fingerprint mapping at lines 73-91 while the real install path keeps the same mapping again later around lines 124-137. That makes the behavior correct today, but it creates a drift risk: adding/changing a platform or miner path later would need to update both blocks or --dry-run could preview a different artifact than the real installer downloads. A small helper/shared case block would make this safer.

  2. The PR contains an unrelated first commit touching node/ergo_raw_tx.py and tests/test_ergo_raw_tx.py, while the PR title/body only describe install.sh --dry-run. Even though the Ergo JSON hardening looks reasonable, merging it together with the installer fix makes review/rollback harder and could accidentally close the dry-run issue with extra behavior bundled in.

I received RTC compensation for this review.

Copy link
Copy Markdown

@shadow88sky shadow88sky left a comment

Choose a reason for hiding this comment

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

Reviewed the installer dry-run change plus the bundled Ergo raw-tx hardening commit.

Validation performed locally:

bash install.sh --dry-run
# exits 0 and prints OS/architecture/miner/fingerprint/checksum/wallet/node/silent plan without prompts or downloads

PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 .venv/bin/python -m pytest tests/test_ergo_raw_tx.py -q
12 passed

.venv/bin/python -m py_compile node/ergo_raw_tx.py tests/test_ergo_raw_tx.py
passed

Two concrete observations:

  1. The installer fix is in the right place for the stated issue: the new early DRY_RUN branch runs immediately after argument parsing, before the banner, prompts, install-directory creation, or download path. That avoids the side effects the old late dry-run check could still trigger.

  2. The Ergo JSON parsing hardening is also reasonable on its own. response_json_object() / response_json_list() prevent non-object or non-list API responses from leaking into wallet box, node height, and signing logic, and the new tests cover malformed unspent-box responses plus non-object signed transactions.

Non-blocking cleanup suggestion: the PR title only mentions install dry-run side effects, while the diff also contains node/ergo_raw_tx.py and tests/test_ergo_raw_tx.py. If this is intentional, retitling the PR to mention both changes would make review/merge history clearer; otherwise the Ergo commit would be cleaner as a separate PR.

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.

Great work! Thanks for contributing to RustChain! 🦀

Copy link
Copy Markdown
Contributor

@minyanyi minyanyi left a comment

Choose a reason for hiding this comment

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

Reviewed the install dry-run change and the Ergo raw transaction response hardening.

Validation performed:

  • python -B -m pytest -q tests/test_ergo_raw_tx.py --tb=short -> 12 passed, 1 warning
  • python -B -m py_compile node/ergo_raw_tx.py tests/test_ergo_raw_tx.py -> passed
  • git diff --check origin/main...HEAD -> passed
  • bash -n install.sh -> passed
  • bash install.sh --dry-run --wallet test-wallet on Windows/MSYS exits as unsupported OS before side effects, which is consistent with the script's Linux/Darwin support gate

No blocking issues found. 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.

Great work! Thanks for contributing 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 work! Thanks for contributing to RustChain! 🦀

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 tests Test suite changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] install.sh --dry-run executes side effects before checking dry-run flag

6 participants