fix(CC-36): node key-consistency self-check — surface a registered-key mismatch that otherwise fails silently [DO NOT MERGE — B6 freeze]

src/modules/blockchain/blockchain.service.ts

@@ -256,7 +256,18 @@ export class BlockchainService {
     }
   }
 
-  async checkNodeRegistration(contractAddress: string, nodeId: string): Promise<boolean> {
+  /**
+   * `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<boolean> {
     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<string> {
+  /** `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<string> {
     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}`);

src/modules/bls/bls.service.spec.ts

@@ -24,7 +24,7 @@
 });
 
 const { BlsService } = await import("./bls.service.js");
 const { BlockchainService } = await import("../blockchain/blockchain.service.js");
 // LocalKeySigner imports the (mocked) bls.util sigs — must load AFTER the mock above.
 const { LocalKeySigner } = await import("../signer/local-key.signer.js");
 import type { PackedUserOp } from "../blockchain/blockchain.service.js";
@@ -255,3 +255,119 @@
     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<string, unknown>) {
+    return { get: (k: string) => overrides[k] } as any;
+  }
+
+  function makeService(configOverrides: Record<string, unknown>) {
+    const blockchain = {} as unknown as InstanceType<typeof BlockchainService>;
+    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<string, string> | 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);
+  });
+});

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<SignatureResult> {
-    const base = this.configService?.get<string>("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<boolean>("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<string>("rustSignerUrl");
+    if (!url) {
+      return null;
+    }
+    return {
+      url,
+      required: this.configService?.get<boolean>("rustSignerRequired") === true,
+      token: this.configService?.get<string>("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<SignatureResult> {
-    const url = `${base.replace(/\/+$/, "")}/sign`;
+    const url = `${rust.url.replace(/\/+$/, "")}/sign`;
     const headers: Record<string, string> = { "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<string>("rustSignerToken");
-    if (signerToken) {
-      headers["X-Signer-Token"] = signerToken;
+    if (rust.token) {
+      headers["X-Signer-Token"] = rust.token;
     }
     const response = await fetch(url, {
       method: "POST",