fix(genesis): bound consensus params to uint16

[Kewe63]

Summary

Fixes #320

This updates the genesis ProtocolConfig schema so consensusParams values are validated against the uint16 range before they are packed into the genesis storage slot.

The consensus params are packed as 8 x uint16 values in the ProtocolConfig storage slot. Previously, the schema accepted arbitrary bigint values for these fields, so values greater than 65535 could be accepted and then overflow into neighboring packed lanes.

---

Changes

• Add a shared uint16 schema bound for genesis consensus params.
• Apply the bound to all packed consensusParams fields:
  • timeoutProposeMs
  • timeoutProposeDeltaMs
  • timeoutPrevoteMs
  • timeoutPrevoteDeltaMs
  • timeoutPrecommitMs
  • timeoutPrecommitDeltaMs
  • timeoutRebroadcastMs
  • targetBlockTimeMs
• Add unit coverage for:
  • accepting the uint16 upper bound, 65535
  • rejecting 65536 for each packed consensusParams field

---

Why

A genesis config should not accept values that cannot be represented correctly in the packed on-chain layout.

Before this fix, a value like 65536n could pass schema validation and then encode as 0 in the intended uint16 lane while spilling into the next lane. This could silently produce a different consensus configuration than the one supplied in genesis.

---

Tests

Regression test before the fix:

npx mocha -r ts-node/register ./tests/unit/protocol-config-genesis.test.ts

Result before fix:

1 passing
1 failing

Failure:

AssertionError: timeoutProposeMs should reject 65536: expected [Function] to throw an error

After the fix:

npx mocha -r ts-node/register ./tests/unit/protocol-config-genesis.test.ts

Result:

2 passing

Formatting:

npx prettier --config ./.prettierrc --check scripts/genesis/ProtocolConfig.ts tests/unit/protocol-config-genesis.test.ts

Result:

All matched files use Prettier code style!

Lint:

npx eslint scripts/genesis/ProtocolConfig.ts tests/unit/protocol-config-genesis.test.ts

Result:

passed

Additional check attempted:

npx tsc -p tsconfig.json --noEmit

Result:

failed on pre-existing repository-wide TypeScript errors unrelated to this PR. No errors from the new test file were reported in that output.

---

Checklist

• Tests pass — 2/2, confirmed failing before fix
• Prettier / ESLint clean
• Follows Conventional Commits
• Changes scoped to this fix only

---

Risk & Impact

Low. The bound only rejects values that were already invalid for the packed uint16 layout — any genesis config using valid values (0–65535) is unaffected. Verified the regression test fails against the old schema and passes with the fix, confirming it exercises the actual overflow path.

Type: 🐛 Bug fix
Fixes: #320

[osr21]

Reviewed at fd09543. The fix is correct, complete for the slot it targets, and the low-risk claim holds — I verified that rather than taking it on trust. A few things worth adding to the record, including one consequence that I think is a stronger motivation than the one in the description.

Verified

• All eight << 0/16/32/48/64/80/96/112 lanes in the packing expression correspond exactly to the eight fields now bound by schemaUint16. No packed field was missed and no unpacked field was over-constrained.
• Backward compatibility is real, not just asserted. I checked every committed genesis config — assets/devnet, assets/localdev, assets/mainnet (both config.json and genesis.config.ts forms). The largest value in any of them is 5000. Nothing currently in the repo starts failing validation.
• The file lands in tests/unit/, which make test-unit-hardhat already globs (npx hardhat test ./tests/helpers/matchers/index.test.ts ./tests/unit/*.test.ts --no-compile), and the style matches the existing sibling tests/unit/deployer-nonce.test.ts — chai expect, relative imports into scripts/genesis. Correct placement.

Nice catch that the description undersells: the field name

#320 listed timeoutCommitMs as one of the eight fields. That field does not exist — the real one is timeoutRebroadcastMs, which is what this PR bounds. Applying the issue's suggested snippet verbatim would have bound a non-existent key and left the real lane unbounded. Worth noting explicitly since the issue text is what a reviewer would check the diff against.

Why only this slot was vulnerable

This is the part I found most interesting, and it confirms the fix is at the right layer. ProtocolConfig packs two slots, and the other one is already immune:

// Slot 0: alpha | kRate | inverseElasticityMultiplier | padding
concat([toHex(0n, { size: 8 }), toHex(feeParams.inverseElasticityMultiplier, { size: 8 }), ...])

viem's toHex with an explicit size is self-validating. Confirmed against viem 2.52.2:

toHex(100n,    { size: 8 }) -> 0x0000000000000064
toHex(2n**64n, { size: 8 }) -> THREW IntegerOutOfRangeError
toHex(-1n,     { size: 8 }) -> THREW IntegerOutOfRangeError

So slot 0 would have blown up loudly even without schema bounds. Slot 5 uses shift-OR into a single toBytes32, which enforces nothing per lane — the width only exists in the shift constants. The bug is a direct consequence of that packing style, which is exactly why a schema bound is the right fix here.

The corruption is subtler than #320 shows

The issue's repro reports lane 1 decoding as 1, but that only holds because its neighbour was 0. Re-running the real packing expression against the actual devnet config (timeoutProposeDeltaMs: 500):

valid                lanes: 3000,500,1000,500,1000,500,1000,500
timeoutProposeMs=65536 lanes:   0,501,1000,500,1000,500,1000,500

It's a bitwise OR, so the neighbour becomes 500 | 1 = 501 — a completely plausible-looking timeout. A corrupted genesis wouldn't look obviously broken on inspection; it would look like someone typed 501. That makes this materially harder to catch by eye than the issue implies, and it strengthens the case for the bound.

The top lane fails differently, and worse — worth adding to the PR description

targetBlockTimeMs << 112n is the last of the eight, so an overflow there spills past bit 128 into the slot's reserved upper half rather than into a sibling:

targetBlockTimeMs=65536  lanes: 3000,500,1000,500,1000,500,1000,0    bits 128+ = 1

The lane reads back as 0 — and 0 is not a neutral value here. crates/eth-engine/src/abi_utils.rs:140 uses contract_params.targetBlockTimeMs != 0 as a presence check, with a test at :466 asserting "target_block_time should be None when 0 is provided."

So an out-of-range targetBlockTimeMs doesn't produce a wrong block time — it silently disables the target-block-time feature entirely, turning a numeric typo into a consensus behaviour change. That's a better headline motivation than lane-spill, and it's the one case where the bad value crosses from the genesis tooling into Rust node behaviour.

.min(0n) fixes a second bug, but it's a different kind

The description only talks about values above 65535, and the checklist doesn't mention the lower bound at all — but schemaBigInt is a bare z.coerce.bigint() with no floor, so -1n passed validation before this PR. (#320's "accepts values as arbitrary non-negative bigint" isn't accurate; nothing enforced non-negativity.)

That said, negatives were not silently corrupting anything. Verified:

pack({...base, timeoutProposeMs: -1n})  ->  -1n
toHex(-1n, { size: 32 })                ->  THREW IntegerOutOfRangeError

BigInt sign extension makes the OR collapse to -1n and toBytes32 rejects it. So .min(0n) upgrades a confusing late crash deep in the genesis writer into a clear validation error at the config boundary. Still worth having — just worth describing accurately, since "silently produces a different consensus configuration" applies to the upper bound only.

Suggestions

1. The lower bound is untested. Eight fields are checked against 65536n, but nothing asserts -1n is rejected, despite .min(0n) being half the change. One loop mirroring the existing one closes that.

2. A round-trip test would be more durable than the bound test. The current tests verify the schema in isolation, so they'd still pass if someone later widened a lane to uint32 and left the bound at 65535 (or vice versa) — schema and layout could drift apart silently. Packing 65535n into all eight lanes and decoding each back would pin the schema bound to the layout it exists to protect, which is the actual invariant.

3. Re-run under the repo's harness. The description used npx mocha -r ts-node/register, but the repo drives these through npx hardhat test via make test-unit-hardhat, which loads hardhat-toolbox-viem and the custom matchers plugin from hardhat.config.ts. It should pass identically, but confirming under the real runner is worth the one command.

4. Heads-up, pre-existing and not yours to fix: no workflow in .github/workflows/ invokes test-unit-hardhat or hardhat at all. ci.yml covers Rust fmt/sort/clippy/nextest, proto lint, contracts lint/build/forge, and eslint — there is no hardhat test job. So this test (and deployer-nonce.test.ts before it) runs only when someone invokes the make target locally. The "Tests pass" checkbox is true, but CI won't be enforcing it.

Good fix — correct layer, correct scope, and the field-name correction against the issue was the right call.