From be10bce477ac122339eee56488cfb13988f865d3 Mon Sep 17 00:00:00 2001 From: jhfnetboy Date: Sun, 27 Sep 2026 13:58:09 +0700 Subject: [PATCH 1/9] =?UTF-8?q?fix(CC-36):=20node=20key-consistency=20self?= =?UTF-8?q?-check=20=E2=80=94=20surface=20a=20registered-key=20mismatch=20?= =?UTF-8?q?that=20otherwise=20fails=20silently?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A DVT node's identity lives in node_state.json (nodeId, publicKey, usually a privateKey), separately from the on-chain registeredKeys(nodeId) mapping set at registration time. If those ever drift apart, registration and startup succeed, the node looks healthy on every existing probe, but its signatures can never verify against the on-chain registration — it silently stops participating with no error anywhere. Add NodeKeyConsistencyService (src/modules/node/node-key-consistency.service.ts), a three-way, fail-loud-not-fail-stop comparison: 1. local: privateKey -> derive pubkey -> compare to node_state.publicKey (skipped_no_private_key for legitimate key-less KMS-TEE nodes) 2. on-chain: node_state.publicKey (EIP-2537-encoded) vs. registeredKeys(nodeId), gated by isRegistered(nodeId); an RPC error is `unknown`, NEVER read as a mismatch or as ok 3. derivedMatches: informational-only nodeId == keccak256(pubkey) check — the registerWithProof path derives nodeId this way, but the older registerPublicKey bootstrap path lets the caller pick any nodeId, so this must never feed into the pass/fail `status` NodeService runs the check once after loading node_state.json, fire-and-forget (never awaited on the startup path, never blocks/fails boot on a slow or down RPC). HealthController exposes the last result as an ADDITIVE `keyConsistency` field on GET /health — status/version/capabilities are unchanged. Also exports `encodeG1Point` from src/utils/bls.util.ts (previously a private method on BlsService) so this read-only check doesn't need to reach into the sign-path service for a pure encoding helper. Does not touch bls.service.ts or the owner-authorization gate. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Pdq9rgkZq9M4JFeTZD7yYs --- src/modules/health/health.controller.spec.ts | 48 ++++ src/modules/health/health.controller.ts | 24 +- src/modules/health/health.module.ts | 6 +- .../node/node-key-consistency.service.spec.ts | 181 ++++++++++++++ .../node/node-key-consistency.service.ts | 220 ++++++++++++++++++ .../node/node.key-consistency-wiring.spec.ts | 104 +++++++++ src/modules/node/node.module.ts | 5 +- src/modules/node/node.service.ts | 36 ++- src/utils/bls.util.ts | 19 ++ 9 files changed, 636 insertions(+), 7 deletions(-) create mode 100644 src/modules/health/health.controller.spec.ts create mode 100644 src/modules/node/node-key-consistency.service.spec.ts create mode 100644 src/modules/node/node-key-consistency.service.ts create mode 100644 src/modules/node/node.key-consistency-wiring.spec.ts diff --git a/src/modules/health/health.controller.spec.ts b/src/modules/health/health.controller.spec.ts new file mode 100644 index 0000000..bd90308 --- /dev/null +++ b/src/modules/health/health.controller.spec.ts @@ -0,0 +1,48 @@ +import { HealthController } from "./health.controller.js"; +import { KeyConsistencyResult } from "../node/node-key-consistency.service.js"; + +/** + * CC-36: /health gains an ADDITIVE `keyConsistency` field. `status`, `version` and + * `capabilities` must stay exactly as they were — external probes depend on them. + */ +describe("HealthController", () => { + it("omits keyConsistency entirely when no self-check has run yet (no shape change for old consumers)", () => { + const controller = new HealthController(undefined, { + getLastResult: () => null, + } as any); + const result = controller.health(); + expect(result).toEqual({ status: "ok", version: expect.any(String), capabilities: [] }); + expect("keyConsistency" in result).toBe(false); + }); + + it("omits keyConsistency when NodeKeyConsistencyService isn't wired in at all", () => { + const controller = new HealthController(undefined, undefined); + const result = controller.health(); + expect("keyConsistency" in result).toBe(false); + expect(result.status).toBe("ok"); + }); + + it("surfaces the self-check result additively once one has run, without touching other fields", () => { + const fakeResult: KeyConsistencyResult = { + status: "mismatch_onchain", + checkedAt: "2026-01-01T00:00:00.000Z", + derivedMatches: true, + nodeId: "0xabc", + }; + const controller = new HealthController(undefined, { + getLastResult: () => fakeResult, + } as any); + const result = controller.health(); + expect(result.status).toBe("ok"); + expect(result.capabilities).toEqual([]); + expect(result.keyConsistency).toEqual(fakeResult); + }); + + it("root() endpoint is unaffected (CC-36 only touches /health)", () => { + const controller = new HealthController(undefined, undefined); + const result = controller.root(); + expect(result.service).toBe("aastar-dvt-node"); + expect(result.status).toBe("ok"); + expect(result.endpoints.health).toBe("GET /health"); + }); +}); diff --git a/src/modules/health/health.controller.ts b/src/modules/health/health.controller.ts index 1bf6f8e..08f2f3c 100644 --- a/src/modules/health/health.controller.ts +++ b/src/modules/health/health.controller.ts @@ -2,6 +2,10 @@ import { createRequire } from "module"; import { Controller, Get, Optional } from "@nestjs/common"; import { ApiOperation, ApiTags } from "@nestjs/swagger"; import { CapabilityRegistry } from "../capability/capability-registry.service.js"; +import { + NodeKeyConsistencyService, + KeyConsistencyResult, +} from "../node/node-key-consistency.service.js"; const require = createRequire(import.meta.url); const { version: APP_VERSION } = require("../../../package.json") as { version: string }; @@ -16,7 +20,12 @@ const { version: APP_VERSION } = require("../../../package.json") as { version: @ApiTags("health") @Controller() export class HealthController { - constructor(@Optional() private readonly capabilities?: CapabilityRegistry) {} + constructor( + @Optional() private readonly capabilities?: CapabilityRegistry, + // CC-36: optional so /health keeps working even if NodeModule ever isn't wired in + // some deployment variant. Additive-only field — see `health()` below. + @Optional() private readonly keyConsistency?: NodeKeyConsistencyService + ) {} @Get() @ApiOperation({ @@ -54,8 +63,19 @@ export class HealthController { status: string; version: string; capabilities: Array<{ name: string; enabled: boolean }>; + keyConsistency?: KeyConsistencyResult; } { - return { status: "ok", version: APP_VERSION, capabilities: this.capList() }; + // Additive-only (CC-36): status/version/capabilities are unchanged and MUST stay + // that way — external probes depend on them. `keyConsistency` is omitted entirely + // (not even `null`) until a self-check has actually run, so existing consumers + // that don't know the field see no shape change. + const result = this.keyConsistency?.getLastResult() ?? undefined; + return { + status: "ok", + version: APP_VERSION, + capabilities: this.capList(), + ...(result ? { keyConsistency: result } : {}), + }; } private capList(): Array<{ name: string; enabled: boolean }> { diff --git a/src/modules/health/health.module.ts b/src/modules/health/health.module.ts index bdc90c1..6d08567 100644 --- a/src/modules/health/health.module.ts +++ b/src/modules/health/health.module.ts @@ -1,11 +1,15 @@ import { Module } from "@nestjs/common"; import { HealthController } from "./health.controller.js"; +import { NodeModule } from "../node/node.module.js"; /** * Always-on liveness endpoint (`GET /health`). The CapabilityRegistry it reads - * is a @Global singleton, so no import is needed here. + * is a @Global singleton, so no import is needed for it here. NodeModule is + * imported (CC-36) so /health can additively surface the node key-consistency + * self-check result via NodeKeyConsistencyService. */ @Module({ + imports: [NodeModule], controllers: [HealthController], }) export class HealthModule {} diff --git a/src/modules/node/node-key-consistency.service.spec.ts b/src/modules/node/node-key-consistency.service.spec.ts new file mode 100644 index 0000000..02cf41b --- /dev/null +++ b/src/modules/node/node-key-consistency.service.spec.ts @@ -0,0 +1,181 @@ +import { jest } from "@jest/globals"; +import { NodeKeyConsistencyService } from "./node-key-consistency.service.js"; +import { bls, sigs, encodeG1Point } from "../../utils/bls.util.js"; +import { NodeState } from "../../interfaces/node.interface.js"; +import { BlockchainService } from "../blockchain/blockchain.service.js"; + +/** + * Real BLS keypairs (not hardcoded fixtures) so every fixture is internally + * consistent by construction, and mismatches are constructed by mixing two + * distinct, genuinely-derived keypairs — never by hand-editing hex. + */ +function makeKeyPair(seed: number) { + const privBytes = new Uint8Array(32); + privBytes[31] = seed; + const privateKey = "0x" + Buffer.from(privBytes).toString("hex"); + const pub = sigs.getPublicKey(privBytes); + const compressedHex = "0x" + pub.toHex(); // node_state.publicKey format (48-byte compressed) + const point = bls.G1.Point.fromHex(pub.toHex()); + const eip2537Hex = "0x" + Buffer.from(encodeG1Point(point)).toString("hex"); // on-chain format (128-byte) + return { privateKey, compressedHex, eip2537Hex }; +} + +const KEY_A = makeKeyPair(1); +const KEY_B = makeKeyPair(2); + +function baseNodeState(overrides: Partial = {}): NodeState { + return { + nodeId: "0x" + "ab".repeat(32), + nodeName: "test-node", + privateKey: KEY_A.privateKey, + publicKey: KEY_A.compressedHex, + createdAt: new Date(0).toISOString(), + description: "test", + ...overrides, + }; +} + +function makeBlockchainMock(overrides: { + checkNodeRegistration?: () => Promise; + getNodePublicKey?: () => Promise; +}): BlockchainService { + return { + checkNodeRegistration: jest.fn( + overrides.checkNodeRegistration ?? (() => Promise.resolve(false)) + ), + getNodePublicKey: jest.fn(overrides.getNodePublicKey ?? (() => Promise.resolve("0x"))), + } as unknown as BlockchainService; +} + +describe("NodeKeyConsistencyService (CC-36)", () => { + it("ok: local privateKey derives publicKey AND on-chain registeredKeys matches", async () => { + const chain = makeBlockchainMock({ + checkNodeRegistration: () => Promise.resolve(true), + getNodePublicKey: () => Promise.resolve(KEY_A.eip2537Hex), + }); + const service = new NodeKeyConsistencyService(chain); + const result = await service.check(baseNodeState(), "0xcontract"); + expect(result.status).toBe("ok"); + expect(service.getLastResult()).toEqual(result); + }); + + it("mismatch_local: privateKey does NOT derive node_state.publicKey", async () => { + const chain = makeBlockchainMock({ + checkNodeRegistration: () => Promise.resolve(true), + getNodePublicKey: () => Promise.resolve(KEY_A.eip2537Hex), + }); + const service = new NodeKeyConsistencyService(chain); + // privateKey is A's, but publicKey on disk is B's — a corrupted/swapped node_state. + const state = baseNodeState({ publicKey: KEY_B.compressedHex }); + const result = await service.check(state, "0xcontract"); + expect(result.status).toBe("mismatch_local"); + }); + + it("mismatch_onchain: registered, but registeredKeys() differs from local publicKey — the silent failure", async () => { + const chain = makeBlockchainMock({ + checkNodeRegistration: () => Promise.resolve(true), + getNodePublicKey: () => Promise.resolve(KEY_B.eip2537Hex), // someone else's key is registered + }); + const service = new NodeKeyConsistencyService(chain); + const result = await service.check(baseNodeState(), "0xcontract"); + expect(result.status).toBe("mismatch_onchain"); + }); + + it("not_registered: isRegistered() false is informational, NOT a failure", async () => { + const chain = makeBlockchainMock({ + checkNodeRegistration: () => Promise.resolve(false), + }); + const service = new NodeKeyConsistencyService(chain); + const result = await service.check(baseNodeState(), "0xcontract"); + expect(result.status).toBe("not_registered"); + // getNodePublicKey must not even be consulted once we know it's unregistered. + expect(chain.getNodePublicKey).not.toHaveBeenCalled(); + }); + + it("unknown: an RPC error must NEVER collapse to mismatch_onchain or ok", async () => { + const chain = makeBlockchainMock({}); + (chain.checkNodeRegistration as jest.Mock).mockRejectedValueOnce( + new Error("RPC timeout") as never + ); + const service = new NodeKeyConsistencyService(chain); + const result = await service.check(baseNodeState(), "0xcontract"); + expect(result.status).toBe("unknown"); + }); + + it("unknown: a getNodePublicKey RPC error (registered, but read fails) also stays unknown, not a mismatch", async () => { + const chain = makeBlockchainMock({ + checkNodeRegistration: () => Promise.resolve(true), + }); + (chain.getNodePublicKey as jest.Mock).mockRejectedValueOnce( + new Error("RPC timeout") as never + ); + const service = new NodeKeyConsistencyService(chain); + const result = await service.check(baseNodeState(), "0xcontract"); + expect(result.status).toBe("unknown"); + }); + + it("skipped_no_private_key: key-less (KMS-TEE) node with a clean on-chain match is NOT reported ok", async () => { + const chain = makeBlockchainMock({ + checkNodeRegistration: () => Promise.resolve(true), + getNodePublicKey: () => Promise.resolve(KEY_A.eip2537Hex), + }); + const service = new NodeKeyConsistencyService(chain); + const state = baseNodeState(); + delete (state as Partial).privateKey; + const result = await service.check(state, "0xcontract"); + expect(result.status).toBe("skipped_no_private_key"); + }); + + it("skipped_no_private_key still surfaces a real on-chain problem instead of masking it", async () => { + const chain = makeBlockchainMock({ + checkNodeRegistration: () => Promise.resolve(true), + getNodePublicKey: () => Promise.resolve(KEY_B.eip2537Hex), + }); + const service = new NodeKeyConsistencyService(chain); + const state = baseNodeState(); + delete (state as Partial).privateKey; + const result = await service.check(state, "0xcontract"); + expect(result.status).toBe("mismatch_onchain"); + }); + + it("REGRESSION: a bootstrap-registered node (nodeId !== keccak256(pubkey)) is still `ok`, never a mismatch", async () => { + // registerPublicKey(nodeId, publicKey) (the old bootstrap path) lets the caller pick ANY + // nodeId — it is NOT required to be keccak256(publicKey). This guards against someone + // later "upgrading" derivedMatches into a consistency judgment by mistake. + const chain = makeBlockchainMock({ + checkNodeRegistration: () => Promise.resolve(true), + getNodePublicKey: () => Promise.resolve(KEY_A.eip2537Hex), + }); + const service = new NodeKeyConsistencyService(chain); + const bootstrapNodeId = "0x" + "11".repeat(32); // deliberately NOT keccak256(KEY_A.eip2537Hex) + const state = baseNodeState({ nodeId: bootstrapNodeId }); + const result = await service.check(state, "0xcontract"); + expect(result.status).toBe("ok"); + expect(result.derivedMatches).toBe(false); + expect(result.nodeId).toBe(bootstrapNodeId); + }); + + it("derivedMatches is true when nodeId genuinely is keccak256(EIP-2537 publicKey)", async () => { + const { ethers } = await import("ethers"); + const derivedNodeId = ethers.keccak256(KEY_A.eip2537Hex); + const chain = makeBlockchainMock({ + checkNodeRegistration: () => Promise.resolve(true), + getNodePublicKey: () => Promise.resolve(KEY_A.eip2537Hex), + }); + const service = new NodeKeyConsistencyService(chain); + const result = await service.check(baseNodeState({ nodeId: derivedNodeId }), "0xcontract"); + expect(result.status).toBe("ok"); + expect(result.derivedMatches).toBe(true); + }); + + it("hex comparisons are case/prefix-insensitive (0x vs bare, upper vs lower)", async () => { + const chain = makeBlockchainMock({ + checkNodeRegistration: () => Promise.resolve(true), + // Same bytes as KEY_A.eip2537Hex, but uppercase and without the 0x prefix. + getNodePublicKey: () => Promise.resolve(KEY_A.eip2537Hex.slice(2).toUpperCase()), + }); + const service = new NodeKeyConsistencyService(chain); + const result = await service.check(baseNodeState(), "0xcontract"); + expect(result.status).toBe("ok"); + }); +}); diff --git a/src/modules/node/node-key-consistency.service.ts b/src/modules/node/node-key-consistency.service.ts new file mode 100644 index 0000000..6ee7d9f --- /dev/null +++ b/src/modules/node/node-key-consistency.service.ts @@ -0,0 +1,220 @@ +import { Injectable, Logger } from "@nestjs/common"; +import { ethers } from "ethers"; +import { bls, sigs, encodeG1Point } from "../../utils/bls.util.js"; +import { BlockchainService } from "../blockchain/blockchain.service.js"; +import { NodeState } from "../../interfaces/node.interface.js"; + +export type KeyConsistencyStatus = + | "ok" + | "mismatch_local" + | "mismatch_onchain" + | "not_registered" + | "unknown" + | "skipped_no_private_key"; + +export interface KeyConsistencyResult { + status: KeyConsistencyStatus; + checkedAt: string; + /** + * Informational only (per CC-36): whether `nodeId === keccak256(EIP-2537 publicKey)`. + * The contract's `registerWithProof` path derives nodeId this way, but the older + * `registerPublicKey(nodeId, publicKey)` bootstrap path lets the caller choose ANY + * nodeId. A bootstrap-registered node legitimately has `derivedMatches === false` + * and MUST still be reported `ok` when its keys actually agree — this field must + * never be folded into `status`. + */ + derivedMatches: boolean | null; + nodeId: string; +} + +function normalizeHex(hex: string | undefined | null): string { + return "0x" + (hex ?? "").replace(/^0x/i, "").toLowerCase(); +} + +/** + * CC-36 — node key-consistency self-check. + * + * Closes a silent failure mode: a DVT node's identity lives in node_state.json + * (nodeId + publicKey + usually a privateKey). If the key actually used to sign + * no longer matches what's registered on-chain for that nodeId, registration and + * startup succeed, the node looks healthy, but every signature it produces will + * fail verification against the on-chain registration forever — the node is + * online and silently non-participating. + * + * Three-way comparison: + * 1. local: privateKey -> derive pubkey -> compare to node_state.publicKey + * 2. on-chain: node_state.publicKey (EIP-2537-encoded) -> compare to + * registeredKeys(nodeId) (only meaningful when isRegistered(nodeId)) + * 3. derivedMatches (informational only): nodeId == keccak256(EIP-2537 publicKey) + * + * Fail-loud, not fail-stop: `check()` never throws. Every failure mode (malformed + * key material, RPC error, timeout) collapses into a `status` value so a broken + * node stays observable instead of taking the process down with it — a node that + * already can't produce valid signatures gains nothing from also losing its + * health/metrics endpoint. + */ +@Injectable() +export class NodeKeyConsistencyService { + private readonly logger = new Logger(NodeKeyConsistencyService.name); + private lastResult: KeyConsistencyResult | null = null; + + constructor(private readonly blockchainService: BlockchainService) {} + + /** The most recent check() result, or null if none has run yet. Read by /health. */ + getLastResult(): KeyConsistencyResult | null { + return this.lastResult; + } + + async check(nodeState: NodeState, contractAddress: string): Promise { + const checkedAt = new Date().toISOString(); + const derivedMatches = this.computeDerivedMatches(nodeState); + + const local = this.checkLocal(nodeState); + const onchain = await this.checkOnchain(nodeState, contractAddress); + const status = this.combine(local, onchain); + + const result: KeyConsistencyResult = { + status, + checkedAt, + derivedMatches, + nodeId: nodeState.nodeId, + }; + + if (status === "mismatch_local" || status === "mismatch_onchain") { + const localPrefix = this.prefix(nodeState.publicKey); + this.logger.error( + `KEY CONSISTENCY FAILURE [${status}] for node ${nodeState.nodeId}: ` + + (status === "mismatch_local" + ? `the private key in node_state.json does NOT derive node_state.publicKey ` + + `(local publicKey=${localPrefix}...). ` + : `node_state.publicKey (${localPrefix}...) does NOT match the on-chain ` + + `registeredKeys() entry for this nodeId. `) + + `This node will keep signing, but its signatures can NEVER verify against the ` + + `on-chain registration — it is online and healthy while being silently ` + + `non-participating. Fix: re-register the correct public key on-chain for this ` + + `nodeId, or restore the matching privateKey/publicKey pair in node_state.json.` + ); + } + + this.lastResult = result; + return result; + } + + private prefix(hex: string | undefined): string { + const h = (hex ?? "").replace(/^0x/i, ""); + return "0x" + h.slice(0, 10); + } + + /** privateKey -> derived pubkey vs. node_state.publicKey. Key-less nodes are a legitimate skip. */ + private checkLocal(nodeState: NodeState): "ok" | "mismatch" | "skipped" { + if (!nodeState.privateKey) { + return "skipped"; + } + try { + const privHex = nodeState.privateKey.replace(/^0x/i, ""); + const privBytes = new Uint8Array(privHex.length / 2); + for (let i = 0; i < privHex.length; i += 2) { + privBytes[i / 2] = parseInt(privHex.substr(i, 2), 16); + } + const derivedPub = sigs.getPublicKey(privBytes); + const derivedHex = normalizeHex(derivedPub.toHex()); + const stateHex = normalizeHex(nodeState.publicKey); + return derivedHex === stateHex ? "ok" : "mismatch"; + } catch (error: any) { + // Malformed key material — cannot prove correctness, so treat as a mismatch rather + // than silently skipping (a corrupt privateKey is itself worth surfacing). + this.logger.warn( + `local key-consistency check could not compute a public key from privateKey: ${error?.message ?? error}` + ); + return "mismatch"; + } + } + + /** node_state.publicKey (EIP-2537-encoded) vs. registeredKeys(nodeId), gated by isRegistered. */ + private async checkOnchain( + nodeState: NodeState, + contractAddress: string + ): Promise<"ok" | "mismatch" | "not_registered" | "unknown"> { + try { + const isRegistered = await this.blockchainService.checkNodeRegistration( + contractAddress, + nodeState.nodeId + ); + if (!isRegistered) { + return "not_registered"; + } + + const localEip2537 = this.toEip2537Hex(nodeState.publicKey); + if (localEip2537 === null) { + // Registered on-chain, but the local publicKey isn't even a parsable curve + // point — we cannot determine agreement either way. Never guess: unknown. + this.logger.warn( + `key-consistency check: node_state.publicKey for ${nodeState.nodeId} is not a valid ` + + "G1 point; cannot compare against the on-chain registration" + ); + return "unknown"; + } + + const onchainKey = await this.blockchainService.getNodePublicKey( + contractAddress, + nodeState.nodeId + ); + return normalizeHex(onchainKey) === normalizeHex(localEip2537) ? "ok" : "mismatch"; + } catch (error: any) { + // RPC failure/timeout/revert. NEVER read this as a mismatch (would falsely accuse a + // healthy node) and NEVER read it as ok (would hide a real mismatch) — indeterminate. + this.logger.warn( + `on-chain key-consistency read failed for node ${nodeState.nodeId}: ${error?.message ?? error}` + ); + return "unknown"; + } + } + + /** Informational: does nodeId == keccak256(EIP-2537 publicKey)? Never feeds into `status`. */ + private computeDerivedMatches(nodeState: NodeState): boolean | null { + if (!nodeState.nodeId) { + return null; + } + const eip2537 = this.toEip2537Hex(nodeState.publicKey); + if (eip2537 === null) { + return null; + } + const derivedNodeId = ethers.keccak256(eip2537); + return normalizeHex(derivedNodeId) === normalizeHex(nodeState.nodeId); + } + + /** Decompress a 48-byte compressed G1 hex pubkey and re-encode it as 128-byte EIP-2537. */ + private toEip2537Hex(compressedPublicKeyHex: string | undefined): string | null { + if (!compressedPublicKeyHex) { + return null; + } + try { + const point = bls.G1.Point.fromHex(compressedPublicKeyHex.replace(/^0x/i, "")); + const encoded = encodeG1Point(point); + return "0x" + Buffer.from(encoded).toString("hex"); + } catch { + return null; + } + } + + /** + * Priority (highest severity first): a proven local mismatch is the most certain + * signal (the key on disk is definitely broken), then a proven on-chain mismatch + * (the exact silent failure this check exists to catch), then the informational + * not_registered / unknown on-chain outcomes, then — only once nothing else is + * wrong — `skipped_no_private_key` if the local dimension could not be checked at + * all (a key-less node is legitimate, but we were only able to verify half the + * picture, so we don't claim full `ok`). + */ + private combine( + local: "ok" | "mismatch" | "skipped", + onchain: "ok" | "mismatch" | "not_registered" | "unknown" + ): KeyConsistencyStatus { + if (local === "mismatch") return "mismatch_local"; + if (onchain === "mismatch") return "mismatch_onchain"; + if (onchain === "not_registered") return "not_registered"; + if (onchain === "unknown") return "unknown"; + if (local === "skipped") return "skipped_no_private_key"; + return "ok"; + } +} diff --git a/src/modules/node/node.key-consistency-wiring.spec.ts b/src/modules/node/node.key-consistency-wiring.spec.ts new file mode 100644 index 0000000..673ffed --- /dev/null +++ b/src/modules/node/node.key-consistency-wiring.spec.ts @@ -0,0 +1,104 @@ +import { jest } from "@jest/globals"; +import { mkdtempSync, writeFileSync, rmSync } from "fs"; +import { tmpdir } from "os"; +import { join } from "path"; +import { ConfigService } from "@nestjs/config"; +import { NodeService } from "./node.service.js"; + +/** + * CC-36 wiring: NodeService kicks off the key-consistency self-check after loading + * node_state.json, WITHOUT awaiting it (startup must never block on a slow/unavailable + * RPC), and exposes the last result via getKeyConsistency(). The check's own branch + * coverage (ok/mismatch_local/mismatch_onchain/not_registered/unknown/skipped) lives in + * node-key-consistency.service.spec.ts — this file only covers the NodeService glue. + */ +describe("NodeService — key-consistency self-check wiring (CC-36)", () => { + const nodeState = { + nodeId: "0x1", + nodeName: "n", + publicKey: "0xabcd", + createdAt: "t", + description: "d", + privateKey: "0x" + "1".padStart(64, "0"), + }; + + it("fires the check without awaiting it — initializeNode resolves before a slow check finishes", async () => { + const dir = mkdtempSync(join(tmpdir(), "kc-")); + const file = join(dir, "node_state.json"); + writeFileSync(file, JSON.stringify(nodeState)); + const config = { + get: (k: string) => (k === "validatorContractAddress" ? "0xcontract" : undefined), + } as unknown as ConfigService; + + let resolveCheck: () => void = () => {}; + const checkPromise = new Promise(resolve => { + resolveCheck = () => + resolve({ status: "ok", checkedAt: "t", derivedMatches: true, nodeId: "0x1" }); + }); + const checkFn = jest.fn(() => checkPromise); + const keyConsistencyService = { check: checkFn, getLastResult: () => null } as any; + + const svc = new NodeService({} as any, {} as any, config, keyConsistencyService); + // initializeNode() recomputes nodeStateFilePath from process.cwd() (fixed-name + // convention), so point cwd at the temp dir rather than poking the private field — + // this exercises the same path onModuleInit() takes in production. + const cwdSpy = jest.spyOn(process, "cwd").mockReturnValue(dir); + + try { + await (svc as any).initializeNode(); + // The check was kicked off … + expect(checkFn).toHaveBeenCalledTimes(1); + expect(checkFn).toHaveBeenCalledWith( + expect.objectContaining({ nodeId: "0x1" }), + "0xcontract" + ); + // … but initializeNode did NOT wait for it to resolve. + resolveCheck(); + await checkPromise; // let it settle for cleanliness + } finally { + cwdSpy.mockRestore(); + rmSync(dir, { recursive: true, force: true }); + } + }); + + it("getKeyConsistency() delegates to the injected service's getLastResult()", () => { + const fakeResult = { status: "ok", checkedAt: "t", derivedMatches: true, nodeId: "0x1" } as any; + const keyConsistencyService = { + check: jest.fn(() => Promise.resolve(fakeResult)), + getLastResult: () => fakeResult, + } as any; + const config = { get: () => undefined } as unknown as ConfigService; + const svc = new NodeService({} as any, {} as any, config, keyConsistencyService); + expect(svc.getKeyConsistency()).toBe(fakeResult); + }); + + it("getKeyConsistency() returns null when no consistency service is wired (defensive @Optional())", () => { + const config = { get: () => undefined } as unknown as ConfigService; + const svc = new NodeService({} as any, {} as any, config); + expect(svc.getKeyConsistency()).toBeNull(); + }); + + it("never throws from initializeNode when the consistency service itself rejects", async () => { + const dir = mkdtempSync(join(tmpdir(), "kc-")); + const file = join(dir, "node_state.json"); + writeFileSync(file, JSON.stringify(nodeState)); + const config = { + get: (k: string) => (k === "validatorContractAddress" ? "0xcontract" : undefined), + } as unknown as ConfigService; + const keyConsistencyService = { + check: jest.fn(() => Promise.reject(new Error("boom — should never happen"))), + getLastResult: () => null, + } as any; + const svc = new NodeService({} as any, {} as any, config, keyConsistencyService); + const cwdSpy = jest.spyOn(process, "cwd").mockReturnValue(dir); + try { + await expect((svc as any).initializeNode()).resolves.toBeUndefined(); + // Drain the microtask queue so the fire-and-forget rejection is observed/logged, + // not left as an unhandled rejection leaking into a later test. + await new Promise(resolve => setImmediate(resolve)); + } finally { + cwdSpy.mockRestore(); + rmSync(dir, { recursive: true, force: true }); + } + }); +}); diff --git a/src/modules/node/node.module.ts b/src/modules/node/node.module.ts index 4b832cc..f9215a2 100644 --- a/src/modules/node/node.module.ts +++ b/src/modules/node/node.module.ts @@ -2,13 +2,14 @@ import { Module, forwardRef } from "@nestjs/common"; import { NodeService } from "./node.service.js"; import { NodeController } from "./node.controller.js"; import { IdentityController } from "./identity.controller.js"; +import { NodeKeyConsistencyService } from "./node-key-consistency.service.js"; import { BlsModule } from "../bls/bls.module.js"; import { BlockchainModule } from "../blockchain/blockchain.module.js"; @Module({ imports: [forwardRef(() => BlsModule), BlockchainModule], - providers: [NodeService], + providers: [NodeService, NodeKeyConsistencyService], controllers: [NodeController, IdentityController], - exports: [NodeService], + exports: [NodeService, NodeKeyConsistencyService], }) export class NodeModule {} diff --git a/src/modules/node/node.service.ts b/src/modules/node/node.service.ts index 2756f8a..7693b46 100644 --- a/src/modules/node/node.service.ts +++ b/src/modules/node/node.service.ts @@ -1,4 +1,4 @@ -import { Injectable, OnModuleInit, Logger, Inject, forwardRef } from "@nestjs/common"; +import { Injectable, OnModuleInit, Logger, Inject, forwardRef, Optional } from "@nestjs/common"; import { ConfigService } from "@nestjs/config"; import { NodeKeyPair, NodeState } from "../../interfaces/node.interface.js"; import { readFileSync, writeFileSync, existsSync, readdirSync } from "fs"; @@ -7,6 +7,10 @@ import { BlsService } from "../bls/bls.service.js"; import { BlockchainService } from "../blockchain/blockchain.service.js"; import { randomBytes, createHash } from "crypto"; import { decryptKeystore, isKeystore } from "../../utils/keystore.util.js"; +import { + NodeKeyConsistencyService, + KeyConsistencyResult, +} from "./node-key-consistency.service.js"; @Injectable() export class NodeService implements OnModuleInit { @@ -19,7 +23,10 @@ export class NodeService implements OnModuleInit { @Inject(forwardRef(() => BlsService)) private blsService: BlsService, private blockchainService: BlockchainService, - private configService: ConfigService + private configService: ConfigService, + // @Optional() so existing 3-arg test constructions keep working, and so a DI + // container without this provider wired (unlikely, but defensive) still boots. + @Optional() private readonly keyConsistencyService?: NodeKeyConsistencyService ) {} async onModuleInit() { @@ -36,6 +43,11 @@ export class NodeService implements OnModuleInit { this.loadExistingNodeState(); if (this.nodeState) { this.logger.log(`Loaded node state: ${this.nodeState.nodeId}`); + // CC-36: best-effort, fire-and-forget — a slow/unavailable RPC must never + // delay or block startup (`void`, not `await`). The check itself never + // throws (fail-loud via logger.error, fail-open on the process), but the + // wrapper still guards against a future bug in the check surfacing here. + void this.runKeyConsistencyCheck(); } } else { this.logger.log("No node state file found. Node is not created yet."); @@ -43,6 +55,26 @@ export class NodeService implements OnModuleInit { } } + /** See NodeKeyConsistencyService — never awaited on the startup path, never throws. */ + private async runKeyConsistencyCheck(): Promise { + if (!this.nodeState || !this.keyConsistencyService) { + return; + } + try { + await this.keyConsistencyService.check(this.nodeState, this.contractAddress); + } catch (error: any) { + this.logger.error( + `Key-consistency self-check crashed unexpectedly (this should never happen — the check ` + + `is designed to fail loud, not throw): ${error?.message ?? error}` + ); + } + } + + /** Last result of the CC-36 key-consistency self-check, or null if none has run/registered. */ + getKeyConsistency(): KeyConsistencyResult | null { + return this.keyConsistencyService?.getLastResult() ?? null; + } + private loadContractAddress(): void { this.contractAddress = this.configService.get("validatorContractAddress")!; this.logger.log(`Using contract address from environment: ${this.contractAddress}`); diff --git a/src/utils/bls.util.ts b/src/utils/bls.util.ts index 0652786..e9e32e4 100644 --- a/src/utils/bls.util.ts +++ b/src/utils/bls.util.ts @@ -24,6 +24,25 @@ export function hexToBytes(hex: string): Uint8Array { return bytes; } +/** + * Encode a G1 point (BLS public key) to the 128-byte EIP-2537 wire format — the + * uncompressed x||y form the on-chain validator stores in `registeredKeys`. + * Mirrors `BlsService.encodeG1Point` (kept private there, on the sign path); this + * copy lives here so read-only consumers (e.g. the node key-consistency + * self-check) don't need to reach into BlsService for a pure encoding helper. + */ +export function encodeG1Point(point: any): Uint8Array { + const result = new Uint8Array(128); + const affine = point.toAffine(); + + const xBytes = hexToBytes(affine.x.toString(16).padStart(96, "0")); + const yBytes = hexToBytes(affine.y.toString(16).padStart(96, "0")); + + result.set(xBytes, 16); // Skip 16 zero bytes at start + result.set(yBytes, 80); // Skip 16 zero bytes at start + return result; +} + export const BLS_DST = "BLS_SIG_BLS12381G2_XMD:SHA-256_SSWU_RO_POP_"; export const bls = bls12_381; export const sigs = bls.longSignatures; From 90953c80d2119215bc0a0c15e7a2a1e6ccadfbce Mon Sep 17 00:00:00 2001 From: jhfnetboy Date: Sun, 27 Sep 2026 14:13:28 +0700 Subject: [PATCH 2/9] fix(CC-36 round 2): use BlsService's own EIP-2537 encoder; retry on unknown, recheck periodically MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-1 review caught two gaps in the key-consistency self-check: 1. The on-chain comparison encoded node_state.publicKey with a COPY of the G1 EIP-2537 encoder (added to bls.util.ts), separate from the one registerOnChain()/gossip/dashboard/quorum-cosigner all actually use (BlsService.encodePublicKeyToEIP2537). A future change to the real encoder would silently diverge from this check's copy — exactly the kind of drift this check exists to catch. Fixed by injecting BlsService into NodeKeyConsistencyService and calling its encodePublicKeyToEIP2537 directly; removed the now-unused bls.util.ts copy. This does not touch the owner-authorization gate (authorizeAndDeriveHash) — encodePublicKeyToEIP2537 is a pure encoding function unrelated to that path. Verified no circular dependency: NodeModule already imports BlsModule (forwardRef), and the full AppModule now boots and shuts down cleanly via NestFactory.createApplicationContext (checked directly, not inferred). Locked with a test that swaps in a stub BlsService returning a sentinel value and asserts the comparison actually consults it. 2. A single startup check meant an `unknown` result (RPC hiccup at boot, the most likely time for network to be flaky) froze forever, and a node kicked off the registry post-boot (the validator's permissionless syncNode(nodeId) deactivating an under-staked/role-losing operator) never showed up in /health. NodeKeyConsistencyService now self-schedules: bounded backoff while unknown (30s/60s/120s/240s, then folding into the regular cadence), and a periodic recheck (every 15 min) once conclusive. A recheck that itself fails RPC NEVER downgrades a held conclusive result (e.g. mismatch_onchain) to unknown — it only advances lastAttemptAt, so a transient blip can't launder away a real finding. /health now also exposes lastAttemptAt alongside checkedAt. Timers are .unref()'d and cleared in both stop() and the new OnModuleDestroy hook. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Pdq9rgkZq9M4JFeTZD7yYs --- src/modules/health/health.controller.spec.ts | 1 + ...key-consistency.service.scheduling.spec.ts | 233 ++++++++++++++++++ .../node/node-key-consistency.service.spec.ts | 73 +++++- .../node/node-key-consistency.service.ts | 214 ++++++++++++---- .../node/node.key-consistency-wiring.spec.ts | 45 ++-- src/modules/node/node.service.ts | 21 +- src/utils/bls.util.ts | 19 -- 7 files changed, 495 insertions(+), 111 deletions(-) create mode 100644 src/modules/node/node-key-consistency.service.scheduling.spec.ts diff --git a/src/modules/health/health.controller.spec.ts b/src/modules/health/health.controller.spec.ts index bd90308..e23381a 100644 --- a/src/modules/health/health.controller.spec.ts +++ b/src/modules/health/health.controller.spec.ts @@ -26,6 +26,7 @@ describe("HealthController", () => { const fakeResult: KeyConsistencyResult = { status: "mismatch_onchain", checkedAt: "2026-01-01T00:00:00.000Z", + lastAttemptAt: "2026-01-01T00:00:00.000Z", derivedMatches: true, nodeId: "0xabc", }; diff --git a/src/modules/node/node-key-consistency.service.scheduling.spec.ts b/src/modules/node/node-key-consistency.service.scheduling.spec.ts new file mode 100644 index 0000000..1e0c559 --- /dev/null +++ b/src/modules/node/node-key-consistency.service.scheduling.spec.ts @@ -0,0 +1,233 @@ +import { jest } from "@jest/globals"; +import { NodeKeyConsistencyService } from "./node-key-consistency.service.js"; +import { bls, sigs } from "../../utils/bls.util.js"; +import { NodeState } from "../../interfaces/node.interface.js"; +import { BlockchainService } from "../blockchain/blockchain.service.js"; +import { BlsService } from "../bls/bls.service.js"; + +/** + * CC-36 round 2: the self-check is not a one-shot — it retries with bounded backoff while + * `unknown` (an RPC blip must not freeze the node on a boot-time hiccup forever) and + * periodically rechecks once conclusive (so a post-boot on-chain change — e.g. the + * validator's permissionless `syncNode(nodeId)` deactivating an under-staked operator — + * eventually surfaces in /health instead of the startup result being shown forever). + * + * Timing assertions use a deliberate few-second margin either side of each boundary + * (mirroring liveness-keeper.service.spec.ts's convention) rather than asserting on the + * exact millisecond: Jest/@sinonjs fake timers' tickAsync does not guarantee sub-second + * precision when a timer callback itself chains further awaits before rescheduling. + */ +const realBls = new BlsService({} as any, {} as any); + +function makeKeyPair(seed: number) { + const privBytes = new Uint8Array(32); + privBytes[31] = seed; + const pub = sigs.getPublicKey(privBytes); + const compressedHex = "0x" + pub.toHex(); + const point = bls.G1.Point.fromHex(pub.toHex()); + const eip2537Hex = realBls.encodePublicKeyToEIP2537(point); + return { privateKey: "0x" + Buffer.from(privBytes).toString("hex"), compressedHex, eip2537Hex }; +} +const KEY_A = makeKeyPair(1); + +function nodeState(overrides: Partial = {}): NodeState { + return { + nodeId: "0x" + "ab".repeat(32), + nodeName: "n", + privateKey: KEY_A.privateKey, + publicKey: KEY_A.compressedHex, + createdAt: new Date(0).toISOString(), + description: "d", + ...overrides, + }; +} + +function makeChain(overrides: { + checkNodeRegistration?: jest.Mock<() => Promise>; + getNodePublicKey?: jest.Mock<() => Promise>; +}): BlockchainService { + return { + checkNodeRegistration: overrides.checkNodeRegistration ?? jest.fn(() => Promise.resolve(true)), + getNodePublicKey: + overrides.getNodePublicKey ?? jest.fn(() => Promise.resolve(KEY_A.eip2537Hex)), + } as unknown as BlockchainService; +} + +describe("NodeKeyConsistencyService — retry/recheck scheduling (CC-36 round 2)", () => { + beforeEach(() => { + jest.useFakeTimers(); + }); + + afterEach(() => { + jest.clearAllTimers(); + jest.useRealTimers(); + }); + + it("retries with bounded backoff (30s/60s/120s/240s) while stuck on unknown, then folds into the 15-minute cadence", async () => { + const checkNodeRegistration = jest.fn(() => Promise.reject(new Error("RPC down"))); + const chain = makeChain({ checkNodeRegistration: checkNodeRegistration as any }); + const service = new NodeKeyConsistencyService(chain, realBls); + + service.start(nodeState(), "0xcontract"); + await jest.advanceTimersByTimeAsync(10); // flush the immediate boot attempt + expect(checkNodeRegistration).toHaveBeenCalledTimes(1); + expect(service.getLastResult()?.status).toBe("unknown"); + + // 1st backoff step (~30s): not yet at 29s, fired by ~31s. + await jest.advanceTimersByTimeAsync(29_000); + expect(checkNodeRegistration).toHaveBeenCalledTimes(1); + await jest.advanceTimersByTimeAsync(3_000); + expect(checkNodeRegistration).toHaveBeenCalledTimes(2); + + // 2nd step (~60s). + await jest.advanceTimersByTimeAsync(57_000); + expect(checkNodeRegistration).toHaveBeenCalledTimes(2); + await jest.advanceTimersByTimeAsync(4_000); + expect(checkNodeRegistration).toHaveBeenCalledTimes(3); + + // 3rd step (~120s). + await jest.advanceTimersByTimeAsync(116_000); + expect(checkNodeRegistration).toHaveBeenCalledTimes(3); + await jest.advanceTimersByTimeAsync(5_000); + expect(checkNodeRegistration).toHaveBeenCalledTimes(4); + + // 4th and LAST backoff step (~240s). + await jest.advanceTimersByTimeAsync(235_000); + expect(checkNodeRegistration).toHaveBeenCalledTimes(4); + await jest.advanceTimersByTimeAsync(6_000); + expect(checkNodeRegistration).toHaveBeenCalledTimes(5); + + // Backoff exhausted: the NEXT retry folds into the regular 15-minute cadence, NOT + // another short step — still 5 calls well past where a 5th short retry would be. + await jest.advanceTimersByTimeAsync(10 * 60_000); + expect(checkNodeRegistration).toHaveBeenCalledTimes(5); + await jest.advanceTimersByTimeAsync(6 * 60_000); + expect(checkNodeRegistration).toHaveBeenCalledTimes(6); + + service.stop(); + }); + + it("rechecks periodically (every ~15 minutes) once a conclusive result is reached", async () => { + const checkNodeRegistration = jest.fn(() => Promise.resolve(true)); + const chain = makeChain({ checkNodeRegistration: checkNodeRegistration as any }); + const service = new NodeKeyConsistencyService(chain, realBls); + + service.start(nodeState(), "0xcontract"); + await jest.advanceTimersByTimeAsync(10); + expect(service.getLastResult()?.status).toBe("ok"); + expect(checkNodeRegistration).toHaveBeenCalledTimes(1); + + await jest.advanceTimersByTimeAsync(14 * 60_000); // well under 15 minutes + expect(checkNodeRegistration).toHaveBeenCalledTimes(1); + await jest.advanceTimersByTimeAsync(2 * 60_000); // push comfortably past it + expect(checkNodeRegistration).toHaveBeenCalledTimes(2); + + service.stop(); + }); + + it("a recheck RPC failure PRESERVES the prior conclusive result — never downgrades it to unknown", async () => { + const checkNodeRegistration = jest + .fn<() => Promise>() + .mockResolvedValueOnce(true) // boot: registered + .mockRejectedValueOnce(new Error("RPC blip") as never); // 15-min recheck: fails + const chain = makeChain({ checkNodeRegistration: checkNodeRegistration as any }); + const service = new NodeKeyConsistencyService(chain, realBls); + + service.start(nodeState(), "0xcontract"); + await jest.advanceTimersByTimeAsync(10); + const first = service.getLastResult(); + expect(first?.status).toBe("ok"); + + // Cross the ~15-minute recheck boundary WITHOUT also reaching the next (unknown-triggered) + // 30s backoff retry — i.e. land in (900s, 930s), not "comfortably past" by minutes, or the + // mock (deliberately configured for exactly 2 calls) would be exhausted by a 3rd attempt. + await jest.advanceTimersByTimeAsync(14 * 60_000); // well under 900s + expect(checkNodeRegistration).toHaveBeenCalledTimes(1); + await jest.advanceTimersByTimeAsync(65_000); // total ~905s: past 900s, still well under 930s + const second = service.getLastResult(); + // status/checkedAt/derivedMatches/nodeId are PRESERVED — a transient RPC failure must + // never launder a known conclusion into "unknown". + expect(second?.status).toBe("ok"); + expect(second?.checkedAt).toBe(first?.checkedAt); + expect(second?.derivedMatches).toBe(first?.derivedMatches); + // …but lastAttemptAt DID advance, proving a retry actually happened rather than the + // loop having silently died. + expect(second?.lastAttemptAt).not.toBe(first?.lastAttemptAt); + expect(checkNodeRegistration).toHaveBeenCalledTimes(2); + + service.stop(); + }); + + it("a recheck RPC failure must not paper over a real mismatch_onchain either", async () => { + const checkNodeRegistration = jest.fn(() => Promise.resolve(true)); + const getNodePublicKey = jest + .fn<() => Promise>() + .mockResolvedValueOnce("0x" + "11".repeat(128)) // boot: someone else's key registered + .mockRejectedValueOnce(new Error("RPC blip") as never); // recheck: RPC fails + const chain = makeChain({ + checkNodeRegistration: checkNodeRegistration as any, + getNodePublicKey: getNodePublicKey as any, + }); + const service = new NodeKeyConsistencyService(chain, realBls); + + service.start(nodeState(), "0xcontract"); + await jest.advanceTimersByTimeAsync(10); + expect(service.getLastResult()?.status).toBe("mismatch_onchain"); + + // Same landing window as the "ok preserved" test above — past the ~900s recheck, well + // short of the ~930s a subsequent unknown-triggered backoff retry would add. + await jest.advanceTimersByTimeAsync(14 * 60_000); + expect(getNodePublicKey).toHaveBeenCalledTimes(1); + await jest.advanceTimersByTimeAsync(65_000); + // Still mismatch_onchain — NOT washed out to "unknown" by the failed recheck attempt. + expect(service.getLastResult()?.status).toBe("mismatch_onchain"); + expect(getNodePublicKey).toHaveBeenCalledTimes(2); + + service.stop(); + }); + + it("stop() clears the timer — no leftover handle after shutdown", async () => { + const chain = makeChain({}); + const service = new NodeKeyConsistencyService(chain, realBls); + service.start(nodeState(), "0xcontract"); + await jest.advanceTimersByTimeAsync(10); + expect(jest.getTimerCount()).toBeGreaterThan(0); + + service.stop(); + expect(jest.getTimerCount()).toBe(0); + }); + + it("onModuleDestroy() also clears the timer (Nest lifecycle hook) — same guarantee as stop()", async () => { + const chain = makeChain({}); + const service = new NodeKeyConsistencyService(chain, realBls); + service.start(nodeState(), "0xcontract"); + await jest.advanceTimersByTimeAsync(10); + + service.onModuleDestroy(); + expect(jest.getTimerCount()).toBe(0); + + // And no further attempts happen even if time is advanced well past any cadence. + const chainCalls = (chain.checkNodeRegistration as jest.Mock).mock.calls.length; + await jest.advanceTimersByTimeAsync(60 * 60_000); + expect((chain.checkNodeRegistration as jest.Mock).mock.calls.length).toBe(chainCalls); + }); + + it("start() is idempotent: calling it again restarts cleanly instead of stacking loops", async () => { + const checkNodeRegistration = jest.fn(() => Promise.resolve(true)); + const chain = makeChain({ checkNodeRegistration: checkNodeRegistration as any }); + const service = new NodeKeyConsistencyService(chain, realBls); + + service.start(nodeState(), "0xcontract"); + await jest.advanceTimersByTimeAsync(10); + expect(checkNodeRegistration).toHaveBeenCalledTimes(1); + + service.start(nodeState(), "0xcontract"); // restart before any timer fires + await jest.advanceTimersByTimeAsync(10); + expect(checkNodeRegistration).toHaveBeenCalledTimes(2); // one fresh immediate check, not two + + // Only ONE timer pending, not a second stacked loop still running from the first start(). + expect(jest.getTimerCount()).toBe(1); + + service.stop(); + }); +}); diff --git a/src/modules/node/node-key-consistency.service.spec.ts b/src/modules/node/node-key-consistency.service.spec.ts index 02cf41b..875f873 100644 --- a/src/modules/node/node-key-consistency.service.spec.ts +++ b/src/modules/node/node-key-consistency.service.spec.ts @@ -1,8 +1,19 @@ import { jest } from "@jest/globals"; import { NodeKeyConsistencyService } from "./node-key-consistency.service.js"; -import { bls, sigs, encodeG1Point } from "../../utils/bls.util.js"; +import { bls, sigs } from "../../utils/bls.util.js"; import { NodeState } from "../../interfaces/node.interface.js"; import { BlockchainService } from "../blockchain/blockchain.service.js"; +import { BlsService } from "../bls/bls.service.js"; + +/** + * A real BlsService — NOT a mock — used purely for its `encodePublicKeyToEIP2537` (a pure + * function of its argument; it reads none of the injected BlockchainService/SignerService + * state). Fixtures are built with the exact same encoder the check itself is required to + * use (CC-36 round-2 review), so "ok" here means byte-identical agreement with what + * `registerOnChain()` actually writes on-chain — not agreement with a second, possibly + * drifted, re-implementation. + */ +const realBls = new BlsService({} as any, {} as any); /** * Real BLS keypairs (not hardcoded fixtures) so every fixture is internally @@ -16,7 +27,7 @@ function makeKeyPair(seed: number) { const pub = sigs.getPublicKey(privBytes); const compressedHex = "0x" + pub.toHex(); // node_state.publicKey format (48-byte compressed) const point = bls.G1.Point.fromHex(pub.toHex()); - const eip2537Hex = "0x" + Buffer.from(encodeG1Point(point)).toString("hex"); // on-chain format (128-byte) + const eip2537Hex = realBls.encodePublicKeyToEIP2537(point); // on-chain format (128-byte) return { privateKey, compressedHex, eip2537Hex }; } @@ -53,7 +64,7 @@ describe("NodeKeyConsistencyService (CC-36)", () => { checkNodeRegistration: () => Promise.resolve(true), getNodePublicKey: () => Promise.resolve(KEY_A.eip2537Hex), }); - const service = new NodeKeyConsistencyService(chain); + const service = new NodeKeyConsistencyService(chain, realBls); const result = await service.check(baseNodeState(), "0xcontract"); expect(result.status).toBe("ok"); expect(service.getLastResult()).toEqual(result); @@ -64,7 +75,7 @@ describe("NodeKeyConsistencyService (CC-36)", () => { checkNodeRegistration: () => Promise.resolve(true), getNodePublicKey: () => Promise.resolve(KEY_A.eip2537Hex), }); - const service = new NodeKeyConsistencyService(chain); + const service = new NodeKeyConsistencyService(chain, realBls); // privateKey is A's, but publicKey on disk is B's — a corrupted/swapped node_state. const state = baseNodeState({ publicKey: KEY_B.compressedHex }); const result = await service.check(state, "0xcontract"); @@ -76,7 +87,7 @@ describe("NodeKeyConsistencyService (CC-36)", () => { checkNodeRegistration: () => Promise.resolve(true), getNodePublicKey: () => Promise.resolve(KEY_B.eip2537Hex), // someone else's key is registered }); - const service = new NodeKeyConsistencyService(chain); + const service = new NodeKeyConsistencyService(chain, realBls); const result = await service.check(baseNodeState(), "0xcontract"); expect(result.status).toBe("mismatch_onchain"); }); @@ -85,7 +96,7 @@ describe("NodeKeyConsistencyService (CC-36)", () => { const chain = makeBlockchainMock({ checkNodeRegistration: () => Promise.resolve(false), }); - const service = new NodeKeyConsistencyService(chain); + const service = new NodeKeyConsistencyService(chain, realBls); const result = await service.check(baseNodeState(), "0xcontract"); expect(result.status).toBe("not_registered"); // getNodePublicKey must not even be consulted once we know it's unregistered. @@ -97,7 +108,7 @@ describe("NodeKeyConsistencyService (CC-36)", () => { (chain.checkNodeRegistration as jest.Mock).mockRejectedValueOnce( new Error("RPC timeout") as never ); - const service = new NodeKeyConsistencyService(chain); + const service = new NodeKeyConsistencyService(chain, realBls); const result = await service.check(baseNodeState(), "0xcontract"); expect(result.status).toBe("unknown"); }); @@ -109,7 +120,7 @@ describe("NodeKeyConsistencyService (CC-36)", () => { (chain.getNodePublicKey as jest.Mock).mockRejectedValueOnce( new Error("RPC timeout") as never ); - const service = new NodeKeyConsistencyService(chain); + const service = new NodeKeyConsistencyService(chain, realBls); const result = await service.check(baseNodeState(), "0xcontract"); expect(result.status).toBe("unknown"); }); @@ -119,7 +130,7 @@ describe("NodeKeyConsistencyService (CC-36)", () => { checkNodeRegistration: () => Promise.resolve(true), getNodePublicKey: () => Promise.resolve(KEY_A.eip2537Hex), }); - const service = new NodeKeyConsistencyService(chain); + const service = new NodeKeyConsistencyService(chain, realBls); const state = baseNodeState(); delete (state as Partial).privateKey; const result = await service.check(state, "0xcontract"); @@ -131,7 +142,7 @@ describe("NodeKeyConsistencyService (CC-36)", () => { checkNodeRegistration: () => Promise.resolve(true), getNodePublicKey: () => Promise.resolve(KEY_B.eip2537Hex), }); - const service = new NodeKeyConsistencyService(chain); + const service = new NodeKeyConsistencyService(chain, realBls); const state = baseNodeState(); delete (state as Partial).privateKey; const result = await service.check(state, "0xcontract"); @@ -146,7 +157,7 @@ describe("NodeKeyConsistencyService (CC-36)", () => { checkNodeRegistration: () => Promise.resolve(true), getNodePublicKey: () => Promise.resolve(KEY_A.eip2537Hex), }); - const service = new NodeKeyConsistencyService(chain); + const service = new NodeKeyConsistencyService(chain, realBls); const bootstrapNodeId = "0x" + "11".repeat(32); // deliberately NOT keccak256(KEY_A.eip2537Hex) const state = baseNodeState({ nodeId: bootstrapNodeId }); const result = await service.check(state, "0xcontract"); @@ -162,7 +173,7 @@ describe("NodeKeyConsistencyService (CC-36)", () => { checkNodeRegistration: () => Promise.resolve(true), getNodePublicKey: () => Promise.resolve(KEY_A.eip2537Hex), }); - const service = new NodeKeyConsistencyService(chain); + const service = new NodeKeyConsistencyService(chain, realBls); const result = await service.check(baseNodeState({ nodeId: derivedNodeId }), "0xcontract"); expect(result.status).toBe("ok"); expect(result.derivedMatches).toBe(true); @@ -174,8 +185,44 @@ describe("NodeKeyConsistencyService (CC-36)", () => { // Same bytes as KEY_A.eip2537Hex, but uppercase and without the 0x prefix. getNodePublicKey: () => Promise.resolve(KEY_A.eip2537Hex.slice(2).toUpperCase()), }); - const service = new NodeKeyConsistencyService(chain); + const service = new NodeKeyConsistencyService(chain, realBls); const result = await service.check(baseNodeState(), "0xcontract"); expect(result.status).toBe("ok"); }); + + it("uses BlsService.encodePublicKeyToEIP2537 — NOT a local re-implementation — to encode for the on-chain comparison (CC-36 round 2)", async () => { + // A stub BlsService that returns an unmistakable sentinel instead of a real encoding. + // If the check ever reverted to a local re-implementation of the encoder (ignoring the + // injected BlsService), this stub's output would never be consulted, the comparison + // would use the REAL encoding instead, `getNodePublicKey`'s sentinel wouldn't match it, + // and this test would go red on "ok" -> "mismatch_onchain". + const sentinelEip2537 = "0x" + "ee".repeat(128); + const encodeSpy = jest.fn(() => sentinelEip2537); + const stubBls = { encodePublicKeyToEIP2537: encodeSpy } as unknown as BlsService; + const chain = makeBlockchainMock({ + checkNodeRegistration: () => Promise.resolve(true), + getNodePublicKey: () => Promise.resolve(sentinelEip2537), + }); + const service = new NodeKeyConsistencyService(chain, stubBls); + const result = await service.check(baseNodeState(), "0xcontract"); + expect(encodeSpy).toHaveBeenCalled(); + expect(result.status).toBe("ok"); + }); + + it("a mismatch is still correctly detected when the STUB encoder disagrees with the on-chain read", async () => { + // Same stub as above, but getNodePublicKey now returns something OTHER than the stub's + // sentinel — proves the comparison is actually USING the injected BlsService's output + // (not, say, ignoring it and always returning ok). + const sentinelEip2537 = "0x" + "ee".repeat(128); + const stubBls = { + encodePublicKeyToEIP2537: jest.fn(() => sentinelEip2537), + } as unknown as BlsService; + const chain = makeBlockchainMock({ + checkNodeRegistration: () => Promise.resolve(true), + getNodePublicKey: () => Promise.resolve("0x" + "11".repeat(128)), + }); + const service = new NodeKeyConsistencyService(chain, stubBls); + const result = await service.check(baseNodeState(), "0xcontract"); + expect(result.status).toBe("mismatch_onchain"); + }); }); diff --git a/src/modules/node/node-key-consistency.service.ts b/src/modules/node/node-key-consistency.service.ts index 6ee7d9f..c06515a 100644 --- a/src/modules/node/node-key-consistency.service.ts +++ b/src/modules/node/node-key-consistency.service.ts @@ -1,7 +1,8 @@ -import { Injectable, Logger } from "@nestjs/common"; +import { Injectable, Logger, OnModuleDestroy } from "@nestjs/common"; import { ethers } from "ethers"; -import { bls, sigs, encodeG1Point } from "../../utils/bls.util.js"; +import { bls, sigs } from "../../utils/bls.util.js"; import { BlockchainService } from "../blockchain/blockchain.service.js"; +import { BlsService } from "../bls/bls.service.js"; import { NodeState } from "../../interfaces/node.interface.js"; export type KeyConsistencyStatus = @@ -13,8 +14,15 @@ export type KeyConsistencyStatus = | "skipped_no_private_key"; export interface KeyConsistencyResult { + /** + * Most recent CONCLUSIVE status (i.e. one this instance actually determined, never a + * transient RPC failure "laundered" into `unknown` over a known conclusion — see `check()`). + */ status: KeyConsistencyStatus; + /** When `status` (and `derivedMatches`) was last actually determined — NOT bumped by a failed retry. */ checkedAt: string; + /** When the most recent check ATTEMPT ran, successful or not — proves the retry loop is alive. */ + lastAttemptAt: string; /** * Informational only (per CC-36): whether `nodeId === keccak256(EIP-2537 publicKey)`. * The contract's `registerWithProof` path derives nodeId this way, but the older @@ -27,6 +35,12 @@ export interface KeyConsistencyResult { nodeId: string; } +/** The outcome of a single, stateless comparison — before it's merged into the held `lastResult`. */ +interface FreshCheck { + status: KeyConsistencyStatus; + derivedMatches: boolean | null; +} + function normalizeHex(hex: string | undefined | null): string { return "0x" + (hex ?? "").replace(/^0x/i, "").toLowerCase(); } @@ -41,63 +55,171 @@ function normalizeHex(hex: string | undefined | null): string { * fail verification against the on-chain registration forever — the node is * online and silently non-participating. * - * Three-way comparison: + * Three-way comparison (one attempt, see `computeFresh`): * 1. local: privateKey -> derive pubkey -> compare to node_state.publicKey - * 2. on-chain: node_state.publicKey (EIP-2537-encoded) -> compare to + * 2. on-chain: node_state.publicKey (EIP-2537-encoded via the SAME encoder the + * registration path writes with — BlsService.encodePublicKeyToEIP2537, + * never a local re-implementation, so this check can never silently + * drift from what registerOnChain() actually wrote) -> compare to * registeredKeys(nodeId) (only meaningful when isRegistered(nodeId)) * 3. derivedMatches (informational only): nodeId == keccak256(EIP-2537 publicKey) * - * Fail-loud, not fail-stop: `check()` never throws. Every failure mode (malformed - * key material, RPC error, timeout) collapses into a `status` value so a broken - * node stays observable instead of taking the process down with it — a node that - * already can't produce valid signatures gains nothing from also losing its - * health/metrics endpoint. + * Fail-loud, not fail-stop: a single attempt never throws. Every failure mode + * (malformed key material, RPC error, timeout) collapses into a `status` value so a + * broken node stays observable instead of taking the process down with it. + * + * Runs as a self-scheduling loop (`start()`), not a one-shot: an `unknown` result + * (RPC error) retries with bounded backoff (30s/60s/120s/240s, then folding into the + * regular cadence) rather than freezing forever on a boot-time hiccup, and a + * CONCLUSIVE result is periodically rechecked (every RECHECK_INTERVAL_MS) so a node + * that gets kicked off the on-chain registry post-boot (the validator's permissionless + * `syncNode(nodeId)` deactivating an under-staked/role-losing operator) surfaces as + * `not_registered` in /health instead of showing its startup `ok` forever. A recheck + * that itself fails RPC never downgrades a known conclusion to `unknown` — see `check()`. */ @Injectable() -export class NodeKeyConsistencyService { +export class NodeKeyConsistencyService implements OnModuleDestroy { + /** Backoff schedule while stuck on `unknown` (RPC down). After this it folds into RECHECK_INTERVAL_MS. */ + private static readonly UNKNOWN_BACKOFF_MS = [30_000, 60_000, 120_000, 240_000]; + /** Cadence once (and while) a conclusive result holds — also the post-backoff fallback cadence. */ + private static readonly RECHECK_INTERVAL_MS = 15 * 60_000; + private readonly logger = new Logger(NodeKeyConsistencyService.name); private lastResult: KeyConsistencyResult | null = null; + /** Consecutive `unknown` FRESH attempts (independent of `lastResult.status`, which may be a preserved older conclusion). */ + private consecutiveUnknown = 0; + private timer: ReturnType | null = null; + private stopped = true; - constructor(private readonly blockchainService: BlockchainService) {} + constructor( + private readonly blockchainService: BlockchainService, + private readonly blsService: BlsService + ) {} - /** The most recent check() result, or null if none has run yet. Read by /health. */ + /** The most recently held result (a preserved conclusion survives a failed retry). Read by /health. */ getLastResult(): KeyConsistencyResult | null { return this.lastResult; } + /** + * Start the self-check loop: an immediate check, then bounded-backoff retries while + * `unknown`, then periodic rechecks once conclusive. Idempotent — calling again restarts + * the loop from scratch (stopping any existing timer first) rather than stacking loops. + */ + start(nodeState: NodeState, contractAddress: string): void { + this.stop(); + this.stopped = false; + void this.loop(nodeState, contractAddress); + } + + /** Stop the loop and clear its timer. Safe to call repeatedly, or before `start()`. */ + stop(): void { + this.stopped = true; + if (this.timer !== null) { + clearTimeout(this.timer); + this.timer = null; + } + } + + /** Nest lifecycle hook — guarantees the timer never survives module/app shutdown. */ + onModuleDestroy(): void { + this.stop(); + } + + private async loop(nodeState: NodeState, contractAddress: string): Promise { + await this.check(nodeState, contractAddress); + if (this.stopped) { + // stop() raced with this in-flight attempt — don't schedule a next one. + return; + } + const delay = this.nextDelayMs(); + this.timer = setTimeout(() => { + void this.loop(nodeState, contractAddress); + }, delay); + // Never keep the process alive just for this self-check (mirrors the rest of the + // codebase's fire-and-forget timers; this one just says so explicitly). + this.timer.unref?.(); + } + + private nextDelayMs(): number { + const backoff = NodeKeyConsistencyService.UNKNOWN_BACKOFF_MS; + if (this.consecutiveUnknown > 0 && this.consecutiveUnknown <= backoff.length) { + return backoff[this.consecutiveUnknown - 1]; + } + return NodeKeyConsistencyService.RECHECK_INTERVAL_MS; + } + + /** + * Run one attempt and merge it into the held result. + * + * The merge rule is the load-bearing part: a fresh `unknown` (this attempt's RPC failed) + * NEVER overwrites a previously-held CONCLUSIVE result — it only bumps `lastAttemptAt`. + * Reading a network blip as "we no longer know" would silently launder away a real + * mismatch_onchain the moment the RPC hiccups, which defeats the whole point of this + * check. `checkedAt` only advances when a conclusion was actually (re-)established. + */ async check(nodeState: NodeState, contractAddress: string): Promise { - const checkedAt = new Date().toISOString(); - const derivedMatches = this.computeDerivedMatches(nodeState); + const now = new Date().toISOString(); + const fresh = await this.computeFresh(nodeState, contractAddress); - const local = this.checkLocal(nodeState); - const onchain = await this.checkOnchain(nodeState, contractAddress); - const status = this.combine(local, onchain); + if (fresh.status === "mismatch_local" || fresh.status === "mismatch_onchain") { + this.logMismatch(fresh.status, nodeState); + } - const result: KeyConsistencyResult = { - status, - checkedAt, - derivedMatches, - nodeId: nodeState.nodeId, - }; - - if (status === "mismatch_local" || status === "mismatch_onchain") { - const localPrefix = this.prefix(nodeState.publicKey); - this.logger.error( - `KEY CONSISTENCY FAILURE [${status}] for node ${nodeState.nodeId}: ` + - (status === "mismatch_local" - ? `the private key in node_state.json does NOT derive node_state.publicKey ` + - `(local publicKey=${localPrefix}...). ` - : `node_state.publicKey (${localPrefix}...) does NOT match the on-chain ` + - `registeredKeys() entry for this nodeId. `) + - `This node will keep signing, but its signatures can NEVER verify against the ` + - `on-chain registration — it is online and healthy while being silently ` + - `non-participating. Fix: re-register the correct public key on-chain for this ` + - `nodeId, or restore the matching privateKey/publicKey pair in node_state.json.` - ); + if (fresh.status === "unknown") { + this.consecutiveUnknown++; + if (this.lastResult && this.lastResult.status !== "unknown") { + this.lastResult = { ...this.lastResult, lastAttemptAt: now }; + } else { + // No prior conclusion to preserve (first-ever attempt, or every attempt so far has + // been unknown) — "unknown" IS the current conclusion in that case. + this.lastResult = { + status: "unknown", + checkedAt: now, + lastAttemptAt: now, + derivedMatches: fresh.derivedMatches, + nodeId: nodeState.nodeId, + }; + } + } else { + this.consecutiveUnknown = 0; + this.lastResult = { + status: fresh.status, + checkedAt: now, + lastAttemptAt: now, + derivedMatches: fresh.derivedMatches, + nodeId: nodeState.nodeId, + }; } - this.lastResult = result; - return result; + return this.lastResult; + } + + private logMismatch(status: "mismatch_local" | "mismatch_onchain", nodeState: NodeState): void { + const localPrefix = this.prefix(nodeState.publicKey); + this.logger.error( + `KEY CONSISTENCY FAILURE [${status}] for node ${nodeState.nodeId}: ` + + (status === "mismatch_local" + ? `the private key in node_state.json does NOT derive node_state.publicKey ` + + `(local publicKey=${localPrefix}...). ` + : `node_state.publicKey (${localPrefix}...) does NOT match the on-chain ` + + `registeredKeys() entry for this nodeId. `) + + `This node will keep signing, but its signatures can NEVER verify against the ` + + `on-chain registration — it is online and healthy while being silently ` + + `non-participating. Fix: re-register the correct public key on-chain for this ` + + `nodeId, or restore the matching privateKey/publicKey pair in node_state.json.` + ); + } + + private async computeFresh( + nodeState: NodeState, + contractAddress: string + ): Promise { + const derivedMatches = this.computeDerivedMatches(nodeState); + const local = this.checkLocal(nodeState); + const onchain = await this.checkOnchain(nodeState, contractAddress); + const status = this.combine(local, onchain); + return { status, derivedMatches }; } private prefix(hex: string | undefined): string { @@ -183,15 +305,21 @@ export class NodeKeyConsistencyService { return normalizeHex(derivedNodeId) === normalizeHex(nodeState.nodeId); } - /** Decompress a 48-byte compressed G1 hex pubkey and re-encode it as 128-byte EIP-2537. */ + /** + * Decompress a 48-byte compressed G1 hex pubkey and re-encode it as 128-byte EIP-2537 — + * via `BlsService.encodePublicKeyToEIP2537`, the SAME encoder `registerOnChain()` writes + * on-chain with (and gossip/dashboard/quorum-cosigner all read on-chain data with). This is + * deliberately NOT a local re-implementation: a future change to the encoder would + * otherwise silently diverge between what got written on-chain and what this check + * compares against, causing exactly the kind of undetected drift this check exists to catch. + */ private toEip2537Hex(compressedPublicKeyHex: string | undefined): string | null { if (!compressedPublicKeyHex) { return null; } try { const point = bls.G1.Point.fromHex(compressedPublicKeyHex.replace(/^0x/i, "")); - const encoded = encodeG1Point(point); - return "0x" + Buffer.from(encoded).toString("hex"); + return this.blsService.encodePublicKeyToEIP2537(point); } catch { return null; } diff --git a/src/modules/node/node.key-consistency-wiring.spec.ts b/src/modules/node/node.key-consistency-wiring.spec.ts index 673ffed..ecf20e0 100644 --- a/src/modules/node/node.key-consistency-wiring.spec.ts +++ b/src/modules/node/node.key-consistency-wiring.spec.ts @@ -6,11 +6,13 @@ import { ConfigService } from "@nestjs/config"; import { NodeService } from "./node.service.js"; /** - * CC-36 wiring: NodeService kicks off the key-consistency self-check after loading - * node_state.json, WITHOUT awaiting it (startup must never block on a slow/unavailable - * RPC), and exposes the last result via getKeyConsistency(). The check's own branch - * coverage (ok/mismatch_local/mismatch_onchain/not_registered/unknown/skipped) lives in - * node-key-consistency.service.spec.ts — this file only covers the NodeService glue. + * CC-36 wiring: NodeService kicks off the key-consistency self-check's self-scheduling + * loop (`start()`) after loading node_state.json — synchronously, never awaited (startup + * must never block on a slow/unavailable RPC) — and exposes the last result via + * getKeyConsistency(). The check's own branch coverage (ok/mismatch_local/mismatch_onchain/ + * not_registered/unknown/skipped) lives in node-key-consistency.service.spec.ts, and its + * retry/backoff/recheck scheduling lives in node-key-consistency.service.scheduling.spec.ts. + * This file only covers the NodeService glue. */ describe("NodeService — key-consistency self-check wiring (CC-36)", () => { const nodeState = { @@ -22,7 +24,7 @@ describe("NodeService — key-consistency self-check wiring (CC-36)", () => { privateKey: "0x" + "1".padStart(64, "0"), }; - it("fires the check without awaiting it — initializeNode resolves before a slow check finishes", async () => { + it("calls start() once after loading node state, and initializeNode does not wait on it", async () => { const dir = mkdtempSync(join(tmpdir(), "kc-")); const file = join(dir, "node_state.json"); writeFileSync(file, JSON.stringify(nodeState)); @@ -30,13 +32,8 @@ describe("NodeService — key-consistency self-check wiring (CC-36)", () => { get: (k: string) => (k === "validatorContractAddress" ? "0xcontract" : undefined), } as unknown as ConfigService; - let resolveCheck: () => void = () => {}; - const checkPromise = new Promise(resolve => { - resolveCheck = () => - resolve({ status: "ok", checkedAt: "t", derivedMatches: true, nodeId: "0x1" }); - }); - const checkFn = jest.fn(() => checkPromise); - const keyConsistencyService = { check: checkFn, getLastResult: () => null } as any; + const startFn = jest.fn(); + const keyConsistencyService = { start: startFn, getLastResult: () => null } as any; const svc = new NodeService({} as any, {} as any, config, keyConsistencyService); // initializeNode() recomputes nodeStateFilePath from process.cwd() (fixed-name @@ -46,15 +43,8 @@ describe("NodeService — key-consistency self-check wiring (CC-36)", () => { try { await (svc as any).initializeNode(); - // The check was kicked off … - expect(checkFn).toHaveBeenCalledTimes(1); - expect(checkFn).toHaveBeenCalledWith( - expect.objectContaining({ nodeId: "0x1" }), - "0xcontract" - ); - // … but initializeNode did NOT wait for it to resolve. - resolveCheck(); - await checkPromise; // let it settle for cleanliness + expect(startFn).toHaveBeenCalledTimes(1); + expect(startFn).toHaveBeenCalledWith(expect.objectContaining({ nodeId: "0x1" }), "0xcontract"); } finally { cwdSpy.mockRestore(); rmSync(dir, { recursive: true, force: true }); @@ -64,7 +54,7 @@ describe("NodeService — key-consistency self-check wiring (CC-36)", () => { it("getKeyConsistency() delegates to the injected service's getLastResult()", () => { const fakeResult = { status: "ok", checkedAt: "t", derivedMatches: true, nodeId: "0x1" } as any; const keyConsistencyService = { - check: jest.fn(() => Promise.resolve(fakeResult)), + start: jest.fn(), getLastResult: () => fakeResult, } as any; const config = { get: () => undefined } as unknown as ConfigService; @@ -78,7 +68,7 @@ describe("NodeService — key-consistency self-check wiring (CC-36)", () => { expect(svc.getKeyConsistency()).toBeNull(); }); - it("never throws from initializeNode when the consistency service itself rejects", async () => { + it("never throws from initializeNode when start() itself throws synchronously", async () => { const dir = mkdtempSync(join(tmpdir(), "kc-")); const file = join(dir, "node_state.json"); writeFileSync(file, JSON.stringify(nodeState)); @@ -86,16 +76,15 @@ describe("NodeService — key-consistency self-check wiring (CC-36)", () => { get: (k: string) => (k === "validatorContractAddress" ? "0xcontract" : undefined), } as unknown as ConfigService; const keyConsistencyService = { - check: jest.fn(() => Promise.reject(new Error("boom — should never happen"))), + start: jest.fn(() => { + throw new Error("boom — should never happen"); + }), getLastResult: () => null, } as any; const svc = new NodeService({} as any, {} as any, config, keyConsistencyService); const cwdSpy = jest.spyOn(process, "cwd").mockReturnValue(dir); try { await expect((svc as any).initializeNode()).resolves.toBeUndefined(); - // Drain the microtask queue so the fire-and-forget rejection is observed/logged, - // not left as an unhandled rejection leaking into a later test. - await new Promise(resolve => setImmediate(resolve)); } finally { cwdSpy.mockRestore(); rmSync(dir, { recursive: true, force: true }); diff --git a/src/modules/node/node.service.ts b/src/modules/node/node.service.ts index 7693b46..23f7ce8 100644 --- a/src/modules/node/node.service.ts +++ b/src/modules/node/node.service.ts @@ -43,11 +43,11 @@ export class NodeService implements OnModuleInit { this.loadExistingNodeState(); if (this.nodeState) { this.logger.log(`Loaded node state: ${this.nodeState.nodeId}`); - // CC-36: best-effort, fire-and-forget — a slow/unavailable RPC must never - // delay or block startup (`void`, not `await`). The check itself never - // throws (fail-loud via logger.error, fail-open on the process), but the - // wrapper still guards against a future bug in the check surfacing here. - void this.runKeyConsistencyCheck(); + // CC-36: best-effort — a slow/unavailable RPC must never delay or block startup. + // This starts the self-scheduling retry/recheck loop; it does not await anything + // itself. The check itself never throws (fail-loud via logger.error, fail-open on + // the process), but the wrapper still guards against a future bug surfacing here. + this.runKeyConsistencyCheck(); } } else { this.logger.log("No node state file found. Node is not created yet."); @@ -55,13 +55,18 @@ export class NodeService implements OnModuleInit { } } - /** See NodeKeyConsistencyService — never awaited on the startup path, never throws. */ - private async runKeyConsistencyCheck(): Promise { + /** + * See NodeKeyConsistencyService — starts its self-scheduling retry/recheck loop. + * `start()` itself is synchronous and fire-and-forget internally (never awaited on the + * startup path); this wrapper only guards against a future bug surfacing as an unhandled + * exception here instead of the check's own designed fail-loud logging. + */ + private runKeyConsistencyCheck(): void { if (!this.nodeState || !this.keyConsistencyService) { return; } try { - await this.keyConsistencyService.check(this.nodeState, this.contractAddress); + this.keyConsistencyService.start(this.nodeState, this.contractAddress); } catch (error: any) { this.logger.error( `Key-consistency self-check crashed unexpectedly (this should never happen — the check ` + diff --git a/src/utils/bls.util.ts b/src/utils/bls.util.ts index e9e32e4..0652786 100644 --- a/src/utils/bls.util.ts +++ b/src/utils/bls.util.ts @@ -24,25 +24,6 @@ export function hexToBytes(hex: string): Uint8Array { return bytes; } -/** - * Encode a G1 point (BLS public key) to the 128-byte EIP-2537 wire format — the - * uncompressed x||y form the on-chain validator stores in `registeredKeys`. - * Mirrors `BlsService.encodeG1Point` (kept private there, on the sign path); this - * copy lives here so read-only consumers (e.g. the node key-consistency - * self-check) don't need to reach into BlsService for a pure encoding helper. - */ -export function encodeG1Point(point: any): Uint8Array { - const result = new Uint8Array(128); - const affine = point.toAffine(); - - const xBytes = hexToBytes(affine.x.toString(16).padStart(96, "0")); - const yBytes = hexToBytes(affine.y.toString(16).padStart(96, "0")); - - result.set(xBytes, 16); // Skip 16 zero bytes at start - result.set(yBytes, 80); // Skip 16 zero bytes at start - return result; -} - export const BLS_DST = "BLS_SIG_BLS12381G2_XMD:SHA-256_SSWU_RO_POP_"; export const bls = bls12_381; export const sigs = bls.longSignatures; From 55c1c10a970709b544c909355f61546b96a1afb3 Mon Sep 17 00:00:00 2001 From: jhfnetboy Date: Sun, 27 Sep 2026 14:34:47 +0700 Subject: [PATCH 3/9] fix(CC-36 round 3): accept both publicKey wire formats, use the real signer, survive any throw, discard stale in-flight checks, and rebind on identity change MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex adversarial review round 1 (REQUEST_CHANGES, 3 High + 2 Medium), all addressed: High-1 (verified by the coordinator): node_state.publicKey is NOT always 48-byte compressed — dashboard.service.ts's create-node path stores the 128-byte EIP-2537 form directly (`encodePublicKeyToEIP2537(publicKeyPoint)`), and import-node.dto.ts accepts either. Assuming compressed-only misreported every dashboard-created node as mismatch_local. `parsePublicKeyPoint()` now accepts BOTH formats: 48-byte compressed via `bls.G1.Point.fromHex`, 128-byte EIP-2537 via `bls.G1.Point.fromAffine({x,y})` (the same @noble/curves primitive `.fromHex()` uses internally) followed by `.assertValidity()` — the identical subgroup/curve check, so a malformed 128-byte value is rejected exactly like a malformed compressed one, never silently accepted. No such decoder existed anywhere else in the codebase to reuse (checked: gossip/dashboard/ gossip-quorum-cosigner only ever go compressed->EIP-2537, never the reverse), so this uses the curve library's own public API rather than inventing a byte-parser. Medium-5: local key derivation now goes through SignerService.forNode() / LocalKeySigner — the REAL signing path — instead of a local hex re-parse. A private key the signer would reject (wrong length, non-hex char, or an uppercase "0X" prefix LocalKeySigner's case-sensitive check doesn't strip) now correctly fails this check too, closing a false-"ok" gap. High-3: hardened `prefix()`/`normalizeHex()` against non-string node_state fields (raw disk JSON has no schema validation), AND added a structural boundary in `loop()`: `check()` is wrapped in its own try/catch so a future unexpected throw can never (a) produce an unhandledRejection or (b) kill the retry loop's future scheduling — it keeps ticking. The outer `.catch(() => {})` at both loop() call sites is defense-in-depth on top of that. Medium-4: `start()`/`stop()` now bump a generation token; `loop()` captures it and `check()` discards (does not merge into lastResult, does not log) a stale attempt whose generation no longer matches — so an in-flight check from a just-superseded identity can never clobber the new identity's result or stack a second timer. `start()` also clears the held result immediately so a previous identity is never shown even momentarily under a new loop. High-2: NodeService.reloadNodeState() (called by dashboard create/import/ delete) now restarts the self-check against the new identity, or stops + clears it when the node was deleted — previously it never touched the check at all, so a deleted node's stale "ok" would sit in /health forever. Mutation-verified (not committed): removing the 128-byte branch from parsePublicKeyPoint() turns 3 High-1 tests red; disabling the generation discard in check() turns the Medium-4 stale-result test red — both confirmed locally, then reverted before this commit. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Pdq9rgkZq9M4JFeTZD7yYs --- ...key-consistency.service.scheduling.spec.ts | 114 ++++++++- .../node/node-key-consistency.service.spec.ts | 137 +++++++++-- .../node/node-key-consistency.service.ts | 220 ++++++++++++++---- .../node/node.key-consistency-wiring.spec.ts | 120 ++++++++++ src/modules/node/node.module.ts | 6 +- src/modules/node/node.service.ts | 13 ++ 6 files changed, 540 insertions(+), 70 deletions(-) diff --git a/src/modules/node/node-key-consistency.service.scheduling.spec.ts b/src/modules/node/node-key-consistency.service.scheduling.spec.ts index 1e0c559..27e34c3 100644 --- a/src/modules/node/node-key-consistency.service.scheduling.spec.ts +++ b/src/modules/node/node-key-consistency.service.scheduling.spec.ts @@ -4,6 +4,7 @@ import { bls, sigs } from "../../utils/bls.util.js"; import { NodeState } from "../../interfaces/node.interface.js"; import { BlockchainService } from "../blockchain/blockchain.service.js"; import { BlsService } from "../bls/bls.service.js"; +import { SignerService } from "../signer/signer.service.js"; /** * CC-36 round 2: the self-check is not a one-shot — it retries with bounded backoff while @@ -18,6 +19,7 @@ import { BlsService } from "../bls/bls.service.js"; * precision when a timer callback itself chains further awaits before rescheduling. */ const realBls = new BlsService({} as any, {} as any); +const realSigner = new SignerService({ get: () => undefined } as any); function makeKeyPair(seed: number) { const privBytes = new Uint8Array(32); @@ -66,7 +68,7 @@ describe("NodeKeyConsistencyService — retry/recheck scheduling (CC-36 round 2) it("retries with bounded backoff (30s/60s/120s/240s) while stuck on unknown, then folds into the 15-minute cadence", async () => { const checkNodeRegistration = jest.fn(() => Promise.reject(new Error("RPC down"))); const chain = makeChain({ checkNodeRegistration: checkNodeRegistration as any }); - const service = new NodeKeyConsistencyService(chain, realBls); + const service = new NodeKeyConsistencyService(chain, realBls, realSigner); service.start(nodeState(), "0xcontract"); await jest.advanceTimersByTimeAsync(10); // flush the immediate boot attempt @@ -110,7 +112,7 @@ describe("NodeKeyConsistencyService — retry/recheck scheduling (CC-36 round 2) it("rechecks periodically (every ~15 minutes) once a conclusive result is reached", async () => { const checkNodeRegistration = jest.fn(() => Promise.resolve(true)); const chain = makeChain({ checkNodeRegistration: checkNodeRegistration as any }); - const service = new NodeKeyConsistencyService(chain, realBls); + const service = new NodeKeyConsistencyService(chain, realBls, realSigner); service.start(nodeState(), "0xcontract"); await jest.advanceTimersByTimeAsync(10); @@ -131,7 +133,7 @@ describe("NodeKeyConsistencyService — retry/recheck scheduling (CC-36 round 2) .mockResolvedValueOnce(true) // boot: registered .mockRejectedValueOnce(new Error("RPC blip") as never); // 15-min recheck: fails const chain = makeChain({ checkNodeRegistration: checkNodeRegistration as any }); - const service = new NodeKeyConsistencyService(chain, realBls); + const service = new NodeKeyConsistencyService(chain, realBls, realSigner); service.start(nodeState(), "0xcontract"); await jest.advanceTimersByTimeAsync(10); @@ -168,7 +170,7 @@ describe("NodeKeyConsistencyService — retry/recheck scheduling (CC-36 round 2) checkNodeRegistration: checkNodeRegistration as any, getNodePublicKey: getNodePublicKey as any, }); - const service = new NodeKeyConsistencyService(chain, realBls); + const service = new NodeKeyConsistencyService(chain, realBls, realSigner); service.start(nodeState(), "0xcontract"); await jest.advanceTimersByTimeAsync(10); @@ -188,7 +190,7 @@ describe("NodeKeyConsistencyService — retry/recheck scheduling (CC-36 round 2) it("stop() clears the timer — no leftover handle after shutdown", async () => { const chain = makeChain({}); - const service = new NodeKeyConsistencyService(chain, realBls); + const service = new NodeKeyConsistencyService(chain, realBls, realSigner); service.start(nodeState(), "0xcontract"); await jest.advanceTimersByTimeAsync(10); expect(jest.getTimerCount()).toBeGreaterThan(0); @@ -199,7 +201,7 @@ describe("NodeKeyConsistencyService — retry/recheck scheduling (CC-36 round 2) it("onModuleDestroy() also clears the timer (Nest lifecycle hook) — same guarantee as stop()", async () => { const chain = makeChain({}); - const service = new NodeKeyConsistencyService(chain, realBls); + const service = new NodeKeyConsistencyService(chain, realBls, realSigner); service.start(nodeState(), "0xcontract"); await jest.advanceTimersByTimeAsync(10); @@ -215,7 +217,7 @@ describe("NodeKeyConsistencyService — retry/recheck scheduling (CC-36 round 2) it("start() is idempotent: calling it again restarts cleanly instead of stacking loops", async () => { const checkNodeRegistration = jest.fn(() => Promise.resolve(true)); const chain = makeChain({ checkNodeRegistration: checkNodeRegistration as any }); - const service = new NodeKeyConsistencyService(chain, realBls); + const service = new NodeKeyConsistencyService(chain, realBls, realSigner); service.start(nodeState(), "0xcontract"); await jest.advanceTimersByTimeAsync(10); @@ -230,4 +232,102 @@ describe("NodeKeyConsistencyService — retry/recheck scheduling (CC-36 round 2) service.stop(); }); + + it("MEDIUM-4: start() while a check is in flight discards the stale check's result and does not stack timers", async () => { + let resolveFirst!: (v: boolean) => void; + const firstCallPromise = new Promise(resolve => { + resolveFirst = resolve; + }); + const checkNodeRegistration = jest + .fn<() => Promise>() + .mockImplementationOnce(() => firstCallPromise) // 1st call: hangs until resolved manually + .mockImplementation(() => Promise.resolve(true)); // every later call: normal ok + const chain = makeChain({ checkNodeRegistration: checkNodeRegistration as any }); + const service = new NodeKeyConsistencyService(chain, realBls, realSigner); + + const oldNodeId = "0x" + "aa".repeat(32); + const newNodeId = "0x" + "bb".repeat(32); + + service.start(nodeState({ nodeId: oldNodeId }), "0xcontract"); + await jest.advanceTimersByTimeAsync(1); // let the loop start awaiting the hung RPC call + expect(checkNodeRegistration).toHaveBeenCalledTimes(1); + expect(service.getLastResult()).toBeNull(); // still in flight — nothing published yet + + // Restart with a NEW identity WHILE the old check is still hanging. + service.start(nodeState({ nodeId: newNodeId }), "0xcontract"); + await jest.advanceTimersByTimeAsync(1); // the new loop's immediate check runs and resolves + expect(checkNodeRegistration).toHaveBeenCalledTimes(2); + expect(service.getLastResult()?.status).toBe("ok"); + expect(service.getLastResult()?.nodeId).toBe(newNodeId); + + // NOW let the OLD hung check finally resolve. + resolveFirst(true); + await jest.advanceTimersByTimeAsync(1); + + // The stale result must NOT have overwritten the new identity's published result. + expect(service.getLastResult()?.nodeId).toBe(newNodeId); + // And only ONE timer is pending — the old (stale) loop must not have rescheduled itself. + expect(jest.getTimerCount()).toBe(1); + + service.stop(); + }); + + it("HIGH-3: an unexpected throw deep inside a check() attempt never produces an unhandledRejection, and the loop survives it", async () => { + const chain = makeChain({}); + const service = new NodeKeyConsistencyService(chain, realBls, realSigner); + + // A pathological nodeState: reading .nodeId throws, simulating some future bug reading a + // raw disk-JSON field this check doesn't (or one day might not) defensively guard. This + // exercises the STRUCTURAL boundary (loop()'s own try/catch around check(), plus the + // outer .catch(() => {}) at each loop() call site) rather than any one specific guard. + const poisoned = { ...nodeState() } as NodeState; + Object.defineProperty(poisoned, "nodeId", { + get(): never { + throw new Error("simulated malformed disk field"); + }, + }); + + const unhandled: unknown[] = []; + const onUnhandledRejection = (reason: unknown) => unhandled.push(reason); + process.on("unhandledRejection", onUnhandledRejection); + try { + service.start(poisoned, "0xcontract"); + await jest.advanceTimersByTimeAsync(1); + // One more flush in case anything was left dangling on a later microtask. + await jest.advanceTimersByTimeAsync(0); + + expect(unhandled).toEqual([]); + // The loop survived: it scheduled a next attempt instead of dying silently. + expect(jest.getTimerCount()).toBe(1); + } finally { + process.off("unhandledRejection", onUnhandledRejection); + service.stop(); + } + }); + + it("HIGH-3 regression: a non-string node_state field (raw disk JSON) does not crash the check", async () => { + const chain = makeChain({ + checkNodeRegistration: jest.fn(() => Promise.resolve(true)) as any, + }); + const service = new NodeKeyConsistencyService(chain, realBls, realSigner); + // Simulates a node_state.json that doesn't match the NodeState type at runtime (no schema + // validation guards this file today) — publicKey as a number, not a string. + const malformed = { ...nodeState(), publicKey: 12345 as unknown as string }; + + const unhandled: unknown[] = []; + const onUnhandledRejection = (reason: unknown) => unhandled.push(reason); + process.on("unhandledRejection", onUnhandledRejection); + try { + service.start(malformed, "0xcontract"); + await jest.advanceTimersByTimeAsync(1); + await jest.advanceTimersByTimeAsync(0); + + expect(unhandled).toEqual([]); + expect(service.getLastResult()).not.toBeNull(); + expect(jest.getTimerCount()).toBe(1); + } finally { + process.off("unhandledRejection", onUnhandledRejection); + service.stop(); + } + }); }); diff --git a/src/modules/node/node-key-consistency.service.spec.ts b/src/modules/node/node-key-consistency.service.spec.ts index 875f873..99e7857 100644 --- a/src/modules/node/node-key-consistency.service.spec.ts +++ b/src/modules/node/node-key-consistency.service.spec.ts @@ -4,6 +4,7 @@ import { bls, sigs } from "../../utils/bls.util.js"; import { NodeState } from "../../interfaces/node.interface.js"; import { BlockchainService } from "../blockchain/blockchain.service.js"; import { BlsService } from "../bls/bls.service.js"; +import { SignerService } from "../signer/signer.service.js"; /** * A real BlsService — NOT a mock — used purely for its `encodePublicKeyToEIP2537` (a pure @@ -15,6 +16,13 @@ import { BlsService } from "../bls/bls.service.js"; */ const realBls = new BlsService({} as any, {} as any); +/** + * A real SignerService (round-2 Medium-5) — resolves to `LocalKeySigner` under the default + * "local" backend (config.get always returns undefined) — so `checkLocal()` derives keys + * through the EXACT validation the node signs with, never a separate hex re-parse. + */ +const realSigner = new SignerService({ get: () => undefined } as any); + /** * Real BLS keypairs (not hardcoded fixtures) so every fixture is internally * consistent by construction, and mismatches are constructed by mixing two @@ -27,7 +35,7 @@ function makeKeyPair(seed: number) { const pub = sigs.getPublicKey(privBytes); const compressedHex = "0x" + pub.toHex(); // node_state.publicKey format (48-byte compressed) const point = bls.G1.Point.fromHex(pub.toHex()); - const eip2537Hex = realBls.encodePublicKeyToEIP2537(point); // on-chain format (128-byte) + const eip2537Hex = realBls.encodePublicKeyToEIP2537(point); // on-chain / dashboard-create format (128-byte) return { privateKey, compressedHex, eip2537Hex }; } @@ -58,13 +66,17 @@ function makeBlockchainMock(overrides: { } as unknown as BlockchainService; } +function makeService(chain: BlockchainService, blsService: BlsService = realBls) { + return new NodeKeyConsistencyService(chain, blsService, realSigner); +} + describe("NodeKeyConsistencyService (CC-36)", () => { it("ok: local privateKey derives publicKey AND on-chain registeredKeys matches", async () => { const chain = makeBlockchainMock({ checkNodeRegistration: () => Promise.resolve(true), getNodePublicKey: () => Promise.resolve(KEY_A.eip2537Hex), }); - const service = new NodeKeyConsistencyService(chain, realBls); + const service = makeService(chain); const result = await service.check(baseNodeState(), "0xcontract"); expect(result.status).toBe("ok"); expect(service.getLastResult()).toEqual(result); @@ -75,19 +87,49 @@ describe("NodeKeyConsistencyService (CC-36)", () => { checkNodeRegistration: () => Promise.resolve(true), getNodePublicKey: () => Promise.resolve(KEY_A.eip2537Hex), }); - const service = new NodeKeyConsistencyService(chain, realBls); + const service = makeService(chain); // privateKey is A's, but publicKey on disk is B's — a corrupted/swapped node_state. const state = baseNodeState({ publicKey: KEY_B.compressedHex }); const result = await service.check(state, "0xcontract"); expect(result.status).toBe("mismatch_local"); }); + it("MEDIUM-5 regression: a private key the REAL signer would reject must NOT be reported ok", async () => { + // LocalKeySigner requires exactly 64 hex chars, all valid hex digits. An odd-length / + // non-hex-char key is something the node itself could never sign with — the check must + // agree, not silently coerce it (e.g. via parseInt-NaN -> 0) into a false "ok". + const chain = makeBlockchainMock({ + checkNodeRegistration: () => Promise.resolve(true), + getNodePublicKey: () => Promise.resolve(KEY_A.eip2537Hex), + }); + const service = makeService(chain); + const state = baseNodeState({ privateKey: "0x" + "zz".repeat(32) }); // non-hex chars + const result = await service.check(state, "0xcontract"); + expect(result.status).not.toBe("ok"); + expect(result.status).toBe("mismatch_local"); + }); + + it("MEDIUM-5: an uppercase 0X-prefixed private key (which LocalKeySigner rejects) is NOT reported ok", async () => { + // LocalKeySigner's `startsWith("0x")` check is case-sensitive, so an uppercase "0X" prefix + // is NOT stripped and the remaining string fails the 64-hex-char regex — the real signer + // throws. A check that case-insensitively strips "0x" before validating would disagree. + const chain = makeBlockchainMock({ + checkNodeRegistration: () => Promise.resolve(true), + getNodePublicKey: () => Promise.resolve(KEY_A.eip2537Hex), + }); + const service = makeService(chain); + const upper = "0X" + KEY_A.privateKey.slice(2); + const state = baseNodeState({ privateKey: upper }); + const result = await service.check(state, "0xcontract"); + expect(result.status).not.toBe("ok"); + }); + it("mismatch_onchain: registered, but registeredKeys() differs from local publicKey — the silent failure", async () => { const chain = makeBlockchainMock({ checkNodeRegistration: () => Promise.resolve(true), getNodePublicKey: () => Promise.resolve(KEY_B.eip2537Hex), // someone else's key is registered }); - const service = new NodeKeyConsistencyService(chain, realBls); + const service = makeService(chain); const result = await service.check(baseNodeState(), "0xcontract"); expect(result.status).toBe("mismatch_onchain"); }); @@ -96,7 +138,7 @@ describe("NodeKeyConsistencyService (CC-36)", () => { const chain = makeBlockchainMock({ checkNodeRegistration: () => Promise.resolve(false), }); - const service = new NodeKeyConsistencyService(chain, realBls); + const service = makeService(chain); const result = await service.check(baseNodeState(), "0xcontract"); expect(result.status).toBe("not_registered"); // getNodePublicKey must not even be consulted once we know it's unregistered. @@ -108,7 +150,7 @@ describe("NodeKeyConsistencyService (CC-36)", () => { (chain.checkNodeRegistration as jest.Mock).mockRejectedValueOnce( new Error("RPC timeout") as never ); - const service = new NodeKeyConsistencyService(chain, realBls); + const service = makeService(chain); const result = await service.check(baseNodeState(), "0xcontract"); expect(result.status).toBe("unknown"); }); @@ -120,7 +162,7 @@ describe("NodeKeyConsistencyService (CC-36)", () => { (chain.getNodePublicKey as jest.Mock).mockRejectedValueOnce( new Error("RPC timeout") as never ); - const service = new NodeKeyConsistencyService(chain, realBls); + const service = makeService(chain); const result = await service.check(baseNodeState(), "0xcontract"); expect(result.status).toBe("unknown"); }); @@ -130,7 +172,7 @@ describe("NodeKeyConsistencyService (CC-36)", () => { checkNodeRegistration: () => Promise.resolve(true), getNodePublicKey: () => Promise.resolve(KEY_A.eip2537Hex), }); - const service = new NodeKeyConsistencyService(chain, realBls); + const service = makeService(chain); const state = baseNodeState(); delete (state as Partial).privateKey; const result = await service.check(state, "0xcontract"); @@ -142,7 +184,7 @@ describe("NodeKeyConsistencyService (CC-36)", () => { checkNodeRegistration: () => Promise.resolve(true), getNodePublicKey: () => Promise.resolve(KEY_B.eip2537Hex), }); - const service = new NodeKeyConsistencyService(chain, realBls); + const service = makeService(chain); const state = baseNodeState(); delete (state as Partial).privateKey; const result = await service.check(state, "0xcontract"); @@ -157,7 +199,7 @@ describe("NodeKeyConsistencyService (CC-36)", () => { checkNodeRegistration: () => Promise.resolve(true), getNodePublicKey: () => Promise.resolve(KEY_A.eip2537Hex), }); - const service = new NodeKeyConsistencyService(chain, realBls); + const service = makeService(chain); const bootstrapNodeId = "0x" + "11".repeat(32); // deliberately NOT keccak256(KEY_A.eip2537Hex) const state = baseNodeState({ nodeId: bootstrapNodeId }); const result = await service.check(state, "0xcontract"); @@ -173,7 +215,7 @@ describe("NodeKeyConsistencyService (CC-36)", () => { checkNodeRegistration: () => Promise.resolve(true), getNodePublicKey: () => Promise.resolve(KEY_A.eip2537Hex), }); - const service = new NodeKeyConsistencyService(chain, realBls); + const service = makeService(chain); const result = await service.check(baseNodeState({ nodeId: derivedNodeId }), "0xcontract"); expect(result.status).toBe("ok"); expect(result.derivedMatches).toBe(true); @@ -185,7 +227,7 @@ describe("NodeKeyConsistencyService (CC-36)", () => { // Same bytes as KEY_A.eip2537Hex, but uppercase and without the 0x prefix. getNodePublicKey: () => Promise.resolve(KEY_A.eip2537Hex.slice(2).toUpperCase()), }); - const service = new NodeKeyConsistencyService(chain, realBls); + const service = makeService(chain); const result = await service.check(baseNodeState(), "0xcontract"); expect(result.status).toBe("ok"); }); @@ -203,7 +245,7 @@ describe("NodeKeyConsistencyService (CC-36)", () => { checkNodeRegistration: () => Promise.resolve(true), getNodePublicKey: () => Promise.resolve(sentinelEip2537), }); - const service = new NodeKeyConsistencyService(chain, stubBls); + const service = makeService(chain, stubBls); const result = await service.check(baseNodeState(), "0xcontract"); expect(encodeSpy).toHaveBeenCalled(); expect(result.status).toBe("ok"); @@ -221,8 +263,75 @@ describe("NodeKeyConsistencyService (CC-36)", () => { checkNodeRegistration: () => Promise.resolve(true), getNodePublicKey: () => Promise.resolve("0x" + "11".repeat(128)), }); - const service = new NodeKeyConsistencyService(chain, stubBls); + const service = makeService(chain, stubBls); const result = await service.check(baseNodeState(), "0xcontract"); expect(result.status).toBe("mismatch_onchain"); }); + + describe("HIGH-1: node_state.publicKey may legitimately be 128-byte EIP-2537 (dashboard.service.ts create path)", () => { + it("a dashboard-created node (128-byte publicKey on disk) is reported ok, not mismatch_local", async () => { + // dashboard.service.ts's createNode() stores `publicKey = + // this.blsService.encodePublicKeyToEIP2537(publicKeyPoint)` — the FULL 128-byte form — + // in node_state.json, never the 48-byte compressed form a signing node normally has. + const chain = makeBlockchainMock({ + checkNodeRegistration: () => Promise.resolve(true), + getNodePublicKey: () => Promise.resolve(KEY_A.eip2537Hex), + }); + const service = makeService(chain); + const state = baseNodeState({ publicKey: KEY_A.eip2537Hex }); // 128 bytes, not compressed + const result = await service.check(state, "0xcontract"); + expect(result.status).toBe("ok"); + }); + + it("a 128-byte publicKey still correctly detects a REAL mismatch (not a blanket pass)", async () => { + const chain = makeBlockchainMock({ + checkNodeRegistration: () => Promise.resolve(true), + getNodePublicKey: () => Promise.resolve(KEY_A.eip2537Hex), + }); + const service = makeService(chain); + // privateKey is A's, but the 128-byte publicKey on disk is B's. + const state = baseNodeState({ publicKey: KEY_B.eip2537Hex }); + const result = await service.check(state, "0xcontract"); + expect(result.status).toBe("mismatch_local"); + }); + + it("on-chain comparison also works when node_state.publicKey is 128-byte (round-trips through the same encoder)", async () => { + const chain = makeBlockchainMock({ + checkNodeRegistration: () => Promise.resolve(true), + getNodePublicKey: () => Promise.resolve(KEY_B.eip2537Hex), // a DIFFERENT key registered + }); + const service = makeService(chain); + const state = baseNodeState({ publicKey: KEY_A.eip2537Hex, privateKey: KEY_A.privateKey }); + const result = await service.check(state, "0xcontract"); + expect(result.status).toBe("mismatch_onchain"); + }); + + it("derivedMatches is computed correctly for a 128-byte publicKey too", async () => { + const { ethers } = await import("ethers"); + const derivedNodeId = ethers.keccak256(KEY_A.eip2537Hex); + const chain = makeBlockchainMock({ + checkNodeRegistration: () => Promise.resolve(true), + getNodePublicKey: () => Promise.resolve(KEY_A.eip2537Hex), + }); + const service = makeService(chain); + const state = baseNodeState({ publicKey: KEY_A.eip2537Hex, nodeId: derivedNodeId }); + const result = await service.check(state, "0xcontract"); + expect(result.derivedMatches).toBe(true); + }); + + it("a malformed 128-byte-length publicKey (not a real point) is rejected, not silently accepted", async () => { + const chain = makeBlockchainMock({ + checkNodeRegistration: () => Promise.resolve(true), + }); + const service = makeService(chain); + // Right length (256 hex chars) but NOT a valid curve point (all zero x, all-ones y — off + // curve). assertValidity() must reject this rather than @noble/curves silently accepting it. + const bogus = "0x" + "00".repeat(64) + "ff".repeat(64); + const state = baseNodeState({ publicKey: bogus, privateKey: undefined as any }); + const result = await service.check(state, "0xcontract"); + // No private key (skipped local) and the bogus on-chain-format key can't be parsed -> + // checkOnchain must say "unknown" (never guess ok/mismatch for something unparsable). + expect(result.status).toBe("unknown"); + }); + }); }); diff --git a/src/modules/node/node-key-consistency.service.ts b/src/modules/node/node-key-consistency.service.ts index c06515a..d632baa 100644 --- a/src/modules/node/node-key-consistency.service.ts +++ b/src/modules/node/node-key-consistency.service.ts @@ -1,8 +1,9 @@ import { Injectable, Logger, OnModuleDestroy } from "@nestjs/common"; import { ethers } from "ethers"; -import { bls, sigs } from "../../utils/bls.util.js"; +import { bls } from "../../utils/bls.util.js"; import { BlockchainService } from "../blockchain/blockchain.service.js"; import { BlsService } from "../bls/bls.service.js"; +import { SignerService } from "../signer/signer.service.js"; import { NodeState } from "../../interfaces/node.interface.js"; export type KeyConsistencyStatus = @@ -41,8 +42,11 @@ interface FreshCheck { derivedMatches: boolean | null; } -function normalizeHex(hex: string | undefined | null): string { - return "0x" + (hex ?? "").replace(/^0x/i, "").toLowerCase(); +/** Type-safe against whatever a raw, unvalidated node_state.json actually contains on disk + * (round-2 High-3): a non-string field must normalize to "", never throw. */ +function normalizeHex(hex: unknown): string { + const s = typeof hex === "string" ? hex : ""; + return "0x" + s.replace(/^0x/i, "").toLowerCase(); } /** @@ -56,17 +60,20 @@ function normalizeHex(hex: string | undefined | null): string { * online and silently non-participating. * * Three-way comparison (one attempt, see `computeFresh`): - * 1. local: privateKey -> derive pubkey -> compare to node_state.publicKey - * 2. on-chain: node_state.publicKey (EIP-2537-encoded via the SAME encoder the + * 1. local: privateKey -> derive pubkey via the REAL signing path (SignerService.forNode / + * LocalKeySigner, never a local re-parse of the hex) -> compare to + * node_state.publicKey (accepting EITHER wire format actually seen in + * production — see `parsePublicKeyPoint`) + * 2. on-chain: node_state.publicKey normalized to EIP-2537 via the SAME encoder the * registration path writes with — BlsService.encodePublicKeyToEIP2537, * never a local re-implementation, so this check can never silently - * drift from what registerOnChain() actually wrote) -> compare to + * drift from what registerOnChain() actually wrote -> compare to * registeredKeys(nodeId) (only meaningful when isRegistered(nodeId)) * 3. derivedMatches (informational only): nodeId == keccak256(EIP-2537 publicKey) * - * Fail-loud, not fail-stop: a single attempt never throws. Every failure mode - * (malformed key material, RPC error, timeout) collapses into a `status` value so a - * broken node stays observable instead of taking the process down with it. + * Fail-loud, not fail-stop: a single attempt is designed to never throw — every failure mode + * (malformed key material, RPC error, timeout) collapses into a `status` value — and `loop()` + * additionally holds a structural boundary in case a future bug throws anyway (round-2 High-3). * * Runs as a self-scheduling loop (`start()`), not a one-shot: an `unknown` result * (RPC error) retries with bounded backoff (30s/60s/120s/240s, then folding into the @@ -76,6 +83,12 @@ function normalizeHex(hex: string | undefined | null): string { * `syncNode(nodeId)` deactivating an under-staked/role-losing operator) surfaces as * `not_registered` in /health instead of showing its startup `ok` forever. A recheck * that itself fails RPC never downgrades a known conclusion to `unknown` — see `check()`. + * + * `start()` is bound to one node identity at a time and is generation-tokened (round-2 + * Medium-4): restarting it (e.g. NodeService.reloadNodeState() after a dashboard + * create/import/delete) invalidates any in-flight attempt from the previous identity, so its + * result — however it eventually resolves — is discarded rather than clobbering the new + * identity's published result or stacking a second timer. */ @Injectable() export class NodeKeyConsistencyService implements OnModuleDestroy { @@ -90,10 +103,14 @@ export class NodeKeyConsistencyService implements OnModuleDestroy { private consecutiveUnknown = 0; private timer: ReturnType | null = null; private stopped = true; + /** Bumped by every start()/stop(). A loop() iteration captures its generation at start() time + * and checks it after each attempt — a mismatch means a newer start()/stop() superseded it. */ + private generation = 0; constructor( private readonly blockchainService: BlockchainService, - private readonly blsService: BlsService + private readonly blsService: BlsService, + private readonly signerService: SignerService ) {} /** The most recently held result (a preserved conclusion survives a failed retry). Read by /health. */ @@ -102,39 +119,73 @@ export class NodeKeyConsistencyService implements OnModuleDestroy { } /** - * Start the self-check loop: an immediate check, then bounded-backoff retries while - * `unknown`, then periodic rechecks once conclusive. Idempotent — calling again restarts - * the loop from scratch (stopping any existing timer first) rather than stacking loops. + * Start the self-check loop for `nodeState`: an immediate check, then bounded-backoff + * retries while `unknown`, then periodic rechecks once conclusive. Idempotent — calling + * again (e.g. against a NEW node identity after a dashboard create/import) restarts the + * loop from scratch: stops any existing timer, bumps the generation token (invalidating an + * in-flight attempt from the previous call), and clears the held result immediately so a + * previous identity's finding is never shown even for a moment under the new loop. */ start(nodeState: NodeState, contractAddress: string): void { this.stop(); + this.lastResult = null; + this.consecutiveUnknown = 0; + this.generation++; + const myGeneration = this.generation; this.stopped = false; - void this.loop(nodeState, contractAddress); + void this.loop(nodeState, contractAddress, myGeneration).catch(() => {}); } - /** Stop the loop and clear its timer. Safe to call repeatedly, or before `start()`. */ + /** Stop the loop and clear its timer. Safe to call repeatedly, or before `start()`. Does NOT + * clear the held result — callers that also want that (e.g. node deletion) call `clearResult()`. */ stop(): void { this.stopped = true; + this.generation++; // invalidate any in-flight loop() from the previous start() if (this.timer !== null) { clearTimeout(this.timer); this.timer = null; } } + /** + * Forget the held result entirely (round-2 High-2): called when the node identity is + * deleted and none replaces it, so /health's `keyConsistency` goes back to being omitted + * rather than confidently showing a stale identity's "ok" (or any other stale status) for + * a node that no longer exists. Does not touch the timer/loop — pair with `stop()`. + */ + clearResult(): void { + this.lastResult = null; + this.consecutiveUnknown = 0; + } + /** Nest lifecycle hook — guarantees the timer never survives module/app shutdown. */ onModuleDestroy(): void { this.stop(); } - private async loop(nodeState: NodeState, contractAddress: string): Promise { - await this.check(nodeState, contractAddress); - if (this.stopped) { - // stop() raced with this in-flight attempt — don't schedule a next one. + private async loop( + nodeState: NodeState, + contractAddress: string, + myGeneration: number + ): Promise { + try { + await this.check(nodeState, contractAddress, myGeneration); + } catch { + // Structural boundary (round-2 High-3): check() is designed to never throw (every + // failure mode collapses into a status), but this is the last line of defense in case a + // future bug does anyway. The retry loop must survive a single bad tick and keep + // scheduling rather than dying silently, and this handler does NOTHING that could + // itself throw (no logger call here — logging happens, if at all, inside check()). + } + if (this.stopped || myGeneration !== this.generation) { + // Either stopped, or superseded by a newer start()/stop() while this attempt was in + // flight — don't reschedule. This is also what keeps exactly one timer alive at a time. return; } const delay = this.nextDelayMs(); this.timer = setTimeout(() => { - void this.loop(nodeState, contractAddress); + // Same structural boundary on the recurring path. + void this.loop(nodeState, contractAddress, myGeneration).catch(() => {}); }, delay); // Never keep the process alive just for this self-check (mirrors the rest of the // codebase's fire-and-forget timers; this one just says so explicitly). @@ -157,11 +208,33 @@ export class NodeKeyConsistencyService implements OnModuleDestroy { * Reading a network blip as "we no longer know" would silently launder away a real * mismatch_onchain the moment the RPC hiccups, which defeats the whole point of this * check. `checkedAt` only advances when a conclusion was actually (re-)established. + * + * `generationToken` is set ONLY by `loop()` (a direct/manual call — as every existing unit + * test makes — omits it and always publishes). When set and stale (round-2 Medium-4: a + * newer start()/stop() ran while this attempt was in flight), the result is discarded + * entirely: not merged into `lastResult`, not logged as a fresh finding. `loop()` itself + * separately checks the generation afterward to decide whether to reschedule. */ - async check(nodeState: NodeState, contractAddress: string): Promise { + async check( + nodeState: NodeState, + contractAddress: string, + generationToken?: number + ): Promise { const now = new Date().toISOString(); const fresh = await this.computeFresh(nodeState, contractAddress); + if (generationToken !== undefined && generationToken !== this.generation) { + return ( + this.lastResult ?? { + status: fresh.status, + checkedAt: now, + lastAttemptAt: now, + derivedMatches: fresh.derivedMatches, + nodeId: nodeState.nodeId, + } + ); + } + if (fresh.status === "mismatch_local" || fresh.status === "mismatch_onchain") { this.logMismatch(fresh.status, nodeState); } @@ -211,42 +284,48 @@ export class NodeKeyConsistencyService implements OnModuleDestroy { ); } - private async computeFresh( - nodeState: NodeState, - contractAddress: string - ): Promise { + private async computeFresh(nodeState: NodeState, contractAddress: string): Promise { const derivedMatches = this.computeDerivedMatches(nodeState); - const local = this.checkLocal(nodeState); + const local = await this.checkLocal(nodeState); const onchain = await this.checkOnchain(nodeState, contractAddress); const status = this.combine(local, onchain); return { status, derivedMatches }; } - private prefix(hex: string | undefined): string { - const h = (hex ?? "").replace(/^0x/i, ""); - return "0x" + h.slice(0, 10); + /** Type-safe against a raw, unvalidated node_state.json field (round-2 High-3). */ + private prefix(hex: unknown): string { + const s = typeof hex === "string" ? hex : ""; + return "0x" + s.replace(/^0x/i, "").slice(0, 10); } - /** privateKey -> derived pubkey vs. node_state.publicKey. Key-less nodes are a legitimate skip. */ - private checkLocal(nodeState: NodeState): "ok" | "mismatch" | "skipped" { + /** + * privateKey -> derived pubkey vs. node_state.publicKey. Key-less nodes are a legitimate + * skip. Delegates key derivation to the REAL signing path (round-2 Medium-5): a bare + * `SignerService.forNode(nodeState)` throws exactly when `LocalKeySigner`'s constructor + * would reject the private key (wrong length, non-hex char, etc.) — the same validation + * the node actually signs with — so this can never say "ok" for a key the signer would + * refuse, and can never silently coerce an invalid hex char into 0. + */ + private async checkLocal(nodeState: NodeState): Promise<"ok" | "mismatch" | "skipped"> { if (!nodeState.privateKey) { return "skipped"; } try { - const privHex = nodeState.privateKey.replace(/^0x/i, ""); - const privBytes = new Uint8Array(privHex.length / 2); - for (let i = 0; i < privHex.length; i += 2) { - privBytes[i / 2] = parseInt(privHex.substr(i, 2), 16); + const signer = this.signerService.forNode(nodeState); + const derivedPub = await signer.getPublicKey(); + const statePoint = this.parsePublicKeyPoint(nodeState.publicKey); + if (statePoint === null) { + // node_state.publicKey isn't a valid point in EITHER accepted wire format. + return "mismatch"; } - const derivedPub = sigs.getPublicKey(privBytes); const derivedHex = normalizeHex(derivedPub.toHex()); - const stateHex = normalizeHex(nodeState.publicKey); + const stateHex = normalizeHex(statePoint.toHex()); return derivedHex === stateHex ? "ok" : "mismatch"; } catch (error: any) { - // Malformed key material — cannot prove correctness, so treat as a mismatch rather - // than silently skipping (a corrupt privateKey is itself worth surfacing). + // The signer rejected the private key (or deriving/comparing otherwise failed) — + // cannot prove correctness, so treat as a mismatch rather than silently skipping. this.logger.warn( - `local key-consistency check could not compute a public key from privateKey: ${error?.message ?? error}` + `local key-consistency check could not derive/compare a public key: ${error?.message ?? error}` ); return "mismatch"; } @@ -268,11 +347,12 @@ export class NodeKeyConsistencyService implements OnModuleDestroy { const localEip2537 = this.toEip2537Hex(nodeState.publicKey); if (localEip2537 === null) { - // Registered on-chain, but the local publicKey isn't even a parsable curve - // point — we cannot determine agreement either way. Never guess: unknown. + // Registered on-chain, but the local publicKey isn't a parsable curve point in + // EITHER accepted wire format — we cannot determine agreement either way. Never + // guess: unknown. this.logger.warn( `key-consistency check: node_state.publicKey for ${nodeState.nodeId} is not a valid ` + - "G1 point; cannot compare against the on-chain registration" + "G1 point in either accepted wire format; cannot compare against the on-chain registration" ); return "unknown"; } @@ -306,19 +386,63 @@ export class NodeKeyConsistencyService implements OnModuleDestroy { } /** - * Decompress a 48-byte compressed G1 hex pubkey and re-encode it as 128-byte EIP-2537 — - * via `BlsService.encodePublicKeyToEIP2537`, the SAME encoder `registerOnChain()` writes + * Parse node_state.publicKey into a validated G1 curve point, accepting BOTH wire formats + * actually seen on real nodes (round-2 review, High-1): 48-byte COMPRESSED (what a signing + * node normally carries — verified against a live node's `/node/info`) and 128-byte + * EIP-2537 UNCOMPRESSED (what dashboard.service.ts's create-node path stores — + * `encodePublicKeyToEIP2537`'s own output — and `import-node.dto.ts` accepts as-is for + * `publicKey`). Assuming only the compressed form would misreport every dashboard-created + * node as `mismatch_local` — a false accusation against a legitimately-created node, the + * same class of bug as trusting `derivedMatches` as a judgment. + * + * The 128-byte branch reconstructs the point via @noble/curves' OWN `Point.fromAffine` (a + * public primitive of the SAME class `.fromHex()` returns) from the raw X/Y coordinates, + * then calls `.assertValidity()` — the identical subgroup/curve-equation check `.fromHex()` + * performs on the compressed form — so a malformed or off-curve 128-byte value is rejected + * exactly like a malformed compressed one would be, never silently accepted. + */ + private parsePublicKeyPoint(publicKeyHex: unknown): any | null { + if (typeof publicKeyHex !== "string" || publicKeyHex.length === 0) { + return null; + } + const hex = publicKeyHex.replace(/^0x/i, ""); + try { + if (hex.length === 96) { + // 48-byte compressed G1. + const point = bls.G1.Point.fromHex(hex); + point.assertValidity(); + return point; + } + if (hex.length === 256) { + // 128-byte EIP-2537: 16 zero-pad + 48-byte X + 16 zero-pad + 48-byte Y — the exact + // inverse layout of BlsService's private encodeG1Point (byte offsets 16 and 80; in + // hex-char offsets that's X at [32,128) and Y at [160,256)). + const x = BigInt("0x" + hex.slice(32, 128)); + const y = BigInt("0x" + hex.slice(160, 256)); + const point = bls.G1.Point.fromAffine({ x, y }); + point.assertValidity(); + return point; + } + return null; // neither accepted wire format + } catch { + return null; + } + } + + /** + * Normalize node_state.publicKey (either accepted wire format) to 128-byte EIP-2537 — via + * `BlsService.encodePublicKeyToEIP2537`, the SAME encoder `registerOnChain()` writes * on-chain with (and gossip/dashboard/quorum-cosigner all read on-chain data with). This is * deliberately NOT a local re-implementation: a future change to the encoder would * otherwise silently diverge between what got written on-chain and what this check * compares against, causing exactly the kind of undetected drift this check exists to catch. */ - private toEip2537Hex(compressedPublicKeyHex: string | undefined): string | null { - if (!compressedPublicKeyHex) { + private toEip2537Hex(publicKeyHex: unknown): string | null { + const point = this.parsePublicKeyPoint(publicKeyHex); + if (point === null) { return null; } try { - const point = bls.G1.Point.fromHex(compressedPublicKeyHex.replace(/^0x/i, "")); return this.blsService.encodePublicKeyToEIP2537(point); } catch { return null; diff --git a/src/modules/node/node.key-consistency-wiring.spec.ts b/src/modules/node/node.key-consistency-wiring.spec.ts index ecf20e0..d687bf8 100644 --- a/src/modules/node/node.key-consistency-wiring.spec.ts +++ b/src/modules/node/node.key-consistency-wiring.spec.ts @@ -91,3 +91,123 @@ describe("NodeService — key-consistency self-check wiring (CC-36)", () => { } }); }); + +/** + * CC-36 round 2 (High-2): the self-check is bound to one node identity. dashboard.service.ts + * calls reloadNodeState() after every create/import/delete — it must restart the check + * against the NEW identity, or stop + clear it entirely when the node was deleted, so + * /health never keeps confidently reporting a status for an identity that no longer exists. + */ +describe("NodeService — reloadNodeState() restarts/stops the self-check on identity change (CC-36 round 2)", () => { + const makeState = (nodeId: string) => ({ + nodeId, + nodeName: "n", + publicKey: "0xabcd", + createdAt: "t", + description: "d", + privateKey: "0x" + "1".padStart(64, "0"), + }); + + const makeSvc = ( + file: string, + keyConsistencyService: { + start: jest.Mock; + stop: jest.Mock; + clearResult: jest.Mock; + getLastResult: () => null; + } + ) => { + const config = { + get: (k: string) => (k === "validatorContractAddress" ? "0xcontract" : undefined), + } as unknown as ConfigService; + const svc = new NodeService({} as any, {} as any, config, keyConsistencyService as any); + (svc as any).nodeStateFilePath = file; + (svc as any).contractAddress = "0xcontract"; // normally set by initializeNode() before boot + return svc; + }; + + it("create: reloadNodeState() starts the check against the newly-created identity", () => { + const dir = mkdtempSync(join(tmpdir(), "kc-reload-")); + const file = join(dir, "node_state.json"); + const start = jest.fn(); + const stop = jest.fn(); + const clearResult = jest.fn(); + const svc = makeSvc(file, { start, stop, clearResult, getLastResult: () => null }); + + try { + // No node yet — the file doesn't exist. reloadNodeState() must stop+clear, not start. + svc.reloadNodeState(); + expect(start).not.toHaveBeenCalled(); + expect(stop).toHaveBeenCalledTimes(1); + expect(clearResult).toHaveBeenCalledTimes(1); + + // Dashboard "create" writes node_state.json, then calls reloadNodeState(). + writeFileSync(file, JSON.stringify(makeState("0xnew"))); + svc.reloadNodeState(); + expect(start).toHaveBeenCalledTimes(1); + expect(start).toHaveBeenCalledWith( + expect.objectContaining({ nodeId: "0xnew" }), + "0xcontract" + ); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + + it("delete: reloadNodeState() stops the check and clears its result — /health must not keep a stale identity's ok", () => { + const dir = mkdtempSync(join(tmpdir(), "kc-reload-")); + const file = join(dir, "node_state.json"); + writeFileSync(file, JSON.stringify(makeState("0xold"))); + const start = jest.fn(); + const stop = jest.fn(); + const clearResult = jest.fn(); + const svc = makeSvc(file, { start, stop, clearResult, getLastResult: () => null }); + + try { + svc.reloadNodeState(); // node exists -> start() + expect(start).toHaveBeenCalledTimes(1); + expect(stop).not.toHaveBeenCalled(); + + // Dashboard "delete" removes node_state.json, then calls reloadNodeState(). + rmSync(file); + svc.reloadNodeState(); + expect(stop).toHaveBeenCalledTimes(1); + expect(clearResult).toHaveBeenCalledTimes(1); + expect(start).toHaveBeenCalledTimes(1); // not called again — still just the one from before + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + + it("replace: reloadNodeState() restarts the check against the NEW identity, never the old one", () => { + const dir = mkdtempSync(join(tmpdir(), "kc-reload-")); + const file = join(dir, "node_state.json"); + writeFileSync(file, JSON.stringify(makeState("0xold"))); + const start = jest.fn(); + const svc = makeSvc(file, { + start, + stop: jest.fn(), + clearResult: jest.fn(), + getLastResult: () => null, + }); + + try { + svc.reloadNodeState(); + expect(start).toHaveBeenLastCalledWith( + expect.objectContaining({ nodeId: "0xold" }), + "0xcontract" + ); + + // A delete followed by an import with a DIFFERENT identity (the dashboard's replace flow). + rmSync(file); + writeFileSync(file, JSON.stringify(makeState("0xnew"))); + svc.reloadNodeState(); + expect(start).toHaveBeenLastCalledWith( + expect.objectContaining({ nodeId: "0xnew" }), + "0xcontract" + ); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); +}); diff --git a/src/modules/node/node.module.ts b/src/modules/node/node.module.ts index f9215a2..0a7c4b2 100644 --- a/src/modules/node/node.module.ts +++ b/src/modules/node/node.module.ts @@ -5,9 +5,13 @@ import { IdentityController } from "./identity.controller.js"; import { NodeKeyConsistencyService } from "./node-key-consistency.service.js"; import { BlsModule } from "../bls/bls.module.js"; import { BlockchainModule } from "../blockchain/blockchain.module.js"; +import { SignerModule } from "../signer/signer.module.js"; @Module({ - imports: [forwardRef(() => BlsModule), BlockchainModule], + // SignerModule is imported directly (not just transitively via BlsModule, which does not + // export SignerService) so NodeKeyConsistencyService can derive keys through the REAL + // signing path (round-2 Medium-5) rather than re-implementing hex validation. + imports: [forwardRef(() => BlsModule), BlockchainModule, SignerModule], providers: [NodeService, NodeKeyConsistencyService], controllers: [NodeController, IdentityController], exports: [NodeService, NodeKeyConsistencyService], diff --git a/src/modules/node/node.service.ts b/src/modules/node/node.service.ts index 23f7ce8..8464a23 100644 --- a/src/modules/node/node.service.ts +++ b/src/modules/node/node.service.ts @@ -285,5 +285,18 @@ export class NodeService implements OnModuleInit { this.nodeState = null; this.logger.log("Node state file not found, cleared internal state"); } + // CC-36 round 2 (High-2): the key-consistency self-check is bound to one node identity. + // A dashboard create/import/delete calls reloadNodeState() and MUST restart the check + // against the new identity — or stop and clear it entirely when the node was deleted. + // Without this, a deleted/replaced node keeps publishing its OLD identity's result + // forever, and /health would confidently report "ok" for a node that no longer exists — + // worse than reporting nothing. NodeKeyConsistencyService's generation token (Medium-4) + // ensures a stale in-flight check from the previous identity can't clobber this. + if (this.nodeState) { + this.runKeyConsistencyCheck(); + } else if (this.keyConsistencyService) { + this.keyConsistencyService.stop(); + this.keyConsistencyService.clearResult(); + } } } From bdb6c7d3c9aa1061f1b4fbd383aef7e19e8112af Mon Sep 17 00:00:00 2001 From: jhfnetboy Date: Sun, 27 Sep 2026 14:57:46 +0700 Subject: [PATCH 4/9] fix(CC-36 round 4): reject the point-at-infinity/malformed padding, match the real signer selection (local vs remote/TEE), and make internal checker bugs fail loud MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex adversarial review round 2 (REQUEST_CHANGES, 2 High + 1 Medium + 1 Low), all addressed. Round-1 close-outs (High-2, Medium-4, process-safety half of High-3) were independently re-verified by the coordinator and are unchanged here. High-1 (Codex confirmed against the installed @noble/curves): parsePublicKeyPoint() was still too permissive. 1. Non-zero padding in the 128-byte EIP-2537 branch was silently accepted — only the X/Y coordinate slices were read, never the 16-byte zero-padding regions either side of them. Now explicitly validated; a corrupted padding value is rejected as malformed, not normalized through as if legitimate. 2. The point-at-infinity was accepted in BOTH wire formats. `.assertValidity()` alone does NOT reject it — @noble/curves deliberately allows the group identity as "on-curve" — verified empirically: 128 zero bytes decode to Point.ZERO and pass assertValidity(). An infinity "public key" makes a pairing check trivially pass with NO private key at all — the exact rogue-key bypass AAStarValidator.registerWithProof's `require(!_isInfinity(publicKey))` guards against on-chain. Both branches now explicitly call `.is0()` and reject. High-2 (the coordinator's own correction to round-1 guidance): LocalKeySigner is only the DEFAULT backend, not "the" signer — bls.service.ts:140's signDerivedHash prefers a remote Rust/TEE-KMS signer whenever RUST_SIGNER_URL is set (and ONLY that signer when RUST_SIGNER_REQUIRED=true), while node.service.ts still allows a node_state.json to carry a (now-irrelevant, possibly stale) local privateKey in that mode. Comparing the local key unconditionally could report ok while every real (remote) signature fails, or report mismatch while every real signature is fine — production topology: node1 is MX93 KMS-TEE hosted (CC-24/CC-30). Fixed by extracting the signer-selection DECISION into one shared method, `BlsService.getRustSignerConfig()`, that both `signDerivedHash()` and the new `BlsService.resolveSigningPublicKey()` read — never duplicated, never able to independently drift. `resolveSigningPublicKey()` asks the remote signer for its own public key (there's no dedicated endpoint; it probes /sign with a fixed, harmless, clearly-marked message and reads `public_key` off the response, same as a real sign does) when remote is configured, and NEVER falls back to the local key if that probe fails — it reports `skipped_remote_signer` instead. Only the owner-authorization gate (authorizeAndDeriveHash) is explicitly untouched; signDerivedHash's own tests (there were none locking its internals) all still pass, and the whole signing/signature test suites remain green. NodeKeyConsistencyService no longer depends on SignerService directly — it goes through BlsService.resolveSigningPublicKey exclusively, which simplified the DI graph back down (removed the SignerModule import round-2 added to NodeModule). Medium (the coordinator's own correction: round-1's "catch does nothing" pattern was right for keeper but wrong here): the INNER catch in loop() (around check() only, not the outermost safety net) now distinguishes "check() itself threw" (a bug) from "an RPC blip" (already handled inside check() as `unknown`, never reaching here). It preserves the prior CONCLUSIVE result (still no downgrade), but stamps `checkerError`/`checkerErrorAt` on it and still advances `lastAttemptAt` — so /health can no longer show an internal bug as an unbroken "still ok" forever; the result is explicitly marked stale-since. The log call inside is wrapped in its own try/catch (logging must never itself escalate a checker bug into a crash) and the outer `.catch(() => {})` at both loop() call sites is kept as a separate, logic-free last resort — the two layers now have distinct jobs, as intended. Low: added tests for non-zero-padding rejection, the point-at-infinity in both wire formats, the two remote-signer A/B misattribution scenarios (silent-failure and false-positive) plus unreachable/non-2xx remote-signer responses, and strengthened the existing "internal exception" tests to assert `checkerError` is actually visible (the previous assertions — no unhandledRejection + timer still running — would have passed even with Medium's bug present). Mutation-verified (3, not committed, each reverted after confirming red): 1. Disabling the `.is0()` guard in both parsePublicKeyPoint() branches turned the infinity-rejection tests red (both the 128-byte and, after fixing one test whose weak assertion masked the bug, the compressed-format test too). 2. Reverting checkLocal() to bypass resolveSigningPublicKey() and always derive from the raw local key turned all 5 HIGH-2 remote-signer tests red. 3. Making the inner catch in loop() skip stamping checkerError/checkerErrorAt turned both the HIGH-3 and the dedicated Medium checker-error test red. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Pdq9rgkZq9M4JFeTZD7yYs --- src/modules/bls/bls.service.ts | 120 ++++++++- ...key-consistency.service.scheduling.spec.ts | 97 ++++++-- .../node/node-key-consistency.service.spec.ts | 220 +++++++++++++++-- .../node/node-key-consistency.service.ts | 227 ++++++++++++++---- src/modules/node/node.module.ts | 9 +- 5 files changed, 574 insertions(+), 99 deletions(-) diff --git a/src/modules/bls/bls.service.ts b/src/modules/bls/bls.service.ts index be20702..c0e1d29 100644 --- a/src/modules/bls/bls.service.ts +++ b/src/modules/bls/bls.service.ts @@ -7,6 +7,13 @@ import { NodeKeyPair } from "../../interfaces/node.interface.js"; import { BlockchainService, PackedUserOp } from "../blockchain/blockchain.service.js"; import { SignerService } from "../signer/signer.service.js"; +/** + * A fixed, harmless probe message (32 bytes, clearly non-random so it's recognizable in any + * signer-side logs) used ONLY by `resolveSigningPublicKey`'s remote-signer probe — see there + * for why no dedicated "get public key" endpoint exists to ask for instead. + */ +const REMOTE_SIGNER_PROBE_HASH = "0x" + "cc36".repeat(16); + @Injectable() export class BlsService { private readonly logger = new Logger(BlsService.name); @@ -138,12 +145,12 @@ export class BlsService { * error it falls back to Node signing, unless RUST_SIGNER_REQUIRED=true (fail-closed). */ async signDerivedHash(userOpHash: string, node: NodeKeyPair): Promise { - const base = this.configService?.get("rustSignerUrl"); - if (base) { + const rust = this.getRustSignerConfig(); + if (rust) { try { - return await this.signViaRust(base, userOpHash, node); + return await this.signViaRust(rust, userOpHash, node); } catch (error: any) { - if (this.configService?.get("rustSignerRequired") === true) { + if (rust.required) { // Production wants Rust — surface the failure instead of silently degrading. this.logger.error(`Rust signer required but failed: ${error?.message ?? error}`); throw error; @@ -156,19 +163,114 @@ export class BlsService { return this.signViaNode(userOpHash, node); } + /** + * The ONE place both `signDerivedHash()` and any other consumer that needs to know + * "which signer is currently authoritative for this node" (the node key-consistency + * self-check, round-3 High-2) read the hybrid-signer config — so they can never + * independently re-read the same env vars and silently drift apart on the decision. + * Returns null when no remote signer is configured at all (plain local signing). + */ + private getRustSignerConfig(): { url: string; required: boolean; token?: string } | null { + const url = this.configService?.get("rustSignerUrl"); + if (!url) { + return null; + } + return { + url, + required: this.configService?.get("rustSignerRequired") === true, + token: this.configService?.get("rustSignerToken"), + }; + } + + /** + * Round-3 High-2: which public key is CURRENTLY authoritative for this node's signing, + * resolved via the EXACT same signer selection `signDerivedHash()` uses + * (`getRustSignerConfig()`), without producing a real signature over caller-supplied + * data. The node key-consistency self-check uses this instead of unconditionally + * comparing the local `privateKey` field — a node can be configured for remote (Rust/ + * TEE-KMS) signing while STILL carrying a (possibly stale) local `privateKey` in + * node_state.json, so comparing the local key would be comparing the wrong thing + * entirely: it could report `ok` while every real (remote) signature actually fails, or + * report a mismatch while every real signature is actually fine. + * + * Deliberately simpler than `signDerivedHash()`'s RUST_SIGNER_REQUIRED fallback nuance: + * a real sign call may fall back to local when the remote is optional and unreachable, + * but for THIS purpose "remote is configured at all" already means it's the FIRST thing + * a real sign attempts (see signDerivedHash), so an unreachable remote here must NEVER + * silently fall back to reporting the local key as authoritative — that is exactly the + * bug this method exists to prevent. `required` only changes whether a REAL sign call + * falls back after a failed remote attempt; it does not change what this method reports. + */ + async resolveSigningPublicKey(node: NodeKeyPair): Promise< + | { source: "local"; publicKey: any } + | { source: "remote"; publicKeyHex: string } + | { source: "remote_unavailable" } + | { source: "no_signer" } + > { + const rust = this.getRustSignerConfig(); + if (!rust) { + if (!node.privateKey) { + return { source: "no_signer" }; + } + const signer = this.signerService.forNode(node); + const publicKey = await signer.getPublicKey(); + return { source: "local", publicKey }; + } + try { + const publicKeyHex = await this.probeRemoteSignerPublicKey(rust, node); + return { source: "remote", publicKeyHex }; + } catch (error: any) { + this.logger.warn( + `remote signer public-key probe failed for node ${node.nodeId}: ${error?.message ?? error}` + ); + return { source: "remote_unavailable" }; + } + } + + /** + * Ask the remote/rust signer for its public key. There is no dedicated "get public key" + * endpoint (the signer only exposes /sign) — every /sign response includes `public_key` + * alongside the signature, so this probes with a fixed, clearly-marked, harmless message + * reserved for this purpose. Shares the exact URL/token construction `signViaRust` uses + * (via the same `rust` config object) so this can never silently drift from the real + * signing request shape. + */ + private async probeRemoteSignerPublicKey( + rust: { url: string; token?: string }, + node: NodeKeyPair + ): Promise { + const url = `${rust.url.replace(/\/+$/, "")}/sign`; + const headers: Record = { "Content-Type": "application/json" }; + if (rust.token) { + headers["X-Signer-Token"] = rust.token; + } + const response = await fetch(url, { + method: "POST", + headers, + body: JSON.stringify({ user_op_hash: REMOTE_SIGNER_PROBE_HASH, node_id: node.nodeId }), + }); + if (!response.ok) { + throw new Error(`remote signer probe HTTP ${response.status}`); + } + const data = (await response.json()) as { public_key?: string }; + if (typeof data.public_key !== "string" || data.public_key.length === 0) { + throw new Error("remote signer probe returned no public_key"); + } + return data.public_key; + } + /** Delegate signing to the local Rust signer. Throws on any transport/HTTP error. */ private async signViaRust( - base: string, + rust: { url: string; token?: string }, userOpHash: string, node: NodeKeyPair ): Promise { - const url = `${base.replace(/\/+$/, "")}/sign`; + const url = `${rust.url.replace(/\/+$/, "")}/sign`; const headers: Record = { "Content-Type": "application/json" }; // KMS-TEE mode: attach the shared secret when configured so a KMS signer with // KMS_BLS_SIGNER_TOKEN set accepts this request (and rejects other local processes). - const signerToken = this.configService?.get("rustSignerToken"); - if (signerToken) { - headers["X-Signer-Token"] = signerToken; + if (rust.token) { + headers["X-Signer-Token"] = rust.token; } const response = await fetch(url, { method: "POST", diff --git a/src/modules/node/node-key-consistency.service.scheduling.spec.ts b/src/modules/node/node-key-consistency.service.scheduling.spec.ts index 27e34c3..15a2c70 100644 --- a/src/modules/node/node-key-consistency.service.scheduling.spec.ts +++ b/src/modules/node/node-key-consistency.service.scheduling.spec.ts @@ -7,7 +7,7 @@ import { BlsService } from "../bls/bls.service.js"; import { SignerService } from "../signer/signer.service.js"; /** - * CC-36 round 2: the self-check is not a one-shot — it retries with bounded backoff while + * CC-36 round 2/3: the self-check is not a one-shot — it retries with bounded backoff while * `unknown` (an RPC blip must not freeze the node on a boot-time hiccup forever) and * periodically rechecks once conclusive (so a post-boot on-chain change — e.g. the * validator's permissionless `syncNode(nodeId)` deactivating an under-staked operator — @@ -18,8 +18,8 @@ import { SignerService } from "../signer/signer.service.js"; * exact millisecond: Jest/@sinonjs fake timers' tickAsync does not guarantee sub-second * precision when a timer callback itself chains further awaits before rescheduling. */ -const realBls = new BlsService({} as any, {} as any); const realSigner = new SignerService({ get: () => undefined } as any); +const realBls = new BlsService({} as any, realSigner, { get: () => undefined } as any); function makeKeyPair(seed: number) { const privBytes = new Uint8Array(32); @@ -55,7 +55,7 @@ function makeChain(overrides: { } as unknown as BlockchainService; } -describe("NodeKeyConsistencyService — retry/recheck scheduling (CC-36 round 2)", () => { +describe("NodeKeyConsistencyService — retry/recheck scheduling (CC-36 round 2/3)", () => { beforeEach(() => { jest.useFakeTimers(); }); @@ -68,7 +68,7 @@ describe("NodeKeyConsistencyService — retry/recheck scheduling (CC-36 round 2) it("retries with bounded backoff (30s/60s/120s/240s) while stuck on unknown, then folds into the 15-minute cadence", async () => { const checkNodeRegistration = jest.fn(() => Promise.reject(new Error("RPC down"))); const chain = makeChain({ checkNodeRegistration: checkNodeRegistration as any }); - const service = new NodeKeyConsistencyService(chain, realBls, realSigner); + const service = new NodeKeyConsistencyService(chain, realBls); service.start(nodeState(), "0xcontract"); await jest.advanceTimersByTimeAsync(10); // flush the immediate boot attempt @@ -112,7 +112,7 @@ describe("NodeKeyConsistencyService — retry/recheck scheduling (CC-36 round 2) it("rechecks periodically (every ~15 minutes) once a conclusive result is reached", async () => { const checkNodeRegistration = jest.fn(() => Promise.resolve(true)); const chain = makeChain({ checkNodeRegistration: checkNodeRegistration as any }); - const service = new NodeKeyConsistencyService(chain, realBls, realSigner); + const service = new NodeKeyConsistencyService(chain, realBls); service.start(nodeState(), "0xcontract"); await jest.advanceTimersByTimeAsync(10); @@ -133,7 +133,7 @@ describe("NodeKeyConsistencyService — retry/recheck scheduling (CC-36 round 2) .mockResolvedValueOnce(true) // boot: registered .mockRejectedValueOnce(new Error("RPC blip") as never); // 15-min recheck: fails const chain = makeChain({ checkNodeRegistration: checkNodeRegistration as any }); - const service = new NodeKeyConsistencyService(chain, realBls, realSigner); + const service = new NodeKeyConsistencyService(chain, realBls); service.start(nodeState(), "0xcontract"); await jest.advanceTimersByTimeAsync(10); @@ -170,7 +170,7 @@ describe("NodeKeyConsistencyService — retry/recheck scheduling (CC-36 round 2) checkNodeRegistration: checkNodeRegistration as any, getNodePublicKey: getNodePublicKey as any, }); - const service = new NodeKeyConsistencyService(chain, realBls, realSigner); + const service = new NodeKeyConsistencyService(chain, realBls); service.start(nodeState(), "0xcontract"); await jest.advanceTimersByTimeAsync(10); @@ -190,7 +190,7 @@ describe("NodeKeyConsistencyService — retry/recheck scheduling (CC-36 round 2) it("stop() clears the timer — no leftover handle after shutdown", async () => { const chain = makeChain({}); - const service = new NodeKeyConsistencyService(chain, realBls, realSigner); + const service = new NodeKeyConsistencyService(chain, realBls); service.start(nodeState(), "0xcontract"); await jest.advanceTimersByTimeAsync(10); expect(jest.getTimerCount()).toBeGreaterThan(0); @@ -201,7 +201,7 @@ describe("NodeKeyConsistencyService — retry/recheck scheduling (CC-36 round 2) it("onModuleDestroy() also clears the timer (Nest lifecycle hook) — same guarantee as stop()", async () => { const chain = makeChain({}); - const service = new NodeKeyConsistencyService(chain, realBls, realSigner); + const service = new NodeKeyConsistencyService(chain, realBls); service.start(nodeState(), "0xcontract"); await jest.advanceTimersByTimeAsync(10); @@ -217,7 +217,7 @@ describe("NodeKeyConsistencyService — retry/recheck scheduling (CC-36 round 2) it("start() is idempotent: calling it again restarts cleanly instead of stacking loops", async () => { const checkNodeRegistration = jest.fn(() => Promise.resolve(true)); const chain = makeChain({ checkNodeRegistration: checkNodeRegistration as any }); - const service = new NodeKeyConsistencyService(chain, realBls, realSigner); + const service = new NodeKeyConsistencyService(chain, realBls); service.start(nodeState(), "0xcontract"); await jest.advanceTimersByTimeAsync(10); @@ -243,7 +243,7 @@ describe("NodeKeyConsistencyService — retry/recheck scheduling (CC-36 round 2) .mockImplementationOnce(() => firstCallPromise) // 1st call: hangs until resolved manually .mockImplementation(() => Promise.resolve(true)); // every later call: normal ok const chain = makeChain({ checkNodeRegistration: checkNodeRegistration as any }); - const service = new NodeKeyConsistencyService(chain, realBls, realSigner); + const service = new NodeKeyConsistencyService(chain, realBls); const oldNodeId = "0x" + "aa".repeat(32); const newNodeId = "0x" + "bb".repeat(32); @@ -274,7 +274,7 @@ describe("NodeKeyConsistencyService — retry/recheck scheduling (CC-36 round 2) it("HIGH-3: an unexpected throw deep inside a check() attempt never produces an unhandledRejection, and the loop survives it", async () => { const chain = makeChain({}); - const service = new NodeKeyConsistencyService(chain, realBls, realSigner); + const service = new NodeKeyConsistencyService(chain, realBls); // A pathological nodeState: reading .nodeId throws, simulating some future bug reading a // raw disk-JSON field this check doesn't (or one day might not) defensively guard. This @@ -299,19 +299,26 @@ describe("NodeKeyConsistencyService — retry/recheck scheduling (CC-36 round 2) expect(unhandled).toEqual([]); // The loop survived: it scheduled a next attempt instead of dying silently. expect(jest.getTimerCount()).toBe(1); + // FAIL-LOUD (round-3 Medium): this is NOT silent — /health must be able to see that the + // checker itself broke, not just that no result exists. + expect(service.getLastResult()?.checkerError).toContain("simulated malformed disk field"); + expect(service.getLastResult()?.checkerErrorAt).toBeDefined(); } finally { process.off("unhandledRejection", onUnhandledRejection); service.stop(); } }); - it("HIGH-3 regression: a non-string node_state field (raw disk JSON) does not crash the check", async () => { + it("HIGH-3 regression: a non-string node_state field (raw disk JSON) does not crash the check, and does NOT spuriously set checkerError", async () => { const chain = makeChain({ checkNodeRegistration: jest.fn(() => Promise.resolve(true)) as any, }); - const service = new NodeKeyConsistencyService(chain, realBls, realSigner); + const service = new NodeKeyConsistencyService(chain, realBls); // Simulates a node_state.json that doesn't match the NodeState type at runtime (no schema - // validation guards this file today) — publicKey as a number, not a string. + // validation guards this file today) — publicKey as a number, not a string. Round-3 + // hardened parsePublicKeyPoint()/normalizeHex() handle this gracefully (return + // null/"" rather than throw), so — unlike the poisoned-getter test above — this one + // should NOT reach the checker-error path at all; it's a plain determinate result. const malformed = { ...nodeState(), publicKey: 12345 as unknown as string }; const unhandled: unknown[] = []; @@ -324,10 +331,70 @@ describe("NodeKeyConsistencyService — retry/recheck scheduling (CC-36 round 2) expect(unhandled).toEqual([]); expect(service.getLastResult()).not.toBeNull(); + expect(service.getLastResult()?.checkerError).toBeUndefined(); expect(jest.getTimerCount()).toBe(1); } finally { process.off("unhandledRejection", onUnhandledRejection); service.stop(); } }); + + it("MEDIUM (round 3): a checker-internal throw preserves the prior conclusive result but stamps checkerError; clears on the next healthy attempt", async () => { + const checkNodeRegistration = jest.fn(() => Promise.resolve(true)); + const chain = makeChain({ checkNodeRegistration: checkNodeRegistration as any }); + const service = new NodeKeyConsistencyService(chain, realBls); + + const state = nodeState(); + const originalNodeId = state.nodeId; + + service.start(state, "0xcontract"); + await jest.advanceTimersByTimeAsync(10); + const first = service.getLastResult(); + expect(first?.status).toBe("ok"); + expect(first?.checkerError).toBeUndefined(); + + // Poison nodeId AFTER the first successful attempt, so the NEXT scheduled (periodic + // recheck) attempt throws internally — simulating a future bug, NOT an RPC blip. + Object.defineProperty(state, "nodeId", { + configurable: true, + get(): never { + throw new Error("simulated internal bug"); + }, + }); + + await jest.advanceTimersByTimeAsync(14 * 60_000); // well under the ~900s recheck + expect(checkNodeRegistration).toHaveBeenCalledTimes(1); + await jest.advanceTimersByTimeAsync(65_000); // past it: the recheck fires and throws + + const second = service.getLastResult(); + // The prior CONCLUSIVE result is preserved — an internal bug is NOT grounds to erase it + // (same non-downgrade principle as an RPC failure). + expect(second?.status).toBe("ok"); + expect(second?.checkedAt).toBe(first?.checkedAt); + // …but it is now explicitly, visibly flagged as checker-broken (fail-loud, not silent), + // and lastAttemptAt still advanced (the loop is alive, still ticking). + expect(second?.checkerError).toContain("simulated internal bug"); + expect(second?.checkerErrorAt).toBeDefined(); + expect(second?.lastAttemptAt).not.toBe(first?.lastAttemptAt); + // checkNodeRegistration must NOT have been reached — the throw happens before checkOnchain. + expect(checkNodeRegistration).toHaveBeenCalledTimes(1); + + // Un-poison — the next attempt completes normally and must clear checkerError, proving + // it isn't a permanent scar once the checker is healthy again. + Object.defineProperty(state, "nodeId", { + configurable: true, + value: originalNodeId, + writable: true, + }); + await jest.advanceTimersByTimeAsync(16 * 60_000); // past the next ~900s recheck + + const third = service.getLastResult(); + expect(third?.checkerError).toBeUndefined(); + expect(third?.checkerErrorAt).toBeUndefined(); + expect(third?.status).toBe("ok"); + expect(third?.checkedAt).not.toBe(second?.checkedAt); // a fresh conclusion was established + expect(checkNodeRegistration).toHaveBeenCalledTimes(2); + + service.stop(); + }); }); diff --git a/src/modules/node/node-key-consistency.service.spec.ts b/src/modules/node/node-key-consistency.service.spec.ts index 99e7857..670c43b 100644 --- a/src/modules/node/node-key-consistency.service.spec.ts +++ b/src/modules/node/node-key-consistency.service.spec.ts @@ -5,23 +5,31 @@ import { NodeState } from "../../interfaces/node.interface.js"; import { BlockchainService } from "../blockchain/blockchain.service.js"; import { BlsService } from "../bls/bls.service.js"; import { SignerService } from "../signer/signer.service.js"; +import { ConfigService } from "@nestjs/config"; -/** - * A real BlsService — NOT a mock — used purely for its `encodePublicKeyToEIP2537` (a pure - * function of its argument; it reads none of the injected BlockchainService/SignerService - * state). Fixtures are built with the exact same encoder the check itself is required to - * use (CC-36 round-2 review), so "ok" here means byte-identical agreement with what - * `registerOnChain()` actually writes on-chain — not agreement with a second, possibly - * drifted, re-implementation. - */ -const realBls = new BlsService({} as any, {} as any); +function makeConfig(overrides: Record = {}): ConfigService { + return { get: (k: string) => overrides[k] } as unknown as ConfigService; +} /** * A real SignerService (round-2 Medium-5) — resolves to `LocalKeySigner` under the default - * "local" backend (config.get always returns undefined) — so `checkLocal()` derives keys + * "local" backend — so the local branch of `BlsService.resolveSigningPublicKey` derives keys * through the EXACT validation the node signs with, never a separate hex re-parse. */ -const realSigner = new SignerService({ get: () => undefined } as any); +const realSigner = new SignerService(makeConfig()); + +/** + * A real BlsService — NOT a mock — built with `realSigner` and a controllable config, so + * `resolveSigningPublicKey` (round-3 High-2) exercises the SAME signer-selection logic + * `signDerivedHash()` uses. Fixtures are built with the exact same `encodePublicKeyToEIP2537` + * encoder the check itself is required to use (round-2 review), so "ok" here means + * byte-identical agreement with what `registerOnChain()` actually writes on-chain — not + * agreement with a second, possibly drifted, re-implementation. + */ +function makeBls(configOverrides: Record = {}): BlsService { + return new BlsService({} as any, realSigner, makeConfig(configOverrides)); +} +const realBls = makeBls(); // no rustSignerUrl -> local signer selection (the common case) /** * Real BLS keypairs (not hardcoded fixtures) so every fixture is internally @@ -67,7 +75,7 @@ function makeBlockchainMock(overrides: { } function makeService(chain: BlockchainService, blsService: BlsService = realBls) { - return new NodeKeyConsistencyService(chain, blsService, realSigner); + return new NodeKeyConsistencyService(chain, blsService); } describe("NodeKeyConsistencyService (CC-36)", () => { @@ -233,14 +241,19 @@ describe("NodeKeyConsistencyService (CC-36)", () => { }); it("uses BlsService.encodePublicKeyToEIP2537 — NOT a local re-implementation — to encode for the on-chain comparison (CC-36 round 2)", async () => { - // A stub BlsService that returns an unmistakable sentinel instead of a real encoding. - // If the check ever reverted to a local re-implementation of the encoder (ignoring the - // injected BlsService), this stub's output would never be consulted, the comparison - // would use the REAL encoding instead, `getNodePublicKey`'s sentinel wouldn't match it, - // and this test would go red on "ok" -> "mismatch_onchain". + // A stub BlsService that returns an unmistakable sentinel instead of a real encoding + // (resolveSigningPublicKey is delegated to the real realBls instance so the LOCAL branch + // still works normally — only the encoder is stubbed). If the check ever reverted to a + // local re-implementation of the encoder (ignoring the injected BlsService), this stub's + // output would never be consulted, the comparison would use the REAL encoding instead, + // getNodePublicKey's sentinel wouldn't match it, and this test would go red on + // "ok" -> "mismatch_onchain". const sentinelEip2537 = "0x" + "ee".repeat(128); const encodeSpy = jest.fn(() => sentinelEip2537); - const stubBls = { encodePublicKeyToEIP2537: encodeSpy } as unknown as BlsService; + const stubBls = { + encodePublicKeyToEIP2537: encodeSpy, + resolveSigningPublicKey: realBls.resolveSigningPublicKey.bind(realBls), + } as unknown as BlsService; const chain = makeBlockchainMock({ checkNodeRegistration: () => Promise.resolve(true), getNodePublicKey: () => Promise.resolve(sentinelEip2537), @@ -258,6 +271,7 @@ describe("NodeKeyConsistencyService (CC-36)", () => { const sentinelEip2537 = "0x" + "ee".repeat(128); const stubBls = { encodePublicKeyToEIP2537: jest.fn(() => sentinelEip2537), + resolveSigningPublicKey: realBls.resolveSigningPublicKey.bind(realBls), } as unknown as BlsService; const chain = makeBlockchainMock({ checkNodeRegistration: () => Promise.resolve(true), @@ -325,7 +339,8 @@ describe("NodeKeyConsistencyService (CC-36)", () => { }); const service = makeService(chain); // Right length (256 hex chars) but NOT a valid curve point (all zero x, all-ones y — off - // curve). assertValidity() must reject this rather than @noble/curves silently accepting it. + // curve, and the padding is non-zero too). Must be rejected rather than @noble/curves + // silently accepting it. const bogus = "0x" + "00".repeat(64) + "ff".repeat(64); const state = baseNodeState({ publicKey: bogus, privateKey: undefined as any }); const result = await service.check(state, "0xcontract"); @@ -333,5 +348,172 @@ describe("NodeKeyConsistencyService (CC-36)", () => { // checkOnchain must say "unknown" (never guess ok/mismatch for something unparsable). expect(result.status).toBe("unknown"); }); + + it("HIGH-1.1: non-zero padding is rejected as malformed, not silently normalized (round 3)", async () => { + // Flip only the FIRST padding nibble (byte offset 0, which must be zero in a real + // EIP-2537 encoding) from '0' to '1'. X and Y are untouched — WITHOUT a padding check, + // this would still parse to the exact same point as KEY_A.eip2537Hex and report "ok". + const body = KEY_A.eip2537Hex.slice(2); + const mutated = "0x1" + body.slice(1); + const chain = makeBlockchainMock({ + checkNodeRegistration: () => Promise.resolve(true), + getNodePublicKey: () => Promise.resolve(KEY_A.eip2537Hex), // the CORRECTLY-encoded key + }); + const service = makeService(chain); + const state = baseNodeState({ publicKey: mutated, privateKey: undefined as any }); + const result = await service.check(state, "0xcontract"); + expect(result.status).not.toBe("ok"); + expect(result.status).toBe("unknown"); // can't parse the local (malformed) key to compare + }); + + it("HIGH-1.2: the point-at-infinity (128-byte all-zero) is rejected, NEVER reported ok — the rogue-key bypass registerWithProof guards against on-chain", async () => { + const infinityKey = "0x" + "00".repeat(128); + const chain = makeBlockchainMock({ + checkNodeRegistration: () => Promise.resolve(true), + // Adversarially assume the chain ALSO somehow has the same (it shouldn't, but even so + // this must never be reported ok). + getNodePublicKey: () => Promise.resolve(infinityKey), + }); + const service = makeService(chain); + const state = baseNodeState({ publicKey: infinityKey, privateKey: undefined as any }); + const result = await service.check(state, "0xcontract"); + expect(result.status).not.toBe("ok"); + expect(result.status).toBe("unknown"); // infinity is rejected -> unparsable -> can't compare + }); + + it("HIGH-1.2b: the point-at-infinity (compressed 48-byte canonical encoding) is rejected too", async () => { + // Canonical BLS12-381 compressed-infinity: compression flag (0x80) | infinity flag + // (0x40) on the first byte, all remaining bytes zero. @noble/curves' assertValidity() + // ALONE does not reject this (it's a mathematically valid group-identity point) — only + // an explicit .is0() check does. + const bytes = new Uint8Array(48); + bytes[0] = 0xc0; + const infinityCompressed = "0x" + Buffer.from(bytes).toString("hex"); + const chain = makeBlockchainMock({ + checkNodeRegistration: () => Promise.resolve(true), + // A REAL registered key (not another all-zero value) — if the infinity guard were + // missing, encoding the (wrongly-accepted) infinity point would ALSO produce all-zero + // 128 bytes, and comparing that against an all-zero on-chain value would coincidentally + // still read "not ok" (skipped_no_private_key) for the WRONG reason, masking the bug. + getNodePublicKey: () => Promise.resolve(KEY_A.eip2537Hex), + }); + const service = makeService(chain); + const state = baseNodeState({ publicKey: infinityCompressed, privateKey: undefined as any }); + const result = await service.check(state, "0xcontract"); + expect(result.status).not.toBe("ok"); + expect(result.status).toBe("unknown"); // infinity rejected -> unparsable -> can't compare + }); + }); + + describe("HIGH-2: signer selection must match the REAL signing path (local @noble/curves vs. remote Rust/TEE-KMS)", () => { + const originalFetch = global.fetch; + afterEach(() => { + global.fetch = originalFetch; + }); + + it("remote signer configured+reachable, reporting a DIFFERENT key (B) than local/state/chain (A) => NOT ok — the silent-failure scenario", async () => { + // Everything LOCAL looks consistent as A (privateKey derives A, state.publicKey is A, + // on-chain registeredKeys is A) — but the node is actually configured for remote + // signing, and the REAL signer signs with B. A check that compares the local key would + // wrongly say "ok" while every real signature the node produces actually uses B. + global.fetch = jest.fn(async () => ({ + ok: true, + status: 200, + json: async () => ({ + public_key: KEY_B.compressedHex, + signature: "0x", + signature_compact: "0x", + }), + })) as any; + const remoteBls = makeBls({ rustSignerUrl: "http://fake-signer.local" }); + const chain = makeBlockchainMock({ + checkNodeRegistration: () => Promise.resolve(true), + getNodePublicKey: () => Promise.resolve(KEY_A.eip2537Hex), + }); + const service = makeService(chain, remoteBls); + const result = await service.check(baseNodeState(), "0xcontract"); // privateKey=A, publicKey=A + expect(result.status).not.toBe("ok"); + expect(result.status).toBe("mismatch_local"); + }); + + it("remote signer configured+reachable, reporting the key state/chain ALREADY reflect (B) => ok, even though local privateKey is a stale A — the false-positive scenario", async () => { + global.fetch = jest.fn(async () => ({ + ok: true, + status: 200, + json: async () => ({ + public_key: KEY_B.compressedHex, + signature: "0x", + signature_compact: "0x", + }), + })) as any; + const remoteBls = makeBls({ rustSignerUrl: "http://fake-signer.local" }); + const chain = makeBlockchainMock({ + checkNodeRegistration: () => Promise.resolve(true), + getNodePublicKey: () => Promise.resolve(KEY_B.eip2537Hex), + }); + const service = makeService(chain, remoteBls); + // privateKey stays A (stale/irrelevant now that remote signing is configured); + // publicKey correctly reflects B (what the remote signer + chain actually use). + const state = baseNodeState({ publicKey: KEY_B.compressedHex }); + const result = await service.check(state, "0xcontract"); + expect(result.status).toBe("ok"); + }); + + it("remote signer configured but UNREACHABLE => skipped_remote_signer, NEVER falls back to comparing the local key", async () => { + global.fetch = jest.fn(async () => { + throw new Error("ECONNREFUSED"); + }) as any; + const remoteBls = makeBls({ rustSignerUrl: "http://fake-signer.local" }); + const chain = makeBlockchainMock({ + checkNodeRegistration: () => Promise.resolve(true), + // Deliberately matches local/state — a fallback-to-local check would wrongly say "ok". + getNodePublicKey: () => Promise.resolve(KEY_A.eip2537Hex), + }); + const service = makeService(chain, remoteBls); + const result = await service.check(baseNodeState(), "0xcontract"); + expect(result.status).not.toBe("ok"); + expect(result.status).not.toBe("mismatch_local"); + expect(result.status).toBe("skipped_remote_signer"); + }); + + it("remote signer HTTP error response (non-2xx) also => skipped_remote_signer, not a crash", async () => { + global.fetch = jest.fn(async () => ({ + ok: false, + status: 503, + json: async () => ({}), + })) as any; + const remoteBls = makeBls({ rustSignerUrl: "http://fake-signer.local" }); + const chain = makeBlockchainMock({ + checkNodeRegistration: () => Promise.resolve(true), + getNodePublicKey: () => Promise.resolve(KEY_A.eip2537Hex), + }); + const service = makeService(chain, remoteBls); + const result = await service.check(baseNodeState(), "0xcontract"); + expect(result.status).toBe("skipped_remote_signer"); + }); + + it("RUST_SIGNER_REQUIRED does not change what this check reports — remote is asked first regardless", async () => { + global.fetch = jest.fn(async () => ({ + ok: true, + status: 200, + json: async () => ({ + public_key: KEY_B.compressedHex, + signature: "0x", + signature_compact: "0x", + }), + })) as any; + const remoteBls = makeBls({ + rustSignerUrl: "http://fake-signer.local", + rustSignerRequired: true, + }); + const chain = makeBlockchainMock({ + checkNodeRegistration: () => Promise.resolve(true), + getNodePublicKey: () => Promise.resolve(KEY_B.eip2537Hex), + }); + const service = makeService(chain, remoteBls); + const state = baseNodeState({ publicKey: KEY_B.compressedHex }); + const result = await service.check(state, "0xcontract"); + expect(result.status).toBe("ok"); + }); }); }); diff --git a/src/modules/node/node-key-consistency.service.ts b/src/modules/node/node-key-consistency.service.ts index d632baa..fbef78a 100644 --- a/src/modules/node/node-key-consistency.service.ts +++ b/src/modules/node/node-key-consistency.service.ts @@ -3,7 +3,6 @@ import { ethers } from "ethers"; import { bls } from "../../utils/bls.util.js"; import { BlockchainService } from "../blockchain/blockchain.service.js"; import { BlsService } from "../bls/bls.service.js"; -import { SignerService } from "../signer/signer.service.js"; import { NodeState } from "../../interfaces/node.interface.js"; export type KeyConsistencyStatus = @@ -12,7 +11,8 @@ export type KeyConsistencyStatus = | "mismatch_onchain" | "not_registered" | "unknown" - | "skipped_no_private_key"; + | "skipped_no_private_key" + | "skipped_remote_signer"; export interface KeyConsistencyResult { /** @@ -34,6 +34,18 @@ export interface KeyConsistencyResult { */ derivedMatches: boolean | null; nodeId: string; + /** + * Round-3 Medium: set when the CHECKER ITSELF threw unexpectedly on the most recent + * attempt (a bug — NOT an RPC blip, which already collapses to `unknown` inside `check()` + * and never reaches here). `status`/`checkedAt`/`derivedMatches` are the last CONCLUSIVE + * result from BEFORE the checker broke — preserved, never downgraded — but this field + * means that conclusion has NOT been re-verified since `checkerErrorAt`; treat it as + * stale. Cleared the next time an attempt completes without the checker itself throwing + * (regardless of what status that attempt computes — even a fresh `unknown` clears it, + * because it proves the checker is running again). + */ + checkerError?: string; + checkerErrorAt?: string; } /** The outcome of a single, stateless comparison — before it's merged into the held `lastResult`. */ @@ -60,10 +72,12 @@ function normalizeHex(hex: unknown): string { * online and silently non-participating. * * Three-way comparison (one attempt, see `computeFresh`): - * 1. local: privateKey -> derive pubkey via the REAL signing path (SignerService.forNode / - * LocalKeySigner, never a local re-parse of the hex) -> compare to - * node_state.publicKey (accepting EITHER wire format actually seen in - * production — see `parsePublicKeyPoint`) + * 1. local: resolve the CURRENTLY AUTHORITATIVE signing public key via + * `BlsService.resolveSigningPublicKey` — the exact same signer selection + * `signDerivedHash()` uses (local @noble/curves vs. remote Rust/TEE-KMS, + * round-3 High-2) — and compare it to node_state.publicKey (accepting + * EITHER wire format actually seen in production — see `parsePublicKeyPoint`, + * which also rejects a malformed or point-at-infinity value, round-3 High-1) * 2. on-chain: node_state.publicKey normalized to EIP-2537 via the SAME encoder the * registration path writes with — BlsService.encodePublicKeyToEIP2537, * never a local re-implementation, so this check can never silently @@ -72,8 +86,11 @@ function normalizeHex(hex: unknown): string { * 3. derivedMatches (informational only): nodeId == keccak256(EIP-2537 publicKey) * * Fail-loud, not fail-stop: a single attempt is designed to never throw — every failure mode - * (malformed key material, RPC error, timeout) collapses into a `status` value — and `loop()` - * additionally holds a structural boundary in case a future bug throws anyway (round-2 High-3). + * (malformed key material, RPC error, timeout) collapses into a `status` value. `loop()` + * additionally holds a structural boundary in case a future bug throws anyway (round-2 + * High-3) — but unlike an RPC blip, an internal throw is FAIL-LOUD, not silent: it stamps + * `checkerError`/`checkerErrorAt` onto the (preserved) result rather than quietly retrying + * forever with no visible trace (round-3 Medium). * * Runs as a self-scheduling loop (`start()`), not a one-shot: an `unknown` result * (RPC error) retries with bounded backoff (30s/60s/120s/240s, then folding into the @@ -109,8 +126,7 @@ export class NodeKeyConsistencyService implements OnModuleDestroy { constructor( private readonly blockchainService: BlockchainService, - private readonly blsService: BlsService, - private readonly signerService: SignerService + private readonly blsService: BlsService ) {} /** The most recently held result (a preserved conclusion survives a failed retry). Read by /health. */ @@ -170,12 +186,14 @@ export class NodeKeyConsistencyService implements OnModuleDestroy { ): Promise { try { await this.check(nodeState, contractAddress, myGeneration); - } catch { - // Structural boundary (round-2 High-3): check() is designed to never throw (every - // failure mode collapses into a status), but this is the last line of defense in case a - // future bug does anyway. The retry loop must survive a single bad tick and keep - // scheduling rather than dying silently, and this handler does NOTHING that could - // itself throw (no logger call here — logging happens, if at all, inside check()). + } catch (error: unknown) { + // Structural boundary (round-2 High-3), FAIL-LOUD not silent (round-3 Medium): see + // recordCheckerError. This is deliberately the INNER catch (around check() only) so + // scheduling below still runs even when check() itself threw — an internal bug must + // not also kill the retry loop's future ticks. The OUTER `.catch(() => {})` at both + // call sites of loop() is a separate, last-resort "never crash" net with no logic of + // its own — the two layers have different jobs and must stay separate. + this.recordCheckerError(nodeState, error); } if (this.stopped || myGeneration !== this.generation) { // Either stopped, or superseded by a newer start()/stop() while this attempt was in @@ -192,6 +210,57 @@ export class NodeKeyConsistencyService implements OnModuleDestroy { this.timer.unref?.(); } + /** + * Round-3 Medium: `check()` is designed to never throw (every failure mode it knows about + * collapses into a status), but if it does anyway — a bug, NOT an RPC blip — this must be + * FAIL-LOUD, never silent. A deterministic internal bug looking identical to "everything's + * fine, still ok" forever is worse than no result at all — it is a confident lie. The prior + * CONCLUSIVE result is preserved (same non-downgrade principle as an RPC failure: an + * internal bug is not grounds to erase a real mismatch_onchain either), but the result is + * stamped `checkerError`/`checkerErrorAt` so /health visibly flags that conclusion as + * stale-since, and `lastAttemptAt` still advances (proving the loop itself is alive). + */ + private recordCheckerError(nodeState: NodeState, error: unknown): void { + const now = new Date().toISOString(); + const message = error instanceof Error ? error.message : String(error); + const base: KeyConsistencyResult = + this.lastResult ?? + ({ + status: "unknown", + checkedAt: now, + lastAttemptAt: now, + derivedMatches: null, + nodeId: this.safeNodeId(nodeState), + } as KeyConsistencyResult); + this.lastResult = { + ...base, + lastAttemptAt: now, + checkerError: message, + checkerErrorAt: now, + }; + try { + this.logger.error( + `Key-consistency self-check threw unexpectedly (this should never happen — check() is ` + + `designed to fail loud via a status value, not throw): ${message}. Preserving the ` + + `last conclusive result (${this.lastResult.status} from ${this.lastResult.checkedAt}) ` + + `— it has NOT been re-verified since; treat it as stale until the next successful attempt.` + ); + } catch { + // Logging itself must never be allowed to turn a checker bug into a crash — this is + // exactly the concern that made the OUTER loop()-call-site catches necessary too. + } + } + + /** Best-effort nodeId read for the synthetic fallback result above — must not itself throw + * (e.g. if nodeId is a throwing getter on a pathological nodeState). */ + private safeNodeId(nodeState: NodeState): string { + try { + return typeof nodeState?.nodeId === "string" ? nodeState.nodeId : ""; + } catch { + return ""; + } + } + private nextDelayMs(): number { const backoff = NodeKeyConsistencyService.UNKNOWN_BACKOFF_MS; if (this.consecutiveUnknown > 0 && this.consecutiveUnknown <= backoff.length) { @@ -204,10 +273,12 @@ export class NodeKeyConsistencyService implements OnModuleDestroy { * Run one attempt and merge it into the held result. * * The merge rule is the load-bearing part: a fresh `unknown` (this attempt's RPC failed) - * NEVER overwrites a previously-held CONCLUSIVE result — it only bumps `lastAttemptAt`. - * Reading a network blip as "we no longer know" would silently launder away a real - * mismatch_onchain the moment the RPC hiccups, which defeats the whole point of this - * check. `checkedAt` only advances when a conclusion was actually (re-)established. + * NEVER overwrites a previously-held CONCLUSIVE result — it only bumps `lastAttemptAt` (and + * clears any stale `checkerError` — reaching this line at all proves the checker itself + * did NOT throw this time, regardless of what status it computed). Reading a network blip + * as "we no longer know" would silently launder away a real mismatch_onchain the moment the + * RPC hiccups, which defeats the whole point of this check. `checkedAt` only advances when + * a conclusion was actually (re-)established. * * `generationToken` is set ONLY by `loop()` (a direct/manual call — as every existing unit * test makes — omits it and always publishes). When set and stale (round-2 Medium-4: a @@ -242,7 +313,12 @@ export class NodeKeyConsistencyService implements OnModuleDestroy { if (fresh.status === "unknown") { this.consecutiveUnknown++; if (this.lastResult && this.lastResult.status !== "unknown") { - this.lastResult = { ...this.lastResult, lastAttemptAt: now }; + this.lastResult = { + ...this.lastResult, + lastAttemptAt: now, + checkerError: undefined, + checkerErrorAt: undefined, + }; } else { // No prior conclusion to preserve (first-ever attempt, or every attempt so far has // been unknown) — "unknown" IS the current conclusion in that case. @@ -273,14 +349,14 @@ export class NodeKeyConsistencyService implements OnModuleDestroy { this.logger.error( `KEY CONSISTENCY FAILURE [${status}] for node ${nodeState.nodeId}: ` + (status === "mismatch_local" - ? `the private key in node_state.json does NOT derive node_state.publicKey ` + + ? `the current signing key does NOT match node_state.publicKey ` + `(local publicKey=${localPrefix}...). ` : `node_state.publicKey (${localPrefix}...) does NOT match the on-chain ` + `registeredKeys() entry for this nodeId. `) + `This node will keep signing, but its signatures can NEVER verify against the ` + `on-chain registration — it is online and healthy while being silently ` + `non-participating. Fix: re-register the correct public key on-chain for this ` + - `nodeId, or restore the matching privateKey/publicKey pair in node_state.json.` + `nodeId, or restore the matching signer/publicKey pair in node_state.json.` ); } @@ -299,33 +375,59 @@ export class NodeKeyConsistencyService implements OnModuleDestroy { } /** - * privateKey -> derived pubkey vs. node_state.publicKey. Key-less nodes are a legitimate - * skip. Delegates key derivation to the REAL signing path (round-2 Medium-5): a bare - * `SignerService.forNode(nodeState)` throws exactly when `LocalKeySigner`'s constructor - * would reject the private key (wrong length, non-hex char, etc.) — the same validation - * the node actually signs with — so this can never say "ok" for a key the signer would - * refuse, and can never silently coerce an invalid hex char into 0. + * The CURRENTLY AUTHORITATIVE signing public key vs. node_state.publicKey. Delegates + * signer selection entirely to `BlsService.resolveSigningPublicKey` (round-3 High-2): a + * node may be configured for remote (Rust/TEE-KMS) signing while STILL carrying a + * (possibly stale) local `privateKey` in node_state.json, so this must NEVER just compare + * the local private key unconditionally — that would compare the wrong thing whenever a + * remote signer is in play, producing either a false "ok" (local matches, but every real + * signature is remote and different) or a false mismatch (local is stale, but every real + * signature is remote and fine). + * + * `"no_signer"` (neither a local key nor a remote signer configured) maps to the + * legitimate `skipped_no_private_key` outcome. `"remote_unavailable"` (a remote signer IS + * configured but did not answer) maps to `skipped_remote_signer` — explicitly NOT a + * fallback to comparing the local key, which is exactly the bug this exists to prevent. */ - private async checkLocal(nodeState: NodeState): Promise<"ok" | "mismatch" | "skipped"> { - if (!nodeState.privateKey) { - return "skipped"; - } + private async checkLocal( + nodeState: NodeState + ): Promise<"ok" | "mismatch" | "skipped_no_private_key" | "skipped_remote_signer"> { try { - const signer = this.signerService.forNode(nodeState); - const derivedPub = await signer.getPublicKey(); + const resolved = await this.blsService.resolveSigningPublicKey(nodeState); + if (resolved.source === "no_signer") { + return "skipped_no_private_key"; + } + if (resolved.source === "remote_unavailable") { + return "skipped_remote_signer"; + } + + let derivedHex: string; + if (resolved.source === "local") { + derivedHex = normalizeHex(resolved.publicKey.toHex()); + } else { + const remotePoint = this.parsePublicKeyPoint(resolved.publicKeyHex); + if (remotePoint === null) { + this.logger.warn( + `remote signer reported a public key for node ${nodeState.nodeId} that is not a ` + + "valid G1 point in either accepted wire format" + ); + return "mismatch"; + } + derivedHex = normalizeHex(remotePoint.toHex()); + } + const statePoint = this.parsePublicKeyPoint(nodeState.publicKey); if (statePoint === null) { // node_state.publicKey isn't a valid point in EITHER accepted wire format. return "mismatch"; } - const derivedHex = normalizeHex(derivedPub.toHex()); - const stateHex = normalizeHex(statePoint.toHex()); - return derivedHex === stateHex ? "ok" : "mismatch"; + return derivedHex === normalizeHex(statePoint.toHex()) ? "ok" : "mismatch"; } catch (error: any) { - // The signer rejected the private key (or deriving/comparing otherwise failed) — - // cannot prove correctness, so treat as a mismatch rather than silently skipping. + // Signer resolution itself failed unexpectedly (e.g. the local signer rejected a + // malformed private key) — cannot prove correctness, so treat as a mismatch rather + // than silently skipping. this.logger.warn( - `local key-consistency check could not derive/compare a public key: ${error?.message ?? error}` + `local key-consistency check could not resolve/compare a signing public key: ${error?.message ?? error}` ); return "mismatch"; } @@ -397,30 +499,52 @@ export class NodeKeyConsistencyService implements OnModuleDestroy { * * The 128-byte branch reconstructs the point via @noble/curves' OWN `Point.fromAffine` (a * public primitive of the SAME class `.fromHex()` returns) from the raw X/Y coordinates, - * then calls `.assertValidity()` — the identical subgroup/curve-equation check `.fromHex()` - * performs on the compressed form — so a malformed or off-curve 128-byte value is rejected - * exactly like a malformed compressed one would be, never silently accepted. + * after first validating the 16-byte zero-padding on EACH coordinate (round-3 High-1.1: a + * non-zero-padded value is not a well-formed EIP-2537 encoding — the encoder never + * produces one — so it must be rejected as malformed, not silently normalized as if it + * were legitimate). + * + * BOTH branches then explicitly reject the point-at-infinity (round-3 High-1.2, the more + * serious gap): `.assertValidity()` alone does NOT reject it — @noble/curves deliberately + * allows the group identity as "on-curve" — but an infinity "public key" is never a real + * one: `e(_, infinity) = 1` trivially, so a pairing-based signature check against it would + * pass with NO private key at all. This is exactly the rogue-key bypass + * `AAStarValidator.registerWithProof`'s `require(!_isInfinity(publicKey))` guards against + * on-chain (contracts/src/AAStarValidator.sol) — this check must refuse it with the same + * finality, not launder it through as a valid-looking "ok" comparison. */ private parsePublicKeyPoint(publicKeyHex: unknown): any | null { if (typeof publicKeyHex !== "string" || publicKeyHex.length === 0) { return null; } - const hex = publicKeyHex.replace(/^0x/i, ""); + const hex = publicKeyHex.replace(/^0x/i, "").toLowerCase(); try { if (hex.length === 96) { // 48-byte compressed G1. const point = bls.G1.Point.fromHex(hex); point.assertValidity(); + if (point.is0()) { + return null; // point-at-infinity — see class doc above. + } return point; } if (hex.length === 256) { // 128-byte EIP-2537: 16 zero-pad + 48-byte X + 16 zero-pad + 48-byte Y — the exact // inverse layout of BlsService's private encodeG1Point (byte offsets 16 and 80; in - // hex-char offsets that's X at [32,128) and Y at [160,256)). + // hex-char offsets that's X at [32,128) and Y at [160,256), padding at [0,32) and + // [128,160)). + const pad1 = hex.slice(0, 32); + const pad2 = hex.slice(128, 160); + if (pad1 !== "0".repeat(32) || pad2 !== "0".repeat(32)) { + return null; // non-zero padding: not a well-formed EIP-2537 encoding. + } const x = BigInt("0x" + hex.slice(32, 128)); const y = BigInt("0x" + hex.slice(160, 256)); const point = bls.G1.Point.fromAffine({ x, y }); point.assertValidity(); + if (point.is0()) { + return null; // point-at-infinity — see class doc above. + } return point; } return null; // neither accepted wire format @@ -454,19 +578,20 @@ export class NodeKeyConsistencyService implements OnModuleDestroy { * signal (the key on disk is definitely broken), then a proven on-chain mismatch * (the exact silent failure this check exists to catch), then the informational * not_registered / unknown on-chain outcomes, then — only once nothing else is - * wrong — `skipped_no_private_key` if the local dimension could not be checked at - * all (a key-less node is legitimate, but we were only able to verify half the - * picture, so we don't claim full `ok`). + * wrong — the two "couldn't fully verify the local dimension" outcomes + * (`skipped_no_private_key` / `skipped_remote_signer`; a legitimate configuration, but we + * only verified half the picture, so we don't claim full `ok`). */ private combine( - local: "ok" | "mismatch" | "skipped", + local: "ok" | "mismatch" | "skipped_no_private_key" | "skipped_remote_signer", onchain: "ok" | "mismatch" | "not_registered" | "unknown" ): KeyConsistencyStatus { if (local === "mismatch") return "mismatch_local"; if (onchain === "mismatch") return "mismatch_onchain"; if (onchain === "not_registered") return "not_registered"; if (onchain === "unknown") return "unknown"; - if (local === "skipped") return "skipped_no_private_key"; + if (local === "skipped_no_private_key") return "skipped_no_private_key"; + if (local === "skipped_remote_signer") return "skipped_remote_signer"; return "ok"; } } diff --git a/src/modules/node/node.module.ts b/src/modules/node/node.module.ts index 0a7c4b2..b8cdec2 100644 --- a/src/modules/node/node.module.ts +++ b/src/modules/node/node.module.ts @@ -5,13 +5,12 @@ import { IdentityController } from "./identity.controller.js"; import { NodeKeyConsistencyService } from "./node-key-consistency.service.js"; import { BlsModule } from "../bls/bls.module.js"; import { BlockchainModule } from "../blockchain/blockchain.module.js"; -import { SignerModule } from "../signer/signer.module.js"; @Module({ - // SignerModule is imported directly (not just transitively via BlsModule, which does not - // export SignerService) so NodeKeyConsistencyService can derive keys through the REAL - // signing path (round-2 Medium-5) rather than re-implementing hex validation. - imports: [forwardRef(() => BlsModule), BlockchainModule, SignerModule], + // NodeKeyConsistencyService gets signer selection through BlsService.resolveSigningPublicKey + // (round-3 High-2) rather than depending on SignerService directly — BlsModule already + // wires SignerService into BlsService, so no separate SignerModule import is needed here. + imports: [forwardRef(() => BlsModule), BlockchainModule], providers: [NodeService, NodeKeyConsistencyService], controllers: [NodeController, IdentityController], exports: [NodeService, NodeKeyConsistencyService], From 8fecc4256ed66a94ca5ec86595545f01272820ab Mon Sep 17 00:00:00 2001 From: jhfnetboy Date: Sun, 27 Sep 2026 15:04:42 +0700 Subject: [PATCH 5/9] =?UTF-8?q?fix(CC-36=20round=205):=20remove=20the=20re?= =?UTF-8?q?mote-signer=20probe=20=E2=80=94=20the=20self-check=20must=20nev?= =?UTF-8?q?er=20itself=20produce=20a=20signature?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex round 3 review (coordinator-flagged): round-4's resolveSigningPublicKey() probed the remote/rust signer's /sign endpoint with a fixed harmless message to read its public key off the response, so the check could distinguish which key a remote signer actually uses. That was the wrong call and is reverted. Why it had to go, in priority order: 1. It breaks this repo's core invariant that a node signs ONLY after the owner-authorization gate (authorizeAndDeriveHash) passes. A background health-check tick is not an authorized request, so it must never itself cause the signer to produce a signature, no matter how "harmless" the message — a KMS/TEE signer's every output should trace back to an authorized request; a probe every recheck cadence is ~96 unauthorized signatures/node/day with no owner-auth record behind them. 2. The remote signer's operational policy (metering, rate limits, anomaly detection, possible presence requirements) is opaque to this repo. A background probe competes with real signing traffic for that budget and could itself trip a limit or delay a real signature. 3. It shared the exact same BLS domain (BLS_DST, "..._RO_POP_") as production traffic, not a separate, clearly-scoped test domain. Fix: `BlsService.resolveSigningPublicKey()` now makes NO network call when a remote signer is configured — it returns `{ source: "remote" }` immediately, with nothing to compare. `NodeKeyConsistencyService.checkLocal()` reports this as `skipped_remote_signer`, same status name as before, but now reached without ever invoking the signer. `getRustSignerConfig()` (the shared selection point) is unchanged — it was a correct extraction and stays. `REMOTE_SIGNER_PROBE_HASH` and `probeRemoteSignerPublicKey()` are deleted, not left as dead code (`grep -rn "cc36" src` returns nothing). Honest, documented consequence (in both BlsService's and NodeKeyConsistencyService's docstrings): a remote-signing node's local dimension is simply NOT verified by this check right now — the A-vs-B misattribution scenarios round-3 tested for can no longer be distinguished, but they also can no longer produce a false "ok" or false "mismatch_local" either, since the local dimension isn't evaluated at all when remote is configured. Documented two follow-ups that would close this without ever causing an extra signature: (A) passive observation — record public_key from real, already-authorized /sign responses instead of asking for a dedicated read; (B) ask repo:kms for a read-only get-public-key endpoint on the remote signer, separate from /sign. Updated the 5 round-3 HIGH-2 tests (which asserted specific A/B outcomes via a mocked fetch) to the new behavior: any remote signer configuration alone produces `skipped_remote_signer`, with no fetch mock needed at all. Added the load-bearing invariant test in two places: a one-shot version in the main spec (check() called once) and a scheduling-spec version that spies on global fetch across a boot attempt plus three ~15-minute periodic recheck cycles, asserting zero calls throughout. Mutation-verified (not committed, reverted after confirming red): temporarily reintroduced a `fetch(...)` call in the remote branch of `resolveSigningPublicKey()` — both invariant tests (one-shot and scheduling-loop) correctly failed with "Expected number of calls: 0, Received: 1" / "...Received: 4". Restored file diffed byte-identical to the pre-mutation version before committing. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Pdq9rgkZq9M4JFeTZD7yYs --- src/modules/bls/bls.service.ts | 103 +++++++---------- ...key-consistency.service.scheduling.spec.ts | 39 +++++++ .../node/node-key-consistency.service.spec.ts | 106 ++++++------------ .../node/node-key-consistency.service.ts | 37 +++--- 4 files changed, 130 insertions(+), 155 deletions(-) diff --git a/src/modules/bls/bls.service.ts b/src/modules/bls/bls.service.ts index c0e1d29..bffe548 100644 --- a/src/modules/bls/bls.service.ts +++ b/src/modules/bls/bls.service.ts @@ -7,13 +7,6 @@ import { NodeKeyPair } from "../../interfaces/node.interface.js"; import { BlockchainService, PackedUserOp } from "../blockchain/blockchain.service.js"; import { SignerService } from "../signer/signer.service.js"; -/** - * A fixed, harmless probe message (32 bytes, clearly non-random so it's recognizable in any - * signer-side logs) used ONLY by `resolveSigningPublicKey`'s remote-signer probe — see there - * for why no dedicated "get public key" endpoint exists to ask for instead. - */ -const REMOTE_SIGNER_PROBE_HASH = "0x" + "cc36".repeat(16); - @Injectable() export class BlsService { private readonly logger = new Logger(BlsService.name); @@ -183,28 +176,50 @@ export class BlsService { } /** - * Round-3 High-2: which public key is CURRENTLY authoritative for this node's signing, - * resolved via the EXACT same signer selection `signDerivedHash()` uses - * (`getRustSignerConfig()`), without producing a real signature over caller-supplied - * data. The node key-consistency self-check uses this instead of unconditionally - * comparing the local `privateKey` field — a node can be configured for remote (Rust/ - * TEE-KMS) signing while STILL carrying a (possibly stale) local `privateKey` in - * node_state.json, so comparing the local key would be comparing the wrong thing + * Round-3 High-2, round-4 correction: which public key is CURRENTLY authoritative for + * this node's signing, resolved via the EXACT same signer selection `signDerivedHash()` + * uses (`getRustSignerConfig()`). The node key-consistency self-check uses this instead of + * unconditionally comparing the local `privateKey` field — a node can be configured for + * remote (Rust/TEE-KMS) signing while STILL carrying a (possibly stale) local `privateKey` + * in node_state.json, so comparing the local key would be comparing the wrong thing * entirely: it could report `ok` while every real (remote) signature actually fails, or * report a mismatch while every real signature is actually fine. * - * Deliberately simpler than `signDerivedHash()`'s RUST_SIGNER_REQUIRED fallback nuance: - * a real sign call may fall back to local when the remote is optional and unreachable, - * but for THIS purpose "remote is configured at all" already means it's the FIRST thing - * a real sign attempts (see signDerivedHash), so an unreachable remote here must NEVER - * silently fall back to reporting the local key as authoritative — that is exactly the - * bug this method exists to prevent. `required` only changes whether a REAL sign call - * falls back after a failed remote attempt; it does not change what this method reports. + * When a remote signer is configured, this method makes NO request to it and returns + * `{ source: "remote" }` with nothing to compare — round-4 review (Codex + coordinator): + * an earlier version probed the remote signer's `/sign` endpoint with a fixed, harmless + * message to read its public key off the response. That was REJECTED, because it breaks + * the repo's core invariant that a node signs ONLY after the owner-authorization gate + * (`authorizeAndDeriveHash`) passes (see CLAUDE.md, and the fail-closed gate above) — a + * background health-check tick is not an authorized request, so it must never itself + * cause the signer to produce a signature, no matter how "harmless" the message is: + * 1. It breaks the audit invariant that every signature a TEE/KMS signer ever produces + * corresponds to an authorized request — a probe every cadence tick is an + * unauthorized signature with no owner-auth record behind it (~96/node/day at the + * 15-minute recheck cadence). + * 2. A KMS-TEE signer's operational policy is opaque to this repo — it may meter, + * rate-limit, run anomaly detection, or require operator/device presence per + * signature. A background probe competes with real traffic for that budget and could + * itself trip a rate limit or an alert, or in the worst case delay a real signature. + * 3. It signs in the SAME BLS domain (`BLS_DST`, the same `_RO_POP_` suffix) as + * production traffic — not a separate, clearly-scoped test domain. + * + * The honest consequence: a remote-signing node is NOT verified by this check right now + * (the self-check reports `skipped_remote_signer` — a deliberate, documented limitation, + * never silently guessed as ok or mismatch in either direction). Two follow-ups would close + * this WITHOUT ever causing an extra signature (neither is implemented here): + * A. Passive observation: every REAL, already-authorized `/sign` response already + * includes `public_key` (see `signViaRust`) — record it from actual signing traffic + * (keyed by node/time) and compare THAT against the on-chain registration instead of + * asking for a dedicated read here. Zero extra signatures; only requires the node to + * have signed at least once recently. + * B. Ask repo:kms to add a read-only "get public key" endpoint to the remote signer, + * distinct from `/sign`, so a health check can query identity without invoking + * signing at all. */ async resolveSigningPublicKey(node: NodeKeyPair): Promise< | { source: "local"; publicKey: any } - | { source: "remote"; publicKeyHex: string } - | { source: "remote_unavailable" } + | { source: "remote" } | { source: "no_signer" } > { const rust = this.getRustSignerConfig(); @@ -216,47 +231,7 @@ export class BlsService { const publicKey = await signer.getPublicKey(); return { source: "local", publicKey }; } - try { - const publicKeyHex = await this.probeRemoteSignerPublicKey(rust, node); - return { source: "remote", publicKeyHex }; - } catch (error: any) { - this.logger.warn( - `remote signer public-key probe failed for node ${node.nodeId}: ${error?.message ?? error}` - ); - return { source: "remote_unavailable" }; - } - } - - /** - * Ask the remote/rust signer for its public key. There is no dedicated "get public key" - * endpoint (the signer only exposes /sign) — every /sign response includes `public_key` - * alongside the signature, so this probes with a fixed, clearly-marked, harmless message - * reserved for this purpose. Shares the exact URL/token construction `signViaRust` uses - * (via the same `rust` config object) so this can never silently drift from the real - * signing request shape. - */ - private async probeRemoteSignerPublicKey( - rust: { url: string; token?: string }, - node: NodeKeyPair - ): Promise { - const url = `${rust.url.replace(/\/+$/, "")}/sign`; - const headers: Record = { "Content-Type": "application/json" }; - if (rust.token) { - headers["X-Signer-Token"] = rust.token; - } - const response = await fetch(url, { - method: "POST", - headers, - body: JSON.stringify({ user_op_hash: REMOTE_SIGNER_PROBE_HASH, node_id: node.nodeId }), - }); - if (!response.ok) { - throw new Error(`remote signer probe HTTP ${response.status}`); - } - const data = (await response.json()) as { public_key?: string }; - if (typeof data.public_key !== "string" || data.public_key.length === 0) { - throw new Error("remote signer probe returned no public_key"); - } - return data.public_key; + return { source: "remote" }; } /** Delegate signing to the local Rust signer. Throws on any transport/HTTP error. */ diff --git a/src/modules/node/node-key-consistency.service.scheduling.spec.ts b/src/modules/node/node-key-consistency.service.scheduling.spec.ts index 15a2c70..0864144 100644 --- a/src/modules/node/node-key-consistency.service.scheduling.spec.ts +++ b/src/modules/node/node-key-consistency.service.scheduling.spec.ts @@ -397,4 +397,43 @@ describe("NodeKeyConsistencyService — retry/recheck scheduling (CC-36 round 2/ service.stop(); }); + + it("INVARIANT (round-4): a remote-signer node NEVER causes the checker to call fetch (no probe, no signature), across boot + several periodic rechecks", async () => { + // Codex + coordinator round-4: the checker must NEVER invoke the remote signer at all — + // an earlier version probed its /sign endpoint to read a public key back, which produces + // a REAL, unauthorized signature (see BlsService.resolveSigningPublicKey's docstring for + // why that breaks this repo's core owner-authorization invariant). This test spies on the + // one shared entry point any such call would have to go through (global fetch) and runs + // the self-check through a boot attempt plus several periodic recheck cycles, asserting + // zero calls throughout — not just on a single one-shot check(). + const fetchSpy = jest.fn(); + const originalFetch = global.fetch; + global.fetch = fetchSpy as any; + try { + const remoteBls = new BlsService( + {} as any, + realSigner, + { get: (k: string) => (k === "rustSignerUrl" ? "http://fake-signer.local" : undefined) } as any + ); + const checkNodeRegistration = jest.fn(() => Promise.resolve(true)); + const chain = makeChain({ checkNodeRegistration: checkNodeRegistration as any }); + const service = new NodeKeyConsistencyService(chain, remoteBls); + + service.start(nodeState(), "0xcontract"); + await jest.advanceTimersByTimeAsync(10); // boot attempt + expect(service.getLastResult()?.status).toBe("skipped_remote_signer"); + + // Several periodic recheck cycles (skipped_remote_signer is non-"unknown", so the + // cadence is the regular ~15-minute one each time, same as a conclusive "ok"). + await jest.advanceTimersByTimeAsync(16 * 60_000); + await jest.advanceTimersByTimeAsync(16 * 60_000); + await jest.advanceTimersByTimeAsync(16 * 60_000); + expect(checkNodeRegistration.mock.calls.length).toBeGreaterThanOrEqual(4); // boot + 3 rechecks actually ran + + expect(fetchSpy).not.toHaveBeenCalled(); + service.stop(); + } finally { + global.fetch = originalFetch; + } + }); }); diff --git a/src/modules/node/node-key-consistency.service.spec.ts b/src/modules/node/node-key-consistency.service.spec.ts index 670c43b..05e50fb 100644 --- a/src/modules/node/node-key-consistency.service.spec.ts +++ b/src/modules/node/node-key-consistency.service.spec.ts @@ -406,102 +406,51 @@ describe("NodeKeyConsistencyService (CC-36)", () => { }); describe("HIGH-2: signer selection must match the REAL signing path (local @noble/curves vs. remote Rust/TEE-KMS)", () => { + // Round-4 correction (coordinator + Codex): an earlier version of this check PROBED the + // remote signer's /sign endpoint to read its public key back, so it could compare A/B + // scenarios directly. That was rejected — it makes a background health check itself + // produce a real, unauthorized signature (see BlsService.resolveSigningPublicKey's + // docstring). The corrected behavior is simpler and more conservative: ANY remote signer + // configuration => skipped_remote_signer, with NO network call at all. This trades away + // the ability to distinguish "remote actually signs A" from "remote actually signs B" — + // an honest, documented limitation — but it also means the OLD false-"ok" and + // false-mismatch_local failure modes (comparing the possibly-stale local privateKey + // against what a remote signer is doing) can no longer happen either, since the local + // dimension is never even evaluated when a remote signer is configured. const originalFetch = global.fetch; afterEach(() => { global.fetch = originalFetch; }); - it("remote signer configured+reachable, reporting a DIFFERENT key (B) than local/state/chain (A) => NOT ok — the silent-failure scenario", async () => { - // Everything LOCAL looks consistent as A (privateKey derives A, state.publicKey is A, - // on-chain registeredKeys is A) — but the node is actually configured for remote - // signing, and the REAL signer signs with B. A check that compares the local key would - // wrongly say "ok" while every real signature the node produces actually uses B. - global.fetch = jest.fn(async () => ({ - ok: true, - status: 200, - json: async () => ({ - public_key: KEY_B.compressedHex, - signature: "0x", - signature_compact: "0x", - }), - })) as any; + it("remote signer configured => skipped_remote_signer, regardless of what node_state.privateKey/publicKey say locally", async () => { const remoteBls = makeBls({ rustSignerUrl: "http://fake-signer.local" }); const chain = makeBlockchainMock({ checkNodeRegistration: () => Promise.resolve(true), getNodePublicKey: () => Promise.resolve(KEY_A.eip2537Hex), }); const service = makeService(chain, remoteBls); - const result = await service.check(baseNodeState(), "0xcontract"); // privateKey=A, publicKey=A - expect(result.status).not.toBe("ok"); - expect(result.status).toBe("mismatch_local"); - }); - - it("remote signer configured+reachable, reporting the key state/chain ALREADY reflect (B) => ok, even though local privateKey is a stale A — the false-positive scenario", async () => { - global.fetch = jest.fn(async () => ({ - ok: true, - status: 200, - json: async () => ({ - public_key: KEY_B.compressedHex, - signature: "0x", - signature_compact: "0x", - }), - })) as any; - const remoteBls = makeBls({ rustSignerUrl: "http://fake-signer.local" }); - const chain = makeBlockchainMock({ - checkNodeRegistration: () => Promise.resolve(true), - getNodePublicKey: () => Promise.resolve(KEY_B.eip2537Hex), - }); - const service = makeService(chain, remoteBls); - // privateKey stays A (stale/irrelevant now that remote signing is configured); - // publicKey correctly reflects B (what the remote signer + chain actually use). - const state = baseNodeState({ publicKey: KEY_B.compressedHex }); - const result = await service.check(state, "0xcontract"); - expect(result.status).toBe("ok"); - }); - - it("remote signer configured but UNREACHABLE => skipped_remote_signer, NEVER falls back to comparing the local key", async () => { - global.fetch = jest.fn(async () => { - throw new Error("ECONNREFUSED"); - }) as any; - const remoteBls = makeBls({ rustSignerUrl: "http://fake-signer.local" }); - const chain = makeBlockchainMock({ - checkNodeRegistration: () => Promise.resolve(true), - // Deliberately matches local/state — a fallback-to-local check would wrongly say "ok". - getNodePublicKey: () => Promise.resolve(KEY_A.eip2537Hex), - }); - const service = makeService(chain, remoteBls); + // Everything LOCAL still looks consistent as A (privateKey derives A, state.publicKey + // is A) — under the OLD (rejected) probing design this would have been compared + // against whatever the remote claimed and could go either way; now it's never compared. const result = await service.check(baseNodeState(), "0xcontract"); expect(result.status).not.toBe("ok"); expect(result.status).not.toBe("mismatch_local"); expect(result.status).toBe("skipped_remote_signer"); }); - it("remote signer HTTP error response (non-2xx) also => skipped_remote_signer, not a crash", async () => { - global.fetch = jest.fn(async () => ({ - ok: false, - status: 503, - json: async () => ({}), - })) as any; + it("remote signer configured => skipped_remote_signer even when local privateKey is stale/irrelevant and state/chain reflect a different key", async () => { const remoteBls = makeBls({ rustSignerUrl: "http://fake-signer.local" }); const chain = makeBlockchainMock({ checkNodeRegistration: () => Promise.resolve(true), - getNodePublicKey: () => Promise.resolve(KEY_A.eip2537Hex), + getNodePublicKey: () => Promise.resolve(KEY_B.eip2537Hex), }); const service = makeService(chain, remoteBls); - const result = await service.check(baseNodeState(), "0xcontract"); + const state = baseNodeState({ publicKey: KEY_B.compressedHex }); // privateKey stays A + const result = await service.check(state, "0xcontract"); expect(result.status).toBe("skipped_remote_signer"); }); - it("RUST_SIGNER_REQUIRED does not change what this check reports — remote is asked first regardless", async () => { - global.fetch = jest.fn(async () => ({ - ok: true, - status: 200, - json: async () => ({ - public_key: KEY_B.compressedHex, - signature: "0x", - signature_compact: "0x", - }), - })) as any; + it("RUST_SIGNER_REQUIRED does not change what this check reports — any remote config => skipped_remote_signer", async () => { const remoteBls = makeBls({ rustSignerUrl: "http://fake-signer.local", rustSignerRequired: true, @@ -513,7 +462,20 @@ describe("NodeKeyConsistencyService (CC-36)", () => { const service = makeService(chain, remoteBls); const state = baseNodeState({ publicKey: KEY_B.compressedHex }); const result = await service.check(state, "0xcontract"); - expect(result.status).toBe("ok"); + expect(result.status).toBe("skipped_remote_signer"); + }); + + it("INVARIANT (round-4, the most important test in this file): a remote-signer node NEVER causes the checker to call fetch — no probe, no signature", async () => { + const fetchSpy = jest.fn(); + global.fetch = fetchSpy as any; + const remoteBls = makeBls({ rustSignerUrl: "http://fake-signer.local" }); + const chain = makeBlockchainMock({ + checkNodeRegistration: () => Promise.resolve(true), + getNodePublicKey: () => Promise.resolve(KEY_A.eip2537Hex), + }); + const service = makeService(chain, remoteBls); + await service.check(baseNodeState(), "0xcontract"); + expect(fetchSpy).not.toHaveBeenCalled(); }); }); }); diff --git a/src/modules/node/node-key-consistency.service.ts b/src/modules/node/node-key-consistency.service.ts index fbef78a..21cbaa2 100644 --- a/src/modules/node/node-key-consistency.service.ts +++ b/src/modules/node/node-key-consistency.service.ts @@ -77,7 +77,14 @@ function normalizeHex(hex: unknown): string { * `signDerivedHash()` uses (local @noble/curves vs. remote Rust/TEE-KMS, * round-3 High-2) — and compare it to node_state.publicKey (accepting * EITHER wire format actually seen in production — see `parsePublicKeyPoint`, - * which also rejects a malformed or point-at-infinity value, round-3 High-1) + * which also rejects a malformed or point-at-infinity value, round-3 High-1). + * When a REMOTE signer is configured, this comparison is deliberately NOT + * performed at all (round-4 correction): resolving the remote key would + * require asking the signer to sign a probe message, which would make this + * background health check itself produce an unauthorized signature — see + * `BlsService.resolveSigningPublicKey`'s docstring for why that is + * unacceptable. The honest, documented consequence is `skipped_remote_signer`: + * a remote-signing node is simply NOT verified by this dimension right now. * 2. on-chain: node_state.publicKey normalized to EIP-2537 via the SAME encoder the * registration path writes with — BlsService.encodePublicKeyToEIP2537, * never a local re-implementation, so this check can never silently @@ -385,9 +392,13 @@ export class NodeKeyConsistencyService implements OnModuleDestroy { * signature is remote and fine). * * `"no_signer"` (neither a local key nor a remote signer configured) maps to the - * legitimate `skipped_no_private_key` outcome. `"remote_unavailable"` (a remote signer IS - * configured but did not answer) maps to `skipped_remote_signer` — explicitly NOT a - * fallback to comparing the local key, which is exactly the bug this exists to prevent. + * legitimate `skipped_no_private_key` outcome. `"remote"` (a remote signer IS configured) + * ALSO maps to `skipped_remote_signer` (round-4 correction) — NEVER a fallback to + * comparing the local key (which is exactly the bug this exists to prevent) and NEVER a + * probe of the remote signer either (`resolveSigningPublicKey` deliberately makes no + * request to it — see its docstring: doing so would make this background check itself + * produce an unauthorized signature). This is a deliberate, honest limitation: a + * remote-signing node's local dimension is simply not verified right now. */ private async checkLocal( nodeState: NodeState @@ -397,24 +408,12 @@ export class NodeKeyConsistencyService implements OnModuleDestroy { if (resolved.source === "no_signer") { return "skipped_no_private_key"; } - if (resolved.source === "remote_unavailable") { + if (resolved.source === "remote") { return "skipped_remote_signer"; } - let derivedHex: string; - if (resolved.source === "local") { - derivedHex = normalizeHex(resolved.publicKey.toHex()); - } else { - const remotePoint = this.parsePublicKeyPoint(resolved.publicKeyHex); - if (remotePoint === null) { - this.logger.warn( - `remote signer reported a public key for node ${nodeState.nodeId} that is not a ` + - "valid G1 point in either accepted wire format" - ); - return "mismatch"; - } - derivedHex = normalizeHex(remotePoint.toHex()); - } + // resolved.source === "local" + const derivedHex = normalizeHex(resolved.publicKey.toHex()); const statePoint = this.parsePublicKeyPoint(nodeState.publicKey); if (statePoint === null) { From 5ea1e1fecf8c0be5204598a37718db4371f37890 Mon Sep 17 00:00:00 2001 From: jhfnetboy Date: Sun, 27 Sep 2026 15:19:27 +0700 Subject: [PATCH 6/9] fix(CC-36 round 6): atomic on-chain read pair, generation check before recording a checker error, lock signDerivedHash's remote path, expose checkerError at the /health contract MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex round 4 review (REQUEST_CHANGES, 0 Critical/0 High, 2 Medium + 2 Low). All of round 3's High-1/High-2/Medium were independently confirmed closed; the signDerivedHash refactor was manually diffed and confirmed behavior-preserving; no checker path was found that could produce a signature or contact the signer. This round addresses the remaining 4 findings. M1 — isRegistered(nodeId) and registeredKeys(nodeId) were two independent `latest` reads. A node deactivated by the validator's permissionless syncNode(nodeId) (which clears registeredKeys) exactly between the two calls would read "registered" (slightly-earlier call) + "empty key" (slightly-later call) and report mismatch_onchain — a status that was never actually true at any single point in time; the real transition is registered -> not_registered with no "registered but wrong key" state in between. Fixed: BlockchainService.checkNodeRegistration/getNodePublicKey now take an optional `blockTag`; NodeKeyConsistencyService.checkOnchain() reads getBlockNumber() ONCE and pins both calls to it (the same pattern the audit module already uses for finality-pinned reads). Added defense-in-depth: an empty registeredKeys() return is treated as not_registered regardless of what isRegistered() said. M2 — the catch in loop() called recordCheckerError() BEFORE checking the generation token, so an in-flight check from a SUPERSEDED identity that REJECTS (not just resolves — Medium-4 only covered the resolving case) could stamp its error onto whichever identity is current now, or resurrect a result clearResult() had just wiped. Fixed: staleness is checked FIRST in the catch; a stale/stopped rejection is discarded entirely (no checkerError, no resurrected result, no reschedule) — extracted the check into isStaleOrStopped() and reused it in both places in loop(). L1 — signDerivedHash()'s hybrid local/remote selection was refactored across rounds 4-5 but had zero test coverage of the actual remote signing path (only the checker's "never calls fetch" tests existed, which say nothing about whether signing itself still works). Added exactly the four tests requested, no broader: remote success response mapping, rustSignerToken sent as X-Signer-Token, RUST_SIGNER_REQUIRED=true + remote failure rethrows without fallback, and remote-optional + failure falls back to local. L2 — checkerError/checkerErrorAt (added round 4) were never asserted through the /health contract layer, only inside the service's own tests. Added a fixture + assertion in health.controller.spec.ts. Mutation-verified (4, not committed, each reverted to a byte-identical diff after confirming red): 1. Dropped the shared blockTag in checkOnchain() -> both new M1 tests failed (exact blockTag assertion, and the race-scenario regression test). 2. Moved the generation check to after recordCheckerError() in loop()'s catch -> both new M2 tests failed, reproducing exactly the described bug (checkerError leaked onto the new identity; a cleared result was resurrected with status "unknown"). 3. Removed the X-Signer-Token header from signViaRust() -> L1 test 2 failed. 4. Disabled the RUST_SIGNER_REQUIRED rethrow branch (forced fallback) -> L1 test 3 failed ("resolved instead of rejected"). Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Pdq9rgkZq9M4JFeTZD7yYs --- src/modules/blockchain/blockchain.service.ts | 25 +++- src/modules/bls/bls.service.spec.ts | 116 ++++++++++++++++++ src/modules/health/health.controller.spec.ts | 24 ++++ ...key-consistency.service.scheduling.spec.ts | 94 ++++++++++++++ .../node/node-key-consistency.service.spec.ts | 57 +++++++++ .../node/node-key-consistency.service.ts | 47 ++++++- 6 files changed, 355 insertions(+), 8 deletions(-) diff --git a/src/modules/blockchain/blockchain.service.ts b/src/modules/blockchain/blockchain.service.ts index c1bb7a5..3512e5f 100644 --- a/src/modules/blockchain/blockchain.service.ts +++ b/src/modules/blockchain/blockchain.service.ts @@ -256,7 +256,18 @@ export class BlockchainService { } } - async checkNodeRegistration(contractAddress: string, nodeId: string): Promise { + /** + * `blockTag` (optional) pins the read to a specific block — CC-36 round-6 M1: a caller + * that also reads `getNodePublicKey` for the SAME nodeId and wants the two reads to be + * atomic (not two independent `latest` reads racing a state change in between, e.g. the + * validator's permissionless `syncNode` deactivating the node between the two calls) must + * pass the SAME `blockTag` to both. Omitted, this reads `latest` exactly as before. + */ + async checkNodeRegistration( + contractAddress: string, + nodeId: string, + blockTag?: number + ): Promise { if (!this.provider) { throw new Error("Blockchain provider not configured"); } @@ -266,7 +277,7 @@ export class BlockchainService { const contract = new ethers.Contract(contractAddress, abi, this.provider); try { - const isRegistered = await contract.isRegistered(nodeId); + const isRegistered = await contract.isRegistered(nodeId, { blockTag }); return isRegistered; } catch (error: any) { this.logger.error(`Failed to check registration status: ${error.message}`); @@ -374,7 +385,13 @@ export class BlockchainService { } } - async getNodePublicKey(contractAddress: string, nodeId: string): Promise { + /** `blockTag` (optional) — see `checkNodeRegistration`'s doc for why a caller reading both + * in one logical check should pin both to the same block. Omitted, reads `latest`. */ + async getNodePublicKey( + contractAddress: string, + nodeId: string, + blockTag?: number + ): Promise { if (!this.provider) { throw new Error("Blockchain provider not configured"); } @@ -384,7 +401,7 @@ export class BlockchainService { const contract = new ethers.Contract(contractAddress, abi, this.provider); try { - const publicKey = await contract.registeredKeys(nodeId); + const publicKey = await contract.registeredKeys(nodeId, { blockTag }); return publicKey; } catch (error: any) { this.logger.error(`Failed to get node public key: ${error.message}`); diff --git a/src/modules/bls/bls.service.spec.ts b/src/modules/bls/bls.service.spec.ts index 048ba6c..48e160a 100644 --- a/src/modules/bls/bls.service.spec.ts +++ b/src/modules/bls/bls.service.spec.ts @@ -255,3 +255,119 @@ describe("BlsService — owner-authorization gate (Fix 2 Stage 1)", () => { expect(isValidOwnerAuth).toHaveBeenCalledWith(victimAccount, derivedHash, ownerAuth); }); }); + +/** + * CC-36 round-6 L1: `signDerivedHash()` (the hybrid local/remote signing selection) was + * refactored across rounds 4-5 (extracting `getRustSignerConfig()`, adding + * `resolveSigningPublicKey()`). Codex manually diffed the refactor against the original and + * confirmed behavior preservation, but the test SUITE never actually exercised the remote + * signing path at all — the node-key-consistency self-check's own remote-signer tests only + * prove the CHECKER doesn't call fetch; they say nothing about whether real remote SIGNING + * still works. These four tests lock signDerivedHash()'s own behavior directly (deliberately + * narrow scope, per the round-6 review — not expanding beyond these four). + */ +describe("BlsService — signDerivedHash hybrid local/remote signing (CC-36 round-6 L1)", () => { + const node: NodeKeyPair = { + nodeId: "node_test", + nodeName: "test", + privateKey: "0x" + "00".repeat(31) + "01", + publicKey: "0x", + description: "test node", + }; + const derivedHash = "0x8bb1b199f427dfc49e5fe40f2f3278cb1a48587824b78263051c8c4d81d77a81"; + + function makeConfig(overrides: Record) { + return { get: (k: string) => overrides[k] } as any; + } + + function makeService(configOverrides: Record) { + const blockchain = {} as unknown as InstanceType; + const signer = { + forNode: (n: { privateKey: string }) => new LocalKeySigner(n.privateKey), + } as unknown as SignerService; + return new BlsService(blockchain, signer, makeConfig(configOverrides)); + } + + const originalFetch = global.fetch; + afterEach(() => { + global.fetch = originalFetch; + }); + + it("1. remote signing succeeds and the response is mapped to SignatureResult correctly", async () => { + global.fetch = jest.fn(async () => ({ + ok: true, + status: 200, + json: async () => ({ + signature: "0xaaaa", + signature_compact: "bbbb", + public_key: "cccc", + }), + })) as any; + const service = makeService({ rustSignerUrl: "http://fake-signer.local" }); + + const result = await service.signDerivedHash(derivedHash, node); + + expect(result).toEqual({ + nodeId: node.nodeId, + signature: "0xaaaa", + signatureCompact: "bbbb", + publicKey: "cccc", + message: derivedHash, + }); + }); + + it("2. rustSignerToken is sent as the X-Signer-Token request header", async () => { + let capturedHeaders: Record | undefined; + global.fetch = jest.fn(async (_url: unknown, init: any) => { + capturedHeaders = init.headers; + return { + ok: true, + status: 200, + json: async () => ({ signature: "0x", signature_compact: "0x", public_key: "0x" }), + }; + }) as any; + const service = makeService({ + rustSignerUrl: "http://fake-signer.local", + rustSignerToken: "s3cr3t-token", + }); + + await service.signDerivedHash(derivedHash, node); + + expect(capturedHeaders?.["X-Signer-Token"]).toBe("s3cr3t-token"); + }); + + it("3. RUST_SIGNER_REQUIRED=true and the remote signer fails => rethrows, does NOT fall back to local", async () => { + global.fetch = jest.fn(async () => ({ + ok: false, + status: 500, + json: async () => ({}), + })) as any; + const service = makeService({ + rustSignerUrl: "http://fake-signer.local", + rustSignerRequired: true, + }); + + await expect(service.signDerivedHash(derivedHash, node)).rejects.toThrow( + /Rust signer HTTP 500/ + ); + }); + + it("4. remote signer is OPTIONAL and fails => falls back to local (@noble/curves) signing", async () => { + global.fetch = jest.fn(async () => ({ + ok: false, + status: 500, + json: async () => ({}), + })) as any; + // rustSignerRequired is NOT set -> optional. + const service = makeService({ rustSignerUrl: "http://fake-signer.local" }); + + const result = await service.signDerivedHash(derivedHash, node); + + // The mocked bls.util at the top of this file makes the local signing path + // deterministic: fakeG1Point.toHex() = "cd".repeat(48), fakeG2Point.toHex() = "ab".repeat(96). + expect(result.nodeId).toBe(node.nodeId); + expect(result.publicKey).toBe("cd".repeat(48)); + expect(result.signatureCompact).toBe("ab".repeat(96)); + expect(result.message).toBe(derivedHash); + }); +}); diff --git a/src/modules/health/health.controller.spec.ts b/src/modules/health/health.controller.spec.ts index e23381a..a29aa12 100644 --- a/src/modules/health/health.controller.spec.ts +++ b/src/modules/health/health.controller.spec.ts @@ -39,6 +39,30 @@ describe("HealthController", () => { expect(result.keyConsistency).toEqual(fakeResult); }); + it("CC-36 round-6 L2: checkerError/checkerErrorAt surface through /health when the checker itself broke", () => { + // These fields (round-3 Medium) mark a preserved conclusive result as stale-since a + // checker-internal bug, rather than silently looking like an unbroken fresh "ok" forever. + // Assert them at the /health CONTRACT layer, not just inside the service's own tests. + const fakeResult: KeyConsistencyResult = { + status: "ok", + checkedAt: "2026-01-01T00:00:00.000Z", + lastAttemptAt: "2026-01-01T00:20:00.000Z", + derivedMatches: true, + nodeId: "0xabc", + checkerError: "simulated internal bug", + checkerErrorAt: "2026-01-01T00:20:00.000Z", + }; + const controller = new HealthController(undefined, { + getLastResult: () => fakeResult, + } as any); + const result = controller.health(); + expect(result.keyConsistency?.checkerError).toBe("simulated internal bug"); + expect(result.keyConsistency?.checkerErrorAt).toBe("2026-01-01T00:20:00.000Z"); + // Still additive-only — the rest of the contract is untouched. + expect(result.status).toBe("ok"); + expect(result.capabilities).toEqual([]); + }); + it("root() endpoint is unaffected (CC-36 only touches /health)", () => { const controller = new HealthController(undefined, undefined); const result = controller.root(); diff --git a/src/modules/node/node-key-consistency.service.scheduling.spec.ts b/src/modules/node/node-key-consistency.service.scheduling.spec.ts index 0864144..17c8e2a 100644 --- a/src/modules/node/node-key-consistency.service.scheduling.spec.ts +++ b/src/modules/node/node-key-consistency.service.scheduling.spec.ts @@ -47,11 +47,15 @@ function nodeState(overrides: Partial = {}): NodeState { function makeChain(overrides: { checkNodeRegistration?: jest.Mock<() => Promise>; getNodePublicKey?: jest.Mock<() => Promise>; + getBlockNumber?: jest.Mock<() => Promise>; }): BlockchainService { return { checkNodeRegistration: overrides.checkNodeRegistration ?? jest.fn(() => Promise.resolve(true)), getNodePublicKey: overrides.getNodePublicKey ?? jest.fn(() => Promise.resolve(KEY_A.eip2537Hex)), + // Round-6 M1: checkOnchain() pins both reads to one shared block — fixed default so + // existing assertions (which predate the blockTag param) are unaffected. + getBlockNumber: overrides.getBlockNumber ?? jest.fn(() => Promise.resolve(12345)), } as unknown as BlockchainService; } @@ -272,6 +276,96 @@ describe("NodeKeyConsistencyService — retry/recheck scheduling (CC-36 round 2/ service.stop(); }); + it("M2 (round 6): an old in-flight check that REJECTS after being superseded checks staleness FIRST — no checkerError leaks onto the new identity", async () => { + // Codex round-3 finding: the catch in loop() called recordCheckerError() BEFORE checking + // the generation, so a superseded (stale) attempt that eventually REJECTS (not just + // resolves — Medium-4 only covered the resolving case) would stamp its error onto + // whichever identity happens to be CURRENT when it settles. This spies on check() itself + // to control exactly when the OLD attempt settles, independent of what internally causes + // it to reject. + const checkNodeRegistration = jest.fn(() => Promise.resolve(true)); + const chain = makeChain({ checkNodeRegistration: checkNodeRegistration as any }); + const service = new NodeKeyConsistencyService(chain, realBls); + + let rejectFirst!: (err: Error) => void; + const firstCheckPromise = new Promise((_resolve, reject) => { + rejectFirst = reject; + }); + const realCheck = service.check.bind(service); + let checkCallCount = 0; + jest.spyOn(service, "check").mockImplementation((...args: any[]) => { + checkCallCount++; + if (checkCallCount === 1) { + return firstCheckPromise; // the FIRST (old-identity) attempt hangs, then rejects + } + return (realCheck as any)(...args); + }); + + const oldNodeId = "0x" + "aa".repeat(32); + const newNodeId = "0x" + "bb".repeat(32); + + service.start(nodeState({ nodeId: oldNodeId }), "0xcontract"); + await jest.advanceTimersByTimeAsync(1); // loop() is now awaiting the hung first check() + expect(service.getLastResult()).toBeNull(); + + // Supersede it with a NEW identity while the old attempt is still hanging. + service.start(nodeState({ nodeId: newNodeId }), "0xcontract"); + await jest.advanceTimersByTimeAsync(1); // the new loop's OWN check() runs for real + expect(service.getLastResult()?.status).toBe("ok"); + expect(service.getLastResult()?.nodeId).toBe(newNodeId); + + // NOW the OLD (superseded) check() finally REJECTS. + rejectFirst(new Error("stale RPC error from a superseded identity")); + await jest.advanceTimersByTimeAsync(1); + + const after = service.getLastResult(); + // Untouched: still the NEW identity's clean "ok" — no checkerError leaked onto it. + expect(after?.nodeId).toBe(newNodeId); + expect(after?.status).toBe("ok"); + expect(after?.checkerError).toBeUndefined(); + // The stale loop must not reschedule either — only ONE timer pending. + expect(jest.getTimerCount()).toBe(1); + + service.stop(); + }); + + it("M2b (round 6): an old in-flight check that REJECTS after clearResult() must not resurrect the cleared result", async () => { + const checkNodeRegistration = jest.fn(() => Promise.resolve(true)); + const chain = makeChain({ checkNodeRegistration: checkNodeRegistration as any }); + const service = new NodeKeyConsistencyService(chain, realBls); + + let rejectFirst!: (err: Error) => void; + const firstCheckPromise = new Promise((_resolve, reject) => { + rejectFirst = reject; + }); + const realCheck = service.check.bind(service); + let checkCallCount = 0; + jest.spyOn(service, "check").mockImplementation((...args: any[]) => { + checkCallCount++; + if (checkCallCount === 1) { + return firstCheckPromise; + } + return (realCheck as any)(...args); + }); + + service.start(nodeState(), "0xcontract"); + await jest.advanceTimersByTimeAsync(1); // still hanging on the first (only) check() + expect(service.getLastResult()).toBeNull(); + + // Node deleted: stop + clearResult (mirrors NodeService.reloadNodeState()'s "no node" path). + service.stop(); + service.clearResult(); + expect(service.getLastResult()).toBeNull(); + + // The OLD hung check now rejects. + rejectFirst(new Error("stale RPC error after deletion")); + await jest.advanceTimersByTimeAsync(1); + + // Must STILL be null — the stale rejection must not resurrect anything. + expect(service.getLastResult()).toBeNull(); + expect(jest.getTimerCount()).toBe(0); + }); + it("HIGH-3: an unexpected throw deep inside a check() attempt never produces an unhandledRejection, and the loop survives it", async () => { const chain = makeChain({}); const service = new NodeKeyConsistencyService(chain, realBls); diff --git a/src/modules/node/node-key-consistency.service.spec.ts b/src/modules/node/node-key-consistency.service.spec.ts index 05e50fb..857b619 100644 --- a/src/modules/node/node-key-consistency.service.spec.ts +++ b/src/modules/node/node-key-consistency.service.spec.ts @@ -65,12 +65,16 @@ function baseNodeState(overrides: Partial = {}): NodeState { function makeBlockchainMock(overrides: { checkNodeRegistration?: () => Promise; getNodePublicKey?: () => Promise; + getBlockNumber?: () => Promise; }): BlockchainService { return { checkNodeRegistration: jest.fn( overrides.checkNodeRegistration ?? (() => Promise.resolve(false)) ), getNodePublicKey: jest.fn(overrides.getNodePublicKey ?? (() => Promise.resolve("0x"))), + // Round-6 M1: checkOnchain() pins both reads to one block — every test gets a fixed + // default so existing assertions (which predate the blockTag param) are unaffected. + getBlockNumber: jest.fn(overrides.getBlockNumber ?? (() => Promise.resolve(12345))), } as unknown as BlockchainService; } @@ -405,6 +409,59 @@ describe("NodeKeyConsistencyService (CC-36)", () => { }); }); + describe("M1 (round 6): isRegistered and registeredKeys must be read at the SAME blockTag — never two independent `latest` reads", () => { + it("both on-chain reads receive the identical blockTag returned by getBlockNumber()", async () => { + const seen: { isRegistered: (number | undefined)[]; registeredKeys: (number | undefined)[] } = + { isRegistered: [], registeredKeys: [] }; + const chain = { + getBlockNumber: jest.fn(() => Promise.resolve(100)), + checkNodeRegistration: jest.fn((_addr: string, _id: string, blockTag?: number) => { + seen.isRegistered.push(blockTag); + return Promise.resolve(true); + }), + getNodePublicKey: jest.fn((_addr: string, _id: string, blockTag?: number) => { + seen.registeredKeys.push(blockTag); + return Promise.resolve(KEY_A.eip2537Hex); + }), + } as unknown as BlockchainService; + const service = makeService(chain); + await service.check(baseNodeState(), "0xcontract"); + expect(seen.isRegistered).toEqual([100]); + expect(seen.registeredKeys).toEqual([100]); + expect(chain.getBlockNumber).toHaveBeenCalledTimes(1); // one shared read, not two + }); + + it("REGRESSION: a node deactivated between the two reads (syncNode race) never surfaces as mismatch_onchain — a status that was never actually true", async () => { + // Simulate: at block 100 the node is registered with the correct key; by block 101 + // (one block later) syncNode() deactivated it and cleared registeredKeys. Pinning both + // reads to ONE getBlockNumber() call means both see the consistent block-100 snapshot + // (registered + correct key -> ok), never the impossible "registered + empty key" blend + // a pair of independent `latest` reads straddling the transition could produce. + const chain = { + getBlockNumber: jest.fn(() => Promise.resolve(100)), + checkNodeRegistration: jest.fn((_addr: string, _id: string, blockTag?: number) => + Promise.resolve(blockTag === 100) // true only "as of block 100" + ), + getNodePublicKey: jest.fn((_addr: string, _id: string, blockTag?: number) => + Promise.resolve(blockTag === 100 ? KEY_A.eip2537Hex : "0x") // empty "as of block 101+" + ), + } as unknown as BlockchainService; + const service = makeService(chain); + const result = await service.check(baseNodeState(), "0xcontract"); + expect(result.status).toBe("ok"); + }); + + it("defense-in-depth: an empty registeredKeys() return is treated as not_registered, never mismatch_onchain, even if isRegistered() somehow still said true", async () => { + const chain = makeBlockchainMock({ + checkNodeRegistration: () => Promise.resolve(true), + getNodePublicKey: () => Promise.resolve("0x"), // empty bytes + }); + const service = makeService(chain); + const result = await service.check(baseNodeState(), "0xcontract"); + expect(result.status).toBe("not_registered"); + }); + }); + describe("HIGH-2: signer selection must match the REAL signing path (local @noble/curves vs. remote Rust/TEE-KMS)", () => { // Round-4 correction (coordinator + Codex): an earlier version of this check PROBED the // remote signer's /sign endpoint to read its public key back, so it could compare A/B diff --git a/src/modules/node/node-key-consistency.service.ts b/src/modules/node/node-key-consistency.service.ts index 21cbaa2..e682054 100644 --- a/src/modules/node/node-key-consistency.service.ts +++ b/src/modules/node/node-key-consistency.service.ts @@ -194,6 +194,15 @@ export class NodeKeyConsistencyService implements OnModuleDestroy { try { await this.check(nodeState, contractAddress, myGeneration); } catch (error: unknown) { + // Round-6 M2: check staleness BEFORE recording anything. An in-flight attempt from a + // SUPERSEDED identity/loop (start()/stop() ran while it was pending) that REJECTS must + // be discarded exactly like one that resolves normally (Medium-4's non-throwing path + // already does this) — otherwise its error would stamp `checkerError` onto (or + // fabricate a synthetic result for) whichever identity is CURRENT now, potentially + // resurrecting a result `clearResult()` had just wiped for a deleted node. + if (this.isStaleOrStopped(myGeneration)) { + return; + } // Structural boundary (round-2 High-3), FAIL-LOUD not silent (round-3 Medium): see // recordCheckerError. This is deliberately the INNER catch (around check() only) so // scheduling below still runs even when check() itself threw — an internal bug must @@ -202,7 +211,7 @@ export class NodeKeyConsistencyService implements OnModuleDestroy { // its own — the two layers have different jobs and must stay separate. this.recordCheckerError(nodeState, error); } - if (this.stopped || myGeneration !== this.generation) { + if (this.isStaleOrStopped(myGeneration)) { // Either stopped, or superseded by a newer start()/stop() while this attempt was in // flight — don't reschedule. This is also what keeps exactly one timer alive at a time. return; @@ -217,6 +226,11 @@ export class NodeKeyConsistencyService implements OnModuleDestroy { this.timer.unref?.(); } + /** A generation captured at loop()-start time is stale once a newer start()/stop() ran. */ + private isStaleOrStopped(myGeneration: number): boolean { + return this.stopped || myGeneration !== this.generation; + } + /** * Round-3 Medium: `check()` is designed to never throw (every failure mode it knows about * collapses into a status), but if it does anyway — a bug, NOT an RPC blip — this must be @@ -432,15 +446,34 @@ export class NodeKeyConsistencyService implements OnModuleDestroy { } } - /** node_state.publicKey (EIP-2537-encoded) vs. registeredKeys(nodeId), gated by isRegistered. */ + /** + * node_state.publicKey (EIP-2537-encoded) vs. registeredKeys(nodeId), gated by isRegistered. + * + * Round-6 M1: `isRegistered` and `registeredKeys` are pinned to the SAME `blockTag` (read + * once up front) rather than each defaulting to its own independent `latest` read. Without + * this, a node deactivated by the validator's permissionless `syncNode(nodeId)` (which + * clears `registeredKeys`) exactly between the two calls would read "registered" (from the + * first, slightly-earlier call) + "empty key" (from the second, slightly-later call) and + * report `mismatch_onchain` — a status that was never actually true at ANY single point in + * time; the real transition is registered -> not_registered, with no "registered but wrong + * key" state in between. Pinning both reads to one block makes them atomic. + * + * The empty-`registeredKeys` check right after is defense-in-depth for the same scenario + * (e.g. an RPC/indexer that doesn't honor `blockTag` consistently) — an empty on-chain key + * is treated as `not_registered` rather than `mismatch_onchain` even if `isRegistered` + * somehow still said true. + */ private async checkOnchain( nodeState: NodeState, contractAddress: string ): Promise<"ok" | "mismatch" | "not_registered" | "unknown"> { try { + const blockTag = await this.blockchainService.getBlockNumber(); + const isRegistered = await this.blockchainService.checkNodeRegistration( contractAddress, - nodeState.nodeId + nodeState.nodeId, + blockTag ); if (!isRegistered) { return "not_registered"; @@ -460,8 +493,14 @@ export class NodeKeyConsistencyService implements OnModuleDestroy { const onchainKey = await this.blockchainService.getNodePublicKey( contractAddress, - nodeState.nodeId + nodeState.nodeId, + blockTag ); + if (!onchainKey || onchainKey === "0x") { + // Defense-in-depth (see docstring above): an empty registeredKeys() return means + // "not actually registered" regardless of what isRegistered() reported. + return "not_registered"; + } return normalizeHex(onchainKey) === normalizeHex(localEip2537) ? "ok" : "mismatch"; } catch (error: any) { // RPC failure/timeout/revert. NEVER read this as a mismatch (would falsely accuse a From e0e8f2f3abeba3d7708483da871e7348f1528d82 Mon Sep 17 00:00:00 2001 From: jhfnetboy Date: Sun, 27 Sep 2026 15:35:51 +0700 Subject: [PATCH 7/9] fix(CC-36 round 7): verify the optional-remote-signer's local fallback key, scoped and tagged so it's never mistaken for full verification MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex round 5 review: round 3's M1/M2/L1/L2 all confirmed closed; BlockchainService's new optional blockTag param confirmed backward-compatible with registerOnChain/ dashboard and all other existing callers. One new High found — again a gap in the coordinator's own round-5/6 spec, this time missing the required/optional distinction. High — round 5/6 made resolveSigningPublicKey() return `{ source: "remote" }" for ANY remote signer configuration, required or optional, so the checker mapped it to skipped_remote_signer unconditionally. But signDerivedHash() SILENTLY FALLS BACK to the local privateKey whenever an OPTIONAL remote call fails (bls.service.ts:145-156) — so in optional mode the local key is a REAL signing key in active use, not a stale leftover, and leaving it unverified reproduced exactly the silent-failure gap the checker exists to close: local key stale (A), chain re-registered (B), optional remote unavailable => every real fallback signature (A) fails against the chain (B) forever, while /health confidently reported "skipped" as if nothing needed checking. node-key-consistency.service.spec.ts had a test that PINNED this unsafe behavior. Fix: required/optional is read from getRustSignerConfig().required (the single selection point — not re-read). Required mode (or optional mode with no local key) is unchanged: `{ source: "remote" }`, skipped_remote_signer, no verification possible without producing a signature. Optional mode WITH a local privateKey now returns `{ source: "local_fallback", publicKey }` — derived by the exact same pure computation as the "local" branch (zero network calls, zero signatures — the "checker never signs" invariant is untouched, since deriving a public key from a private key needs no signing). NodeKeyConsistencyService.checkLocal() runs the normal three-way comparison against this key and reports ok/mismatch_local/ mismatch_onchain/not_registered/unknown same as always. Scope tagging (the safety-critical part): any result built from the local_fallback path is stamped `scope: "local_fallback_only"` on the KeyConsistencyResult, exposed through /health. An "ok" here proves ONLY that the fallback key is internally consistent — it says nothing about the remote signer's own (primary, unverifiable-without-signing) key, and must never be read as full verification. Without this tag a fallback-scoped "ok" would become a new source of false comfort. Replaced the round-4/5 test that pinned the unsafe "always skip" behavior with the silent-failure regression it should have caught, and added the required-mode / no-local-key / consistent-ok scenarios the coordinator specified. Extended both "checker never calls fetch" invariant tests (main spec + scheduling spec) to also cover the new local_fallback derivation path (pure computation, confirmed still zero network I/O) and added a dedicated required-mode invariant test alongside it. Mutation-verified (3, not committed, each reverted to a byte-identical diff after confirming red): 1. Disabled the optional-mode local_fallback branch (forced skip) -> the silent-failure regression test failed on the missing `scope` field (status alone didn't change, since skipped_remote_signer + mismatch_onchain still combines to mismatch_onchain — the scope assertion is what actually catches this, confirming why it's required, not decorative). 2. Dropped the `scope` field from the conclusive-result branch in check() -> the "ok scoped to local_fallback_only" test failed (scope undefined). 3. Made required mode ALSO check the local key -> the "required + stale local key" test failed (mismatch_local instead of the correct skipped_remote_signer). Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Pdq9rgkZq9M4JFeTZD7yYs --- src/modules/bls/bls.service.ts | 37 +++++- src/modules/health/health.controller.spec.ts | 21 ++++ ...key-consistency.service.scheduling.spec.ts | 52 +++++++- .../node/node-key-consistency.service.spec.ts | 114 +++++++++++++----- .../node/node-key-consistency.service.ts | 59 ++++++--- 5 files changed, 228 insertions(+), 55 deletions(-) diff --git a/src/modules/bls/bls.service.ts b/src/modules/bls/bls.service.ts index bffe548..55292d3 100644 --- a/src/modules/bls/bls.service.ts +++ b/src/modules/bls/bls.service.ts @@ -204,10 +204,11 @@ export class BlsService { * 3. It signs in the SAME BLS domain (`BLS_DST`, the same `_RO_POP_` suffix) as * production traffic — not a separate, clearly-scoped test domain. * - * The honest consequence: a remote-signing node is NOT verified by this check right now - * (the self-check reports `skipped_remote_signer` — a deliberate, documented limitation, - * never silently guessed as ok or mismatch in either direction). Two follow-ups would close - * this WITHOUT ever causing an extra signature (neither is implemented here): + * The honest consequence: the remote signer's OWN key is NOT verified by this check right + * now (the self-check reports `skipped_remote_signer` for that dimension — a deliberate, + * documented limitation, never silently guessed as ok or mismatch in either direction). Two + * follow-ups would close this WITHOUT ever causing an extra signature (neither is + * implemented here): * A. Passive observation: every REAL, already-authorized `/sign` response already * includes `public_key` (see `signViaRust`) — record it from actual signing traffic * (keyed by node/time) and compare THAT against the on-chain registration instead of @@ -216,9 +217,29 @@ export class BlsService { * B. Ask repo:kms to add a read-only "get public key" endpoint to the remote signer, * distinct from `/sign`, so a health check can query identity without invoking * signing at all. + * + * Round-7 correction (Codex + coordinator): the round-4/5 version returned `{ source: + * "remote" }` for ANY remote configuration, required or optional. That was itself a gap in + * OPTIONAL mode (`rust.required === false`): `signDerivedHash()` SILENTLY FALLS BACK to the + * local key (`signViaNode`) whenever the remote call fails — so in optional mode the local + * `privateKey` is a REAL signing key that gets used, not a stale leftover. Skipping it + * entirely left exactly the silent-failure gap this whole check exists to close (a stale + * local fallback key, never verified, signing against the wrong on-chain registration). + * Required mode has NO fallback path at all, so the local key stays signing-irrelevant + * there and is still never touched. + * + * So now: required mode (or optional-but-no-local-key) -> `{ source: "remote" }` + * (unverifiable, no fallback exists to check). Optional mode WITH a local `privateKey` -> + * `{ source: "local_fallback", publicKey }`, derived by the exact same PURE computation as + * the `"local"` branch (no network call, no signature — `resolveSigningPublicKey` still + * never causes the checker to sign or contact any signer). The caller (the node + * key-consistency self-check) MUST tag any result built from `"local_fallback"` with an + * explicit scope (`scope: "local_fallback_only"`) — an `ok` here proves only that the + * FALLBACK key is internally consistent, never that the remote signer's primary key is. */ async resolveSigningPublicKey(node: NodeKeyPair): Promise< | { source: "local"; publicKey: any } + | { source: "local_fallback"; publicKey: any } | { source: "remote" } | { source: "no_signer" } > { @@ -231,6 +252,14 @@ export class BlsService { const publicKey = await signer.getPublicKey(); return { source: "local", publicKey }; } + if (!rust.required && node.privateKey) { + // Optional remote + a local key exists: signDerivedHash() WILL fall back to signing + // with this exact key if the remote call fails, so verifying it (pure derivation, zero + // signatures) closes a real gap rather than opening one — see the docstring above. + const signer = this.signerService.forNode(node); + const publicKey = await signer.getPublicKey(); + return { source: "local_fallback", publicKey }; + } return { source: "remote" }; } diff --git a/src/modules/health/health.controller.spec.ts b/src/modules/health/health.controller.spec.ts index a29aa12..116a3fb 100644 --- a/src/modules/health/health.controller.spec.ts +++ b/src/modules/health/health.controller.spec.ts @@ -63,6 +63,27 @@ describe("HealthController", () => { expect(result.capabilities).toEqual([]); }); + it("CC-36 round-7: scope: \"local_fallback_only\" surfaces through /health — an ok result must be readable as scoped, not as a full verification", () => { + // Round-7: in optional remote-signer mode, `ok` may come from checking only the LOCAL + // fallback key (never the remote signer's own key, which can't be checked without + // producing a signature). Assert this scope flag is visible at the /health CONTRACT + // layer so a caller that reads `status === "ok"` can also see it's scope-limited. + const fakeResult: KeyConsistencyResult = { + status: "ok", + checkedAt: "2026-01-01T00:00:00.000Z", + lastAttemptAt: "2026-01-01T00:00:00.000Z", + derivedMatches: true, + nodeId: "0xabc", + scope: "local_fallback_only", + }; + const controller = new HealthController(undefined, { + getLastResult: () => fakeResult, + } as any); + const result = controller.health(); + expect(result.keyConsistency?.scope).toBe("local_fallback_only"); + expect(result.status).toBe("ok"); + }); + it("root() endpoint is unaffected (CC-36 only touches /health)", () => { const controller = new HealthController(undefined, undefined); const result = controller.root(); diff --git a/src/modules/node/node-key-consistency.service.scheduling.spec.ts b/src/modules/node/node-key-consistency.service.scheduling.spec.ts index 17c8e2a..0bd974d 100644 --- a/src/modules/node/node-key-consistency.service.scheduling.spec.ts +++ b/src/modules/node/node-key-consistency.service.scheduling.spec.ts @@ -492,7 +492,7 @@ describe("NodeKeyConsistencyService — retry/recheck scheduling (CC-36 round 2/ service.stop(); }); - it("INVARIANT (round-4): a remote-signer node NEVER causes the checker to call fetch (no probe, no signature), across boot + several periodic rechecks", async () => { + it("INVARIANT (round-4/7): a remote-signer node NEVER causes the checker to call fetch (no probe, no signature), across boot + several periodic rechecks — OPTIONAL mode (exercises the local_fallback derivation path)", async () => { // Codex + coordinator round-4: the checker must NEVER invoke the remote signer at all — // an earlier version probed its /sign endpoint to read a public key back, which produces // a REAL, unauthorized signature (see BlsService.resolveSigningPublicKey's docstring for @@ -500,6 +500,11 @@ describe("NodeKeyConsistencyService — retry/recheck scheduling (CC-36 round 2/ // one shared entry point any such call would have to go through (global fetch) and runs // the self-check through a boot attempt plus several periodic recheck cycles, asserting // zero calls throughout — not just on a single one-shot check(). + // + // Round-7: this config (rustSignerUrl set, rustSignerRequired NOT set) is OPTIONAL mode, + // and nodeState() carries a privateKey — so this now exercises the local_fallback + // derivation path (a REAL local key check, but pure @noble/curves computation, zero + // network I/O) on every single tick. The invariant must still hold: fetch stays at 0. const fetchSpy = jest.fn(); const originalFetch = global.fetch; global.fetch = fetchSpy as any; @@ -515,10 +520,11 @@ describe("NodeKeyConsistencyService — retry/recheck scheduling (CC-36 round 2/ service.start(nodeState(), "0xcontract"); await jest.advanceTimersByTimeAsync(10); // boot attempt - expect(service.getLastResult()?.status).toBe("skipped_remote_signer"); + // Optional mode + a local key + node_state internally consistent -> ok, scoped. + expect(service.getLastResult()?.status).toBe("ok"); + expect(service.getLastResult()?.scope).toBe("local_fallback_only"); - // Several periodic recheck cycles (skipped_remote_signer is non-"unknown", so the - // cadence is the regular ~15-minute one each time, same as a conclusive "ok"). + // Several periodic recheck cycles. await jest.advanceTimersByTimeAsync(16 * 60_000); await jest.advanceTimersByTimeAsync(16 * 60_000); await jest.advanceTimersByTimeAsync(16 * 60_000); @@ -530,4 +536,42 @@ describe("NodeKeyConsistencyService — retry/recheck scheduling (CC-36 round 2/ global.fetch = originalFetch; } }); + + it("INVARIANT (round-7): REQUIRED mode also never calls fetch, across boot + several periodic rechecks", async () => { + const fetchSpy = jest.fn(); + const originalFetch = global.fetch; + global.fetch = fetchSpy as any; + try { + const remoteBls = new BlsService( + {} as any, + realSigner, + { + get: (k: string) => + k === "rustSignerUrl" + ? "http://fake-signer.local" + : k === "rustSignerRequired" + ? true + : undefined, + } as any + ); + const checkNodeRegistration = jest.fn(() => Promise.resolve(true)); + const chain = makeChain({ checkNodeRegistration: checkNodeRegistration as any }); + const service = new NodeKeyConsistencyService(chain, remoteBls); + + service.start(nodeState(), "0xcontract"); + await jest.advanceTimersByTimeAsync(10); + expect(service.getLastResult()?.status).toBe("skipped_remote_signer"); + expect(service.getLastResult()?.scope).toBeUndefined(); + + await jest.advanceTimersByTimeAsync(16 * 60_000); + await jest.advanceTimersByTimeAsync(16 * 60_000); + await jest.advanceTimersByTimeAsync(16 * 60_000); + expect(checkNodeRegistration.mock.calls.length).toBeGreaterThanOrEqual(4); + + expect(fetchSpy).not.toHaveBeenCalled(); + service.stop(); + } finally { + global.fetch = originalFetch; + } + }); }); diff --git a/src/modules/node/node-key-consistency.service.spec.ts b/src/modules/node/node-key-consistency.service.spec.ts index 857b619..755439d 100644 --- a/src/modules/node/node-key-consistency.service.spec.ts +++ b/src/modules/node/node-key-consistency.service.spec.ts @@ -464,74 +464,130 @@ describe("NodeKeyConsistencyService (CC-36)", () => { describe("HIGH-2: signer selection must match the REAL signing path (local @noble/curves vs. remote Rust/TEE-KMS)", () => { // Round-4 correction (coordinator + Codex): an earlier version of this check PROBED the - // remote signer's /sign endpoint to read its public key back, so it could compare A/B - // scenarios directly. That was rejected — it makes a background health check itself - // produce a real, unauthorized signature (see BlsService.resolveSigningPublicKey's - // docstring). The corrected behavior is simpler and more conservative: ANY remote signer - // configuration => skipped_remote_signer, with NO network call at all. This trades away - // the ability to distinguish "remote actually signs A" from "remote actually signs B" — - // an honest, documented limitation — but it also means the OLD false-"ok" and - // false-mismatch_local failure modes (comparing the possibly-stale local privateKey - // against what a remote signer is doing) can no longer happen either, since the local - // dimension is never even evaluated when a remote signer is configured. + // remote signer's /sign endpoint to read its public key back. That was rejected — it + // makes a background health check itself produce a real, unauthorized signature (see + // BlsService.resolveSigningPublicKey's docstring). No probe, no network call, ever. + // + // Round-7 correction (coordinator + Codex): round-4/5 then made ANY remote config — + // required OR optional — collapse to skipped_remote_signer unconditionally. That itself + // was a gap: in OPTIONAL mode, signDerivedHash() SILENTLY FALLS BACK to the local + // privateKey whenever the remote call fails, so the local key is a REAL signing key that + // must be verified — skipping it left exactly the silent-failure gap this check exists to + // close (see the "silent failure" test below, which replaces the round-4/5 test that used + // to pin this unsafe behavior). Required mode has no fallback path, so it still skips. const originalFetch = global.fetch; afterEach(() => { global.fetch = originalFetch; }); - it("remote signer configured => skipped_remote_signer, regardless of what node_state.privateKey/publicKey say locally", async () => { - const remoteBls = makeBls({ rustSignerUrl: "http://fake-signer.local" }); + it("REQUIRED mode: skipped_remote_signer regardless of what node_state.privateKey/publicKey say locally — no fallback path exists, so the local key is signing-irrelevant", async () => { + const remoteBls = makeBls({ + rustSignerUrl: "http://fake-signer.local", + rustSignerRequired: true, + }); const chain = makeBlockchainMock({ checkNodeRegistration: () => Promise.resolve(true), getNodePublicKey: () => Promise.resolve(KEY_A.eip2537Hex), }); const service = makeService(chain, remoteBls); // Everything LOCAL still looks consistent as A (privateKey derives A, state.publicKey - // is A) — under the OLD (rejected) probing design this would have been compared - // against whatever the remote claimed and could go either way; now it's never compared. + // is A) — required mode never even looks at it. const result = await service.check(baseNodeState(), "0xcontract"); expect(result.status).not.toBe("ok"); expect(result.status).not.toBe("mismatch_local"); expect(result.status).toBe("skipped_remote_signer"); + expect(result.scope).toBeUndefined(); // never scoped — the local dimension wasn't touched }); - it("remote signer configured => skipped_remote_signer even when local privateKey is stale/irrelevant and state/chain reflect a different key", async () => { - const remoteBls = makeBls({ rustSignerUrl: "http://fake-signer.local" }); + it("REQUIRED mode + a stale local privateKey (A) that no longer matches state/chain (B): still skipped_remote_signer — the stale local key is irrelevant when there's no fallback", async () => { + const remoteBls = makeBls({ + rustSignerUrl: "http://fake-signer.local", + rustSignerRequired: true, + }); const chain = makeBlockchainMock({ checkNodeRegistration: () => Promise.resolve(true), getNodePublicKey: () => Promise.resolve(KEY_B.eip2537Hex), }); const service = makeService(chain, remoteBls); - const state = baseNodeState({ publicKey: KEY_B.compressedHex }); // privateKey stays A + const state = baseNodeState({ publicKey: KEY_B.compressedHex }); // privateKey stays A (stale) const result = await service.check(state, "0xcontract"); expect(result.status).toBe("skipped_remote_signer"); + expect(result.scope).toBeUndefined(); }); - it("RUST_SIGNER_REQUIRED does not change what this check reports — any remote config => skipped_remote_signer", async () => { - const remoteBls = makeBls({ - rustSignerUrl: "http://fake-signer.local", - rustSignerRequired: true, - }); + it("OPTIONAL mode + NO local private key: no fallback exists to verify => skipped_remote_signer", async () => { + const remoteBls = makeBls({ rustSignerUrl: "http://fake-signer.local" }); // required NOT set const chain = makeBlockchainMock({ checkNodeRegistration: () => Promise.resolve(true), - getNodePublicKey: () => Promise.resolve(KEY_B.eip2537Hex), + getNodePublicKey: () => Promise.resolve(KEY_A.eip2537Hex), }); const service = makeService(chain, remoteBls); - const state = baseNodeState({ publicKey: KEY_B.compressedHex }); + const state = baseNodeState(); + delete (state as Partial).privateKey; const result = await service.check(state, "0xcontract"); expect(result.status).toBe("skipped_remote_signer"); + expect(result.scope).toBeUndefined(); }); - it("INVARIANT (round-4, the most important test in this file): a remote-signer node NEVER causes the checker to call fetch — no probe, no signature", async () => { - const fetchSpy = jest.fn(); - global.fetch = fetchSpy as any; - const remoteBls = makeBls({ rustSignerUrl: "http://fake-signer.local" }); + it("OPTIONAL mode + local fallback key (A) internally consistent with state and chain: ok, but EXPLICITLY scoped to local_fallback_only", async () => { + const remoteBls = makeBls({ rustSignerUrl: "http://fake-signer.local" }); // required NOT set const chain = makeBlockchainMock({ checkNodeRegistration: () => Promise.resolve(true), getNodePublicKey: () => Promise.resolve(KEY_A.eip2537Hex), }); const service = makeService(chain, remoteBls); - await service.check(baseNodeState(), "0xcontract"); + // baseNodeState(): privateKey=A, publicKey=A — internally consistent. + const result = await service.check(baseNodeState(), "0xcontract"); + expect(result.status).toBe("ok"); + // The scope tag is load-bearing: without it, this "ok" would be misread as "the remote + // signer's primary key is fine too" — it was never checked (no probe, no signature). + expect(result.scope).toBe("local_fallback_only"); + }); + + it("SILENT-FAILURE regression (replaces the round-4/5 test that used to pin this unsafe behavior): OPTIONAL mode + stale local fallback key (A) + on-chain re-registered to B => mismatch_onchain, scoped to local_fallback_only", async () => { + // The exact scenario the whole feature exists to catch: signDerivedHash() will fall + // back to signing with the stale local key A whenever the remote call fails, but the + // chain has since been re-registered to B — every such fallback signature fails + // verification forever. Previously (round-4/5) this silently reported + // skipped_remote_signer; now the fallback key is actually checked. + const remoteBls = makeBls({ rustSignerUrl: "http://fake-signer.local" }); // optional + const chain = makeBlockchainMock({ + checkNodeRegistration: () => Promise.resolve(true), + getNodePublicKey: () => Promise.resolve(KEY_B.eip2537Hex), // chain has moved on to B + }); + const service = makeService(chain, remoteBls); + // baseNodeState(): privateKey=A, publicKey=A — the fallback key (A) is internally + // consistent with node_state, so the LOCAL dimension alone reports ok; it's the + // ON-CHAIN dimension (state A vs. chain B) that catches the drift. + const result = await service.check(baseNodeState(), "0xcontract"); + expect(result.status).toBe("mismatch_onchain"); + expect(result.scope).toBe("local_fallback_only"); + }); + + it("INVARIANT (round-4/7, the most important test in this file): a remote-signer node NEVER causes the checker to call fetch — no probe, no signature, in EITHER required or optional mode", async () => { + const fetchSpy = jest.fn(); + global.fetch = fetchSpy as any; + + // Optional mode (exercises the NEW local_fallback derivation path too — pure + // computation, must still never touch the network). + const optionalBls = makeBls({ rustSignerUrl: "http://fake-signer.local" }); + const chainA = makeBlockchainMock({ + checkNodeRegistration: () => Promise.resolve(true), + getNodePublicKey: () => Promise.resolve(KEY_A.eip2537Hex), + }); + await makeService(chainA, optionalBls).check(baseNodeState(), "0xcontract"); + + // Required mode. + const requiredBls = makeBls({ + rustSignerUrl: "http://fake-signer.local", + rustSignerRequired: true, + }); + const chainB = makeBlockchainMock({ + checkNodeRegistration: () => Promise.resolve(true), + getNodePublicKey: () => Promise.resolve(KEY_A.eip2537Hex), + }); + await makeService(chainB, requiredBls).check(baseNodeState(), "0xcontract"); + expect(fetchSpy).not.toHaveBeenCalled(); }); }); diff --git a/src/modules/node/node-key-consistency.service.ts b/src/modules/node/node-key-consistency.service.ts index e682054..f7447b8 100644 --- a/src/modules/node/node-key-consistency.service.ts +++ b/src/modules/node/node-key-consistency.service.ts @@ -46,12 +46,24 @@ export interface KeyConsistencyResult { */ checkerError?: string; checkerErrorAt?: string; + /** + * Round-7: set when `status` was determined using the OPTIONAL-remote-signer LOCAL + * FALLBACK key (see `BlsService.resolveSigningPublicKey`), not a node's sole/primary + * signing key. `signDerivedHash()` silently falls back to this exact local key whenever an + * optional remote signer's call fails, so it IS a real signing key worth checking — but an + * `ok` here proves ONLY that the fallback key is internally consistent; it says NOTHING + * about whether the remote signer's own (primary, unverifiable-without-signing) key is + * correct. Never read `status === "ok"` as "fully verified" without also checking this is + * absent. + */ + scope?: "local_fallback_only"; } /** The outcome of a single, stateless comparison — before it's merged into the held `lastResult`. */ interface FreshCheck { status: KeyConsistencyStatus; derivedMatches: boolean | null; + scope?: "local_fallback_only"; } /** Type-safe against whatever a raw, unvalidated node_state.json actually contains on disk @@ -323,6 +335,7 @@ export class NodeKeyConsistencyService implements OnModuleDestroy { lastAttemptAt: now, derivedMatches: fresh.derivedMatches, nodeId: nodeState.nodeId, + scope: fresh.scope, } ); } @@ -349,6 +362,7 @@ export class NodeKeyConsistencyService implements OnModuleDestroy { lastAttemptAt: now, derivedMatches: fresh.derivedMatches, nodeId: nodeState.nodeId, + scope: fresh.scope, }; } } else { @@ -359,6 +373,7 @@ export class NodeKeyConsistencyService implements OnModuleDestroy { lastAttemptAt: now, derivedMatches: fresh.derivedMatches, nodeId: nodeState.nodeId, + scope: fresh.scope, }; } @@ -385,8 +400,8 @@ export class NodeKeyConsistencyService implements OnModuleDestroy { const derivedMatches = this.computeDerivedMatches(nodeState); const local = await this.checkLocal(nodeState); const onchain = await this.checkOnchain(nodeState, contractAddress); - const status = this.combine(local, onchain); - return { status, derivedMatches }; + const status = this.combine(local.outcome, onchain); + return { status, derivedMatches, scope: local.scope }; } /** Type-safe against a raw, unvalidated node_state.json field (round-2 High-3). */ @@ -406,35 +421,43 @@ export class NodeKeyConsistencyService implements OnModuleDestroy { * signature is remote and fine). * * `"no_signer"` (neither a local key nor a remote signer configured) maps to the - * legitimate `skipped_no_private_key` outcome. `"remote"` (a remote signer IS configured) - * ALSO maps to `skipped_remote_signer` (round-4 correction) — NEVER a fallback to - * comparing the local key (which is exactly the bug this exists to prevent) and NEVER a - * probe of the remote signer either (`resolveSigningPublicKey` deliberately makes no - * request to it — see its docstring: doing so would make this background check itself - * produce an unauthorized signature). This is a deliberate, honest limitation: a - * remote-signing node's local dimension is simply not verified right now. + * legitimate `skipped_no_private_key` outcome. `"remote"` — required mode, or optional mode + * with no local key to fall back to — maps to `skipped_remote_signer`: NEVER a fallback to + * comparing an irrelevant local key, and NEVER a probe of the remote signer either + * (`resolveSigningPublicKey` deliberately makes no request to it in this branch). + * + * `"local_fallback"` (round-7 correction, optional remote mode WITH a local key) IS + * checked, via the normal derive-and-compare below — that key is a REAL fallback signing + * key `signDerivedHash()` silently uses whenever the remote call fails, so leaving it + * unverified was itself the exact silent-failure gap this check exists to close. The + * result is tagged with `scope: "local_fallback_only"` (see `computeFresh`/`check`) so an + * `ok` here is never misread as "the remote signer's own key is fine too" — it isn't + * checked and can't be without producing a signature. */ - private async checkLocal( - nodeState: NodeState - ): Promise<"ok" | "mismatch" | "skipped_no_private_key" | "skipped_remote_signer"> { + private async checkLocal(nodeState: NodeState): Promise<{ + outcome: "ok" | "mismatch" | "skipped_no_private_key" | "skipped_remote_signer"; + scope?: "local_fallback_only"; + }> { try { const resolved = await this.blsService.resolveSigningPublicKey(nodeState); if (resolved.source === "no_signer") { - return "skipped_no_private_key"; + return { outcome: "skipped_no_private_key" }; } if (resolved.source === "remote") { - return "skipped_remote_signer"; + return { outcome: "skipped_remote_signer" }; } - // resolved.source === "local" + // resolved.source === "local" | "local_fallback" + const scope = resolved.source === "local_fallback" ? ("local_fallback_only" as const) : undefined; const derivedHex = normalizeHex(resolved.publicKey.toHex()); const statePoint = this.parsePublicKeyPoint(nodeState.publicKey); if (statePoint === null) { // node_state.publicKey isn't a valid point in EITHER accepted wire format. - return "mismatch"; + return { outcome: "mismatch", scope }; } - return derivedHex === normalizeHex(statePoint.toHex()) ? "ok" : "mismatch"; + const outcome = derivedHex === normalizeHex(statePoint.toHex()) ? "ok" : "mismatch"; + return { outcome, scope }; } catch (error: any) { // Signer resolution itself failed unexpectedly (e.g. the local signer rejected a // malformed private key) — cannot prove correctness, so treat as a mismatch rather @@ -442,7 +465,7 @@ export class NodeKeyConsistencyService implements OnModuleDestroy { this.logger.warn( `local key-consistency check could not resolve/compare a signing public key: ${error?.message ?? error}` ); - return "mismatch"; + return { outcome: "mismatch" }; } } From e2f3be6bd812be82b1a5f5ca58e0ae62452a673f Mon Sep 17 00:00:00 2001 From: jhfnetboy Date: Sun, 27 Sep 2026 15:37:48 +0700 Subject: [PATCH 8/9] test(CC-36 round 8): close a coverage gap in the round-7 silent-failure regression test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Coordinator review of round 7's mutation-1 output: the existing "stale local fallback key + on-chain re-registered" regression test used node_state.publicKey = A (the same stale key as privateKey), so the ON-CHAIN comparison (A vs. chain B) already caught the drift on its own. Disabling local_fallback entirely left `status` at mismatch_onchain unchanged — only the `scope` field went missing. The test's status assertion therefore did not distinguish the new local_fallback logic from the old "always skip" behavior; only the scope assertion did. The scenario Codex flagged as the real danger: privateKey is stale (A), but node_state.publicKey was separately (and correctly) re-imported as B — matching the chain — since import-node.dto.ts accepts publicKey and privateKey as two independent fields with nothing forcing them to be a matching pair. Here the on-chain comparison is B vs. chain B => ok on its own; ONLY the local_fallback comparison (privateKey A's derived key vs. node_state.publicKey B) can catch that the private key on disk doesn't correspond to the identity everything else agrees on. Added that exact test: optional remote mode + privateKey A + node_state.publicKey B + on-chain B => mismatch_local, scoped to local_fallback_only. Mutation-verified (not committed, reverted to a byte-identical diff after confirming red): disabled the optional-mode local_fallback branch (forced skip, same mutation as round 7's) — this time the new test fails ON THE STATUS ASSERTION itself (expected mismatch_local, received skipped_remote_signer), not just on scope, proving it actually distinguishes the local_fallback logic from the old behavior. Test-only change; no production code touched. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Pdq9rgkZq9M4JFeTZD7yYs --- .../node/node-key-consistency.service.spec.ts | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/src/modules/node/node-key-consistency.service.spec.ts b/src/modules/node/node-key-consistency.service.spec.ts index 755439d..7a6a127 100644 --- a/src/modules/node/node-key-consistency.service.spec.ts +++ b/src/modules/node/node-key-consistency.service.spec.ts @@ -564,6 +564,33 @@ describe("NodeKeyConsistencyService (CC-36)", () => { expect(result.scope).toBe("local_fallback_only"); }); + it("SILENT-FAILURE regression #2 (the coverage gap the previous test's status assertion didn't catch): OPTIONAL mode + stale local privateKey (A) but node_state.publicKey correctly re-imported as B (= on-chain B) => mismatch_local, scoped to local_fallback_only", async () => { + // The previous test's fixture had node_state.publicKey === A (the same stale key as + // privateKey), so the ON-CHAIN comparison (A vs. chain B) already caught the drift on + // its own — disabling local_fallback entirely still left `status` at mismatch_onchain + // (only `scope` went missing), so that test alone could not prove the LOCAL check was + // the one doing the work. + // + // This fixture is the scenario Codex flagged as the real danger: privateKey is stale + // (A), but node_state.publicKey was separately (and correctly) re-imported as B, and + // the chain ALSO has B (import-node.dto.ts accepts publicKey and privateKey as two + // independent fields — nothing forces them to be a matching pair). Now the ON-CHAIN + // comparison is B vs. chain B => ok, so ONLY the local_fallback comparison (privateKey + // A's derived key vs. node_state.publicKey B) can catch that the private key on disk + // doesn't actually correspond to the identity everything else agrees on. + const remoteBls = makeBls({ rustSignerUrl: "http://fake-signer.local" }); // optional + const chain = makeBlockchainMock({ + checkNodeRegistration: () => Promise.resolve(true), + getNodePublicKey: () => Promise.resolve(KEY_B.eip2537Hex), // chain: B + }); + const service = makeService(chain, remoteBls); + // privateKey stays A (stale/mismatched); publicKey correctly reflects B (= chain). + const state = baseNodeState({ publicKey: KEY_B.compressedHex }); + const result = await service.check(state, "0xcontract"); + expect(result.status).toBe("mismatch_local"); + expect(result.scope).toBe("local_fallback_only"); + }); + it("INVARIANT (round-4/7, the most important test in this file): a remote-signer node NEVER causes the checker to call fetch — no probe, no signature, in EITHER required or optional mode", async () => { const fetchSpy = jest.fn(); global.fetch = fetchSpy as any; From 5f2b514827a635b67e4bd3dc591d847388e58ea4 Mon Sep 17 00:00:00 2001 From: jhfnetboy Date: Sat, 10 Oct 2026 11:04:43 +0700 Subject: [PATCH 9/9] =?UTF-8?q?style(CC-36):=20apply=20prettier=20to=20the?= =?UTF-8?q?=207=20files=20the=20PR=20touched=20=E2=80=94=20format:check=20?= =?UTF-8?q?was=20never=20run?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit pr-daemon's REQUEST_CHANGES on #378 was purely mechanical: `npm run format:check` fails on all seven files this PR adds or changes. Five Codex rounds and every mutation check ran type-check, lint:check and jest, but the verification list the author was given omitted format:check, although CLAUDE.md asks for `npm run format` before every commit. Formatting only; no behaviour change — tsc clean and the same 113 tests in node/bls/health pass before and after. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Pdq9rgkZq9M4JFeTZD7yYs --- src/modules/bls/bls.service.ts | 4 ++- src/modules/health/health.controller.spec.ts | 2 +- ...key-consistency.service.scheduling.spec.ts | 28 ++++++++----------- .../node/node-key-consistency.service.spec.ts | 13 ++++----- .../node/node-key-consistency.service.ts | 3 +- .../node/node.key-consistency-wiring.spec.ts | 5 +++- src/modules/node/node.service.ts | 5 +--- 7 files changed, 28 insertions(+), 32 deletions(-) diff --git a/src/modules/bls/bls.service.ts b/src/modules/bls/bls.service.ts index 55292d3..42f5c66 100644 --- a/src/modules/bls/bls.service.ts +++ b/src/modules/bls/bls.service.ts @@ -237,7 +237,9 @@ export class BlsService { * explicit scope (`scope: "local_fallback_only"`) — an `ok` here proves only that the * FALLBACK key is internally consistent, never that the remote signer's primary key is. */ - async resolveSigningPublicKey(node: NodeKeyPair): Promise< + async resolveSigningPublicKey( + node: NodeKeyPair + ): Promise< | { source: "local"; publicKey: any } | { source: "local_fallback"; publicKey: any } | { source: "remote" } diff --git a/src/modules/health/health.controller.spec.ts b/src/modules/health/health.controller.spec.ts index 116a3fb..be5d955 100644 --- a/src/modules/health/health.controller.spec.ts +++ b/src/modules/health/health.controller.spec.ts @@ -63,7 +63,7 @@ describe("HealthController", () => { expect(result.capabilities).toEqual([]); }); - it("CC-36 round-7: scope: \"local_fallback_only\" surfaces through /health — an ok result must be readable as scoped, not as a full verification", () => { + it('CC-36 round-7: scope: "local_fallback_only" surfaces through /health — an ok result must be readable as scoped, not as a full verification', () => { // Round-7: in optional remote-signer mode, `ok` may come from checking only the LOCAL // fallback key (never the remote signer's own key, which can't be checked without // producing a signature). Assert this scope flag is visible at the /health CONTRACT diff --git a/src/modules/node/node-key-consistency.service.scheduling.spec.ts b/src/modules/node/node-key-consistency.service.scheduling.spec.ts index 0bd974d..8e11f12 100644 --- a/src/modules/node/node-key-consistency.service.scheduling.spec.ts +++ b/src/modules/node/node-key-consistency.service.scheduling.spec.ts @@ -509,11 +509,9 @@ describe("NodeKeyConsistencyService — retry/recheck scheduling (CC-36 round 2/ const originalFetch = global.fetch; global.fetch = fetchSpy as any; try { - const remoteBls = new BlsService( - {} as any, - realSigner, - { get: (k: string) => (k === "rustSignerUrl" ? "http://fake-signer.local" : undefined) } as any - ); + const remoteBls = new BlsService({} as any, realSigner, { + get: (k: string) => (k === "rustSignerUrl" ? "http://fake-signer.local" : undefined), + } as any); const checkNodeRegistration = jest.fn(() => Promise.resolve(true)); const chain = makeChain({ checkNodeRegistration: checkNodeRegistration as any }); const service = new NodeKeyConsistencyService(chain, remoteBls); @@ -542,18 +540,14 @@ describe("NodeKeyConsistencyService — retry/recheck scheduling (CC-36 round 2/ const originalFetch = global.fetch; global.fetch = fetchSpy as any; try { - const remoteBls = new BlsService( - {} as any, - realSigner, - { - get: (k: string) => - k === "rustSignerUrl" - ? "http://fake-signer.local" - : k === "rustSignerRequired" - ? true - : undefined, - } as any - ); + const remoteBls = new BlsService({} as any, realSigner, { + get: (k: string) => + k === "rustSignerUrl" + ? "http://fake-signer.local" + : k === "rustSignerRequired" + ? true + : undefined, + } as any); const checkNodeRegistration = jest.fn(() => Promise.resolve(true)); const chain = makeChain({ checkNodeRegistration: checkNodeRegistration as any }); const service = new NodeKeyConsistencyService(chain, remoteBls); diff --git a/src/modules/node/node-key-consistency.service.spec.ts b/src/modules/node/node-key-consistency.service.spec.ts index 7a6a127..38384b8 100644 --- a/src/modules/node/node-key-consistency.service.spec.ts +++ b/src/modules/node/node-key-consistency.service.spec.ts @@ -171,9 +171,7 @@ describe("NodeKeyConsistencyService (CC-36)", () => { const chain = makeBlockchainMock({ checkNodeRegistration: () => Promise.resolve(true), }); - (chain.getNodePublicKey as jest.Mock).mockRejectedValueOnce( - new Error("RPC timeout") as never - ); + (chain.getNodePublicKey as jest.Mock).mockRejectedValueOnce(new Error("RPC timeout") as never); const service = makeService(chain); const result = await service.check(baseNodeState(), "0xcontract"); expect(result.status).toBe("unknown"); @@ -439,11 +437,12 @@ describe("NodeKeyConsistencyService (CC-36)", () => { // a pair of independent `latest` reads straddling the transition could produce. const chain = { getBlockNumber: jest.fn(() => Promise.resolve(100)), - checkNodeRegistration: jest.fn((_addr: string, _id: string, blockTag?: number) => - Promise.resolve(blockTag === 100) // true only "as of block 100" + checkNodeRegistration: jest.fn( + (_addr: string, _id: string, blockTag?: number) => Promise.resolve(blockTag === 100) // true only "as of block 100" ), - getNodePublicKey: jest.fn((_addr: string, _id: string, blockTag?: number) => - Promise.resolve(blockTag === 100 ? KEY_A.eip2537Hex : "0x") // empty "as of block 101+" + getNodePublicKey: jest.fn( + (_addr: string, _id: string, blockTag?: number) => + Promise.resolve(blockTag === 100 ? KEY_A.eip2537Hex : "0x") // empty "as of block 101+" ), } as unknown as BlockchainService; const service = makeService(chain); diff --git a/src/modules/node/node-key-consistency.service.ts b/src/modules/node/node-key-consistency.service.ts index f7447b8..0ba5072 100644 --- a/src/modules/node/node-key-consistency.service.ts +++ b/src/modules/node/node-key-consistency.service.ts @@ -448,7 +448,8 @@ export class NodeKeyConsistencyService implements OnModuleDestroy { } // resolved.source === "local" | "local_fallback" - const scope = resolved.source === "local_fallback" ? ("local_fallback_only" as const) : undefined; + const scope = + resolved.source === "local_fallback" ? ("local_fallback_only" as const) : undefined; const derivedHex = normalizeHex(resolved.publicKey.toHex()); const statePoint = this.parsePublicKeyPoint(nodeState.publicKey); diff --git a/src/modules/node/node.key-consistency-wiring.spec.ts b/src/modules/node/node.key-consistency-wiring.spec.ts index d687bf8..5031386 100644 --- a/src/modules/node/node.key-consistency-wiring.spec.ts +++ b/src/modules/node/node.key-consistency-wiring.spec.ts @@ -44,7 +44,10 @@ describe("NodeService — key-consistency self-check wiring (CC-36)", () => { try { await (svc as any).initializeNode(); expect(startFn).toHaveBeenCalledTimes(1); - expect(startFn).toHaveBeenCalledWith(expect.objectContaining({ nodeId: "0x1" }), "0xcontract"); + expect(startFn).toHaveBeenCalledWith( + expect.objectContaining({ nodeId: "0x1" }), + "0xcontract" + ); } finally { cwdSpy.mockRestore(); rmSync(dir, { recursive: true, force: true }); diff --git a/src/modules/node/node.service.ts b/src/modules/node/node.service.ts index 8464a23..501e5cf 100644 --- a/src/modules/node/node.service.ts +++ b/src/modules/node/node.service.ts @@ -7,10 +7,7 @@ import { BlsService } from "../bls/bls.service.js"; import { BlockchainService } from "../blockchain/blockchain.service.js"; import { randomBytes, createHash } from "crypto"; import { decryptKeystore, isKeystore } from "../../utils/keystore.util.js"; -import { - NodeKeyConsistencyService, - KeyConsistencyResult, -} from "./node-key-consistency.service.js"; +import { NodeKeyConsistencyService, KeyConsistencyResult } from "./node-key-consistency.service.js"; @Injectable() export class NodeService implements OnModuleInit {