fix(agent): refuse to register a CG that can never satisfy global quorum#374
Open
branarakic wants to merge 2 commits intomainfrom
Open
fix(agent): refuse to register a CG that can never satisfy global quorum#374branarakic wants to merge 2 commits intomainfrom
branarakic wants to merge 2 commits intomainfrom
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // implement `getMinimumRequiredSignatures` (mock/legacy adapters): the | ||
| // existing on-chain create call is still subject to its own constraints, | ||
| // and we don't want to regress test fixtures that use a permissive mock. | ||
| if (typeof this.chain.getMinimumRequiredSignatures === 'function') { |
There was a problem hiding this comment.
🔴 Bug: This guard only protects registerContextGraph(). The daemon/API path that calls registerContextGraphOnChain() directly still accepts participantIdentityIds/requiredSignatures combinations below minimumRequiredSignatures, so clients can still mint a context graph that will always fail on publish. Move this validation into registerContextGraphOnChain() (or a shared helper used by both entry points) and add coverage for the direct registration path.
| let globalMin: number; | ||
| try { | ||
| globalMin = await this.chain.getMinimumRequiredSignatures(); | ||
| } catch (err) { |
There was a problem hiding this comment.
🔴 Bug: Swallowing getMinimumRequiredSignatures() failures reintroduces the exact footgun this PR is trying to prevent. A transient eth_call/RPC failure here can still be followed by a successful createContextGraph transaction, leaving the operator with a permanently unpublishable CG. For adapters that expose this getter, fail closed instead of logging and continuing; if tests need to bypass the check, make that an explicit opt-in rather than the default on read errors.
7 tasks
Operator footgun observed during the Base Sepolia cgId=10/cgId=11 incident: running `dkg context-graph register <local-name>` against a CG with no recorded `participantIdentityIds` in `_meta` silently fell back to `[selfIdentityId]` plus `requiredSignatures = 1`, then succeeded on-chain because `ContextGraphStorage.createContextGraph` only enforces `requiredSignatures <= hostingNodes.length` — it never reads `ParametersStorage.minimumRequiredSignatures`. The minimum-signatures check happens later, inside `KnowledgeAssetsV10.publishDirect`, so every publish to that CG reverts forever with `MinSignaturesRequirementNotMet` and the on-chain CG is paid-for and permanently broken. Pre-flight in `DKGAgent.registerContextGraph`: - When `chain.getMinimumRequiredSignatures` is implemented, refuse when `effectiveParticipantIdentityIds.length < globalMin` or `effectiveRequiredSignatures < globalMin`, with explicit hints naming the future revert (`MinSignaturesRequirementNotMet(globalMin, X)`). - Surface the silent `[self]` fallback as a structured WARN log so operators can scrape for it instead of finding out at publish time. - Skip the check when the adapter does not expose `getMinimumRequiredSignatures` (mock/legacy adapters), keeping existing fixtures green. `MockChainAdapter.minimumRequiredSignatures` defaults to 1, so the check is a no-op for the existing agent-test fixtures (verified locally: Genesis Knowledge registration tests still pass). Adds 5 focused tests covering all four branches plus the legacy single-host backwards-compatible case. Co-authored-by: Cursor <cursoragent@cursor.com>
…um gate Codex review on PR #374 flagged two real bugs in the global-quorum pre-flight added in the previous commit: 1. The validation only ran inside `registerContextGraph(id, opts)`. The daemon's `/api/context-graph` route calls `registerContextGraphOnChain` directly (`packages/cli/src/daemon/routes/context-graph.ts:429`) and therefore bypasses the check entirely — an HTTP client could still mint a permanently-broken CG by submitting bad params at the lower entry point. The footgun the PR was supposed to close was only half-closed. 2. The `try { getMinimumRequiredSignatures() } catch { warn-and-continue }` shape silently skipped the pre-flight on a transient RPC failure, after which the chain's own `createContextGraph` would happily mint the CG (it doesn't check the global floor) and every subsequent publish would revert. That is exactly the bug the pre-flight exists to prevent: a single flaky `eth_call` reintroduces the footgun. Refactor the validation into a private `assertGlobalQuorumOrThrow` helper, called from both `registerContextGraph` (preserves existing test messages, gives operators a nicer error with the local CG `id` and an early failure before the lazy curator-stamping work) AND `registerContextGraphOnChain` (closes the daemon-HTTP bypass). Belt + braces; the lower-level chokepoint is the security boundary that all callers must traverse, the upper-level call is the better UX surface. Switch the RPC-error branch from `warn-and-continue (globalMin = 0)` to `throw`. Skip silently only when the adapter does not implement `getMinimumRequiredSignatures` at all — i.e. legacy / pre-V10 mock adapters that don't model the floor. This keeps the existing test fixtures working without re-introducing the swallowed-error footgun on real chain adapters. Tests: - Existing 5 high-level tests still pass. - 3 new tests assert the bypass is closed (call `registerContextGraphOnChain` directly with bad hostingNodes / requiredSignatures combos and assert it throws before any chain mutation). - 2 new tests assert fail-closed semantics (override `getMinimumRequiredSignatures` to reject; assert both the high-level and direct-call paths throw with a clear "Refusing to proceed (fail-closed)" message instead of silently proceeding). Verified by sabotage: removing the inner-call validation makes exactly the 3 new direct-call + RPC-error tests fail, leaving the original 7 passing — proving the new tests genuinely catch the bugs, not just the happy path. Co-authored-by: Cursor <cursoragent@cursor.com>
branarakic
force-pushed
the
fix/register-cg-quorum-preflight
branch
from
23a1ef1 to
74202c1
Compare
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
1 participant
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.
Summary
DKGAgent.registerContextGraphnow refuses on-chain registration when the proposed CG configuration cannot satisfyParametersStorage.minimumRequiredSignatures(the global quorum floor).This closes the operator footgun observed during the Base Sepolia cgId=10 → cgId=11 incident, where running
dkg context-graph register <local-name>against a CG with no recordedparticipantIdentityIdsin_metasilently fell back to[selfIdentityId]+requiredSignatures = 1, then succeeded on-chain becauseContextGraphStorage.createContextGraphonly enforcesrequiredSignatures <= hostingNodes.length— it never reads the global minimum. The minimum-signatures check is deferred toKnowledgeAssetsV10.publishDirect, so every publish to that CG reverts forever withMinSignaturesRequirementNotMet. The on-chain CG is paid-for, persistent, and permanently broken; the operator only finds out at publish time.Repro chain (pre-PR)
dkg context-graph create my-cg --name 'My CG'(no--participants).dkg context-graph register my-cgsucceeds, mints CG #N on-chain withhostingNodes=[selfIdentityId],requiredSignatures=1.dkg publish ...to that CG reverts withMinSignaturesRequirementNotMet(3, 1)(assuming production-typicalminimumRequiredSignatures = 3).After this PR
Step 2 throws synchronously with one of:
The silent
[self]fallback is preserved (some single-host dev/test flows depend on it) but is now surfaced as a structuredWARNlog so operators can grep for it. The new check is gated onchain.getMinimumRequiredSignaturesbeing implemented, so legacy/mock adapters that don't expose it are unaffected.Why pre-flight (vs let chain revert)
The chain revert is the symptom, not the root cause. With pre-flight:
Test plan
pnpm --filter @origintrail-official/dkg-agent buildclean.packages/agent/test/register-quorum-preflight.test.ts(5 tests) all green:[self]fallback when globalMin=3requiredSignaturesbelow globalMinGenesis Knowledgeregistration tests inagent.test.ts(which useMockChainAdapterwith defaultminimumRequiredSignatures=1) all pass — no regression.per-cg-quorum-extra.test.tsfailure verified unrelated (intentional RED spec anchor for production bug A-5; identical failure on unmodifiedmain).Out of scope
--soloopt-out flag to allow registering a single-host CG when globalMin > 1 (no use case yet).dkg context-graph createtime (the create call is purely local — the user isn't paying gas yet, and the existingrequiredSignatures > participantCountcheck there already covers the most common typo).Made with Cursor