fix(chain): select at most two provider origins so shipped RPC pools construct - #2059
Conversation
| CURRENT_FINALIZED_EVM_READ_MAX_ATTEMPTS_V1, | ||
| } from './current-finalized-evm-read-profile.js'; | ||
|
|
||
| export interface StrictFinalizedEndpointSelectionV1 { |
There was a problem hiding this comment.
🟡 Issue: The endpoint selector is over-modeled for the behavior this PR needs
What's wrong
This change fixes a simple construction problem, but it introduces a broad “session selection” abstraction with unused metadata, future corroboration semantics, and a generic bound. That forces readers to understand concepts that are not part of the current production flow and makes the tests validate speculative branches instead of simplifying the implementation.
Example
The actual product path normalizes endpoints, calls selectStrictFinalizedEndpointSessionV1(normalized, CURRENT_FINALIZED_EVM_READ_MAX_ATTEMPTS_V1), checks session.selected.length, and returns only session.selected. The returned origin metadata, same-origin backfill model, and generic maxOrigins ordering repair are not used by this PR's production behavior.
Suggested direction
Collapse this into a narrow private helper that returns readonly string[] for the current config path, or make the selector own only the one current invariant: validated endpoints in, bounded endpoints out. Defer selectedOrigins, arbitrary bounds, and corroboration-facing semantics until a production caller actually needs them.
For Agents
Look at snapshotNormalizedEndpoints and strict-finalized-endpoint-session.ts. Preserve the behavior that a validated shipped 3-endpoint pool constructs and yields at most two dialable endpoints, preferring distinct origins. Prove that with the shipped-pool/config tests, but remove or privatize unused future-facing API surface.
There was a problem hiding this comment.
Accepted in part, and the part I accepted let me delete code rather than defend it. Pushed in aef40c244.
Taken — the generic bound is gone. You are right that it had no production justification: snapshotNormalizedEndpoints was the only caller and it always passed CURRENT_FINALIZED_EVM_READ_MAX_ATTEMPTS_V1. Every other call site was a test. The selector now reads that constant directly, which deletes the parameter, its Number.isSafeInteger guard, and the ordering repair — because the repair only existed to fix a reordering failure mode that is unreachable at the shipped bound. At a fixed bound of 2, pass 2 runs only when pass 1 selected fewer than the bound, which means the pool held exactly one distinct origin, so pass 1 holds index 0 and pass 2 appends index 1. Order is preserved by construction. Net −14 lines, and a whole defect class removed rather than tested.
Not taken — selectedOrigins, with a reason. It is not future-facing decoration; it is the only way to express the semantic this change exists to enforce. Provider identity is the ORIGIN, not the URL, so selected alone cannot distinguish "two providers" from "two spellings of one provider" — and selected.length is actively misleading, because backfill can seat two endpoints sharing one origin. Without it the test treats path/query/credential/case variants of one host as ONE provider cannot be written at all. A mutant that sources it from the whole pool instead of the session kills 4 tests.
Not taken — same-origin backfill, because removing it is a regression. It is not speculative modelling: at base, a 2-URL pool of https://lb.example/v2/KEY_A and .../KEY_B constructed as two dialable endpoints. Origin dedup alone collapses those to one, halving failover for a deliberate two-endpoint operator config. Backfill preserves the base behaviour. On shipped pools (3 distinct origins) it never fires — it exists precisely for the config shape that would otherwise silently lose redundancy.
Already done — the surface is not public. selectStrictFinalizedEndpointSessionV1 is not exported from the package root; the export was removed in an earlier round precisely because every consumer imports it by relative path. It is internal to packages/chain.
Verification after the change: chain lane 44 files / 881 passed / 1 skipped, and nine mutants each killed by their named test.
There was a problem hiding this comment.
🟡 Issue: Origin normalization adds an unintended full-URL length limit
What's wrong
The selector only needs provider identity, but it now runs core's proof-oriented normalizeEndpointOrigin on the whole RPC endpoint. Because that helper bounds the input string before parsing, this introduces a new construction-time rejection for otherwise valid RPC URLs with long paths or query credentials, including single-endpoint configs that are unrelated to the oversized-pool fix.
Example
A config with one endpoint such as https://rpc.example.com/${'k'.repeat(4100)} previously constructed and would be dialed. Now it throws TypeError: Strict finalized endpoint must be an absolute URL string within the canonical scalar bound, even though the origin used for diversity is only https://rpc.example.com.
Suggested direction
Do not use the VM proof scalar bound as validation for the complete RPC URL; derive the origin from the already-normalized URL and bound only the origin/proof value.
Confidence note
The PR tests pin the new rejection, but I could not find a prior public contract that limited RPC endpoint URL length to the VM scalar bound.
For Agents
Look at packages/chain/src/strict-finalized-endpoint-session.ts around the normalizeEndpointOrigin(endpoint, ...) call. Preserve the shared origin-normalization predicate, but apply it to the derived origin, not to the full dial URL, or add a core helper that accepts an already-validated URL and returns the normalized origin. Add a regression where a one- or two-endpoint config with a long path/query still constructs and records the short origin.
There was a problem hiding this comment.
🟡 Issue: Collapse the selector to the actual two-endpoint policy
What's wrong
This adds a large abstraction for a very small policy. The config layer already owns the untrusted input boundary, but the new selector partially revalidates input, deduplicates again, and models a generic selection session even though the bound is fixed and there is one production caller. That makes the invariant harder to audit because readers must reconcile two validators and a long comment trail instead of seeing the simple endpoint policy directly.
Example
For the policy this PR needs, the fixed bound is 2: select the first URL, then the first later URL with a different origin, otherwise backfill with the second URL. Cases like [sameA, sameB, distinctC] -> [sameA, distinctC] and [sameA, sameB] -> [sameA, sameB] do not require a generic N-slot selector with originOf, firstUrlForOrigin, seen, ordered, and two passes.
Suggested direction
Move origin derivation into the existing config normalization boundary, or pass a small typed { href, origin } model into a private selector. With the inputs already normalized and deduped, the selector can be a direct two-slot policy instead of a second validator/generic session engine. That would delete the maps, duplicate handling, broad try/catch, and most of the explanatory comments without changing behavior.
For Agents
Look at snapshotNormalizedEndpoints and strict-finalized-endpoint-session.ts. Preserve the current behavior: validate every configured endpoint before selection, keep configuration order, prefer a second distinct origin, backfill same-origin only when no distinct second origin is selected, and return at most CURRENT_FINALIZED_EVM_READ_MAX_ATTEMPTS_V1 endpoints. Prove those cases with focused selector/config tests.
There was a problem hiding this comment.
🟡 Issue: Avoid exporting future-only corroboration metadata from the selector
What's wrong
selectedOrigins turns a local truncation helper into a broader corroboration/session API before that API has a real consumer. This is speculative surface area: it forces the module and tests to pin semantics for a future feature, while the only production caller immediately throws the field away. That extra model makes the change harder to maintain and increases the chance that future code treats this helper as the canonical corroboration boundary by accident.
Example
The production behavior introduced by the PR is only snapshot.endpoints = selected. The extra selectedOrigins surface exists for a future corroboration consumer described in comments, but no runtime path consumes it today.
Suggested direction
Return only the selected endpoint list for this PR, or keep origin metadata inside a private normalized endpoint model. Introduce a public selectedOrigins contract when the corroboration path actually consumes it and can define the exact boundary it needs.
For Agents
Inspect StrictFinalizedEndpointSelectionV1 and its call site in strict-current-finalized-evm-config.ts. Preserve endpoint selection behavior, but remove future-facing metadata from the returned contract unless a production caller uses it. Tests that need to assert diversity can compute origins locally from the selected endpoints or from an internal typed model.
There was a problem hiding this comment.
Both of the points you added here are taken, in 3d06b644d.
Collapsed into the canonical normalization boundary
normalizeEndpoint now yields { href, origin } — the dial URL and provider identity from one parse — and the selector is policy over that model. Concretely, it now has no input validation, no URL parsing, no error translation and no deduplication, because the states a second validator would defend against cannot reach it. The whole module is the policy plus one unreachable-but-cheap empty guard:
const first = endpoints[0];
if (first === undefined) throw new TypeError(…);
const distinctOrigin = endpoints.slice(1).find((e) => e.origin !== first.origin);
const second = distinctOrigin ?? endpoints[1];
return Object.freeze(second === undefined ? [first.href] : [first.href, second.href]);You were right that the duplicate parse was real: the config parsed each endpoint and the selector parsed it again to derive an origin. That is gone.
The test surface moved with the responsibility, which I think is the more important half. Origin derivation is no longer tested from the selector — asserting it against hand-built { href, origin } records would only restate the rule. It is now proven end-to-end at the config boundary, through the real pipeline: case-insensitive host and path/query/credential variants collapse to one provider, an explicit :443 collapses with the portless form, and non-default ports stay distinct (the discriminating half — a rule that dropped the port would pick the third host instead). The selector suite is now purely the two-slot policy.
Two-slot invariant encoded in code
Answered in detail on your other comment, but in summary: the misleading STRICT_FINALIZED_ENDPOINT_SLOTS_V1 alias is removed, replaced by a literal SLOTS = 2 and a module-load assertion that throws if CURRENT_FINALIZED_EVM_READ_MAX_ATTEMPTS_V1 stops matching it. Your example — ceiling raised to 3, constant reads 3, selector still returns two — is exactly what that prevents, and it now fails at import rather than waiting for a test to notice. The assertion itself is covered by mocking the read profile, because a mutant deleting it survived every ordinary suite.
Verification across this round: chain lane 45 files / 885 passed / 1 skipped, agent precommit suites green, and nine mutants each killed by their named test with byte-identical restores and a green baseline before each.
There was a problem hiding this comment.
🟡 Issue: The endpoint ceiling is modeled as duplicated state plus an import-time side effect
What's wrong
This adds a new module-level runtime trap to enforce a relationship between two constants, then repeats a related bound check in the config layer. That makes the design harder to reason about: changing the read profile now requires knowing about an import-time assertion in a selector module, and tests have to mock module loading to exercise the invariant. The cleaner structure is to keep the ceiling and selection policy owned by one boundary so future changes do not depend on side effects.
Example
If CURRENT_FINALIZED_EVM_READ_MAX_ATTEMPTS_V1 changes from 2 to 3, importing strict-finalized-endpoint-session.ts now fails at module load. That invariant is enforced by a side effect plus a separate caller postcondition instead of being represented by one canonical policy boundary.
Suggested direction
Make the bound unrepresentable instead of guarded twice. For example, put the slot count and selection function in one read-profile policy object, or pass the canonical max-attempt value into the selector and have that helper be the only place that bounds the returned endpoints. Avoid package import failures as an invariant mechanism.
For Agents
Look at packages/chain/src/strict-finalized-endpoint-session.ts and packages/chain/src/strict-current-finalized-evm-config.ts. Preserve the current two-endpoint behavior, but collapse the duplicated invariant into one explicit policy value or selector boundary, then keep the config snapshot output bounded without an import-time assertion. A focused unit test should prove the config never exposes more endpoints than the policy permits.
There was a problem hiding this comment.
Taken — the import-time side effect is gone, replaced by a compile-time assertion. b366f88d0.
const SLOTS = 2;
type AssertPolicyMatchesCeiling =
[typeof CURRENT_FINALIZED_EVM_READ_MAX_ATTEMPTS_V1] extends [typeof SLOTS] ? true : never;
const _assertPolicyMatchesCeiling: AssertPolicyMatchesCeiling = true;CURRENT_FINALIZED_EVM_READ_MAX_ATTEMPTS_V1 is declared = 2, so its inferred type is the literal 2. Raising it makes this a type error and tsc fails the build. No runtime cost, no package that refuses to import, and no test that has to mock module loading — while still satisfying the earlier round's point that the invariant belong in code rather than only in a test.
I verified the guard fires rather than assuming it, and the first version was decorative. I initially wrote const _assert: AssertPolicyMatchesCeiling = undefined as never;. That type-checks against never, so the assertion could never fail — flipping the ceiling to 3 produced no error at all. Assigning true is what makes the mismatch unassignable. Measured:
ceiling = 2 -> tsc clean
ceiling = 3 -> src/strict-finalized-endpoint-session.ts(71,7):
error TS2322: Type 'true' is not assignable to type 'never'.
(then restored byte-identically)
That is the same failure class this PR keeps surfacing, and it is worth being explicit that a type-level guard is just as capable of being vacuous as a test — arguably more so, because nothing runs it.
On the duplication you flagged: what remains is a compile-time invariant (slot count vs ceiling) plus a runtime postcondition on the config's output. I've kept both deliberately, because they answer different questions — one is "is this policy still written against the right ceiling", the other is "did this call actually return no more than the attested number of endpoints". Collapsing them would mean either passing the bound back into the selector, which the earlier round asked me to remove, or dropping the caller-facing guarantee on the attested value. If you'd still prefer one of those, say which and I'll make it — but I'd rather not oscillate between them without a decision.
The now-obsolete module-load test is deleted, and the corresponding mutant with it, since a compile-time guard has no runtime behaviour to mutate.
There was a problem hiding this comment.
🟡 Issue: The endpoint selector is split out before it owns a coherent boundary
What's wrong
The new module is labeled as policy-only, but the real endpoint boundary remains in the config module. That leaves a thin abstraction that exports an internal data shape, depends on undocumented preconditions from its caller, hard-codes the slot count separately, and forces the caller to keep a defensive postcondition. This spreads one small concept across two files without reducing the amount of state a maintainer has to hold.
Example
To understand one endpoint snapshot, a reader now has to follow validation/dedupe in strict-current-finalized-evm-config.ts, the exported intermediate { href, origin } model in strict-finalized-endpoint-session.ts, the hard-coded two-slot selector, and then the postcondition back in config. A future change to endpoint identity or the attempt ceiling has to reason across both modules even though only one production caller constructs this intermediate model.
Suggested direction
Either keep the selector as a private helper beside snapshotNormalizedEndpoints, or move the whole endpoint snapshot pipeline into one cohesive helper such as snapshotStrictFinalizedEndpointSessionV1(input, maxAttempts). The cleaner version should delete the exported intermediate type and the cross-module invariant that currently requires config to trust then re-check the selector.
For Agents
Look at snapshotNormalizedEndpoints, normalizeEndpoint, and selectStrictFinalizedEndpointSessionV1. Preserve the current behavior: validate every raw endpoint, dedupe by normalized href, prefer the first later distinct origin, backfill with the second same-origin URL, and return no more than the finalized-read ceiling. Prove the same shipped-pool and same-origin cases after the restructure.
There was a problem hiding this comment.
Agreed on the substance, deferred to #2071 rather than done here.
Your reading is right: the selector is labelled policy-only while the endpoint boundary lives in the config, so one snapshot spans validation/dedupe, an exported intermediate model, a hard-coded slot count, and a postcondition back in config. That is more places than the concept needs.
I'm not doing it in this PR, and the reason is specific rather than reflexive. This change fixes a live defect — RFC64's finalized-VM precommit cannot construct a snapshot scope on any shipped EVM network — and the endpoint-selection code has already been restructured three times across review rounds here: a generic bound removed, then origin derivation moved into normalization, then the ceiling invariant moved from a runtime alias to a load-time throw to a compile-time assertion. Each restructure was an improvement, and each also introduced a defect that the next round caught (two stale call sites passing raw strings, a decorative type assertion, a dropped credential fixture). A fourth restructure of the same code, for no behavioural change, is where I'd expect the next one to come from too.
#2071 carries your suggested direction verbatim, plus the behaviour that must survive it — every raw endpoint validated rather than only the selected ones, dedupe by normalized href, first later distinct origin preferred, same-origin backfill, ceiling respected, credentials not creating a distinct provider while the dial href stays dialable. All of those are currently pinned by tests and mutants, so the restructure has a falsifiable target rather than a description.
The >1k-line transport suite is filed in the same issue, since it is the same decomposition.
| // (`control-object-signature-verifier.ts:56`). If EIP-1271 is ever given its | ||
| // own literal, this postcondition silently stops protecting the attestation it | ||
| // exists for. | ||
| if (session.selected.length > CURRENT_FINALIZED_EVM_READ_MAX_ATTEMPTS_V1) { |
There was a problem hiding this comment.
🟡 Issue: The attempt ceiling invariant is enforced through a mock-only postcondition in the wrong layer
What's wrong
The config validator now carries an attestation invariant that actually belongs to the strict read execution policy. The branch is unreachable with the real selector, and the accompanying test has to mock the collaborator into an impossible state. That makes the design harder to reason about because the real invariant is split between selector behavior, config postconditions, and a separate attestation constant alias.
Example
If CONTROL_EIP1271_MAX_ATTEMPTS_V1 stops aliasing CURRENT_FINALIZED_EVM_READ_MAX_ATTEMPTS_V1, this config postcondition can still pass while no longer protecting the value the control envelope attests. The code itself calls out that caveat, which is a sign the invariant is not owned in one canonical place.
Suggested direction
Move the ceiling ownership to the strict finalized read profile/runner boundary, or derive both the selector bound and attested value from one canonical strict-read policy. Then the config does not need an unreachable defensive branch whose validity depends on a constant alias staying true.
Confidence note
This is a design-boundary concern rather than a functional failure; the current code does enforce the limit for today’s constants.
For Agents
Follow the attempt-count invariant through strict-current-finalized-evm-config.ts, control-object-signature-verifier.ts, and strict-current-finalized-evm-lifecycle.ts. Preserve the two-attempt production behavior, but make the runner/profile or a shared strict-read policy object enforce and expose the bound used by both endpoint selection and attestation. Replace the mock-only postcondition suite with tests around that canonical boundary.
There was a problem hiding this comment.
Your concrete failure mode is real, but it is already pinned — and I had to correct myself to establish that, so here is the evidence rather than an assertion.
On layering — the config guarding the generic constant is deliberate, not an accident. current-finalized-evm-read-profile.ts states it explicitly: "These limits belong to the generic finalized-read boundary. EIP-1271 is one specialization of this profile; changing signature-verification policy must not implicitly redefine unrelated finalized reads." Importing CONTROL_EIP1271_MAX_ATTEMPTS_V1 into the generic config would invert that dependency — a generic validator taking a signature-verification constant as its authority. So the guard correctly references the generic value, and the attestation reaches it through the alias.
On your example — "if the alias breaks, the postcondition silently stops protecting the attested value". I went to add a test for that and found one already exists: strict-current-finalized-evm-rpc.unit.test.ts:72, "keeps the EIP-1271 specialization pinned to the generic finalized-read profile", which asserts CONTROL_EIP1271_MAX_ATTEMPTS_V1 === CURRENT_FINALIZED_EVM_READ_MAX_ATTEMPTS_V1 at :79-80 along with the whole alias set. I verified it discriminates rather than trusting the name: mutating the alias to its own literal (= 3) kills that test. So the alias cannot break silently, and the caveat comment that prompted your review has been replaced with a pointer to that pin.
(For transparency: my first pass concluded the alias was unpinned and I nearly added a duplicate test. The grep I used was truncated by head -3 and hid the hit. The mutant is what surfaced it — it killed a test I had not written.)
On the unreachable postcondition and its mocked test. The branch is unreachable by design and labelled as such. It exists because the runner never reads maxAttempts — it attempts one endpoint per entry of profile.endpoints (strict-current-finalized-evm-lifecycle.ts:104) — so the attested 2 is truthful only while something bounds that list, and this change removed the rejection that used to bound it. The mock is the only way to reach the branch, and the suite deliberately includes both discriminating cases (at-ceiling accepted, over-ceiling rejected) so it is proved a bound rather than a blanket rejection; a > → >= mutant kills 24 tests.
On moving ownership to the runner/profile. I think that is the right long-term shape and I have not done it here. It touches the attestation boundary and the strict-read lifecycle, which is a materially larger blast radius than this PR — which fixes a live defect that currently prevents RFC64 finalized-VM precommit from constructing a scope on every shipped EVM network. Doing it in the same change would couple a production fix to an architectural refactor of attestation-adjacent code. Happy to open a follow-up issue for the canonical strict-read policy object if you would like that tracked.
131b191 to
aef40c2
Compare
| // Pass 1 — one URL per distinct origin, in configuration order. | ||
| const selected: string[] = []; | ||
| for (const url of ordered) { | ||
| if (selected.length >= CURRENT_FINALIZED_EVM_READ_MAX_ATTEMPTS_V1) break; |
There was a problem hiding this comment.
🟡 Issue: Endpoint selection can skip a higher-priority configured fallback
What's wrong
The new pass prefers distinct origins before filling remaining attempt slots. That means a later endpoint can displace an earlier same-origin URL, even though the transport currently treats the selected array purely as an ordered failover list. This can make a configured backup unreachable and reduce availability for pools that use multiple credentials or paths behind one provider plus a later fallback.
Example
For ["https://lb.provider.io/v2/KEY_A", "https://lb.provider.io/v2/KEY_B", "https://backup.example.com"], selection returns KEY_A and backup, so KEY_B is never attempted. If KEY_A is rate-limited and backup is down, the read fails even though the configured second endpoint could have succeeded.
Suggested direction
Keep failover selection aligned with configured priority, and separate future distinct-origin proof selection from the endpoint list that is actually dialed today.
Confidence note
This assumes the endpoint array remains a failover-priority list for strict finalized reads, as documented by StrictCurrentFinalizedEvmRpcConfigV1 and as consumed by the endpoint runner today.
For Agents
Look at packages/chain/src/strict-finalized-endpoint-session.ts. Preserve the shipped three-origin fix, but decide selection against the current failover contract: either select the first two configured URLs for the transport and carry origin diversity separately for future corroboration, or make the priority inversion explicit at the caller that actually needs corroboration. Add a regression proving an earlier same-origin backup is not skipped for current failover reads unless that is an intentional API contract change.
There was a problem hiding this comment.
Valid, and I took the second of your two suggested directions: the inversion is now explicit and pinned, not removed. Addressed in 83e404aec.
Your example is exactly right — ["https://lb.provider.io/v2/KEY_A", "https://lb.provider.io/v2/KEY_B", "https://backup.example.com"] selects [KEY_A, backup] and never attempts KEY_B, and the transport does treat that array purely as an ordered failover list today. Your confidence note is also correct about the contract.
Why I kept the preference rather than reverting to configured order. KEY_A and KEY_B share a provider, so the dominant failure mode — that provider down, or rate-limiting the account rather than the individual credential — takes both out together and leaves a same-origin pair with no working endpoint at all. A distinct origin is the only selection that survives that. The case your example describes (KEY_A degraded, KEY_B healthy, and backup down) is real but narrower. And the key point: with two slots and three configured endpoints, every possible policy skips some configured URL — selecting the first two would skip backup, which fails the whole provider-outage case. There is no policy here that skips nothing.
So this is a genuine trade, and your criticism that it was implicit was fair. It now has:
- a named regression test,
SKIPS an earlier same-origin URL to reach a distinct provider — stated trade, asserting your exact example - a mutant (
M17) that removes origin preference and takes the first two configured URLs; it kills that test plus three others - the reasoning in the module docblock rather than in a review thread, including what the trade loses
If you would prefer strict configured priority, I will change it — it is a two-line change to the policy and the test. My concern with that direction is that it makes the shipped three-origin fix select [KEY_A, KEY_B] on a pool whose whole point is that they are one provider, which reintroduces the single-point-of-failure this PR exists to avoid on the dialing path, not just the future corroboration path. But it is one decision and it should be made visibly. Tell me which you want.
One thing I want to be straight about: I have not separated "endpoints dialed for failover" from "origins carried for future corroboration", which is your other suggested direction. That is the right long-term shape and it needs the dual-endpoint runner that does not exist yet — holding two endpoints under one scope lease is deferred to the page-transport PR. Doing it here would mean building the corroboration boundary before its consumer, which is what a separate comment on this PR rightly objected to.
aef40c2 to
83e404a
Compare
| }); | ||
| }); | ||
|
|
||
| function plan(): Readonly<Rfc64PublicCatalogNativeBeforeAppliedHeadCommitPlanV1> { |
There was a problem hiding this comment.
🟡 Issue: Extract the copied RFC64 precommit fixtures before adding another full copy
What's wrong
The new test duplicates large, highly structured fixture builders instead of sharing the existing precommit fixture setup. That makes the test file much larger than the behavior it verifies and increases fixture drift risk across the RFC64 suite.
Example
This test only needs to vary rpcEndpoints, but it adds roughly 50 lines of copied RFC64 catalog-scope and policy construction. If the precommit plan or accepted-policy fixture shape changes, several tests now need synchronized edits before the actual shipped-pool assertion can even run.
Suggested direction
Move the plan, accepted policy, and base precommit options into a focused support fixture, then make this test a small scenario around the real shipped pool. That keeps the regression clear without spreading RFC64 fixture internals across more files.
For Agents
Extract shared RFC64 precommit fixture helpers under packages/agent/test/support/, for example rfc64FinalizedVmPrecommitPlan(), acceptedRfc64VmPolicySnapshot(), and rfc64FinalizedVmPrecommitOptions(overrides). Update the existing precommit tests and this shipped-pool test to reuse them while preserving the shipped network pool assertion and fetch dial log behavior.
There was a problem hiding this comment.
Done in 6a1ae8c69 — −161 lines across the two test files, replaced by one shared fixture.
packages/agent/test/support/rfc64-finalized-vm-precommit-fixture.ts now exports the three helpers you named:
rfc64FinalizedVmPrecommitPlan()acceptedRfc64VmPolicySnapshot()rfc64FinalizedVmPrecommitOptions(overrides)
Both suites consume them, so the shipped-pool regression is now what it should always have been — a scenario that varies exactly one field:
const precommit = createRfc64FinalizedVmAgentPrecommitV1(
rfc64FinalizedVmPrecommitOptions({ rpcEndpoints: pool }),
);Your drift argument was the persuasive part: the plan and accepted-policy shapes are structural, and with two copies a change to either needed synchronized edits in both files before either suite's actual assertion could run. That is a maintenance cost paid by whoever changes the shape next, not by me now.
Two notes on the extraction itself, since both are the kind of thing worth stating rather than leaving to be discovered:
rfc64FinalizedVmPrecommitOptionsbuilds a freshOxigraphStoreper call. Hoisting a single shared store into module scope would have been the natural way to write this and would have let state from an earlier test leak into a later assertion. It is a factory for that reason, not by accident.- The existing precommit suite still imports four constants directly (
RFC64_VM_CHAIN_ID,RFC64_VM_CONTEXT_GRAPH_NAME,RFC64_VM_KA_STORAGE,RFC64_VM_ON_CHAIN_CONTEXT_GRAPH_ID) because its noncanonical-input cases assert against them. I left those at the call site rather than re-exporting through the new fixture — they belong to the placement fixture that already owns them, and routing them through a second module would just add a hop.
Caught during the extraction and worth flagging for the repo generally: packages/agent/tsconfig.json has "include": ["src"], so agent test files are not typechecked at all. tsc --noEmit passed cleanly while the refactor had a ReferenceError waiting in it; only running the suite found it. That is not something this PR introduces or should fix, but it does mean typecheck is not a safety net for agent test refactors.
Verification: both precommit suites plus the runtime suite green (11 tests), and the shipped-pool assertions — which endpoints reach fetch, and that the third stays stranded — are unchanged.
83e404a to
6a1ae8c
Compare
| // `maxOrigins` against `NaN` precisely because it is published surface, | ||
| // and leaving the array itself unbounded was the inconsistent half. | ||
| // Generous versus the shipped maximum of three. | ||
| maxLength: STRICT_FINALIZED_ENDPOINT_POOL_MAX_V1, |
There was a problem hiding this comment.
🔴 Bug: Raw endpoint cap rejects configs that previously collapsed to a valid session
What's wrong
The new maxLength is applied to the raw configured array, not to the normalized unique endpoints or the selected session. That turns harmless duplicate or same-origin backup entries into a construction failure, changing the published API from “at most two distinct normalized endpoints” to “at most 32 raw entries” for existing callers.
Example
createStrictCurrentFinalizedEvmReadV1({ chainId: '8453', endpoints: Array.from({ length: 33 }, () => 'https://a.example.com/rpc') }) previously normalized/deduped to one effective endpoint and constructed. With this change it throws TypeError before selection, even though the session would still dial only one endpoint.
Suggested direction
Apply any compatibility-preserving bound after normalization/deduplication or after selection, or make the raw 32-entry cap an explicit public contract change with migration handling. If the goal is only to keep attempts bounded, the selected endpoint list already enforces that.
Confidence note
This is based on the prior validator behavior and the exported public config type; I did not find a documented endpoint-count limit in the public type.
For Agents
Look at snapshotNormalizedEndpoints in packages/chain/src/strict-current-finalized-evm-config.ts. Preserve the new two-attempt/session behavior without rejecting previously valid pools that collapse to a valid selected session. Add a regression case for >32 duplicate or same-origin URLs that should still construct if compatibility is intended, plus a case for the actual upper-bound behavior you want to enforce.
There was a problem hiding this comment.
Confirmed and fixed in 3d06b644d. You are right, and I reproduced it against the built package before changing anything:
33 IDENTICAL urls -> REJECTED (base: dedups to 1, constructs)
20x a + 20x b -> REJECTED (base: dedups to 2, constructs)
32 identical -> ACCEPTED, selects 1
Base deduplicated by href at strict-current-finalized-evm-config.ts:147 before its count check at :153, so those pools normalized to a valid session and constructed. My cap sat on the wrong side of that dedup and turned them into construction failures — exactly the contract narrowing you describe, from "at most two distinct normalized endpoints" to "at most N raw entries". Your confidence note was right too: there is no documented endpoint-count limit in the public type.
The cap is gone entirely, not moved. The only reason it existed was the O(n²) normalized.includes(...) scan, and that motive is now void: dedup is a Set (O(n)) and selection is a fixed two-slot scan, so an oversized array costs one linear pass. Base had no bound at all, so removing it restores base behaviour exactly rather than inventing a new limit.
Regressions added, all at the config boundary:
- 33 identical URLs -> constructs, selects 1
- 40 entries collapsing to two -> constructs, selects both
- and the inverse: an invalid entry inside a 41-element pool still fails closed, so removing the bound did not turn the pool into an unvalidated region — every entry is normalized, not only the selected ones
A mutant that disables the dedup kills the first of those, so the guarantee is pinned rather than asserted.
One consequence worth stating: the { cause } assertion I had was attached to the removed cap's test, so deleting the cap left { cause } uncovered — a mutant caught that, and it now hangs off the empty-array rejection instead.
6a1ae8c to
3d06b64
Compare
| import { resolveRpcUrls } from '../src/evm-adapter-rpc.js'; | ||
| import { snapshotStrictCurrentFinalizedEvmConfigV1 } from '../src/strict-current-finalized-evm-config.js'; | ||
| import { | ||
| STRICT_FINALIZED_ENDPOINT_SLOTS_V1, |
There was a problem hiding this comment.
🔴 Bug: New endpoint-session test imports a symbol the selector module does not export
What's wrong
The newly added suite is wired into the RFC64 unit test list, but this static named import has no matching export in the new selector module. Because ES module imports are resolved before test bodies run, the suite cannot load at all, blocking the regression coverage added by this PR.
Example
Running the new strict-finalized-endpoint-session.unit.test.ts suite will fail during module linking with an error equivalent to: requested module '../src/strict-finalized-endpoint-session.js' does not provide an export named STRICT_FINALIZED_ENDPOINT_SLOTS_V1.
Suggested direction
Align the test and module contract: export the slot constant if it is meant to be asserted externally, or drop the unused import if the load-time guard is the only contract.
For Agents
In packages/chain/src/strict-finalized-endpoint-session.ts, either export the intended slot constant under the imported name or remove the unused import from packages/chain/test/strict-finalized-endpoint-session.unit.test.ts. The chain unit suite should load and run before any endpoint-selection assertions execute.
The selector tests do not respect the selector's own boundary model
What's wrong
The new abstraction says it is policy-only over already-normalized endpoints, but the new test file mixes that with raw RPC URL strings and an import for a slot constant that the module does not export. Even in test code, this weakens the contract the abstraction is trying to establish and makes future readers guess whether the selector owns URL normalization or not.
Example
shippedPool() returns resolveRpcUrls(...), i.e. raw URL strings. Passing that directly to the selector contradicts the selector's documented boundary: validation and origin derivation should already have happened before this function is called.
Suggested direction
Keep selector-only tests on explicit { href, origin } fixtures, and keep real shipped-pool tests at the config boundary where URL parsing and origin derivation are owned. That removes the boundary confusion and the stale constant import.
For Agents
In packages/chain/test/strict-finalized-endpoint-session.unit.test.ts, remove the stale slot import and either drive real network pools through snapshotStrictCurrentFinalizedEvmConfigV1 or map selector-only fixtures to StrictFinalizedEndpointV1 records. Preserve the current assertions about one- and two-endpoint selections.
New endpoint regression suite imports a symbol that is not exported
What's wrong
The added validation evidence is currently not runnable: the test file statically imports a non-existent named export. That blocks the suite before it can verify the oversized shipped-pool behavior this PR is trying to cover.
Example
The chain unit suite will fail during module loading with a missing named export before any of the new endpoint-selection regression tests can run.
Suggested direction
Make the test imports match the module’s actual public surface, or export the slot constant if the test is meant to assert it.
For Agents
In packages/chain/test/strict-finalized-endpoint-session.unit.test.ts, remove the unused import or export the intended constant from packages/chain/src/strict-finalized-endpoint-session.ts; then run the chain unit Vitest lane so the new regression tests actually execute.
There was a problem hiding this comment.
Both confirmed, both mine, both fixed in b366f88d0. These are regressions from my previous round, not pre-existing — when I changed the selector's input to { href, origin } I updated the policy suite but left two older call sites passing raw strings.
Reproduced against the built package before changing anything:
exports: [ 'selectStrictFinalizedEndpointSessionV1' ]
SLOTS exported? false
selector(raw string[]) -> [null] | length 1
toHaveLength(1) would PASS: true <- vacuous
On the stale import: you predicted a module-linking failure. Under Vitest's transform it does not hard-fail — the binding resolves to undefined — which is worse than the failure you described, because the suite loads and passes instead of announcing the problem. The import is removed.
On the single-endpoint case: exactly as you said, and the value is [null] rather than [undefined]. toHaveLength(1) passed on it. It now goes through snapshotStrictCurrentFinalizedEvmConfigV1 and asserts the exact normalized URL, so it can fail. I also swept for other raw-string call sites; there were none left.
What this says about my verification, which is the part worth recording. My chain lane was green and every mutant died — and neither noticed, because a vacuous assertion is invisible to both. A mutant only proves the tests in its kill set are load-bearing; it says nothing about tests that assert nothing. Changing a function's input type is exactly the moment to re-read every call site rather than trust the suite, and I did not.
| ); | ||
| const pool = resolveRpcUrls(cfg.chain.rpcUrl, cfg.chain.rpcUrls); | ||
| expect(pool).toHaveLength(1); | ||
| const selection = selectStrictFinalizedEndpointSessionV1(pool); |
There was a problem hiding this comment.
🔴 Bug: Single-endpoint regression test passes the wrong shape and can pass with [undefined]
What's wrong
This test gives false confidence for the single-endpoint shipped-network case. It feeds raw strings into a selector whose contract is validated endpoint objects, so the selector reads .href from a string. Because the assertion checks only length, a broken result still passes once the import issue is fixed.
Example
With the current selector shape, selectStrictFinalizedEndpointSessionV1(['https://rpc.example'] as any) returns [undefined], and expect(selection).toHaveLength(1) still passes. That would not prove the single shipped endpoint is preserved or dialable.
Suggested direction
Assert the selected endpoint value through the real config boundary, or provide correctly shaped selector inputs and compare exact URLs instead of only checking array length.
Confidence note
This is based on static inspection; Vitest could not be run in this workspace because pnpm is not installed.
For Agents
In packages/chain/test/strict-finalized-endpoint-session.unit.test.ts, either drive the neuroweb case through snapshotStrictCurrentFinalizedEvmConfigV1({ chainId: numericChainId('mainnet-neuroweb'), endpoints: pool }) and assert the normalized URL, or convert the pool into { href, origin } records and assert the exact returned URL.
There was a problem hiding this comment.
Both confirmed, both mine, both fixed in b366f88d0. These are regressions from my previous round, not pre-existing — when I changed the selector's input to { href, origin } I updated the policy suite but left two older call sites passing raw strings.
Reproduced against the built package before changing anything:
exports: [ 'selectStrictFinalizedEndpointSessionV1' ]
SLOTS exported? false
selector(raw string[]) -> [null] | length 1
toHaveLength(1) would PASS: true <- vacuous
On the stale import: you predicted a module-linking failure. Under Vitest's transform it does not hard-fail — the binding resolves to undefined — which is worse than the failure you described, because the suite loads and passes instead of announcing the problem. The import is removed.
On the single-endpoint case: exactly as you said, and the value is [null] rather than [undefined]. toHaveLength(1) passed on it. It now goes through snapshotStrictCurrentFinalizedEvmConfigV1 and asserts the exact normalized URL, so it can fail. I also swept for other raw-string call sites; there were none left.
What this says about my verification, which is the part worth recording. My chain lane was green and every mutant died — and neither noticed, because a vacuous assertion is invisible to both. A mutant only proves the tests in its kill set are load-bearing; it says nothing about tests that assert nothing. Changing a function's input type is exactly the moment to re-read every call site rather than trust the suite, and I did not.
There was a problem hiding this comment.
🔴 Bug: Credential-origin behavior is claimed but not tested
What's wrong
The changed behavior depends on deriving provider identity from URL origin, which intentionally ignores credentials. The test name says credentials are covered, but the fixture does not include any credential-bearing URL, so a regression that treats two credentials on the same host as different providers would still pass green.
Example
A regression that derived provider identity as https://token-a@a.example.com instead of new URL(url).origin would still pass this test, because no credential-bearing endpoint is present. A discriminating case would be ['https://token-a@a.example.com/rpc', 'https://token-b@A.EXAMPLE.COM/other?k=1', 'https://b.example.com'] expecting the first and b, not both credential variants.
Suggested direction
Add an endpoint with username/password or token userinfo to the config-boundary test and assert it collapses by provider origin while keeping the first URL as the dial href.
For Agents
Update the origin-identity tests in packages/chain/test/strict-finalized-endpoint-session.unit.test.ts to include credential-bearing endpoint URLs, either in the existing case or in a separate case. Preserve the expectation that credentials do not create a distinct provider origin, while the selected dial URL still preserves the normalized credential-bearing href.
There was a problem hiding this comment.
Confirmed and fixed in 498df5d29. You are right, and the distinction matters: the behaviour was correct, the test was not.
new URL(...).origin excludes userinfo, so two tokens on one host already collapsed to one provider. But the fixture in a test named "path/query/credential/case variants" contained no credential-bearing URL — the earlier revision had https://user:pw@a.example.com/third and I dropped it when I rewrote the case to route through the config boundary. So the name claimed coverage the fixture did not provide, and your regression would indeed have passed green.
Now uses your discriminating case, and asserts the dial href keeps its credentials:
expect(sel([
'https://token-a@a.example.com/rpc',
'https://token-b@A.EXAMPLE.COM/other?k=1',
'https://b.example.com',
])).toEqual(['https://token-a@a.example.com/rpc', 'https://b.example.com/']);Pinned by a new mutant rather than by the fixture looking right: M20 derives identity as `${origin}#${url.username}` — exactly the regression you describe — and it kills that test alone (1 failed / 58 passed).
That also covers the second half of your point: the selected URL must remain dialable, so it keeps the credential. Collapsing to a bare origin would break the endpoint it selected.
Chain lane after the change: 44 files / 883 passed / 1 skipped.
| * from an earlier case leak into a later assertion. | ||
| */ | ||
| export function rfc64FinalizedVmPrecommitOptions( | ||
| overrides: Record<string, unknown> = {}, |
There was a problem hiding this comment.
🟡 Issue: The shared precommit fixture loses the production options contract
What's wrong
This helper was extracted to centralize a canonical precommit setup, but Record<string, unknown> makes the most important part of that setup untyped. It invites misspelled or wrongly-shaped overrides and hides the real invariant from the fixture API, which is exactly the sort of cast-heavy boundary drift this PR is otherwise trying to avoid.
Example
A caller can write rfc64FinalizedVmPrecommitOptions({ rpcEndpoint: pool }) or override getEvmChainId with a non-function and the fixture helper itself will not express that as a type error at the boundary where the bad override is introduced.
Suggested direction
Use the exported production options type for the fixture boundary, preferably with a narrow typed override shape. That keeps the helper flexible without turning every override into unknown.
Confidence note
This is in test support rather than production code, so the risk is mostly fixture drift and weaker type feedback.
For Agents
In packages/agent/test/support/rfc64-finalized-vm-precommit-fixture.ts, import Rfc64FinalizedVmAgentPrecommitOptionsV1, type the return value as that interface, and type overrides as Partial<Rfc64FinalizedVmAgentPrecommitOptionsV1> or a narrower Pick for the fields tests are meant to vary. Preserve the fresh OxigraphStore per call.
There was a problem hiding this comment.
Taken — b366f88d0.
export function rfc64FinalizedVmPrecommitOptions(
overrides: Partial<Rfc64FinalizedVmAgentPrecommitOptionsV1> = {},
): Rfc64FinalizedVmAgentPrecommitOptionsV1 {Your rpcEndpoint typo example is the right test of it: with Record<string, unknown> that silently produced a fixture with the default endpoint and an ignored extra key, which is precisely the fixture drift the extraction was supposed to prevent. The fresh OxigraphStore per call is preserved.
One caveat worth stating rather than leaving implied: packages/agent/tsconfig.json has "include": ["src"], so agent test files are not typechecked by the package build. This typing gives correct feedback in an editor and to anything that typechecks tests directly, but it is not currently enforced by CI for this directory. That is pre-existing and out of scope here, but it does mean the improvement is weaker in practice than it looks — worth knowing before relying on it.
3d06b64 to
b366f88
Compare
| )).toThrow(TypeError); | ||
| } | ||
| }); | ||
|
|
There was a problem hiding this comment.
🟡 Issue: This pushes the RPC unit test file past 1k lines with config-selection coverage
What's wrong
The PR takes an already large test file from 980 to 1009 lines, crossing the 1k-line threshold, and the added coverage is not actually raw JSON-RPC transport coverage. That makes the suite sprawl further and blurs ownership between transport tests and endpoint configuration policy.
Example
The new SELECTS the first two origins from a larger pool instead of rejecting it test drives snapshotStrictCurrentFinalizedEvmConfigV1 directly, while the PR also adds packages/chain/test/strict-finalized-endpoint-session.unit.test.ts for exactly this endpoint-selection behavior.
Suggested direction
Do the decomposition before landing this. The existing RPC transport suite should stay focused on transport behavior; endpoint selection belongs in the new focused endpoint-session/config suite or a dedicated strict-config test file.
For Agents
Move this selection/config regression into the new endpoint-session/config-focused test file, or split strict config tests out of strict-current-finalized-evm-rpc.unit.test.ts. Preserve the unsafe-config rejection coverage and the three-endpoint construction assertion after moving it.
There was a problem hiding this comment.
Agreed on the substance, deferred to #2071 rather than done here.
Your reading is right: the selector is labelled policy-only while the endpoint boundary lives in the config, so one snapshot spans validation/dedupe, an exported intermediate model, a hard-coded slot count, and a postcondition back in config. That is more places than the concept needs.
I'm not doing it in this PR, and the reason is specific rather than reflexive. This change fixes a live defect — RFC64's finalized-VM precommit cannot construct a snapshot scope on any shipped EVM network — and the endpoint-selection code has already been restructured three times across review rounds here: a generic bound removed, then origin derivation moved into normalization, then the ceiling invariant moved from a runtime alias to a load-time throw to a compile-time assertion. Each restructure was an improvement, and each also introduced a defect that the next round caught (two stale call sites passing raw strings, a decorative type assertion, a dropped credential fixture). A fourth restructure of the same code, for no behavioural change, is where I'd expect the next one to come from too.
#2071 carries your suggested direction verbatim, plus the behaviour that must survive it — every raw endpoint validated rather than only the selected ones, dedupe by normalized href, first later distinct origin preferred, same-origin backfill, ceiling respected, credentials not creating a distinct provider while the dial href stays dialable. All of those are currently pinned by tests and mutants, so the restructure has a falsifiable target rather than a description.
The >1k-line transport suite is filed in the same issue, since it is the same decomposition.
…construct RFC64's finalized-VM precommit could not construct its snapshot scope on ANY shipped EVM network. Measured against the merged validator: testnet pool=3 THROWS -> Strict current-finalized RPC requires 1..2 distinct endpoints mainnet-base pool=3 THROWS -> ... mainnet-gnosis pool=3 THROWS -> ... The finalized-read pool is `resolveRpcUrls(chain.rpcUrl, chain.rpcUrls)` — primary PLUS backups (`dkg-agent-rfc64-catalog.ts:777`) — which is three URLs on every EVM network. `snapshotNormalizedEndpoints` rejects more than `CURRENT_FINALIZED_EVM_READ_MAX_ATTEMPTS_V1 = 2` distinct endpoints, and the precommit passes the pool through unsliced. No test covered it: grepping the error message across the chain and agent suites returned zero hits. The fix is what plan §5.2 always specified — session selection. A new `selectStrictFinalizedEndpointSessionV1` reduces the pool to at most two distinct provider ORIGINS before the ceiling applies, so the ceiling is satisfied by construction rather than by rejection. Provider identity is core's `normalizeEndpointOrigin` — imported, not copied. It is the same predicate `canonicalPageProof` uses to decide `dual-origin-corroborated`, so the two layers cannot drift. (A working revision of this branch forked that rule into chain on the theory that core was not importable; it never reached a commit, so the diff below is purely additive. Core exports the function from the package root and chain already depends on core in 26 files, so the premise was simply wrong.) Note what this is NOT: base never computed an origin at all. `url.href` was the DEDUP key, and that href dedup is still here, unchanged. Origin identity is a coarser layer added on top of it, not a replacement. What inheriting core's rule actually changes, through the only production caller, is the byte bound and the error class below — NOT parsing strictness. The config's `normalizeEndpoint` runs `input.trim()` then `new URL(...)` and returns `url.href`, so the selector only ever sees an already-parsed canonical href; endpoints prefixed with U+00A0/U+FEFF/U+2028/U+3000 all normalize to `https://a.example.com/` and reach selection identically before and after. Those inputs could only have mattered to a direct caller of the exported selector, which is surface this change removes. The attempt ceiling is asserted where it is attested. `CONTROL_EIP1271_MAX_ATTEMPTS_V1` is verified at `current-finalized-evm-call.ts:182`, but it is NOT self-enforcing: the runner never reads `maxAttempts` and instead attempts one endpoint per entry of `profile.endpoints` (`strict-current-finalized-evm-lifecycle.ts:104`). Since this change removes the validator's count rejection, the config keeps an explicit postcondition on the selection result — otherwise an attested "2" would depend on a `>=` inside another module. (A separate constant, `CONTROL_EIP1271_ENDPOINT_ATTEMPT_POLICY_V1`, checked at `:185`, is the policy STRING; an earlier version of this message and both docblocks conflated the two.) The bound is NOT a parameter. Only one caller exists and it always passes the canonical constant, so a caller-supplied bound bought nothing while introducing a reordering failure mode reachable only ABOVE the shipped value — which then needed an explicit ordering repair to fix. Reading the constant directly deletes the parameter, its validation guard, the ordering repair, and that whole failure mode. At a fixed bound of 2 the repair is provably unnecessary: pass 2 runs only when pass 1 selected fewer than the bound, which means the pool held exactly one distinct origin, so pass 1 holds index 0 and pass 2 appends index 1. Ordering is the contract: selection runs AFTER `assertConfigDataProperties` and BEFORE normalization. Before descriptor validation it would re-execute attacker-supplied getters (the hole closed in #2051); after normalization it would never run, because normalization already threw. Both directions have a test and a mutant. Core's predicate is handed the ORIGIN, never the full dial URL. Core bounds its input at the canonical scalar limit, so passing the whole URL imposed a 4096-byte cap that RPC endpoints never had — a config carrying a long path or query credential constructed at base and would have begun throwing. Deriving the origin from the already-parsed URL keeps core as the single source of the origin rule while confining its bound to a string that is always short. Regression-tested in both directions, and a mutant that reinstates the full-URL form dies on it. No bound is placed on the RAW configured array. An earlier revision capped it at 32 entries, which was a compatibility break: base deduplicated by href BEFORE its count check, so 33 identical URLs — or 40 entries collapsing to two — normalized to a valid pool and constructed. A raw cap turns those into a construction failure and silently narrows the contract from "at most two distinct normalized endpoints" to "at most N raw entries". Both cases are now regression-tested, and the performance motive for the cap is gone: dedup is a `Set` (O(n)) and selection is a fixed two-slot scan, so an oversized array costs a linear pass. Endpoint normalization is the single boundary. `normalizeEndpoint` now yields `{ href, origin }` — the dial URL and provider identity from one parse — and the selector is policy over that model. It performs no validation, no parsing and no deduplication, because the states a second validator would defend against cannot reach it. That removed the duplicate URL parse, the defensive branches and the error-translation the selector previously carried. Selection is a TWO-SLOT policy, not a generic selector: first configured URL, then the first later URL with a different origin, else the second configured URL. The bound is fixed, there is one caller, and a caller-supplied bound bought nothing while introducing a reordering failure mode reachable only above the shipped value. The slot count is a LITERAL, not an alias of the attempt ceiling — aliasing would export a value that can change while the behaviour does not — and a COMPILE-TIME assertion fails `tsc` if the ceiling stops matching it. The constant is declared `= 2`, so its type is the literal `2`; raising it makes the assertion a type error. No runtime cost, no import-time side effect, and it reports at the place the mistake is made. Proven by flipping the ceiling to 3 and confirming `tsc` errors, then restoring byte-identically. Preferring a distinct origin CAN skip an earlier same-origin URL — `[KEY_A, KEY_B, backup]` selects `[KEY_A, backup]` and never attempts `KEY_B`. That is a deliberate trade, now pinned by test rather than left implicit: two credentials behind one load balancer share a provider, so a provider outage or account-level rate limit takes both out together and leaves a same-origin pair with nothing working, while a distinct origin survives it. With two slots and three endpoints, every possible policy skips some configured URL. Consequences stated rather than buried: * This WIDENS accepted configs. A 3-URL pool with two same-origin aliases was rejected and now constructs. Asserted deliberately. * Selection backfills spare slots with same-origin URLs rather than shrinking the pool, so a load-balancer config carrying per-key URLs keeps its failover. Callers needing real provider diversity must count `selectedOrigins`, never `selected.length`. This function deliberately exposes NO diversity scalar: an earlier revision returned `distinctOriginCount` computed over the whole pool BEFORE truncation while the module header pointed corroboration consumers straight at it — on every shipped EVM network that reported 3 for a session that dials 2 origins, and `canonicalPageProof` requires EXACTLY two distinct origins. A derived value that can disagree with `selectedOrigins` is a trap, so the array is now the single truth. * `selectStrictFinalizedEndpointSessionV1` is NOT exported from the package root. Every consumer imports it by relative path, so exporting it would have published surface with no caller. * The third shipped endpoint (`*.drpc.org`) is now permanently unreachable by strict finalized reads. Selection is health-blind and configuration-ordered, so a degraded primary is still dialled first. Pinned by assertion in the new agent test so it is a known limitation rather than a silent one. * `/api/status` still reports `resolveRpcUrls(...).length` = 3 while a strict session dials 2. Not reconciled here. This does NOT enable dual-origin corroboration. A scope still pins one endpoint for its lifetime; holding two under one lease needs a runner change, deferred to the page-transport PR so §10.3 is not planned against a capability that does not exist. `strict-current-finalized-evm-rpc.unit.test.ts` had one case asserting a 3-endpoint config throws — that case encoded the defect. It is now a positive assertion that selection happens; its other five rejection cases are unchanged. No `instanceof TypeError` rethrow guard sits in the conversion: core routes every fault through `fail()`, including the `new URL()` parse failure it catches and re-fails, so there is no native `TypeError` path to preserve. A guard for it was dead code — deleting it left the suite green. Element faults are reported as `TypeError`. Core throws `VmUpdateConvergenceError`, which extends `Error`, and the config's translating try/catch closes BEFORE the selection call — so without conversion, core's error escaped a validator whose entire documented contract is `TypeError`, carrying a `vm-update:` message for an RPC-config fault. The pre-existing `toThrow(TypeError)` assertion could not catch this: no fixture in any suite reached the 4096-byte bound. The new case is message-pinned rather than type-pinned for that reason. Only ONE fault class actually reaches that conversion from the config. Chain's own `normalizeEndpoint` catches non-strings, null/undefined, objects and unparseable URLs inside the loop before selection runs, each with its own `TypeError`; measured end-to-end, the byte bound is the only element fault that gets through. So the conversion is defense-in-depth rather than the contract's load-bearing layer — and the message-pinned oversize row is not merely good practice, it is the ONLY end-to-end assertion that exercises the conversion at all. Without it the conversion would be covered solely by direct-selector tests. The conversion is at the core call site rather than a pre-check in chain's own `normalizeEndpoint`, which would avoid the conversion entirely by rejecting oversize endpoints before core sees them. That was considered and rejected: core's `MAX_SCALAR_BYTES` is module-private, so a pre-check must hardcode 4096 and would silently disagree the moment core changes it — reintroducing the exact drift class that importing core's function removes. Converting whatever core decides needs no knowledge of the bound. An empty pool now fails closed too; it was the one input the function did not guard. RFC64 precommit fixtures are shared, not copied. The shipped-pool regression varies exactly one field (`rpcEndpoints`), so the plan, accepted-policy snapshot and base options now live in `test/support/rfc64-finalized-vm-precommit-fixture.ts` and both precommit suites consume them: -161 lines across the two test files. Previously a change to the plan or policy shape needed synchronized edits in two places before either suite's actual assertion could run. Verification: * Chain unit lane 44 files, 882 tests, 1 skipped. A rotating handful of real-timer loopback tests fail under machine load: FIVE different tests observed across runs, each passing on a later run. They are load-dependent rather than lane-dependent — running the file alone does not reliably fix them, so "passes solo" is not the argument. Not attributable here for two independent reasons. First, selection is the IDENTITY function for every config involved, checked individually: three carry a single endpoint, one carries two distinct-port origins, and `applies the two-endpoint ceiling after normalized endpoint deduplication` carries three entries that href-dedup to two distinct origins. None exceeds the bound, so no truncation occurs and the endpoint list is byte-identical before and after. Second, independently reproduced in review: the file passes at `fa9dca15e` (30/30) and at `c557f1a61` (31/31) on one machine minutes apart, which rules the change out directly rather than arguing from timing. Later commits add only input guards and touch no timing path. * Nine mutants at this revision (fifteen across the earlier rounds, two of which targeted code this simplification deletes), each killed by its NAMED test, every restore byte-identical, baseline green before each — plus independent auditor mutants from review. Three assertions added in the final round because a mutant proved they could not fail: the pool cap was one-sided (any value in [3,32] was indistinguishable), the `{ cause }` fix was entirely unasserted (deleting it left the suite green), and the default-port test asserted only `toHaveLength(2)`, which backfill guarantees whether or not the port is part of provider identity. * The runner refuses to start unless the tree is green first. It fired: a twelve-mutant run aborted with "tree not green before mutation" because a concurrent lane had pushed a load-sensitive test to 14.3s. Without it that run would have produced six results indistinguishable from real kills. * Agent seam suites green against a force-rebuilt `dist` (`tsbuildinfo` removed — `composite: true` otherwise skips emit and can leave stale or mutated code in `dist` while reporting success). * New agent test drives the REAL precommit with the REAL shipped testnet pool and stubs only global `fetch`. Fail-before proved by reverting the config to its base body: reverted -> exit 1 with "requires 1..2 distinct endpoints" and zero dials; restored byte-identical -> exit 0, two endpoints dialled. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SaXdkmteqStxGBpXcUPGd1
b366f88 to
498df5d
Compare
otReviewAgent
left a comment
otReviewAgent
left a comment
There was a problem hiding this comment.
Operational Notice: Review Agent could not complete this review.
Business logic reviewer failed: retry_exhausted
Live testnet validation — PASS (9/9 metrics, 3/3 falsifiers)Run This is a feature-confirmation run, not a regression-only one. The defect was reproduced on real shipped config and the fix measured against live RPC providers — every prior proof in this PR used a stubbed
The base contrast was a real build ( Regression surfaces, all clean:
Two things stated rather than glossed:
|
2 participants
Summary
This fixes a live defect, it is not preparatory work. On every shipped EVM network, RFC64's finalized-VM precommit could not construct its snapshot scope at all. The finalized-read pool is
resolveRpcUrls(chain.rpcUrl, chain.rpcUrls)— primary plus backups — which is three URLs on testnet, mainnet-base and mainnet-gnosis, whilesnapshotNormalizedEndpointsrejected more thanCURRENT_FINALIZED_EVM_READ_MAX_ATTEMPTS_V1 = 2. Measured against the merged validator, all three throwStrict current-finalized RPC requires 1..2 distinct endpoints. No test covered it: grepping that message across the chain and agent suites returned zero hits.selectStrictFinalizedEndpointSessionV1reduces the pool to at most two endpoints — distinct provider origins preferred, spare slots backfilled with same-origin URLs so failover is never reduced, then ordered back to configuration order. The attempt ceiling is satisfied by construction rather than by rejection, and is left untouched.Provider identity is core's
normalizeEndpointOrigin, imported rather than copied: the same predicatecanonicalPageProofuses to decidedual-origin-corroborated, so the two layers cannot drift.This does NOT enable dual-origin corroboration. A scope still pins one endpoint for its lifetime; holding two under one lease needs a runner change, deferred to the page-transport PR so §10.3 is not planned against a capability that does not exist.
Behaviour changes, stated rather than buried
> 2rejection removed the only bound, and both dedup and selection scan linearly inside a loop — ~6s at 16k entries).*.drpc.org) is now permanently unreachable by strict finalized reads. Selection is health-blind and configuration-ordered, so a degraded primary is still dialled first. Pinned by assertion in the new agent test so it is a known limitation rather than a silent one./api/statusstill reportsresolveRpcUrls(...).length= 3 while a strict session dials 2. Not reconciled here.Deliberate API decisions
distinctOriginCountcomputed over the whole pool before truncation while the module header pointed corroboration consumers at it — on every shipped network that reported3for a session dialling 2 origins, andcanonicalPageProofrequires exactly two. A derived value that can disagree withselectedOriginsis a trap, so the array is the single truth.selectStrictFinalizedEndpointSessionV1is not exported from the package root. Every consumer imports it by relative path, so exporting it would publish surface with no caller.Related
agent-docs/plans/2026-08-02-w2-update-convergence.md§5.2, §10.2 (local-only)testnet-canary@fa9dca15eFINALIZED_VM_CHAIN_SCAN_MAX_ROWS_V1 = 818assumes the scan owns the whole 1024-batch budget, but the runtime spends one batch on the CG policy read first — safe ceiling is 817.Diagrams
RFC64 finalized-VM precommit → snapshot scope construction
Before:
sequenceDiagram participant Precommit as RFC64 precommit participant Config as snapshotStrictCurrentFinalizedEvmConfigV1 participant Transport as snapshot transport Precommit->>Config: endpoints = [primary, backup1, backup2] Note over Config: 3 > MAX_ATTEMPTS (2) Config--)Precommit: THROW "requires 1..2 distinct endpoints" Note over Transport: never reached — scope cannot be builtAfter:
sequenceDiagram participant Precommit as RFC64 precommit participant Config as snapshotStrictCurrentFinalizedEvmConfigV1 participant Select as selectStrictFinalizedEndpointSessionV1 participant Transport as snapshot transport Precommit->>Config: endpoints = [primary, backup1, backup2] Config->>Config: validate descriptors, normalize each endpoint Config->>Select: normalized pool (all 3 already validated) Select-->>Config: selected = [primary, backup1] (2 distinct origins) Config->>Config: postcondition: selected.length <= MAX_ATTEMPTS Config-->>Precommit: config { endpoints: 2 } Precommit->>Transport: preflight attempt 1 -> primary Precommit->>Transport: preflight attempt 2 -> backup1 Note over Transport: backup2 never dialled (stranded, asserted in test)Files changed
packages/chain/src/strict-finalized-endpoint-session.tsVmUpdateConvergenceErrortoTypeError, preservingcause.packages/chain/src/strict-current-finalized-evm-config.tssnapshotNormalizedEndpoints, after descriptor validation and after per-endpoint normalization. Adds the attested-ceiling postcondition, a 32-entry pool bound, and{ cause }on the pre-existing translating catch.packages/chain/test/strict-finalized-endpoint-session.unit.test.tspackages/chain/test/strict-finalized-endpoint-ceiling-postcondition.unit.test.tspackages/chain/test/strict-current-finalized-evm-rpc.unit.test.tspackages/agent/test/rfc64-finalized-vm-precommit-shipped-pool.test.tsfetch, asserting which endpoints reach the wire.packages/agent/vitest.rfc64-unit-tests.tsTest plan
dist. Ran against code identical to the final commit, differing only in comments (proven by stripping comments and diffing the remainder, plus a check that no@ts-ignore/eslint-disable/v8 ignoredirective was added or removed — a coverage directive would make a strip-and-diff true while changing what the suite measures).dist(tsbuildinforemoved —composite: trueotherwise skips emit:rm -rf distalone emits zero files at exit 0, which is strictly worse than not deleting it)snapshotNormalizedEndpointsto its base body makes the new agent test fail withrequires 1..2 distinct endpointsand zero dials; restored byte-identical, it passes with two endpoints dialled. Reproduced independently in review withdistverified in both directions.{ cause }fix was entirely unasserted, and the default-port test asserted onlytoHaveLength(2), which backfill guarantees whether or not the port is part of provider identity.Pre-existing Windows fixture failure — measured, NOT caused by this PR
packages/agent/test/rfc64-public-catalog-native-gate1.integration.test.tsfails 29/29 locally on Windows. It is not attributable to this PR, established by measurement rather than argument — each run alone on an idle machine, basedistverified before the run (64 emitted files, base marker present, selector file absent), and the process-emptiness check folded into the launch command:131b191b5fa9dca15eTest timed out in 30000ms:159 await setupLiveReceiver()It is one shared fixture cascading 29 times, not 29 independent failures — every failing test dies on its first statement.
setupLiveReceiverstarts two libp2p nodes, opens SQLite persistence and connects peers; it constructs no chain adapter, no strict-finalized config, and references norpcEndpoints. The file is unchanged sincead230c238(2026-07-23), an ancestor of this PR's base, and was already in the lane list at base — so "newly exposed by this PR" is excluded too.It also reproduced slower alone (969s) than in a 35-file lane (926s), which rules out contention as the cause.
CI is unaffected:
ci.ymlhas no agent vitest lane, and CI runsubuntu-latestwhere the WindowsEBUSY-on-unlink teardown fault cannot occur — Linux permits unlinking open files.Known flakiness (not introduced here)
packages/chain/test/strict-current-finalized-evm-rpc.unit.test.tscontains real-timer loopback tests that fail under machine load — five different tests observed across runs, each passing on a later run. They are load-dependent, not lane-dependent, so "passes solo" is not the argument. On an unloaded machine the full lane is green with zero flakes (882 passed / 1 skipped), which settles it empirically. Independently: selection is the identity function for every config involved — checked individually, each carries one endpoint, or two distinct-port origins, or three entries that href-dedup to two, and none exceeds the bound, so the endpoint list is byte-identical before and after this change.