Skip to content

fix(consensus): exclude node-local peerlist from the block hash - #995

Draft
Shitikyan wants to merge 1 commit into
stabilisationfrom
fix/consensus-block-hash-determinism
Draft

fix(consensus): exclude node-local peerlist from the block hash#995
Shitikyan wants to merge 1 commit into
stabilisationfrom
fix/consensus-block-hash-determinism

Conversation

@Shitikyan

Copy link
Copy Markdown
Contributor

Incident

The live network stopped finalizing blocks and is stuck at height 249445 — zero [CONSENSUS] Block added to the chain since a redeploy; the deadlock (Candidate block not formed: refusing the block hash) began ~2 min after that deploy. Vote tallies top out at pro=2, con=2 (need floor(4*2/3)+1 = 3).

Validators repeatedly reject each other's candidate with Hash does not correspond to our candidate block, while the tx-set diff shows missingFromUs=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

createBlock computes:

block.hash = sha256(serializeBlockContent(block.content, blockNumber))

and serializeBlockContent is JSON.stringify(content). BlockContent carries peerlist — the proposer's own live peer list (block.content.peerlist = peerlist in createBlock). Every validator holds a different peer view, so JSON.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 deterministicBlockHash fork. When active at a block's height, the hash is taken over the same content with peerlist neutralised to []:

if (isForkActive("deterministicBlockHash", blockHeight)) {
    return JSON.stringify({ ...content, peerlist: [] })
}
return JSON.stringify(content)
  • Everything except peerlist is byte-identical to the pre-fork serialization (the Map fields already stringify to {}), so the two paths differ only in the peerlist bytes.
  • verifyBlock re-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.
  • peerlist stays in the stored block for downstream readers; only the hash input ignores it.

Activation

  • data/genesis.json249445 (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.json0 (fresh chains are fully deterministic from genesis).
  • Code default → null (inactive). Unlike the value-format forks, this is a mid-chain remediation, so it stays off until an operator pins the coordinated height — an existing chain that activated it below its tip would reject its own history.

Rollout (operators)

  1. This is consensus-critical: all validators must be rolled in lock-step to a build carrying this fork + the 249445 activation 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.
  2. Confirm the deployed genesis for the live network matches data/genesis.json and that 249445 is 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.
  3. A fresh multi-validator network bootstrapped from data/genesis.json must set this to 0 instead of 249445.

Testing

  • New unit test 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.
  • Existing fork suites green (testing/forks/*: serializer, gates, boundary, integration, getNetworkInfo, postForkSerializer, disableForkMachineryFlag, amountCanonical) and verifyBlock.test.ts.
  • tsc --noEmit adds zero new errors vs. the stabilisation baseline.

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.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-code-review

Copy link
Copy Markdown
Contributor

ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing

@coderabbitai

coderabbitai Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Team

Run ID: 264ada58-64a1-4bcc-9266-f503f6f8be21


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@greptile-apps

greptile-apps Bot commented Sep 7, 2026

Copy link
Copy Markdown

Greptile Summary

This PR introduces a height-gated consensus serialization rule that excludes the node-local peer list from block-hash input while retaining it in stored block content.

  • Registers and loads the deterministicBlockHash fork with an inactive code default.
  • Activates it at height 249445 in the production genesis and at genesis height 0 for devnet.
  • Adds serializer tests for divergent peer views, non-peer content, and the activation boundary.
  • The implementation paths are aligned, but the new test bypasses verification of the actual fork name and configuration wiring.

Confidence Score: 4/5

The implementation appears safe to merge, with a non-blocking test gap around the production fork gate and configuration wiring.

Block creation and verification consistently use the block's own height, the fork is fully registered, and the serializer preserves all content except the node-local peer list; the only accepted concern is that the mocked test would not catch selection of the wrong fork.

Files Needing Attention: src/forks/serializerGate.blockHash.test.ts

Important Files Changed

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]
Loading

Fix all with Greploop Fix All in Claude Code

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

Comment on lines +11 to +14
jest.mock("./forkGates", () => ({
isForkActive: (_name: string, height: number) =>
activationHeight !== null && height >= activationHeight,
}))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 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.

Suggested change
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.

Fix in Claude Code

@Shitikyan
Shitikyan marked this pull request as draft September 9, 2026 09:20
@Shitikyan

Copy link
Copy Markdown
Contributor Author

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).

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant