Repository navigation
fix(CC-36): node key-consistency self-check — surface a registered-key mismatch that otherwise fails silently [DO NOT MERGE — B6 freeze] - #378
Open
jhfnetboy wants to merge 9 commits into
Conversation
…y mismatch that otherwise fails silently
A DVT node's identity lives in node_state.json (nodeId, publicKey, usually a
privateKey), separately from the on-chain registeredKeys(nodeId) mapping set at
registration time. If those ever drift apart, registration and startup succeed,
the node looks healthy on every existing probe, but its signatures can never
verify against the on-chain registration — it silently stops participating
with no error anywhere.
Add NodeKeyConsistencyService (src/modules/node/node-key-consistency.service.ts),
a three-way, fail-loud-not-fail-stop comparison:
1. local: privateKey -> derive pubkey -> compare to node_state.publicKey
(skipped_no_private_key for legitimate key-less KMS-TEE nodes)
2. on-chain: node_state.publicKey (EIP-2537-encoded) vs. registeredKeys(nodeId),
gated by isRegistered(nodeId); an RPC error is `unknown`, NEVER
read as a mismatch or as ok
3. derivedMatches: informational-only nodeId == keccak256(pubkey) check — the
registerWithProof path derives nodeId this way, but the older
registerPublicKey bootstrap path lets the caller pick any nodeId,
so this must never feed into the pass/fail `status`
NodeService runs the check once after loading node_state.json, fire-and-forget
(never awaited on the startup path, never blocks/fails boot on a slow or down
RPC). HealthController exposes the last result as an ADDITIVE `keyConsistency`
field on GET /health — status/version/capabilities are unchanged.
Also exports `encodeG1Point` from src/utils/bls.util.ts (previously a private
method on BlsService) so this read-only check doesn't need to reach into the
sign-path service for a pure encoding helper.
Does not touch bls.service.ts or the owner-authorization gate.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Pdq9rgkZq9M4JFeTZD7yYs
…nknown, recheck periodically Round-1 review caught two gaps in the key-consistency self-check: 1. The on-chain comparison encoded node_state.publicKey with a COPY of the G1 EIP-2537 encoder (added to bls.util.ts), separate from the one registerOnChain()/gossip/dashboard/quorum-cosigner all actually use (BlsService.encodePublicKeyToEIP2537). A future change to the real encoder would silently diverge from this check's copy — exactly the kind of drift this check exists to catch. Fixed by injecting BlsService into NodeKeyConsistencyService and calling its encodePublicKeyToEIP2537 directly; removed the now-unused bls.util.ts copy. This does not touch the owner-authorization gate (authorizeAndDeriveHash) — encodePublicKeyToEIP2537 is a pure encoding function unrelated to that path. Verified no circular dependency: NodeModule already imports BlsModule (forwardRef), and the full AppModule now boots and shuts down cleanly via NestFactory.createApplicationContext (checked directly, not inferred). Locked with a test that swaps in a stub BlsService returning a sentinel value and asserts the comparison actually consults it. 2. A single startup check meant an `unknown` result (RPC hiccup at boot, the most likely time for network to be flaky) froze forever, and a node kicked off the registry post-boot (the validator's permissionless syncNode(nodeId) deactivating an under-staked/role-losing operator) never showed up in /health. NodeKeyConsistencyService now self-schedules: bounded backoff while unknown (30s/60s/120s/240s, then folding into the regular cadence), and a periodic recheck (every 15 min) once conclusive. A recheck that itself fails RPC NEVER downgrades a held conclusive result (e.g. mismatch_onchain) to unknown — it only advances lastAttemptAt, so a transient blip can't launder away a real finding. /health now also exposes lastAttemptAt alongside checkedAt. Timers are .unref()'d and cleared in both stop() and the new OnModuleDestroy hook. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Pdq9rgkZq9M4JFeTZD7yYs
…signer, survive any throw, discard stale in-flight checks, and rebind on identity change
Codex adversarial review round 1 (REQUEST_CHANGES, 3 High + 2 Medium), all addressed:
High-1 (verified by the coordinator): node_state.publicKey is NOT always 48-byte
compressed — dashboard.service.ts's create-node path stores the 128-byte
EIP-2537 form directly (`encodePublicKeyToEIP2537(publicKeyPoint)`), and
import-node.dto.ts accepts either. Assuming compressed-only misreported every
dashboard-created node as mismatch_local. `parsePublicKeyPoint()` now accepts
BOTH formats: 48-byte compressed via `bls.G1.Point.fromHex`, 128-byte EIP-2537
via `bls.G1.Point.fromAffine({x,y})` (the same @noble/curves primitive
`.fromHex()` uses internally) followed by `.assertValidity()` — the identical
subgroup/curve check, so a malformed 128-byte value is rejected exactly like a
malformed compressed one, never silently accepted. No such decoder existed
anywhere else in the codebase to reuse (checked: gossip/dashboard/
gossip-quorum-cosigner only ever go compressed->EIP-2537, never the reverse),
so this uses the curve library's own public API rather than inventing a
byte-parser.
Medium-5: local key derivation now goes through SignerService.forNode() /
LocalKeySigner — the REAL signing path — instead of a local hex re-parse.
A private key the signer would reject (wrong length, non-hex char, or an
uppercase "0X" prefix LocalKeySigner's case-sensitive check doesn't strip)
now correctly fails this check too, closing a false-"ok" gap.
High-3: hardened `prefix()`/`normalizeHex()` against non-string node_state
fields (raw disk JSON has no schema validation), AND added a structural
boundary in `loop()`: `check()` is wrapped in its own try/catch so a future
unexpected throw can never (a) produce an unhandledRejection or (b) kill the
retry loop's future scheduling — it keeps ticking. The outer `.catch(() => {})`
at both loop() call sites is defense-in-depth on top of that.
Medium-4: `start()`/`stop()` now bump a generation token; `loop()` captures it
and `check()` discards (does not merge into lastResult, does not log) a stale
attempt whose generation no longer matches — so an in-flight check from a
just-superseded identity can never clobber the new identity's result or stack
a second timer. `start()` also clears the held result immediately so a
previous identity is never shown even momentarily under a new loop.
High-2: NodeService.reloadNodeState() (called by dashboard create/import/
delete) now restarts the self-check against the new identity, or stops +
clears it when the node was deleted — previously it never touched the check
at all, so a deleted node's stale "ok" would sit in /health forever.
Mutation-verified (not committed): removing the 128-byte branch from
parsePublicKeyPoint() turns 3 High-1 tests red; disabling the generation
discard in check() turns the Medium-4 stale-result test red — both confirmed
locally, then reverted before this commit.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Pdq9rgkZq9M4JFeTZD7yYs
…atch the real signer selection (local vs remote/TEE), and make internal checker bugs fail loud
Codex adversarial review round 2 (REQUEST_CHANGES, 2 High + 1 Medium + 1 Low), all
addressed. Round-1 close-outs (High-2, Medium-4, process-safety half of High-3)
were independently re-verified by the coordinator and are unchanged here.
High-1 (Codex confirmed against the installed @noble/curves): parsePublicKeyPoint()
was still too permissive.
1. Non-zero padding in the 128-byte EIP-2537 branch was silently accepted — only
the X/Y coordinate slices were read, never the 16-byte zero-padding regions
either side of them. Now explicitly validated; a corrupted padding value is
rejected as malformed, not normalized through as if legitimate.
2. The point-at-infinity was accepted in BOTH wire formats. `.assertValidity()`
alone does NOT reject it — @noble/curves deliberately allows the group
identity as "on-curve" — verified empirically: 128 zero bytes decode to
Point.ZERO and pass assertValidity(). An infinity "public key" makes a
pairing check trivially pass with NO private key at all — the exact
rogue-key bypass AAStarValidator.registerWithProof's
`require(!_isInfinity(publicKey))` guards against on-chain. Both branches
now explicitly call `.is0()` and reject.
High-2 (the coordinator's own correction to round-1 guidance): LocalKeySigner is
only the DEFAULT backend, not "the" signer — bls.service.ts:140's signDerivedHash
prefers a remote Rust/TEE-KMS signer whenever RUST_SIGNER_URL is set (and ONLY
that signer when RUST_SIGNER_REQUIRED=true), while node.service.ts still allows a
node_state.json to carry a (now-irrelevant, possibly stale) local privateKey in
that mode. Comparing the local key unconditionally could report ok while every
real (remote) signature fails, or report mismatch while every real signature is
fine — production topology: node1 is MX93 KMS-TEE hosted (CC-24/CC-30).
Fixed by extracting the signer-selection DECISION into one shared method,
`BlsService.getRustSignerConfig()`, that both `signDerivedHash()` and the new
`BlsService.resolveSigningPublicKey()` read — never duplicated, never able to
independently drift. `resolveSigningPublicKey()` asks the remote signer for its
own public key (there's no dedicated endpoint; it probes /sign with a fixed,
harmless, clearly-marked message and reads `public_key` off the response, same
as a real sign does) when remote is configured, and NEVER falls back to the
local key if that probe fails — it reports `skipped_remote_signer` instead. Only
the owner-authorization gate (authorizeAndDeriveHash) is explicitly untouched;
signDerivedHash's own tests (there were none locking its internals) all still
pass, and the whole signing/signature test suites remain green.
NodeKeyConsistencyService no longer depends on SignerService directly — it goes
through BlsService.resolveSigningPublicKey exclusively, which simplified the DI
graph back down (removed the SignerModule import round-2 added to NodeModule).
Medium (the coordinator's own correction: round-1's "catch does nothing" pattern
was right for keeper but wrong here): the INNER catch in loop() (around check()
only, not the outermost safety net) now distinguishes "check() itself threw" (a
bug) from "an RPC blip" (already handled inside check() as `unknown`, never
reaching here). It preserves the prior CONCLUSIVE result (still no downgrade),
but stamps `checkerError`/`checkerErrorAt` on it and still advances
`lastAttemptAt` — so /health can no longer show an internal bug as an unbroken
"still ok" forever; the result is explicitly marked stale-since. The log call
inside is wrapped in its own try/catch (logging must never itself escalate a
checker bug into a crash) and the outer `.catch(() => {})` at both loop() call
sites is kept as a separate, logic-free last resort — the two layers now have
distinct jobs, as intended.
Low: added tests for non-zero-padding rejection, the point-at-infinity in both
wire formats, the two remote-signer A/B misattribution scenarios (silent-failure
and false-positive) plus unreachable/non-2xx remote-signer responses, and
strengthened the existing "internal exception" tests to assert `checkerError`
is actually visible (the previous assertions — no unhandledRejection + timer
still running — would have passed even with Medium's bug present).
Mutation-verified (3, not committed, each reverted after confirming red):
1. Disabling the `.is0()` guard in both parsePublicKeyPoint() branches turned
the infinity-rejection tests red (both the 128-byte and, after fixing one
test whose weak assertion masked the bug, the compressed-format test too).
2. Reverting checkLocal() to bypass resolveSigningPublicKey() and always
derive from the raw local key turned all 5 HIGH-2 remote-signer tests red.
3. Making the inner catch in loop() skip stamping checkerError/checkerErrorAt
turned both the HIGH-3 and the dedicated Medium checker-error test red.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Pdq9rgkZq9M4JFeTZD7yYs
…ust never itself produce a signature
Codex round 3 review (coordinator-flagged): round-4's resolveSigningPublicKey()
probed the remote/rust signer's /sign endpoint with a fixed harmless message to
read its public key off the response, so the check could distinguish which key a
remote signer actually uses. That was the wrong call and is reverted.
Why it had to go, in priority order:
1. It breaks this repo's core invariant that a node signs ONLY after the
owner-authorization gate (authorizeAndDeriveHash) passes. A background
health-check tick is not an authorized request, so it must never itself
cause the signer to produce a signature, no matter how "harmless" the
message — a KMS/TEE signer's every output should trace back to an
authorized request; a probe every recheck cadence is ~96 unauthorized
signatures/node/day with no owner-auth record behind them.
2. The remote signer's operational policy (metering, rate limits, anomaly
detection, possible presence requirements) is opaque to this repo. A
background probe competes with real signing traffic for that budget and
could itself trip a limit or delay a real signature.
3. It shared the exact same BLS domain (BLS_DST, "..._RO_POP_") as production
traffic, not a separate, clearly-scoped test domain.
Fix: `BlsService.resolveSigningPublicKey()` now makes NO network call when a
remote signer is configured — it returns `{ source: "remote" }` immediately,
with nothing to compare. `NodeKeyConsistencyService.checkLocal()` reports this
as `skipped_remote_signer`, same status name as before, but now reached without
ever invoking the signer. `getRustSignerConfig()` (the shared selection point)
is unchanged — it was a correct extraction and stays. `REMOTE_SIGNER_PROBE_HASH`
and `probeRemoteSignerPublicKey()` are deleted, not left as dead code (`grep -rn
"cc36" src` returns nothing).
Honest, documented consequence (in both BlsService's and NodeKeyConsistencyService's
docstrings): a remote-signing node's local dimension is simply NOT verified by
this check right now — the A-vs-B misattribution scenarios round-3 tested for
can no longer be distinguished, but they also can no longer produce a false "ok"
or false "mismatch_local" either, since the local dimension isn't evaluated at
all when remote is configured. Documented two follow-ups that would close this
without ever causing an extra signature: (A) passive observation — record
public_key from real, already-authorized /sign responses instead of asking for
a dedicated read; (B) ask repo:kms for a read-only get-public-key endpoint on
the remote signer, separate from /sign.
Updated the 5 round-3 HIGH-2 tests (which asserted specific A/B outcomes via a
mocked fetch) to the new behavior: any remote signer configuration alone
produces `skipped_remote_signer`, with no fetch mock needed at all.
Added the load-bearing invariant test in two places: a one-shot version in the
main spec (check() called once) and a scheduling-spec version that spies on
global fetch across a boot attempt plus three ~15-minute periodic recheck
cycles, asserting zero calls throughout.
Mutation-verified (not committed, reverted after confirming red): temporarily
reintroduced a `fetch(...)` call in the remote branch of
`resolveSigningPublicKey()` — both invariant tests (one-shot and
scheduling-loop) correctly failed with "Expected number of calls: 0, Received:
1" / "...Received: 4". Restored file diffed byte-identical to the pre-mutation
version before committing.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Pdq9rgkZq9M4JFeTZD7yYs
…e recording a checker error, lock signDerivedHash's remote path, expose checkerError at the /health contract
Codex round 4 review (REQUEST_CHANGES, 0 Critical/0 High, 2 Medium + 2 Low). All
of round 3's High-1/High-2/Medium were independently confirmed closed; the
signDerivedHash refactor was manually diffed and confirmed behavior-preserving;
no checker path was found that could produce a signature or contact the
signer. This round addresses the remaining 4 findings.
M1 — isRegistered(nodeId) and registeredKeys(nodeId) were two independent
`latest` reads. A node deactivated by the validator's permissionless
syncNode(nodeId) (which clears registeredKeys) exactly between the two calls
would read "registered" (slightly-earlier call) + "empty key"
(slightly-later call) and report mismatch_onchain — a status that was never
actually true at any single point in time; the real transition is
registered -> not_registered with no "registered but wrong key" state in
between. Fixed: BlockchainService.checkNodeRegistration/getNodePublicKey now
take an optional `blockTag`; NodeKeyConsistencyService.checkOnchain() reads
getBlockNumber() ONCE and pins both calls to it (the same pattern the audit
module already uses for finality-pinned reads). Added defense-in-depth: an
empty registeredKeys() return is treated as not_registered regardless of what
isRegistered() said.
M2 — the catch in loop() called recordCheckerError() BEFORE checking the
generation token, so an in-flight check from a SUPERSEDED identity that
REJECTS (not just resolves — Medium-4 only covered the resolving case) could
stamp its error onto whichever identity is current now, or resurrect a result
clearResult() had just wiped. Fixed: staleness is checked FIRST in the catch;
a stale/stopped rejection is discarded entirely (no checkerError, no
resurrected result, no reschedule) — extracted the check into
isStaleOrStopped() and reused it in both places in loop().
L1 — signDerivedHash()'s hybrid local/remote selection was refactored across
rounds 4-5 but had zero test coverage of the actual remote signing path (only
the checker's "never calls fetch" tests existed, which say nothing about
whether signing itself still works). Added exactly the four tests requested,
no broader: remote success response mapping, rustSignerToken sent as
X-Signer-Token, RUST_SIGNER_REQUIRED=true + remote failure rethrows without
fallback, and remote-optional + failure falls back to local.
L2 — checkerError/checkerErrorAt (added round 4) were never asserted through
the /health contract layer, only inside the service's own tests. Added a
fixture + assertion in health.controller.spec.ts.
Mutation-verified (4, not committed, each reverted to a byte-identical diff
after confirming red):
1. Dropped the shared blockTag in checkOnchain() -> both new M1 tests failed
(exact blockTag assertion, and the race-scenario regression test).
2. Moved the generation check to after recordCheckerError() in loop()'s
catch -> both new M2 tests failed, reproducing exactly the described bug
(checkerError leaked onto the new identity; a cleared result was
resurrected with status "unknown").
3. Removed the X-Signer-Token header from signViaRust() -> L1 test 2 failed.
4. Disabled the RUST_SIGNER_REQUIRED rethrow branch (forced fallback) ->
L1 test 3 failed ("resolved instead of rejected").
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Pdq9rgkZq9M4JFeTZD7yYs
…k key, scoped and tagged so it's never mistaken for full verification
Codex round 5 review: round 3's M1/M2/L1/L2 all confirmed closed; BlockchainService's
new optional blockTag param confirmed backward-compatible with registerOnChain/
dashboard and all other existing callers. One new High found — again a gap in the
coordinator's own round-5/6 spec, this time missing the required/optional distinction.
High — round 5/6 made resolveSigningPublicKey() return `{ source: "remote" }" for
ANY remote signer configuration, required or optional, so the checker mapped it to
skipped_remote_signer unconditionally. But signDerivedHash() SILENTLY FALLS BACK to
the local privateKey whenever an OPTIONAL remote call fails (bls.service.ts:145-156)
— so in optional mode the local key is a REAL signing key in active use, not a
stale leftover, and leaving it unverified reproduced exactly the silent-failure gap
the checker exists to close: local key stale (A), chain re-registered (B), optional
remote unavailable => every real fallback signature (A) fails against the chain (B)
forever, while /health confidently reported "skipped" as if nothing needed checking.
node-key-consistency.service.spec.ts had a test that PINNED this unsafe behavior.
Fix: required/optional is read from getRustSignerConfig().required (the single
selection point — not re-read). Required mode (or optional mode with no local key)
is unchanged: `{ source: "remote" }`, skipped_remote_signer, no verification
possible without producing a signature. Optional mode WITH a local privateKey now
returns `{ source: "local_fallback", publicKey }` — derived by the exact same pure
computation as the "local" branch (zero network calls, zero signatures — the
"checker never signs" invariant is untouched, since deriving a public key from a
private key needs no signing). NodeKeyConsistencyService.checkLocal() runs the
normal three-way comparison against this key and reports ok/mismatch_local/
mismatch_onchain/not_registered/unknown same as always.
Scope tagging (the safety-critical part): any result built from the local_fallback
path is stamped `scope: "local_fallback_only"` on the KeyConsistencyResult, exposed
through /health. An "ok" here proves ONLY that the fallback key is internally
consistent — it says nothing about the remote signer's own (primary,
unverifiable-without-signing) key, and must never be read as full verification.
Without this tag a fallback-scoped "ok" would become a new source of false comfort.
Replaced the round-4/5 test that pinned the unsafe "always skip" behavior with the
silent-failure regression it should have caught, and added the required-mode /
no-local-key / consistent-ok scenarios the coordinator specified. Extended both
"checker never calls fetch" invariant tests (main spec + scheduling spec) to also
cover the new local_fallback derivation path (pure computation, confirmed still
zero network I/O) and added a dedicated required-mode invariant test alongside it.
Mutation-verified (3, not committed, each reverted to a byte-identical diff after
confirming red):
1. Disabled the optional-mode local_fallback branch (forced skip) -> the
silent-failure regression test failed on the missing `scope` field (status
alone didn't change, since skipped_remote_signer + mismatch_onchain still
combines to mismatch_onchain — the scope assertion is what actually catches
this, confirming why it's required, not decorative).
2. Dropped the `scope` field from the conclusive-result branch in check() -> the
"ok scoped to local_fallback_only" test failed (scope undefined).
3. Made required mode ALSO check the local key -> the "required + stale local key"
test failed (mismatch_local instead of the correct skipped_remote_signer).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Pdq9rgkZq9M4JFeTZD7yYs
…re regression test Coordinator review of round 7's mutation-1 output: the existing "stale local fallback key + on-chain re-registered" regression test used node_state.publicKey = A (the same stale key as privateKey), so the ON-CHAIN comparison (A vs. chain B) already caught the drift on its own. Disabling local_fallback entirely left `status` at mismatch_onchain unchanged — only the `scope` field went missing. The test's status assertion therefore did not distinguish the new local_fallback logic from the old "always skip" behavior; only the scope assertion did. The scenario Codex flagged as the real danger: privateKey is stale (A), but node_state.publicKey was separately (and correctly) re-imported as B — matching the chain — since import-node.dto.ts accepts publicKey and privateKey as two independent fields with nothing forcing them to be a matching pair. Here the on-chain comparison is B vs. chain B => ok on its own; ONLY the local_fallback comparison (privateKey A's derived key vs. node_state.publicKey B) can catch that the private key on disk doesn't correspond to the identity everything else agrees on. Added that exact test: optional remote mode + privateKey A + node_state.publicKey B + on-chain B => mismatch_local, scoped to local_fallback_only. Mutation-verified (not committed, reverted to a byte-identical diff after confirming red): disabled the optional-mode local_fallback branch (forced skip, same mutation as round 7's) — this time the new test fails ON THE STATUS ASSERTION itself (expected mismatch_local, received skipped_remote_signer), not just on scope, proving it actually distinguishes the local_fallback logic from the old behavior. Test-only change; no production code touched. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Pdq9rgkZq9M4JFeTZD7yYs
clestons
requested changes
Sep 27, 2026
clestons
left a comment
clestons
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES — 设计与实现都核实过，正确；阻塞项是一个真实、机械、易修的 CI 失败。
阻塞：npm run format:check 真的红
gh pr checks 显示 CI 里 Code Quality 失败，本机复现一致：
npx prettier --check "**/*.{js,jsx,ts,tsx,json,css,md}"
[warn] src/modules/bls/bls.service.ts
[warn] src/modules/health/health.controller.spec.ts
[warn] src/modules/node/node-key-consistency.service.scheduling.spec.ts
[warn] src/modules/node/node-key-consistency.service.spec.ts
[warn] src/modules/node/node-key-consistency.service.ts
[warn] src/modules/node/node.key-consistency-wiring.spec.ts
[warn] src/modules/node/node.service.ts
7 个文件全部是本 PR 新增/改动的文件。审阅记录里写了 5 轮 Codex 对抗评审 + mutation 验证，但没有一处提到跑过 format:check——这条纯机械检查显然被漏跑了。修法：npx prettier --write 这 7 个文件，提交。
Security Audit 的红（multer<=2.2.0/@nestjs/platform-express 两条 high advisory）是本仓库既有欠账（同一账本在 #377 review 里已记录过，npm audit fix 修不掉），与本 PR 无关，不算阻塞。CI Success` 是聚合门，会跟着上面两条一起转绿。
我自己验证过的核心声称（不是照抄评审记录）
- 零信令交互（"the most important test in this file"）：直接读
resolveSigningPublicKey，required/optional 两条分支都只做本地纯计算（signerService.forNode(node).getPublicKey()），从未fetch。真实逆向变异：在 remote 分支里手动加回一次fetch(rust.url + "/sign", ...)，对应测试立刻从 0 次变成 1 次调用而失败；还原后git status/git diff --stat干净。这条 fail-safe 是真的，不是测试碰巧没覆盖到。 - 同一套编码器/同一处选签逻辑：
grep确认toEip2537Hex调的是this.blsService.encodePublicKeyToEIP2537（不是本地拷贝），signer 选择走的是getRustSignerConfig()（signDerivedHash()用的同一处）。 - 两种 wire 格式的无穷远点拒绝：
parsePublicKeyPoint的 96-hex（48 字节压缩）和 256-hex（128 字节 EIP-2537）两条分支都在assertValidity()之后显式point.is0()拒绝（@noble/curves本身接受无穷远点，链上合约的require(!_isInfinity(publicKey))也是同样的顾虑）。跑了HIGH-1.2/HIGH-1.2b两条现成测试，均通过。 - 瞬时失败不冲掉确定性结论：跑了 "a recheck RPC failure PRESERVES the prior conclusive result — never downgrades it to unknown"，通过——一次 RPC blip 不会把
mismatch_onchain洗成unknown。 - 全量相关测试：
src/modules/node+src/modules/bls+src/modules/health共 113 passed（本机实跑，非照抄 PR 声称的数字）。
顺带一提（不阻塞）
R1b（DeepSeek 安全视角）标了一条 Medium：未鉴权的 /health 会把 checkerError（源自 error.message）暴露给任何调用方。读了 recordCheckerError，这个字段只在检查器自身抛出意外异常时才会出现（正常失败路径全部走状态值，不抛异常），触发条件很窄；但既然 /health 目前确实无鉴权，一条内部 bug 的裸 error.message（可能带路径片段）值得记一笔，供后续决定要不要脱敏或加鉴权，不要求本 PR 处理。
耗时: ~40 分钟。轮次: [利用已披露的 5 轮 Codex 对抗评审历史，重点做独立复核而非从零重跑全流程：零信令交互项做了真实逆向变异，编码器复用/无穷远点拒绝/瞬时失败降级三项各自实跑现成测试]。
…heck was never run pr-daemon's REQUEST_CHANGES on #378 was purely mechanical: `npm run format:check` fails on all seven files this PR adds or changes. Five Codex rounds and every mutation check ran type-check, lint:check and jest, but the verification list the author was given omitted format:check, although CLAUDE.md asks for `npm run format` before every commit. Formatting only; no behaviour change — tsc clean and the same 113 tests in node/bls/health pass before and after. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Pdq9rgkZq9M4JFeTZD7yYs
clestons
approved these changes
Oct 10, 2026
clestons
left a comment
clestons
left a comment
There was a problem hiding this comment.
Round 2 — APPROVE（阻塞项已修；新一轮R1发现均核实为假阳性或已知非阻塞项）
head: 5f2b514827a635b67e4bd3dc591d847388e58ea4
reviewed: 2026-10-10
阻塞项已修复：新commit(5f2b514,"apply prettier to the 7 files the PR touched")只改了格式,直接读diff确认全部是参数换行/引号风格这类纯格式改动,无逻辑变化。gh pr checks现查:Code Quality已转绿;Security Audit/CI Success仍红,但这是上一轮已记录的既有欠账(multer/@nestjs/platform-express,npm audit fix修不掉,与本PR无关),不是新问题。
R1重跑(针对完整PR diff)发现与处置：
- R1a三条(check()在generation-stale分支返回可能为null的lastResult、synthetic用nodeId不用safeNodeId、combine()在skipped_remote_signer时误返回ok)→全部驳回(假阳性,直接读源码核对):loop()的stale-generation早退是
return;(void),根本不返回lastResult;recordCheckerError(真正用到lastResult??synthetic的地方)已经正确用了safeNodeId;combine()在local==="skipped_remote_signer"时确实会先返回该值,文档注释也显式写明了这个优先级顺序。 - R1bMedium→与第一轮已记录的非阻塞项同一条。
- R1bLow→非问题,标准本地sidecar鉴权模式。
此前阻塞项：已修复。确认裁决：APPROVE。
本评审只给结论，不做合并。
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes the gap reported by
repo:sdkin CC-36.The silent failure
A node's identity is
node_state.json(nodeId,publicKey, usually a BLS private key); the validator bindsnodeId → registeredKeys(nodeId). If the key a node actually signs with differs from the key registered for its nodeId, registration still succeeds — but every signature the node produces fails verification. The node is up,/healthis green, and it silently never participates. Until now nothing on the startup or signing path checked for this;isRegisteredwas read only by a dashboard badge.What it does
A
NodeKeyConsistencyServiceruns a three-way comparison and exposes the result as a new, additivekeyConsistencyfield on/health(status,version,capabilitiesare unchanged):node_state.publicKeymismatch_localnode_state.publicKey↔ on-chainregisteredKeys(nodeId)mismatch_onchain·not_registeredunknownskipped_remote_signerderivedMatches(nodeId == keccak256(publicKey)) is reported for information only and never affects the status: the legacy bootstrap pathregisterPublicKey(nodeId, publicKey)lets callers choose their nodeId, so treating it as a criterion would flag legitimate nodes.Design decisions worth reviewing
The checker never produces a signature and never contacts the signer. An intermediate revision learned the remote (Rust/TEE) signer's public key by sending a fixed probe hash to its
/signendpoint. It was removed. It technically worked, but it made a health check produce signatures outside the owner-authorization gate, breaking the invariant "every signature this node produces went through authorization", polluting TEE/KMS signing audit trails (~96 unauthorised signatures per node per day), and depending on unknown signer policy (rate limits, user presence) that could block real signing. A test asserts the signer's HTTP endpoint is called 0 times across startup plus three periodic re-checks while the check provably ran ≥4 times; re-adding the probe turns it red (1 and 4 calls).Single source of truth — the checker reuses the system's own implementations, never its own copies. Three bugs in review had this one root cause:
BlsService.encodePublicKeyToEIP2537, the exact encoderregisterOnChain()writes with (an early revision used a copied encoder that could silently drift);getRustSignerConfig(), the same pointsignDerivedHash()uses (an early revision usedLocalKeySigner, which is only the fallback);publicKeywire formats are accepted, because the dashboard create path stores 128-byte EIP-2537 while live nodes hold 48-byte compressed keys.Optional vs required remote signing. Required mode has no fallback, so the local key is irrelevant ⇒
skipped_remote_signer. In optional mode,signDerivedHash()silently falls back to the local key when the remote fails, so that key is a live signing key ⇒ it is verified (pure derivation, zero signing) and the result is taggedscope: "local_fallback_only", so anokthere can never be read as "the remote primary key is verified".Malformed and degenerate keys. 128-byte keys must have zero padding on both coordinates; the point at infinity is rejected explicitly in both branches (
@noble/curves'assertValidity()deliberately accepts it, and an infinity pubkey makes the pairing hold with no secret — the same reason the contract hasrequire(!_isInfinity(publicKey))).Fail-loud, not fail-stop, and never "confidently wrong".
unknownretries with bounded backoff (30s → 240s), then re-checks every 15 min — because the validator's permissionlesssyncNode()can deactivate a node after startup.mismatch_onchainis not washed intounknownby an RPC blip).checkerError/checkerErrorAton it, surfaced on/health, so a staleokis visibly qualified. The outermost boundary is a pure no-crash backstop; fail-loud lives in the inner layer.reloadNodeState()rebinds on dashboard create/import/delete, so/healthnever reports a deleted identity.mismatch_onchainthat never held in any chain state.Review record
Written by a cheaper model; planned and reviewed by the planner; Codex (Tier 1) adversarial review × 5 rounds. Every fix is mutation-verified — the change under test was reverted, the test confirmed red, then restored (mutations not committed). One review lesson worth recording: a mutation turning a test red is not enough — which assertion fails matters. One regression test failed only on its
scopeassertion because its fixture's stalepublicKeywas caught independently by the on-chain comparison; a discriminating test (stale private key, correctpublicKeyfield) was added so the status assertion itself distinguishes old from new behaviour.Explicitly deferred (Codex round 5, Medium)
On two error paths in optional mode — key derivation throwing, and a first-attempt structural checker exception — the result is not tagged with
scope. Deferred deliberately:scopeexists to stop an untaggedokbeing read as full verification, and Codex confirmed these paths yield onlymismatch_localorunknown, never an untaggedok. The false-reassurance property the tag protects holds; what remains is metadata completeness on non-reassuring states. Suggested fix recorded by Codex: carry a discriminatedsource: "local_fallback"through the fallible derivation and intorecordCheckerError().Known limitation
Remote-signer primary keys are not verified — obtaining them would require the probe that was removed. Follow-ups that need no extra signing: (1) passively record
public_keyfrom real, already-authorised/signresponses and compare that to the registered key; (2) askrepo:kmsfor a read-only public-key endpoint.Verification
Full suite 591 / 591 (44 suites) ·
tsc --noEmitclean ·lint:check0 errors (20 pre-existing warnings, unchanged) · signature-path suites (bls,signature) all pass · the fullAppModulewas booted viaNestFactory.createApplicationContext(BOOT_OK/CLOSE_OK) to prove the new DI edges introduce no cycle · the pre-existing jest "worker failed to exit" notice is unchanged. Most of the ~2,500 added lines are tests.🤖 Generated with Claude Code
https://claude.ai/code/session_01Pdq9rgkZq9M4JFeTZD7yYs