Genesis ProtocolConfig should reject zero blockGasLimit #321

Activity

1. huklaa commented on Sep 3, 2026

   

   huklaa

   on Sep 3, 2026

   More actions

   I’m working on this. I’ll add a focused fix and regression coverage.
2. osr21 commented on Sep 3, 2026

   

   osr21

   on Sep 3, 2026

   More actions

   @huklaa before you go further — this is already fixed in an open PR, and your #332 duplicates it exactly.

   #327 was opened by @Kewe63 (the issue author) at 08:09 UTC, ~12.5 hours before #332 at 20:37. Both diffs produce the byte-identical blob:

   #327: scripts/genesis/ProtocolConfig.ts  index 6b52ce6b..7cf7933b
   #332: scripts/genesis/ProtocolConfig.ts  index 6b52ce6b..7cf7933b
   

   Same one-line change, same three test cases; only the test filename and assertion style differ (expect(...).to.throw() vs safeParse().success). I reviewed #327 earlier today. Worth consolidating rather than having maintainers triage two identical fixes — and there's more useful work available, below.

   The premise checks out — verified by execution, not just reading

   I installed zod 3.25.76 (the version pinned in package.json) and ran the real schemaProtocolConfig against the fixture both PRs use:

   blockGasLimit = 0n              ACCEPTED
   blockGasLimit = 1n              ACCEPTED
   blockGasLimit = maxUint64+1n    rejected: BigInt must be <= 18446744073709551615
   

   So the reported behaviour is real and the third test case is meaningful. On the contract side I checked whether the single-field setter was missing the guard — it isn't; both mutative paths enforce it (ProtocolConfig.sol:141 in updateFeeParams, :175 in updateBlockGasLimit), so the invariant genesis is being aligned to is genuinely uniform.

   Two things worth stating in the PR that neither currently says:

   • min(1n) is exactly right — don't let anyone talk you into a higher floor. A limit of 1 is useless in practice (intrinsic cost alone is 21000), so "why not min(21000n)?" is a natural review question. The answer is that the goal is parity with the runtime API, and a stricter genesis bound would reintroduce the same divergence in the opposite direction. Say so pre-emptively.
   • The one-line change is sufficient for every config path. assets/localdev/genesis.config.ts:41 declares its own looser blockGasLimit: z.bigint().optional(), which looks like a bypass — it isn't, because buildProtocolConfigGenesisAllocs re-runs schemaProtocolConfig.parse(config) internally. That chokepoint is what makes the fix complete, and it's worth naming.

   The same bug class exists in three more places in this exact schema

   This is where a second PR would actually add value. I ran these against the current schema:

   all 7 consensus timeouts = 0n     ACCEPTED   <- updateConsensusParams() rejects every one
   timeoutProposeMs = 65536n         ACCEPTED   <- field is uint16 on-chain
   timeoutProposeMs = 2n**64n        ACCEPTED
   minBaseFee=1000 > maxBaseFee=1    ACCEPTED   <- updateFeeParams() rejects this
   

   1. The seven consensus timeouts have no bounds at all. Lines 78–84 are bare schemaBigInt (z.coerce.bigint()) — no .min(), no .max(). updateConsensusParams requires all seven to be > 0 (:156–:162). That is the identical genesis-vs-runtime mismatch as #321, times seven, and arguably worse since blockGasLimit at least had a .min(0n) someone had thought about.

   2. The missing upper bound causes silent state corruption, not just a bad accept. These are uint16 in ConsensusParams, packed eight-to-a-slot by shifts at :186–:193. A value ≥ 2^16 collides with its neighbour's bits. Running the real packing expression:

   genesis author WROTE    : timeoutProposeMs=65536, timeoutProposeDeltaMs=1
   packed slot value       : 0x10001000100010001000100010000
   contract READS BACK     : timeoutProposeMs=0,     timeoutProposeDeltaMs=1
   

   The overflow silently manufactures timeoutProposeMs = 0 — precisely the value updateConsensusParams refuses with InvalidTimeoutProposeMs. No error, no warning, a chain booted with a zero propose timeout. That's a strictly nastier failure than the one #321 reports, and .max(65535n) on all eight fields closes it.

   3. minBaseFee <= maxBaseFee isn't enforced at genesis. The contract requires it (:140, InvalidBaseFeeRange). The schema's only superRefine is enforceOperatorsNotProxyAdmin, so there's no cross-field check. Needs a superRefine, not a bound.

   For contrast, the three fields that are bounded — alpha ≤ 100, kRate ≤ 10000, inverseElasticityMultiplier ≤ 10000 — match the contract exactly. So the file's existing convention is on your side; these are gaps in it, not a different philosophy.

   Heads-up: the regression test will not run in CI

   This affects both PRs. tests/unit/*.test.ts is globbed by exactly one Makefile target:

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

   and nothing in .github/ invokes it — grepping the whole directory for hardhat, tests/unit, or test-unit returns zero matches. Public CI runs cargo nextest, forge test, forge lint, and make up; no hardhat step exists. The lint job doesn't cover the file either — it runs npx eslint --ignore-pattern 'scripts/' --ignore-pattern 'tests/', which excludes both the schema and the test.

   So the "3 passing" in either PR is a local-only result, and the regression guard would not stop a future revert. That doesn't make the test worthless, but it's worth being explicit with reviewers about, and adding a CI step that runs make test-unit-hardhat would give the fix real teeth. tests/unit/deployer-nonce.test.ts is currently the only other resident of that directory, so this isn't a problem you'd be creating.

   Suggestion

   Close #332 in favour of #327 (earlier, and from the issue author), and put a follow-up behind the three sibling mismatches above — timeout bounds are the highest-value piece, since #2 is silent corruption rather than a permissive accept. That's a clearly distinct PR nobody has claimed.

   ---

   Schema results above were produced by executing the real scripts/genesis/ProtocolConfig.ts against pinned zod 3.25.76 and viem 2.33.0; the read-back figures come from running the file's own packing expression. Solidity and CI claims are source review — no forge or hardhat in my environment.
3. kutluhaneth46 commented on Sep 5, 2026

   

   kutluhaneth46

   on Sep 5, 2026

   Contributor

   More actions

   Opened a follow-up PR for the sibling genesis/runtime gaps called out here (zero consensus timeouts + uint16 packing bound, and minBaseFee <= maxBaseFee): will link shortly. Leaving blockGasLimit > 0 to the existing #321 PRs.
4. kutluhaneth46 commented on Sep 5, 2026

   

   kutluhaneth46

   on Sep 5, 2026

   Contributor

   More actions

   Follow-up PR: #359
5. ZhiyuCircle

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

   fix(genesis): align ProtocolConfig schema with runtime consensus/fee …

   ...

   ac565f2