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/bls/bls.service.ts b/src/modules/bls/bls.service.ts index be20702..42f5c66 100644 --- a/src/modules/bls/bls.service.ts +++ b/src/modules/bls/bls.service.ts @@ -138,12 +138,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 +156,127 @@ 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, 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. + * + * 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: 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 + * 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. + * + * 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" } + > { + 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 }; + } + 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" }; + } + /** 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/health/health.controller.spec.ts b/src/modules/health/health.controller.spec.ts new file mode 100644 index 0000000..be5d955 --- /dev/null +++ b/src/modules/health/health.controller.spec.ts @@ -0,0 +1,94 @@ +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", + lastAttemptAt: "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("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('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(); + 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.scheduling.spec.ts b/src/modules/node/node-key-consistency.service.scheduling.spec.ts new file mode 100644 index 0000000..8e11f12 --- /dev/null +++ b/src/modules/node/node-key-consistency.service.scheduling.spec.ts @@ -0,0 +1,571 @@ +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"; +import { SignerService } from "../signer/signer.service.js"; + +/** + * 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 — + * 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 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); + 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>; + 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; +} + +describe("NodeKeyConsistencyService — retry/recheck scheduling (CC-36 round 2/3)", () => { + 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(); + }); + + 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); + + 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("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); + + // 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); + // 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, and does NOT spuriously set checkerError", async () => { + const chain = makeChain({ + checkNodeRegistration: jest.fn(() => Promise.resolve(true)) as any, + }); + 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. 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[] = []; + 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(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(); + }); + + 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 + // 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(). + // + // 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; + 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 + // 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. + 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; + } + }); + + 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 new file mode 100644 index 0000000..38384b8 --- /dev/null +++ b/src/modules/node/node-key-consistency.service.spec.ts @@ -0,0 +1,620 @@ +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"; +import { SignerService } from "../signer/signer.service.js"; +import { ConfigService } from "@nestjs/config"; + +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 — 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(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 + * 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 = realBls.encodePublicKeyToEIP2537(point); // on-chain / dashboard-create 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; + 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; +} + +function makeService(chain: BlockchainService, blsService: BlsService = realBls) { + return new NodeKeyConsistencyService(chain, blsService); +} + +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 = makeService(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 = 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 = makeService(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 = 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. + 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 = makeService(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 = makeService(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 = makeService(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 = makeService(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 = 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"); + 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 = makeService(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 = makeService(chain); + 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 + // (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, + resolveSigningPublicKey: realBls.resolveSigningPublicKey.bind(realBls), + } as unknown as BlsService; + const chain = makeBlockchainMock({ + checkNodeRegistration: () => Promise.resolve(true), + getNodePublicKey: () => Promise.resolve(sentinelEip2537), + }); + const service = makeService(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), + resolveSigningPublicKey: realBls.resolveSigningPublicKey.bind(realBls), + } as unknown as BlsService; + const chain = makeBlockchainMock({ + checkNodeRegistration: () => Promise.resolve(true), + getNodePublicKey: () => Promise.resolve("0x" + "11".repeat(128)), + }); + 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, 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"); + // 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"); + }); + + 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("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. 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("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) — 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("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 (stale) + const result = await service.check(state, "0xcontract"); + expect(result.status).toBe("skipped_remote_signer"); + expect(result.scope).toBeUndefined(); + }); + + 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_A.eip2537Hex), + }); + const service = makeService(chain, remoteBls); + 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("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); + // 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("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; + + // 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 new file mode 100644 index 0000000..0ba5072 --- /dev/null +++ b/src/modules/node/node-key-consistency.service.ts @@ -0,0 +1,659 @@ +import { Injectable, Logger, OnModuleDestroy } from "@nestjs/common"; +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 { NodeState } from "../../interfaces/node.interface.js"; + +export type KeyConsistencyStatus = + | "ok" + | "mismatch_local" + | "mismatch_onchain" + | "not_registered" + | "unknown" + | "skipped_no_private_key" + | "skipped_remote_signer"; + +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 + * `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; + /** + * 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; + /** + * 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 + * (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(); +} + +/** + * 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 (one attempt, see `computeFresh`): + * 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). + * 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 + * 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 is designed to never throw — every failure mode + * (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 + * 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()`. + * + * `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 { + /** 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; + /** 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 + ) {} + + /** 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 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, myGeneration).catch(() => {}); + } + + /** 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, + myGeneration: number + ): Promise { + 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 + // 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.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; + } + const delay = this.nextDelayMs(); + this.timer = setTimeout(() => { + // 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). + 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 + * 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) { + 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` (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 + * 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, + 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, + scope: fresh.scope, + } + ); + } + + if (fresh.status === "mismatch_local" || fresh.status === "mismatch_onchain") { + this.logMismatch(fresh.status, nodeState); + } + + if (fresh.status === "unknown") { + this.consecutiveUnknown++; + if (this.lastResult && this.lastResult.status !== "unknown") { + 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. + this.lastResult = { + status: "unknown", + checkedAt: now, + lastAttemptAt: now, + derivedMatches: fresh.derivedMatches, + nodeId: nodeState.nodeId, + scope: fresh.scope, + }; + } + } else { + this.consecutiveUnknown = 0; + this.lastResult = { + status: fresh.status, + checkedAt: now, + lastAttemptAt: now, + derivedMatches: fresh.derivedMatches, + nodeId: nodeState.nodeId, + scope: fresh.scope, + }; + } + + 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 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 signer/publicKey pair in node_state.json.` + ); + } + + private async computeFresh(nodeState: NodeState, contractAddress: string): Promise { + const derivedMatches = this.computeDerivedMatches(nodeState); + const local = await this.checkLocal(nodeState); + const onchain = await this.checkOnchain(nodeState, contractAddress); + 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). */ + private prefix(hex: unknown): string { + const s = typeof hex === "string" ? hex : ""; + return "0x" + s.replace(/^0x/i, "").slice(0, 10); + } + + /** + * 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"` — 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<{ + 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 { outcome: "skipped_no_private_key" }; + } + if (resolved.source === "remote") { + return { outcome: "skipped_remote_signer" }; + } + + // 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 { outcome: "mismatch", scope }; + } + 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 + // than silently skipping. + this.logger.warn( + `local key-consistency check could not resolve/compare a signing public key: ${error?.message ?? error}` + ); + return { outcome: "mismatch" }; + } + } + + /** + * 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, + blockTag + ); + if (!isRegistered) { + return "not_registered"; + } + + const localEip2537 = this.toEip2537Hex(nodeState.publicKey); + if (localEip2537 === null) { + // 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 in either accepted wire format; cannot compare against the on-chain registration" + ); + return "unknown"; + } + + const onchainKey = await this.blockchainService.getNodePublicKey( + contractAddress, + 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 + // 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); + } + + /** + * 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, + * 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, "").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), 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 + } 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(publicKeyHex: unknown): string | null { + const point = this.parsePublicKeyPoint(publicKeyHex); + if (point === null) { + return null; + } + try { + return this.blsService.encodePublicKeyToEIP2537(point); + } 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 — 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_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_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.key-consistency-wiring.spec.ts b/src/modules/node/node.key-consistency-wiring.spec.ts new file mode 100644 index 0000000..5031386 --- /dev/null +++ b/src/modules/node/node.key-consistency-wiring.spec.ts @@ -0,0 +1,216 @@ +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'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 = { + nodeId: "0x1", + nodeName: "n", + publicKey: "0xabcd", + createdAt: "t", + description: "d", + privateKey: "0x" + "1".padStart(64, "0"), + }; + + 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)); + const config = { + get: (k: string) => (k === "validatorContractAddress" ? "0xcontract" : undefined), + } as unknown as ConfigService; + + 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 + // 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(); + expect(startFn).toHaveBeenCalledTimes(1); + expect(startFn).toHaveBeenCalledWith( + expect.objectContaining({ nodeId: "0x1" }), + "0xcontract" + ); + } 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 = { + start: jest.fn(), + 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 start() itself throws synchronously", 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 = { + 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(); + } finally { + cwdSpy.mockRestore(); + rmSync(dir, { recursive: true, force: true }); + } + }); +}); + +/** + * 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 4b832cc..b8cdec2 100644 --- a/src/modules/node/node.module.ts +++ b/src/modules/node/node.module.ts @@ -2,13 +2,17 @@ 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({ + // 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], + 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..501e5cf 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,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"; @Injectable() export class NodeService implements OnModuleInit { @@ -19,7 +20,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 +40,11 @@ export class NodeService implements OnModuleInit { this.loadExistingNodeState(); if (this.nodeState) { this.logger.log(`Loaded node state: ${this.nodeState.nodeId}`); + // 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."); @@ -43,6 +52,31 @@ export class NodeService implements OnModuleInit { } } + /** + * 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 { + 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 ` + + `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}`); @@ -248,5 +282,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(); + } } }