Skip to content

fix(Makefile): prevent NUM_VALIDATORS from evaluating to two tokens - #339

Closed
Ocheretovich wants to merge 1 commit into
circlefin:mainfrom
Ocheretovich:patch-1
Closed

Ocheretovich wants to merge 1 commit into
circlefin:mainfrom
Ocheretovich:patch-1

Conversation

@Ocheretovich

@Ocheretovich Ocheretovich commented Sep 4, 2026 •

Copy link
Copy Markdown

Fixed a bug in NUM_VALIDATORS: when the manifest had no [nodes.validator...] entries, grep -c returned 0 but exited with status 1, which triggered the || echo 5 fallback resulting in the variable evaluating to "0 5" instead of a single number. The check is now explicit: fall back to 5 only when grep's count is missing or zero, otherwise use the actual count.

I tested the old and new logic against three cases missing manifest, manifest with zero validators, and manifest with 3 validators. The first and third cases behave identically either way, but the middle one exposes the bug: the old logic returned "0\n5" (two values instead of one) when the manifest existed but had no validator entries, while the fix correctly returns 5.

@osr21

osr21 commented Sep 11, 2026 •

Copy link
Copy Markdown

Disclosure: I’m an external community contributor, not affiliated with Circle, not a maintainer, and I have no write access to this repository.

This is advisory review only.\n\nI independently re-ran the old and new Makefile expressions against the three relevant manifest states, plus a missing-file control:\n\n| manifest | old expression | this PR |\n|---|---:|---:|\n| missing file | 5 | 5 |\n| existing file with no validator sections | 0 5 | 5 |\n| existing file with three validator sections | 3 | 3 |\n\nThe reported bug is real, and the patch fixes the actual failure without changing the existing fallback policy. The important detail is that GNU grep -c prints 0 but exits with status 1 when there are no matches; the old || echo 5 therefore appended a second value.

The revised shell conditional treats a positive count as authoritative and falls back only for an empty/zero/non-numeric result.\n\nThe shell syntax is POSIX-compatible (make invokes /bin/sh here), and QUAKE_MANIFEST remains recursively expanded, so targets that override the manifest still recompute NUM_VALIDATORS as intended. I also checked the current Makefile usage: the value is passed as the single --num-validators argument to the genesis command, so eliminating the second token directly removes the malformed invocation.\n\nI did not find a correctness issue in the three-line change. The three cases in the PR description are the right regression matrix; the missing-file case is a useful control because it confirms the fallback behavior was not accidentally changed. This looks ready from the code/behavior side, subject to the repository’s normal CI/maintainer review.

@ZhiyuCircle

Copy link
Copy Markdown
Contributor

Hi @Ocheretovich,

Thank you for your interest in contributing to Arc Node, and apologies for the delay in getting back to this PR.

We're closing out the pull request backlog that predates our current contribution policy. This PR is being closed because it does not reference a GitHub issue. All PRs must reference an existing issue using the format Closes: #XXX, and the author must be assigned to that issue before the PR is opened.

This is not a judgement on the change itself. If you'd still like to land it:

  1. Open an issue describing the problem, or find the existing one
  2. Comment on the issue requesting assignment, and wait for maintainer approval
  3. Open a fresh PR once you have been assigned

Please see CONTRIBUTING.md for details. Thanks again for taking the time to contribute.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants