Repository navigation
cluster-tool: pass --slots-in-an-epoch to the run-time anvil at spawn - #111
Conversation
brianjohnson5972
left a comment
brianjohnson5972
left a comment
There was a problem hiding this comment.
Peer review alongside wire-ethereum#216 and wire-sysio#660. Looks good to merge. It is independent of the other two PRs and has no ordering constraint. The fix is correct and well tested. Inline: one FYI about production ETH finality latency, plus two test nits.
| AnvilProcess.StateFilename | ||
| ) | ||
| ), | ||
| slotsInAnEpoch: AnvilProcess.SlotsInAnEpoch |
There was a problem hiding this comment.
This looks right. anvil has no RPC to change --slots-in-an-epoch, so it has to be set at spawn. --block-time correctly stays off the spawn, so the Hardhat deploy keeps instamine, and this now matches the rendered start.sh on the run path.
FYI, not a change request for this PR: the measurement here (tip-to-read 74 s → 15 s, flow 842 s → 563 s) shows how much of each epoch is the single ETH→depot hop, read at finalized. On mainnet finalized trails latest by ~12.8 minutes (two epochs), so in production every depot epoch waits at least one ETH finality period, whatever epoch_duration_sec is set to. Worth flagging to whoever sizes production epochs and the depot's path-2 boundary.
There was a problem hiding this comment.
Noted, thanks. With mainnet's finalized about 12.8 minutes behind latest, the ETH→depot hop alone sets the floor on a production depot epoch, whatever epoch_duration_sec says. That belongs with production epoch sizing and the depot's path-2 boundary, not this harness PR.
| // ProcessManager is a set-once singleton: one sandbox for the whole block. | ||
| beforeAll(() => { | ||
| dir = Fs.mkdtempSync(Path.join(Os.tmpdir(), "anvilsteps-")) | ||
| ProcessManager.setClusterPath(dir) |
There was a problem hiding this comment.
Nit: ProcessManager.setClusterPath is set-once per process. That's fine here because jest gives each test file its own module registry, but a one-line note that this file must keep a single sandbox for the whole describe would stop someone adding a second describe with its own beforeAll from silently inheriting the first path.
There was a problem hiding this comment.
While @brianjohnson5972 is correct, this approach has been ignored by most of the engineers contributing to this repo; it may lead to random errors, if said errors arise, jest config cleanup could be helpful
There was a problem hiding this comment.
The block carries that note: "ProcessManager is a set-once singleton: one sandbox for the whole block." Agreed on jest config cleanup if this starts causing errors.
| it("spawns anvil with the finality window and without interval mining", async () => { | ||
| const ctx = fixtureContext({ clusterPath: dir, dataPath: Path.join(dir, "data") }), | ||
| start = jest.fn(), | ||
| anvil = { start } as Partial<AnvilProcess> as AnvilProcess, |
There was a problem hiding this comment.
Nit: { start } as Partial<AnvilProcess> as AnvilProcess (here and at line 76) is fine in a test. If jest.mocked / a typed stub helper already exists in the repo's test utilities, prefer it so the double cast doesn't spread.
There was a problem hiding this comment.
This is bad code and breaks the style guidelines
There was a problem hiding this comment.
Fixed in e9365fd. Neither test casts now: "spawns anvil…" spies AnvilProcess.create through to the real factory with /bin/true as the binary and spies start on the prototype, so a real AnvilProcess comes back and nothing spawns. The no-op case registers a real process in place of mocking get. afterEach stops the singleton manager's processes.
a0b6f0b to
0613555
Compare
0613555 to
22a07e7
Compare
|
@brianjohnson5972 two commits landed after your approval on
|
`AnvilProcessSteps.runStart` spawned anvil with only host/port/chain-id/state and switched interval mining on afterward over RPC. anvil has no RPC for the epoch slot count, so every `create` and flow cluster ran on anvil's default 32-slot epoch: `finalized` sat 64 blocks behind `latest`, and the batch operators — which read the outpost's outbound envelope at `finalized` — only saw each ETH envelope a minute after the tip that emitted it. Every depot epoch stretched to about two minutes waiting on that one hop. Ride `AnvilProcess.SlotsInAnEpoch` on the spawn (finality two blocks behind), exactly as the rendered start.sh already does for the `run` path, and keep `--block-time` off the spawn so the Hardhat deploy keeps its instamine. Co-Authored-By: Claude Fable 5.1 <[email protected]>
…he Solana envelope log allows The underwriter verifies each request's statement against the Solana outpost's outbound envelope log, which keeps the last 10 epochs (`MAX_TRACKED_ENVELOPES` in liqsol-core). The second request is bonded only after the first bond is paid back: a 589 s challenge window plus the approve, claim and bond passes after that bond. At 60 s epochs the window alone is about ten epochs, so request 2's statement can leave the log first. It then reads UNVERIFIED on every pass and the flow times out on `second-bonded`. Whether that happens depends on where a 15 s plugin pass falls inside an epoch. Two runs show it, both with epochs at the configured 60 s: - e2e 39447579858, which failed: the pass came 15 s after epoch 13 pruned epoch 3. - e2e 39479148320, sysio master and ethereum next, which passed: the pass came in the same second as epoch 13. On master the flow looked stable only because the harness's anvil finality lag stretched epochs to about two minutes. The previous commit, which passes `--slots-in-an-epoch` to anvil, removes that lag. The flow now runs on its own epoch, derived as the shortest one at which the window (one epoch plus its fixed 529 s), the three passes and one epoch of headroom fit inside the log. That is 72 s, which makes the first window 601 s. Slower epochs only add margin, since the log counts epochs, not seconds. The window's total with the two later 60 s windows is 721 s, so the old `<= 3 * 240` assertion is replaced by tests that pin: - the retention margin at the derived epoch; - that one second less, or 60 s, fails; - that the scenario runs on the flow's epoch. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…nstead of casting a stub
Both runStart cases passed a `{ start } as Partial<AnvilProcess> as
AnvilProcess` stub. Now:
- "spawns anvil…" spies `AnvilProcess.create` through to the real factory
with `/bin/true` as the binary, and spies `start` on the prototype so
nothing spawns.
- "is a no-op when already registered" registers a real process in place
of mocking `get`.
- `afterEach` stops the singleton manager's processes.
The spawn-option assertions are unchanged.
Co-Authored-By: Claude Opus 5.5 <[email protected]>
e9365fd to
59be394
Compare
valthon
left a comment
valthon
left a comment
There was a problem hiding this comment.
Requesting changes for two required corrections:
-
Issue the new runner test's Anvil port through the bind registry. In
packages/cluster-tool/tests/orchestration/steps/processes/AnvilProcessSteps.test.ts:56,fixtureContext()suppliesDefaultAnvildirectly. The test passes that port through to the real factory and asserts on it; supplyingoptions.portbypassesBindConfigProvider.findAvailable(). The repository explicitly requires registry-issued ports in tests, including mocked spawns and assertions. Allocate the port throughBindConfigProvider.findAvailable()and override the fixture's Anvil binding with it. -
Correct the contradictory AnvilProcess class documentation. In
packages/cluster-tool/src/cluster/processes/AnvilProcess.ts:37-41, the class comment still says bothslotsInAnEpochandblockTimeSec“MUST NOT be set during the deploy phase.” This PR correctly setsslotsInAnEpochduring that phase. Update the comment to explain that finality is configured at spawn, while interval mining remains off until after deployment.
Strong recommendation for this revision: consolidate the cluster-derived Anvil configuration while revising the runner tests. The options are repeated in AnvilProcessSteps.runStart, ClusterManager, and StartScriptSteps.resolveAnvilConfig, and DaemonConfig.planAnvil reapplies the finality/mining settings. That duplication allowed create/flow startup to omit the finality setting while restart and generated scripts already supplied it. Share the cluster-to-Anvil option derivation with an explicit deployment/relaunch distinction controlling interval mining, and have the startup and script paths consume it. Add a parity test covering their common settings and the intentional mining difference. This would prevent the same drift from recurring; the port and documentation corrections above remain required independently.
Coverage: read every changed line in all 7 files at 59be394c5b88cc1e6329b007bb12e021e5494e5b, plus related startup/configuration and Solana retention/underwriter paths. The 4 focused Anvil step tests passed locally using existing dependencies; GitHub build-and-test checks are green. Full local gates and live flows were not rerun.
…d registry The runStart tests passed the persisted fixture's `DefaultAnvil` into a real `AnvilProcess.create` and asserted on it, so the port never came from the registry. The block now takes it from `BindConfigProvider.findAvailable` and binds the fixture's anvil to it. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…ry launch The class comment still said neither `slotsInAnEpoch` nor `blockTimeSec` may be set during the deploy phase, which this branch's runStart change contradicts. Only interval mining stays off for the deploy; finality rides every spawn. Co-Authored-By: Claude Opus 5.5 <[email protected]>
jglanz
left a comment
jglanz
left a comment
There was a problem hiding this comment.
@qhool there are 2 standards issues related to packages/flow-syndication-plugin-underwriting-cap/tests/SyndicationPluginUnderwritingCapSettlement.test.cjs
- first, no tests are allowed under
flow-*packages as each flow is a test - second, you are not using
jest, the repo standard (your codeconst { test, afterEach, mock } = require("node:test"))
Due to the time pressure, I am approving, but I've been assured these will be resolved ASAP
…oy or relaunch Four paths built the cluster's anvil options separately: - `AnvilProcessSteps.runStart`, the create and flow spawn; - `ClusterManager`'s run path; - `StartScriptSteps.resolveAnvilConfig`; - `DaemonConfig.planAnvil`, which reapplied the finality and mining flags. That is how the create path came to miss the finality flag the others carried. `AnvilProcess.optionsFor(config, AnvilProcess.Launch)` now derives host, port, chain id, state file and finality from the cluster config. Only interval mining depends on the launch: `deploy` leaves it off for the Hardhat deploy, `relaunch` spawns with it. runStart uses `deploy`; ClusterManager and resolveAnvilConfig use `relaunch`; planAnvil renders the config it is given. The binary stays with each path, as before: PATH for a live spawn, the persisted executable for a start.sh. Tests: optionsFor's parity (the launches differ only in `blockTimeSec`, in options and in argv), runStart's options, resolveAnvilConfig's relaunch form, and the planned anvil's argv carrying both flags. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…poch derivation and shared-verify coverage move to cluster-tool
A flow is itself the test, so a flow package carries no unit tests, and
unit tests run under jest. This flow's `node:test` suite is removed with
its `test:unit` script.
- `BatchOperatorUnderwritingSteps.shortestRetainingEpochSec` and
`SolanaRetainedEnvelopeEpochs` (mirroring liqsol-core's
`MAX_TRACKED_ENVELOPES`) move to cluster-tool. The flow's constants call
them, and its epoch is still 72 s with a 601 s first window. Jest
covers the derivation: the result fits with its headroom, one second
less does not, and the 60 s shortest epoch is refused for a window about
the log's own depth.
- The suite's cases for the shared verifies move to cluster-tool's
`BatchOperatorUnderwritingSteps.test.ts`:
- `planVerifyBonded` passes a funded-only bond, refuses an unfunded
bonder, and refuses unpaid bonds over the cap.
- `planVerifyFirstRelease` accepts the second envelope released not at
all or in full, and refuses a partial release.
The unfunded-holds case was already covered there.
- The rest exercised this flow's own `CapSteps`, which the flow run
covers, and is dropped.
- The `sysio.bond`/`sysio.synd` row builders move from
`WireSyndicationTool.test.ts` into `tests/tools/wire/syndicationRowFixture.ts`,
shared by both suites.
Co-Authored-By: Claude Opus 5.5 <[email protected]>
|
@valthon thanks. All three are in, pushed as
Separately, per the repo rule that a flow package carries no unit tests and unit tests run under jest ( Gate on |
|
E2E on the rebased calldata set, both green:
|
Summary
Two harness changes found while running the whole-envelope delivery branches end to end.
1. Pass
--slots-in-an-epochto the run-time anvil at spawn (22a07e71).AnvilProcessSteps.runStartspawned anvil with only host/port/chain-id/state and switched interval mining on afterwards over RPC. anvil has no RPC for the epoch slot count, so everycreateand flow cluster ran on the default 32-slot epoch:finalizedsat 64 blocks behindlatest, and the batch operators — which read the outpost's outbound envelope atfinalized— only saw each Ethereum envelope a minute after the tip that emitted it. Every depot epoch stretched to about two minutes waiting on that one hop.AnvilProcess.SlotsInAnEpochnow rides the spawn (finality two blocks behind), exactly as the renderedstart.shalready did for therunpath;--block-timestays off the spawn so the Hardhat deploy keeps instamine andplanEnableIntervalMiningstill turns mining on afterwards. Measured onflow-operator-collateral-deposit: tip-to-read 74s → 15s, flow 842s → 563s. Runner tests added inAnvilProcessSteps.test.ts.2.
flow-syndication-plugin-underwriting-capruns on 72 s epochs (1bce3e4d; the derivation moved to cluster-tool in07714858).With epochs at their configured 60 s, this flow (added in #127) can fail on
second-bonded. The batch-operator underwriter verifies each request's statement against the Solana outpost's outbound envelope log, which keeps the last 10 epochs (MAX_TRACKED_ENVELOPESin liqsol-core). The second request is bonded only after the first bond is paid back: a challenge window plus the approve, claim and bond passes after that bond. The window was 589 s, about ten 60 s epochs, so the second request's statement could leave the log first; the underwriter then reports itUNVERIFIEDon every pass.Whether it did depended on where a 15 s plugin pass fell inside an epoch:
UNVERIFIEDmaster, ethereumnext, this PR's anvil commit onlyOn
masterthe flow looked stable only because the anvil finality lag above stretched epochs to about two minutes.The flow now derives its own epoch: the shortest at which the window (one epoch plus its fixed 529 s), the three passes and one epoch of headroom fit inside the 10-epoch log. That is 72 s, which makes the first window 601 s and leaves about 74 s before the statement is pruned; slower epochs only add margin, since the log counts epochs. The derivation is
BatchOperatorUnderwritingSteps.shortestRetainingEpochSecin cluster-tool, withSolanaRetainedEnvelopeEpochsmirroring the Solana constant, and jest pins it: the result fits with its headroom, one second less does not, and 60 s is refused. The README explains the epoch.The durable fix is in the underwriter (wire-sysio #671): remembering a statement once verified would make the flow safe at any epoch length.
Gate on
cf76628b: lint, build, jest 4471 tests in 456 suites, everytest:unit(the cap flow's 50 included), and the pre-commit check — all green.3. Runner tests without casts (
59be394c). The tworunStarttests build a realAnvilProcess(the real factory with/bin/true,startspied on the prototype) instead of aPartial<AnvilProcess>double cast.4. valthon's review.
d090492f: therunStarttests take their Anvil port from the bind registry.d26eb80c:AnvilProcess's class comment says finality rides every spawn and only interval mining stays off for the deploy.d38fe72f:AnvilProcess.optionsFor(config, AnvilProcess.Launch.deploy | relaunch)is the one derivation of a cluster's Anvil options.runStartusesdeploy;ClusterManager's run path andStartScriptSteps.resolveAnvilConfiguserelaunch;DaemonConfig.planAnvilno longer reapplies the flags. Parity and consumer tests in jest.5. No tests in the flow package (
07714858). The cap flow'snode:testsuite andtest:unitscript are removed. Its cases on the shared verifies (planVerifyBonded,planVerifyFirstRelease) are ported to cluster-tool'sBatchOperatorUnderwritingSteps.test.ts; the ones on the flow's ownCapStepsare dropped, since the flow run covers them. Thesysio.bond/sysio.syndrow builders are now a shared test fixture.Gate on
07714858: lint, build, jest 4516 tests in 457 suites, everytest:unit, and the pre-commit check.Companion PRs
E2E gate
e2e-tests run 39640361829:
success. Full platform build plus every discovered flow, dispatched withBRANCH_WIRE_SYSIO,BRANCH_WIRE_ETHEREUMandBRANCH_WIRE_TOOLS_TS=feature/eth_opp_calldata_delivery, everything else on its manifest branch. Resolved revisions: wire-sysiod7a87bc39a, wire-ethereum824bced09f, wire-tools-ts59be394c, wire-libraries-tsf0e5773142, wire-solanad4b1d64a99.All 14 flows passed. The review fixes after
59be394c(d090492f..07714858) touch the anvil spawn paths and the cap flow, so a smoke run on07714858covered those two: 39650122308,success(operator-collateral-deposit 505s, syndication-plugin-underwriting-cap 1404s).🤖 Generated with Claude Code