bug(genesis): ValidatorManager accepts validator sets with no positive voting power #344

Activity

1. sanjeebdas1979

   added a commit that references this issue on Sep 4, 2026

   fix(genesis): require positive validator voting power

   ...

   9157e65
2. osr21 commented on Sep 4, 2026

   

   osr21

   on Sep 4, 2026

   More actions

   Disclosure: I'm not affiliated with Circle — an external community contributor, not a maintainer, with no write access. This is one contributor's technical assessment, advisory only; please defer to Circle maintainers.

   ---

   Both reported cases reproduce. Unlike most of this repo, this part is TypeScript, so I ran the real schema rather than reading it — npx tsx against scripts/genesis/ValidatorManager.ts at main, with a minimal otherwise-valid config:

   []        empty          ACCEPTED
   [0, 0]    all-zero       ACCEPTED
   [20]      positive only  ACCEPTED
   [0, 20]   mixed          ACCEPTED
   

   And your proposed fix works verbatim. Chaining exactly the snippet from the issue onto the exported schema and re-running the same four cases:

   []        empty     REJECTED  <- validators: At least one validator must have positive voting power
   [0, 0]    all-zero  REJECTED  <- validators: At least one validator must have positive voting power
   [20]      positive  ACCEPTED
   [0, 20]   mixed     ACCEPTED
   

   That matches your expected-behaviour table exactly. Two notes for whoever implements it:

   • The schema already terminates in a .superRefine(...) at ValidatorManager.ts:102 (uniqueness of public keys, controller uniqueness, proxy-admin separation). The check belongs inside that block rather than chained as a second one — chaining is how I tested it and it works, but one refine block is the file's convention.
   • votingPower is schemaBigInt, i.e. z.coerce.bigint(), so by the time the refinement runs it is a real bigint — I confirmed "20" coerces to 20n. So > 0n is safe and won't silently become a string comparison.

   One correction to the rationale — the downstream failure is loud, not silent

   The issue says the decoder "ignores validators with zero voting power" and that this "can leave consensus with no usable validators." The first half is right; the second overstates it.

   In crates/eth-engine/src/abi_utils.rs:72, abi_decode_validator_set does skip them:

   if cv.status != ContractValidatorStatus::Active || cv.votingPower == 0 {
       continue;
   }

   But it does not then hand consensus an empty set — it refuses:

   if validators.is_empty() {
       eyre::bail!("No active validators with valid public keys — validator set would be empty ...");
   }

   There is a test pinning that behaviour, with the comment "returns an error rather than producing an empty ValidatorSet."

   Why this still argues for your fix, just differently. The problem isn't silence, it's when and where the error lands. The CL doesn't read validators from genesis JSON — it reads the ValidatorRegistry contract at runtime via getActiveValidatorSetCall (ipc/ethereum_ipc.rs:173). So the chain of causation is: bad genesis config → registry storage allocs generated without complaint → genesis file built and distributed → node starts → hard failure on the first registry read. Catching it in safeParse moves a startup abort to config-authoring time, which is the right trade. That framing is more defensible than "consensus is left with no usable validators," and I'd rewrite that sentence before a maintainer pushes back on it.

   A detail that happens to confirm your predicate. The decoder filters on status != Active || votingPower == 0, which is two conditions, while your check tests only voting power. That's correct here rather than a gap: the genesis validator schema has no status field (only publicKey, votingPower, controllers), so every genesis-configured validator is Active by construction, and positive voting power is precisely the half that genesis controls.

   The proposed regression test would not run in CI

   Worth knowing before anyone relies on it. .github/workflows/ci.yml runs rust-fmt, rust-deps, rust-lint, rust-test, rust-integration-test, proto, contracts-lint, contracts-build, contracts-test, docker-build. The only Node steps anywhere are two npm ci dependency installs. Nothing invokes mocha, hardhat test, or npm test — and package.json's "test": "make smoke" isn't called by CI either. contracts-test runs forge test; rust-test runs cargo nextest.

   contracts-lint goes further and runs:

   npx eslint --ignore-pattern 'scripts/' --ignore-pattern 'tests/'

   So scripts/genesis/ — where this schema lives — is excluded even from linting. A TS regression test alongside the fix is still worth writing for local runs and as executable documentation, but it would guard nothing in CI, and it shouldn't be described as regression coverage without that caveat. If maintainers want the guarantee enforced, that's either a CI step that runs the genesis TS tests, or a Foundry/Rust-side assertion.

   Severity, honestly calibrated

   Narrower than it first looks. The supported path for authoring validators is AccountCreator, which defaults votingPower to 20 and constrains explicit values with z.array(z.number().min(1)) — so zero can't come through it, and the shipped assets/localdev and assets/mainnet configs can't produce either bad state. Exposure is limited to hand-authored genesis configs or direct schema use.

   So: a real gap and a correct, cheap, well-scoped fix — defence-in-depth against a config mistake rather than a live risk to any running chain. I'd support it landing, without urgency.

   Caveat on method: the TypeScript results above are executed output. The Rust side is source review only — no cargo/rustc in my environment — so I traced the decode path and its test rather than running it.
3. sanjeebdas1979 commented on Sep 4, 2026

   

   sanjeebdas1979

   on Sep 4, 2026

   ContributorAuthor

   More actions

   I reviewed your points against the current implementation and the downstream validator-set path.

   The core issue and the fix are sound: the genesis schema currently allows both an empty validator set and an all-zero voting-power set, while the effective validator set must contain at least one validator with positive voting power.

   The PR addresses exactly that invariant at the schema boundary. The check is intentionally set-level, so valid mixed configurations such as [0, 20] remain accepted, while [] and [0, 0] are rejected. It is also implemented inside the existing superRefine block, which matches the current schema structure.

   Your clarification on the downstream behavior is fair: the decoder does not silently continue with an empty validator set; it fails explicitly. That does not change the purpose of the fix - it strengthens the case for validating the invariant earlier, during genesis configuration, rather than allowing the failure to surface later during runtime validator-set decoding.

   I also confirmed that the TypeScript test is currently local regression coverage rather than CI-enforced coverage, since the existing workflow does not invoke these tests.

   So overall, the implementation remains correctly scoped as a fail-fast configuration correctness improvement, and the current PR behavior matches the intended validator-set invariant.
4. osr21 commented on Sep 4, 2026

   

   osr21

   on Sep 4, 2026

   More actions

   Disclosure: I'm not affiliated with Circle — an external community contributor, not a maintainer, with no write access. Advisory only; please defer to Circle maintainers.

   ---

   Thanks — and I checked #346 by running it rather than reading the description.

   Confirmed: the check sits at the top of the existing superRefine block, so the structure is as you describe. Your test file executes clean for me (mocha + tsx, no contract compilation):

   ValidatorManager genesis validator-set validation
     ✔ rejects an empty validator set
     ✔ rejects a validator set with no positive voting power
     ✔ accepts a validator set with positive voting power
     ✔ accepts zero-power validators when another validator has positive power
   
   4 passing (9ms)
   

   My own four cases against your branch agree. The set-level design is the right call — [0, 20] staying valid matters, since zero-power entries are legitimate registry state.

   Correcting myself on the CI point — it's better than I said

   I told you TypeScript tests aren't CI-enforced and you took that at face value; it's true but I under-researched it, and the accurate version is more useful to you.

   A make target for exactly this already exists:

   test-unit-hardhat: ## Run hardhat unit tests
   	npx hardhat test ./tests/helpers/matchers/index.test.ts ./tests/unit/*.test.ts --no-compile

   It globs ./tests/unit/*.test.ts, so your new file is already picked up with zero wiring — you placed it correctly, alongside the existing tests/unit/deployer-nonce.test.ts. And --no-compile means it needs no contract artifacts, which is why it finishes in milliseconds.

   The gap is narrower than "TS tests don't run in CI": no workflow invokes that target. I grepped all five files in .github/workflows/ for test-unit-hardhat, hardhat test, npm test and make test — no matches; the only Node steps anywhere are two npm ci installs. So the missing piece is a step calling an existing target, not new test infrastructure. Worth saying plainly: deployer-nonce.test.ts is in the same position today, so this predates your PR and isn't yours to fix.

   The other half of the invariant your PR doesn't close

   This is the part I'd actually like you to consider, because you're already in the right field.

   votingPower is schemaBigInt.max(UINT64_MAX) — bounded above, unbounded below. Run against your branch:

   [-5]        negative only        REJECTED  <- At least one validator must have positive voting power
   [-5, 20]    negative + positive  ACCEPTED
   

   Your check incidentally catches the first. The second passes genesis validation and then hits this downstream:

   toBytes32(-5n) THROWS: IntegerOutOfRangeError:
     Number "-5n" is not in safe 256-bit unsigned integer range (0n to 1157920892373...935n)
   

   buildValidatorManagerGenesisAllocs writes voting power via toBytes32(validator.votingPower), so a negative value paired with any positive one clears the schema and crashes the alloc builder instead.

   That is precisely the failure shape this PR exists to eliminate — a configuration error surfacing after validation rather than at it. It's pre-existing on main and your change neither causes nor worsens it, so it's legitimately out of scope if you'd rather keep the diff tight. But a lower bound on the field (zod bigint supports .min(0n)) composes cleanly with what you've built: the field rejects negatives, the set-level check requires at least one positive, and [0, 20] still passes. votingPowerLimit on controllers has the same unbounded-below shape.

   One tidy-up before merge

   The PR description still carries the framing you've just walked back here:

   Consensus later filters zero-power validators, which can leave no usable validator set.

   The decoder bails explicitly rather than continuing with an empty set. Since squash-merges take the PR body as the commit message, that sentence would outlive the discussion that corrected it — worth rewording to the fail-fast framing you used above.

   Method note: the test run, the four-case results, the negative-value results and the toBytes32 error are executed output from your branch, not recalled. The workflow and Makefile findings are file inspection.
5. ZhiyuCircle commented on Sep 8, 2026

   

   ZhiyuCircle

   on Sep 8, 2026

   Last edited by ZhiyuCircle

   Contributor

   More actions

   Zero voting power is our soft-delete/deactivation path: updateValidatorVotingPower(id, 0) leaves a validator Active and registered while removing it from the effective consensus set — see the doc comment at contracts/src/validator-manager/ValidatorRegistry.sol:169, removeValidator is the hard delete that prunes storage.

   Check at least one validator must have positive voting power makes sense to me
6. ZhiyuCircle

   closed this as completedon Sep 8, 2026
7. ZhiyuCircle

   reopened this on Sep 8, 2026
8. ZhiyuCircle

   closed this as completedin #346on Sep 8, 2026
9. ZhiyuCircle

   added a commit that references this issue on Sep 8, 2026

   fix(genesis): require positive validator voting power (#346)

   ...

   This issue will close once commit 186d124 is merged into the 'main' branch.

   186d124