fix(consensus): exclude node-local peerlist from the block hash - #995
fix(consensus): exclude node-local peerlist from the block hash#995Shitikyan wants to merge 1 commit into
Conversation
The block hash is sha256(JSON.stringify(BlockContent)), and BlockContent carries `peerlist` — the proposer's own live peer list. Each validator holds a different peer view, so once peer topologies diverge the same (height, tx-set) hashes differently on every node, no two validators agree on a candidate hash, and BFT quorum (floor(n*2/3)+1) can never be reached. The live network stalled at height 249445 this way after a peer-management change made peerlists diverge across validators. Gate the fix behind a new `deterministicBlockHash` fork: post-activation, the hash is taken over the same content with `peerlist` neutralised to []. Historical blocks (below the activation height) keep verifying via the pre-fork path, keyed on the block's own number, so there is no re-sync break. peerlist stays in the stored block; only the hash ignores it. - data/genesis.json: activate at 249445 (the current stalled height). - devnet genesis: activate at 0 (fresh chains fully deterministic). - default null (inactive): existing chains stay bit-identical until an operator pins the coordinated height and rolls all validators together.
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
| Filename | Overview |
|---|---|
| src/forks/serializerGate.ts | Height-gates block serialization and neutralizes only peerlist after activation. |
| src/forks/serializerGate.blockHash.test.ts | Covers serializer outcomes and boundaries, but its permissive fork-gate mock cannot detect use of the wrong fork name. |
| src/forks/forkConfig.ts | Adds the fork's types, registry entry, inactive default, and cloned configuration. |
| src/forks/loadForkConfig.ts | Adds exhaustive validation and shared-state loading for the new fork. |
| data/genesis.json | Coordinates production activation at block height 249445. |
| testing/devnet/genesis.devnet.json | Enables deterministic block hashing from genesis on fresh devnet chains. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Block content and block height] --> B{deterministicBlockHash active?}
B -- No --> C[JSON.stringify full content]
B -- Yes --> D[Copy content and replace peerlist with empty list]
D --> E[JSON.stringify deterministic content]
C --> F[SHA-256 block hash]
E --> F
A --> G[Stored block retains original peerlist]
Prompt To Fix All With AI
### Issue 1
src/forks/serializerGate.blockHash.test.ts:11-14
**Mock Ignores Fork Name**
The mock ignores the requested fork name and activates every fork based only on height. If `serializeBlockContent` used the wrong fork name, this consensus-critical suite would still pass while production never enabled the new hashing rule. Make the mock reject unexpected names so the test also verifies the serializer's gate selection.
```suggestion
jest.mock("./forkGates", () => ({
isForkActive: (name: string, height: number) => {
if (name !== "deterministicBlockHash") {
throw new Error(`Unexpected fork name: ${name}`)
}
return activationHeight !== null && height >= activationHeight
},
}))
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "fix(consensus): exclude node-local peerl..." | Re-trigger Greptile
| jest.mock("./forkGates", () => ({ | ||
| isForkActive: (_name: string, height: number) => | ||
| activationHeight !== null && height >= activationHeight, | ||
| })) |
There was a problem hiding this comment.
The mock ignores the requested fork name and activates every fork based only on height. If serializeBlockContent used the wrong fork name, this consensus-critical suite would still pass while production never enabled the new hashing rule. Make the mock reject unexpected names so the test also verifies the serializer's gate selection.
| jest.mock("./forkGates", () => ({ | |
| isForkActive: (_name: string, height: number) => | |
| activationHeight !== null && height >= activationHeight, | |
| })) | |
| jest.mock("./forkGates", () => ({ | |
| isForkActive: (name: string, height: number) => { | |
| if (name !== "deterministicBlockHash") { | |
| throw new Error(`Unexpected fork name: ${name}`) | |
| } | |
| return activationHeight !== null && height >= activationHeight | |
| }, | |
| })) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/forks/serializerGate.blockHash.test.ts
Line: 11-14
Comment:
**Mock Ignores Fork Name**
The mock ignores the requested fork name and activates every fork based only on height. If `serializeBlockContent` used the wrong fork name, this consensus-critical suite would still pass while production never enabled the new hashing rule. Make the mock reject unexpected names so the test also verifies the serializer's gate selection.
```suggestion
jest.mock("./forkGates", () => ({
isForkActive: (name: string, height: number) => {
if (name !== "deterministicBlockHash") {
throw new Error(`Unexpected fork name: ${name}`)
}
return activationHeight !== null && height >= activationHeight
},
}))
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.|
Holding this as draft per review. The block peerlist is intentionally committed as the deterministic pool for next-round shard selection, so omitting it from the hash (as this PR does) would let peerlists diverge across peers and break shard selection. The observed stall was also traced to a firewall rule on one node making its peer view differ, which has since been resolved — so the net is unstuck without this change. Better direction (follow-up): keep the peerlist committed but derive the block peerlist canonically from the agreed validator/shard set rather than each node's live gossip view — that preserves the shard-selection commitment while removing the transient-divergence fragility (any firewall/restart/gossip-timing difference currently stalls liveness). |
Incident
The live network stopped finalizing blocks and is stuck at height 249445 — zero
[CONSENSUS] Block added to the chainsince a redeploy; the deadlock (Candidate block not formed: refusing the block hash) began ~2 min after that deploy. Vote tallies top out atpro=2, con=2(needfloor(4*2/3)+1 = 3).Validators repeatedly reject each other's candidate with
Hash does not correspond to our candidate block, while thetx-set diffshowsmissingFromUs=0, missingFromThem=0— the transaction sets are identical (same 26 hashes, same order, same block number). Two validators on the same build still compute different block hashes.Root cause
createBlockcomputes:and
serializeBlockContentisJSON.stringify(content).BlockContentcarriespeerlist— the proposer's own live peer list (block.content.peerlist = peerlistincreateBlock). Every validator holds a different peer view, soJSON.stringify(content)differs across nodes for the same(height, tx-set). No two validators ever agree on a candidate hash → BFT quorum is unreachable → permanent liveness stall.The block-hashing code itself did not change recently; a peer-management change (peer URL / hello-peer / connection-pool handling) made the peerlists diverge across validators, which is what tipped the always-latent
peerlist-in-hash into a hard stall. Consensus must not depend on node-local peer topology.Fix
New
deterministicBlockHashfork. When active at a block's height, the hash is taken over the same content withpeerlistneutralised to[]:peerlistis byte-identical to the pre-fork serialization (theMapfields already stringify to{}), so the two paths differ only in the peerlist bytes.verifyBlockre-hashes with the block's own number, so historical blocks (below the activation height) keep verifying via the pre-fork path — no re-sync / no history break.peerliststays in the stored block for downstream readers; only the hash input ignores it.Activation
data/genesis.json→ 249445 (the current stalled height; the first block the fixed nodes will form). Safe for any chain — it never re-hashes blocks ≤ 249444.testing/devnet/genesis.devnet.json→ 0 (fresh chains are fully deterministic from genesis).Rollout (operators)
249445activation in their deployed genesis. A partial rollout keeps splitting (fixed nodes exclude peerlist, unfixed nodes still include it) until every validator is on the fix.data/genesis.jsonand that249445is still ≥ the tip at deploy time (the chain is stalled, so it will not advance on its own). Bump the height if a coordinated window needs headroom.data/genesis.jsonmust set this to0instead of249445.Testing
src/forks/serializerGate.blockHash.test.ts: pre-fork two peer views hash differently; post-fork they hash identically; non-peerlist content still changes the hash; activation-height boundary respected.testing/forks/*: serializer, gates, boundary, integration, getNetworkInfo, postForkSerializer, disableForkMachineryFlag, amountCanonical) andverifyBlock.test.ts.tsc --noEmitadds zero new errors vs. thestabilisationbaseline.