scripts: load generator for arctic-1 and atlantic-2 - #3850
Conversation
PR SummaryMedium Risk Overview The runner covers capture/validate, schema-v4 fixture deploy (canonical SushiSwap V2 plus deterministic DeFi-shaped contracts), worker provisioning, timed/buffered continuous replay, TPS and gas/calldata caps, and Prometheus/Grafana observability. Pacific traffic is classified and translated (semantic swaps/lending/staking/etc. onto fixtures; unknown EVM via Hardhat compiles multi-version Solidity (0.6.12 Sushi + 0.8.x fixtures), ships GPL third-party notices, and adds fixture tests for Sushi V2, synthetic CREATE/CREATE2, and delegatecall/callback paths. Reviewed by Cursor Bugbot for commit 078e86a. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 089374e. Configure here.
| callType > 4 || | ||
| (i == 0 && depth != 0) || | ||
| (i != 0 && depth > previousDepth + 1) | ||
| ) revert MalformedSpec(); |
There was a problem hiding this comment.
Call graph bounds mismatch
Medium Severity
CallGraphHarness caps depth at 8 and frames at 64, but capture config allows TRACE_MAX_DEPTH up to 32 and TRACE_MAX_FRAMES up to 256. encodeCallGraphHarness clamps depth with Math.min(8, ...) instead of truncating, which flattens deep trees into invalid specs that revert in _children, and oversized frame lists are still submitted instead of falling back to ProfileLoadHarness.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 089374e. Configure here.
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
Well-structured, thoroughly documented standalone load-generator package with no production code changes and good defensive design (dry-run defaults, manifest/bytecode verification, bounded gas/calldata/value). One blocking safety gap: the replay executor never verifies the target Cosmos chain ID, so a misconfigured TARGET_COSMOS_RPC can broadcast load to an unintended network despite the README claiming otherwise.
Findings: 1 blocking | 13 non-blocking | 8 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- The Cursor second-opinion review file (
cursor-review.md) is empty — that pass produced no output. Codex's review had two findings, both confirmed and reflected below. - Nothing in CI runs this package.
.github/workflows/and theMakefilecontain no reference tointegration_test/load_generator, sonpm run typecheck,npm test(mocha), andnpm run test:fixtures(hardhat) are never executed automatically. For ~5k lines of new TS plus a 720-line spec suite, the tests will silently rot. Consider a workflow gated onpaths: integration_test/load_generator/**. README.md:9states "EVM and Cosmos chain IDs, user/deployment manifests, and deployed bytecode are verified before submission." That is accurate forreplay:usersbut not forreplay:run— update the claim (or, preferably, close the gap flagged inline inrunReplay.ts).- I could not execute the suite in the review environment (
node_modulesabsent, deps not installed), so the assertions intest/core.spec.ts,test/corpusCleanup.spec.ts, andtest/observability.spec.tswere reviewed by reading only. I did verify the hand-rolledSetCodeTx/SetCodeAuthorizationprotobuf decoders inevmCorrelation.tsfield-by-field againstproto/eth/tx.proto, andassociation.tsagainstproto/evm/query.proto— all field numbers and wire types match. traceCapture.tsnormalizeCallTraceboundsvisitrecursion bymaxDepth, but thecountSubtreehelper it calls on truncation recurses over the entire remaining subtree with no depth bound. A pathologically deep captured call trace could overflow the stack and fail a whole segment. Low likelihood; an explicit depth cap or iterative walk would remove it.- Consider adding a test for the Cosmos adapter (
cosmosAdapters.ts).test/core.spec.tscovers the EVM adapters, config parsing, trace normalization, scheduling, and correlation well, butbuildCosmosReplay— including privileged-pattern detection and theMsgSendamount-bounding fallback — has no unit coverage. - 7 suggestion(s)/nit(s) flagged inline on specific lines.
|
|
||
| const provider = new ethers.JsonRpcProvider(target.evmRpcUrl); | ||
| provider.pollingInterval = 200; | ||
| await verifyTargetRpc(target, provider); |
There was a problem hiding this comment.
[blocker] The target Cosmos chain ID is never verified before the Cosmos lane broadcasts.
verifyTargetRpc only checks evmChainId against the EVM provider. The Cosmos lane goes through cosmosClient() (line 937), which calls SigningStargateClient.connectWithSigner(rpcUrl, ...) — cosmjs auto-detects the chain ID from the node and signs with it, so there is no local mismatch guard anywhere on this path.
That this is an oversight rather than a decision is clear from provisionUsers.ts:60-66, which does exactly the missing check:
const cosmosChainId = await admin.getChainId();
if (cosmosChainId !== target.cosmosChainId) { /* refuse */ }and from README.md:9, which promises "EVM and Cosmos chain IDs … are verified before submission."
Failure scenario: an operator sets TARGET_NETWORK=arctic-1 but leaves a stale/typo'd TARGET_COSMOS_RPC pointing at a pacific-1 endpoint (the two default hostnames in config.ts differ by one label). The EVM check passes against the correct arctic-1 EVM RPC, so startup succeeds; every Cosmos-lane transaction — bank sends and shaped privileged traffic — is then signed for pacific-1 and broadcast to mainnet, funded by the same mnemonic. Because MAX_TPS defaults to 25 and runs default to 2 hours, this is sustained rather than a single stray tx.
Suggest asserting the Cosmos chain ID once at startup, next to verifyTargetRpc, so the run refuses to begin rather than discovering the mismatch per-worker.
| await source.verifyChain(); | ||
| await fs.mkdir(OUTPUT_DIRECTORY, { recursive: true }); | ||
| const existingManifest = await readOptionalJson<CaptureManifest>(MANIFEST_PATH); | ||
| if (existingManifest?.complete) { |
There was a problem hiding this comment.
[suggestion] This early return short-circuits validateCaptureManifest() and never compares the manifest's range against START_BLOCK/END_BLOCK.
A complete: true manifest is accepted unconditionally, so reusing a REPLAY_DIR after changing MAINNET_RPC/COSMOS_RPC, SEGMENT_BLOCKS, or the requested block range silently reports "Capture already complete" and exits 0 — downstream replay:run then replays stale data captured under different settings. The incomplete-manifest branch below (line 94-97) has the inverse problem: it validates the source/segment size but then adopts manifest.source.firstBlock/lastBlock, discarding a changed START_BLOCK/END_BLOCK without warning.
Suggest calling validateCaptureManifest(existingManifest) before the complete check, and rejecting (or at least logging) when an explicit START_BLOCK/END_BLOCK disagrees with the recorded range.
| params: [address, toHex(blockTag)], | ||
| })); | ||
| const responses = await this.rpcBatch<string>(requests); | ||
| responses.forEach((response, index) => { |
There was a problem hiding this comment.
[suggestion] Batch responses are correlated to requests by array position, but nothing guarantees the array is complete.
rpcBatch sorts by id and checks for error entries, but never asserts body.length === requests.length. If an endpoint or intermediary proxy omits a single entry, responses[index] no longer lines up with batch[index], and every subsequent address gets the next address's code hash — silently written into the segment file with no error. attachCreationRuntime (line 417) has the same shape, mis-assigning deployedRuntimeCodeBytes/deployedRuntimeCodeHash across contract creations.
Those values feed adapter routing (recipientCodeHash picks the ERC4626 target in evmAdapters.ts:381-390) and encodeSyntheticCreationHarness, so corruption is silent and persistent in the corpus.
Suggest either validating the length in rpcBatch, or keying results by response.id (each request's id is already captured in the request objects) instead of relying on positional alignment.
| const isUnmatched = record.fidelity !== 'semantic'; | ||
| if (isUnmatched) this.unmatched++; | ||
| const line = `${JSON.stringify(record)}\n`; | ||
| this.queue = this.queue.then(async () => { |
There was a problem hiding this comment.
[suggestion] A single appendFile failure permanently poisons this promise chain.
this.queue is reassigned to the chained promise with no .catch, so once it rejects, every later record() returns a promise that rejects without running its callback, and flush() rejects forever. Since record() is awaited in the hot loop (runReplay.ts:423, 472) and again in the finally block (line 565), one transient ENOSPC/EMFILE aborts the whole replay run and the finally rejection can mask the original error.
Suggest terminating the chain with a .catch that logs and increments a dropped-records counter, so audit-write failures degrade the audit trail rather than the run:
this.queue = this.queue.then(async () => { /* ... */ }).catch(error => {
console.error('Bucket audit write failed:', error);
});Related nit: initialize() writes to auditPath/unmatchedPath without mkdir -p on the parent, so a user-supplied BUCKET_AUDIT_PATH in a non-existent directory fails at startup (the default path works only because replayDirectory already exists).
| targetTransactionBytes, | ||
| }); | ||
| } catch (error) { | ||
| if (error instanceof TimeoutError) { |
There was a problem hiding this comment.
[suggestion] A timed-out Cosmos broadcast is counted as both submitted and rejected.
The comment correctly notes the node accepted the transaction and only the poll window elapsed, but control falls through to metrics.rejected++ / recordOutcome('cosmos', 'rejected') on line 873-874, and the audit record is written with outcome 'rejected'. So submitted + rejected double-counts these, and the rejected metric — the main signal for "the target chain refused our load" — is inflated by transactions that were in fact accepted.
Suggest an early return after recording submitted (plus a distinct timeout outcome or reason in the audit record) so the two cases stay distinguishable on the dashboard.
| return body.sort((a, b) => a.id - b.id); | ||
| } catch (error) { | ||
| lastError = error; | ||
| await retryDelay(attempt); |
There was a problem hiding this comment.
[nit] retryDelay(attempt) also runs after the final attempt, so a permanently failing endpoint sleeps an extra ~15s before throwing (and again in cosmosRpcBatch, line 665). traceRpc gets this right with if (attempt + 1 < this.traceMaxRetries) — worth matching that guard here for consistency. With maxRetries defaulting to 10 this adds a pointless delay to every hard failure.
| "version": "1.0.0", | ||
| "private": true, | ||
| "description": "Capture Pacific traffic and safely replay equivalent load on Sei testnets.", | ||
| "license": "MIT", |
There was a problem hiding this comment.
[nit] "license": "MIT" declares a single license for a package that ships GPL-3.0-only Solidity sources under contracts/uniswapv2/ and contracts/mocks/WETH9.sol, plus their compiled artifacts under vendor/sushiswap-v2/.
THIRD_PARTY_NOTICES.md documents the situation well and the package is private: true (so never published), which limits the practical impact. Still, the metadata contradicts the notices file. Consider "license": "SEE LICENSE IN THIRD_PARTY_NOTICES.md" — or dropping the field entirely for a private package — so automated license scanners don't record this tree as pure MIT.
| minBufferMinutes: positiveNumber(env, 'MIN_BUFFER_MINUTES', 5), | ||
| resumeBufferMinutes: positiveNumber(env, 'RESUME_BUFFER_MINUTES', 20), | ||
| metricsPort: nonNegativeInteger(env, 'METRICS_PORT', 9465), | ||
| metricsHost: string(env, 'METRICS_HOST', '0.0.0.0'), |
There was a problem hiding this comment.
[nit] METRICS_HOST defaults to 0.0.0.0, so the Prometheus endpoint binds every interface by default. The README explains this is needed for the bundled host.docker.internal scrape, and the endpoint is read-only, so this is a reasonable trade-off — but 127.0.0.1 as the default (documented as "set to 0.0.0.0 when using the bundled Docker stack") would be the safer direction, especially since these runs happen on shared/remote load-generation hosts.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3850 +/- ##
==========================================
- Coverage 61.74% 60.66% -1.08%
==========================================
Files 2381 2269 -112
Lines 201667 188665 -13002
==========================================
- Hits 124513 114460 -10053
+ Misses 66074 64127 -1947
+ Partials 11080 10078 -1002
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
A large, self-contained Pacific→testnet replay load generator added under integration_test/load_generator/; no production chain code is touched, the safety posture is good (EXECUTE gating, chain-id verification on both lanes, bounded gas/value/calldata, privileged-message filtering, GPL provenance documented), and there is real unit coverage. Findings are non-blocking correctness/hygiene issues — deadline overshoot in paced replay, capture-manifest reuse validation gaps, a CREATE2 salt that collides when the same corpus is replayed twice, MAX_TPS being a pre-flight check rather than a runtime limiter, and no CI job that actually runs the new tests.
Findings: 0 blocking | 15 non-blocking | 10 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- No CI wiring: nothing in
.github/workflows/or the rootMakefilereferencesintegration_test/load_generator, sonpm test(~1000 lines of new mocha specs),npm run typecheck, andhardhat compilenever run in CI. Consider a small workflow gated on path filters so these specs don't silently rot. - The Cursor second-opinion pass (
cursor-review.md) produced no output — this review merges only my findings with Codex's. MAX_TPSreads like a runtime rate limit but is only a pre-flight check over the already-captured corpus (validatePeakTps); at runtime the only bound isWORKER_COUNT * MAX_PENDING_PER_LANEin-flight, with overflow dropped. Worth stating explicitly inREADME.md/.env.exampleso operators don't size runs assuming a throttle.package.jsondeclares"license": "MIT"while the repo root is Apache-2.0 and this same package tree ships GPL-3.0 sources undercontracts/uniswapv2/andcontracts/mocks/WETH9.sol. The package isprivate: trueandTHIRD_PARTY_NOTICES.mdcorrectly disclaims relicensing, so this is only a metadata inaccuracy — but it's the one machine-readable license statement in the package.runBuffered.tsdefaults (BUFFER_START_MODE=latest,CLEANUP_CONSUMED_SEGMENTS=1for buffered mode) mean a plainnpm run replay:start:bufferedrenames the existing corpus to<dir>-archive-<ts>and then deletes every segment file in it. That's re-capturable data and it is logged, but a first-time operator reusing the defaultREPLAY_DIRloses a prior capture; consider requiring an explicit opt-in for the delete step.- 10 suggestion(s)/nit(s) flagged inline on specific lines.
| firstTimestamp ??= block.timestamp; | ||
| const targetElapsedMs = | ||
| ((block.timestamp - firstTimestamp) * 1_000) / TIME_SCALE; | ||
| await sleepUntil(replayStartedAt + pausedMilliseconds + targetElapsedMs); |
There was a problem hiding this comment.
[suggestion] sleepUntil is unbounded by runDeadline, and the deadline is not re-checked after it returns. The check at lines 400-405 happens before the sleep, so a large source timestamp gap (source-chain stall) or TIME_SCALE < 1 lets a RUN_DURATION_SECONDS-bounded run sleep well past its deadline and then still submit the entire next block plus wait for inclusion at line 525. sleepBounded(..., runDeadline) already exists and is used on the buffer-pause paths — using it here and re-testing the deadline afterwards would make the bound hold. (Also raised by Codex.)
| gasBurn, | ||
| initcodeBytes, | ||
| useCreate2, | ||
| ethers.zeroPadValue(ethers.toBeHex(seed), 32), |
There was a problem hiding this comment.
[suggestion] The CREATE2 salt is derived purely from seed, which is context.sequence — a per-process counter starting at 0. The synthetic initcode is a pure function of runtimeBytes/requestedInitcodeBytes, so replaying the same corpus a second time against the same testnet reproduces identical (harness, salt, keccak(initcode)) triples. create2 then returns address(0) because the account already has code, and SyntheticCreationHarness.deploy reverts with CreationFailed. Every creation-shape transaction whose source used CREATE2 fails on any re-replay (REPLAY_FROM_START=1, or a fresh run over a retained corpus), silently degrading into included_failed rather than an obvious error. Mixing something run-unique into the salt (the worker address, the audit run id, or block.prevrandao inside the harness) would make it idempotent across runs.
| ); | ||
| } | ||
|
|
||
| function validateCaptureManifest(manifest: CaptureManifest): void { |
There was a problem hiding this comment.
[suggestion] validateCaptureManifest checks the schema, network, both RPC URLs, segmentBlocks, and START_BLOCK/END_BLOCK when explicitly set — but not captureId, recordMinutes, or tipLagBlocks. Combined with the early return at line 85 for complete: true manifests and the fixed default REPLAY_DIR (runtime/replay/pacific-1/pacific-1-20m), re-running with e.g. RECORD_MINUTES=60 reports "Capture already complete" and hands back the old 20-minute corpus. The printed block range is the only hint. Including requestedMinutes/tipLagBlocks in the comparison (or refusing to reuse a directory whose captureId differs) would fail loudly instead. (Also raised by Codex.)
| peakCheckedThroughBlock = | ||
| newSegments[newSegments.length - 1].source.lastBlock; | ||
| const peak = scaledPeakTps(newSegments); | ||
| if (peak > MAX_TPS) { |
There was a problem hiding this comment.
[suggestion] The comment claims "worker-queue backpressure bounds the overflow", but the mechanism at lines 431 and 475 is a drop, not backpressure: when evmPending/cosmosPending hits MAX_PENDING_PER_LANE, the transaction is recorded as a queueFull skip and discarded. So in follow mode an over-MAX_TPS burst is neither throttled nor rejected — the excess source load is thrown away. It is at least attributed (worker_queue_full skip reason, pacific_replay_skipped_transactions_total), so this is a fidelity concern rather than a target-safety one, but the wording oversells it. Either reword to "excess is dropped and audited" or add a real throttle so MAX_TPS means the same thing before and after the run starts. (Codex rated this P1; I read it as a documented tradeoff rather than a defect, hence the lower severity.)
| const liquidHash = context.deployment.codeHashes?.liquidStakingProxy; | ||
| const hashChoosesLiquid = | ||
| source.recipientCodeHash !== undefined && | ||
| BigInt(source.recipientCodeHash) % 2n === 0n; |
There was a problem hiding this comment.
[suggestion] hashChoosesLiquid (even code hash ⇒ liquid staking) is evaluated as a peer of the exact liquidStakingProxy hash match rather than as a fallback, so it can override an exact match against the other candidate: a source transaction whose recipientCodeHash happens to equal codeHashes.strategyVaultProxy and is even routes to liquidStakingProxy. The comment describes exact hash → parity-of-hash → sequence parity as an ordered ladder; the code doesn't implement that order. Checking codeHashes.strategyVaultProxy explicitly before falling back to the parity heuristic would match the stated intent.
| } | ||
|
|
||
| recordTraceProfile(source: ReplayEvmTransaction): void { | ||
| this.traceAvailability.inc({ availability: source.trace?.availability ?? 'not_captured' }); |
There was a problem hiding this comment.
[nit] The Prometheus label uses 'not_captured' here while traceAuditFields in runReplay.ts:1283 writes 'not-captured' into the bucket audit for the same condition. Correlating dashboards against the JSONL audit means normalizing the spelling by hand; pick one.
| minBufferMinutes: positiveNumber(env, 'MIN_BUFFER_MINUTES', 5), | ||
| resumeBufferMinutes: positiveNumber(env, 'RESUME_BUFFER_MINUTES', 20), | ||
| metricsPort: nonNegativeInteger(env, 'METRICS_PORT', 9465), | ||
| metricsHost: string(env, 'METRICS_HOST', '0.0.0.0'), |
There was a problem hiding this comment.
[suggestion] METRICS_HOST defaults to 0.0.0.0, so the unauthenticated /metrics and /healthz endpoints are exposed on every interface of whatever host runs the replay. The scrape config only needs host.docker.internal, which resolves via extra_hosts: host-gateway — binding 127.0.0.1 by default and requiring an explicit override for remote scraping would be a safer default for a tool operators run on shared boxes.
| } | ||
|
|
||
| function optionalPositiveInteger(env: Environment, key: string): number | undefined { | ||
| if (!env[key]) return undefined; |
There was a problem hiding this comment.
[nit] if (!env[key]) return undefined treats START_BLOCK=0 as unset rather than rejecting it, and a whitespace-only value falls through to positiveInteger(env, key, 1) where string() trims to '' and returns the fallback — so START_BLOCK=" " silently becomes block 1. Trimming first and erroring on a present-but-unparseable value would match the strictness of the other helpers in this file.
| - prometheus | ||
| environment: | ||
| GF_SECURITY_ADMIN_USER: ${GRAFANA_ADMIN_USER:-admin} | ||
| GF_SECURITY_ADMIN_PASSWORD: ${GRAFANA_ADMIN_PASSWORD:-replay} |
There was a problem hiding this comment.
[nit] Grafana falls back to admin/replay and both services publish on all interfaces (3000:3000, 9090:9090). Fine for a laptop, less so for the shared hosts these long replay runs tend to live on — consider 127.0.0.1:3000:3000 and requiring GRAFANA_ADMIN_PASSWORD rather than defaulting it.
| } finally { | ||
| await bucketAudit.flush(); | ||
| for (const worker of workers) worker.cosmosClient?.disconnect(); | ||
| await liveMetrics.close(); |
There was a problem hiding this comment.
[nit] The cleanup block disconnects the Cosmos clients and closes the metrics server but never calls provider.destroy() on the ethers.JsonRpcProvider created at line 176. With pollingInterval = 200 the provider can keep a timer alive, so a successful run may not exit on its own (the failure path calls process.exit(1), which masks it). Adding provider.destroy() here makes termination deterministic.
Superseded: latest AI review found no blocking issues.
There was a problem hiding this comment.
Large, self-contained TypeScript load generator under integration_test/load_generator with no production code changes; the design is careful (chain-ID/manifest/bytecode preflights, bounded adapters, continuity validation, audit trail, unit tests). I found no security or app-hash concerns, but several robustness and accounting issues in the replay loop, capture finalization, and corpus reads that are worth fixing before this is run unattended for hours.
Findings: 0 blocking | 14 non-blocking | 9 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- No CI wiring:
npm run typecheck,npm test(900+ lines of unit tests) andnpm run test:fixturesare not invoked by any workflow (.github/workflows/*only referenceintegration_test/dapp_tests,runner,rpc_tests,precompile_tests). These tests will silently rot. Consider a small workflow gated onintegration_test/load_generator/**. - The Cursor second-opinion file (
cursor-review.md) is empty, so that pass contributed no findings; the Codex file contained a single medium finding, which is folded into the inline comment onsrc/capture.ts:180. - EIP-7702 replay installs a delegation on the worker EOA (
worker.wallet.authorize({address: profileHarness})) and never revokes it, so after one type-4 replay the worker accounts stay permanently delegated toProfileLoadHarnesson the shared testnet. SubsequentnativeTransferreplays to a delegated recipient also pay the extra delegated-account access on top of intrinsic gas against the hard-codedgasLimit: 30_000n— it still fits today, but the headroom is thin and the state change outlives the run. Worth documenting in the README safety section (or resetting delegation at shutdown). - Memory growth in follow mode:
readReplaySegmentsparses/caches every segment in the directory on each poll, andpendingBlocksflatMaps all of them per iteration. Buffered mode bounds this viaCLEANUP_CONSUMED_SEGMENTS=1, but a manually drivenFOLLOW_SEGMENTS=1 npm run replay:run(cleanup defaults to0there) will grow the in-memory corpus for the whole run — withTRACE_CAPTURE_MODE=fullsegments are not small. signCosmosToTargetSizecan callclient.sign()up to 6 times per Cosmos transaction (each performing an account query) purely to pad the memo to an exact byte target. Bounded, but it is the most expensive part of the Cosmos lane; consider computing the memo length from the first signed size only, or capping the retry at 2.- 9 suggestion(s)/nit(s) flagged inline on specific lines.
| metrics.submitted++; | ||
| liveMetrics.recordOutcome('evm', 'submitted'); | ||
| try { | ||
| const receipt = await response.wait(); |
There was a problem hiding this comment.
[suggestion] response.wait() has no timeout, so a broadcast transaction that never gets mined (mempool eviction, underpriced after a fee spike — fees only refresh every 60s) leaves this job pending forever. That wedges the whole run, not just the lane: worker.evmPending never decrements, and the per-block barrier await Promise.allSettled([...active]) (line 533) never resolves, so the loop stops re-checking runDeadline. Because SIGINT/SIGTERM are handled by setting stopRequested (only read between entries/blocks), the process also can't be stopped without SIGKILL — including by runBuffered's currentChild?.kill('SIGTERM'). Suggest response.wait(1, <timeoutMs>) and treating the timeout like the Cosmos lane's poll_timeout outcome (submitted, not rejected).
| targetCalldataBytes: built.producedCalldataBytes, | ||
| }); | ||
| } catch (error) { | ||
| metrics.rejected++; |
There was a problem hiding this comment.
[nit] A transaction that was successfully broadcast (already counted in metrics.submitted on line 699) but whose wait() fails for a non-status-0 reason lands here and is also counted as rejected, and liveMetrics.recordOutcome('evm','rejected') fires after 'submitted'. That inflates rejected for transactions that may still be included, and the nonce is resynced from pending for a nonce that was in fact consumed. Consider distinguishing "broadcast failed" from "receipt unavailable" (the latter mirrors the Cosmos poll_timeout bucket).
| } | ||
|
|
||
| const elapsedSeconds = Math.max(0.001, (Date.now() - startedAt) / 1_000); | ||
| const segmentFiles = (await fs.readdir(OUTPUT_DIRECTORY)) |
There was a problem hiding this comment.
[suggestion] Agreeing with the Codex finding, with a narrower blast radius than stated: finalization globs every SEGMENT_FILENAME match in the output directory, so totals.canonicalTransactions/sourceBytes (and segmentFiles) can include segments outside the requested [start, end], while totals.blocks on line 211 is computed as end - start + 1 — the two halves of totals can describe different ranges. validateCaptureManifest and validateAgainstCheckpoint do block most reuse paths, so the reachable case is a directory that already holds out-of-range segments (e.g. one the buffered collector appended to, since collectContinuously writes segments without updating capture-manifest.json) and an incomplete manifest. Cheap fix: filter the globbed files to those whose parsed first/last block falls inside [start, end] and assert contiguity before writing complete: true.
| } | ||
| return segments; | ||
| } catch (error) { | ||
| if (allowMissing && (error as NodeJS.ErrnoException).code === 'ENOENT') return []; |
There was a problem hiding this comment.
[suggestion] allowMissing swallows any ENOENT raised anywhere in the try, including a readFile for a segment that was unlinked between readdir and the read. That is a live race: runBuffered's collector polls this directory while the replay:run child is deleting consumed segments via cleanupConsumedReplaySegments. The result is [] rather than "the corpus minus one file", which the caller then reports as "Initial replay buffer disappeared" and backs off for up to 30s. Suggest scoping the ENOENT tolerance to the readdir (directory absent) and skipping just the file that vanished during the per-file read.
| while (!stopping) { | ||
| try { | ||
| const segments = await readReplaySegments(REPLAY_DIRECTORY, true, segmentCache); | ||
| if (segments.length === 0) throw new Error('Initial replay buffer disappeared'); |
There was a problem hiding this comment.
[suggestion] The collector's cursor is derived purely from the last segment file on disk, so it depends on the replay child leaving one behind. With RETAIN_COMPLETED_SEGMENTS=0 (an accepted config value — nonNegativeInteger) the child can legitimately empty the directory, and this throw then repeats forever with exponential backoff: collection never resumes even though capture-checkpoint.json holds exactly the continuity data needed (nextCollectHeight, lastCollectedEvmHash, lastCollectedCosmosHash). Suggest falling back to the checkpoint here, or refusing RETAIN_COMPLETED_SEGMENTS=0 in buffered mode.
| context: CosmosAdapterContext, | ||
| ): BuiltCosmosReplay { | ||
| if (source.isEvm) { | ||
| return skipped(source, 'Wrapped EVM transaction is replayed through the EVM lane'); |
There was a problem hiding this comment.
[nit] This reason is misleading for one real case. An ante-failed EVM wrapper is captured with isEvm: true but no attached evm (see the if (!evm) continue path in pacificSource.captureSegment), so replayEntriesForBlock routes it to the Cosmos lane with evm: undefined — it is skipped here and is not replayed on the EVM lane either. Consider distinguishing source.evm ? 'replayed through the EVM lane' : 'wrapped EVM transaction with no linked EVM entry (ante-failed)' so the unbucketed audit doesn't attribute these to the other lane.
| { amount: coins('21000', 'usei'), gas: '200000' }, | ||
| `associate ${target.network} replay user`, | ||
| ); | ||
| client.disconnect(); |
There was a problem hiding this comment.
[nit] client.disconnect() is not in a finally, so a throwing signAndBroadcast (or the explicit throw on line 120) leaks the Tendermint client for that user; with mapConcurrent at 5 and 100 users a mid-run failure leaks several. Also worth noting: the association-buffer funding pass on line 75 tops up every user below ASSOCIATION_BUFFER_USEI before checking who actually needs associating, so already-associated users get an unnecessary send.
| return orderRpcBatchResponses(requests, body); | ||
| } catch (error) { | ||
| lastError = error; | ||
| await retryDelay(attempt); |
There was a problem hiding this comment.
[nit] retryDelay(attempt) runs unconditionally, including after the final attempt, so a hard failure sleeps up to ~15s before rethrowing (same in cosmosRpcBatch). With maxRetries: 10 and exponential backoff a permanently failing batch takes ~100s to surface. traceRpc already guards this correctly (if (attempt + 1 < this.traceMaxRetries)); mirroring that here would be consistent.
| transaction.receipt.contractAddress, | ||
| ); | ||
| if (creations.length === 0) return; | ||
| const responses = await this.rpcBatch<string>( |
There was a problem hiding this comment.
[nit] This eth_getCode batch is unchunked, unlike fetchRecipientCodeHashes which caps batches at 50. A block with many successful contract creations issues one oversized JSON-RPC batch array, which some gateways reject outright (and a rejection fails the whole segment after the full retry ladder). Reusing the same chunk(..., 50) loop would make the two paths consistent.
There was a problem hiding this comment.
A large, well-documented, self-contained TypeScript load generator under integration_test/load_generator/ that changes no production code. I found no blocking correctness or security defects — the hand-rolled SetCodeTx/SetCodeAuthorization protobuf decoders match proto/eth/tx.proto exactly, chain-ID/manifest/bytecode guards are in place, secrets are env-only and .env is gitignored, and the Docker observability stack binds to localhost with a required Grafana password. Remaining notes are hardening, CI wiring, and docs.
Findings: 0 blocking | 12 non-blocking | 7 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- The Cursor second-opinion pass produced no output (
cursor-review.mdis empty), so only Codex's two findings were available to merge. Both of Codex's points are real but non-blocking; see the inline comments onsrc/runReplay.ts:178and:309. - The new test suites are not wired into CI.
npm test(mocha, ~1,100 lines acrosstest/core.spec.ts,test/corpusCleanup.spec.ts,test/observability.spec.ts),npm run typecheck, andnpm run test:fixturesare documented in the README's Verification section but no workflow under.github/workflows/referencesintegration_test/load_generator. Consider a small job (Node setup +npm ci+typecheck+test) so regressions in the adapters and config parsing are caught — right now the tests are effectively documentation. vendor/sushiswap-v2/PROVENANCE.jsonrecordssha256/gitBlobfor every vendored Solidity file and artifact, but nothing verifies them. Only the three bytecode hashes (pairInitCodeHash,factoryCreationCodeHash,routerCreationCodeHash) are checked, and only at deploy time invalidateSushiArtifacts. A unit test that hashes the vendored files against the manifest would make the provenance claims enforceable rather than aspirational.README.md(Metrics section) tells operators to "keepMETRICS_HOST=0.0.0.0" when using the bundled Docker stack. That exposes the unauthenticated/metricsendpoint (which leaks target network, time scale, and privileged mode viapacific_replay_run_info) on every interface..env.exampleis appropriately cautious about this; the README should match — suggest documenting the Docker bridge gateway address or adding an explicit firewall caveat.- No prompt-injection or instruction-like content was found in the diff, commit messages, or PR description.
- 7 suggestion(s)/nit(s) flagged inline on specific lines.
| cosmosVerifier.disconnect(); | ||
| } | ||
| await verifyDeploymentCode(deployment, provider); | ||
| const users = { ...manifest, users: manifest.users.slice(0, WORKER_COUNT) }; |
There was a problem hiding this comment.
[suggestion] The two-user precondition on line 166 checks the full manifest, but the working set is sliced to WORKER_COUNT here. With WORKER_COUNT=1 against a 100-user manifest the check passes and the run proceeds with a single worker, at which point every worker-to-worker translation degenerates into a self-transfer: recipient = users[(workerIndex + 1) % users.length] resolves back to worker in buildEvmReplay (native + ERC20 transfer) and in buildCosmosReplay's bank paths. The generated load changes materially and silently.
Suggest validating after the slice, e.g. move the check to if (users.users.length < 2) throw new Error('At least two replay users are required') — or reject WORKER_COUNT < 2 in loadReplayConfig. (Raised by Codex as P2; agreed.)
| if (newSegments.length > 0) { | ||
| peakCheckedThroughBlock = newSegments[newSegments.length - 1].source.lastBlock; | ||
| const peak = scaledPeakTps(newSegments); | ||
| if (peak > MAX_TPS) { |
There was a problem hiding this comment.
[suggestion] MAX_TPS is a hard precondition for the initially selected corpus (validatePeakTps throws) but only a warning for segments discovered during follow mode. The comment is honest that excess is dropped, and in practice the per-worker evmQueue/cosmosQueue serialization plus the per-source-block Promise.allSettled barrier do bound in-flight work — so this isn't unbounded. The gap is that admission is governed by MAX_PENDING_PER_LANE × WORKER_COUNT, which is unrelated to the configured MAX_TPS ceiling, so a continuous run can sustain a rate well above it while excess is recorded as queueFull skips.
Worth either deriving the queue bounds from MAX_TPS, or renaming/redocumenting MAX_TPS as a startup-only corpus check so operators don't read it as a live rate limiter. (Raised by Codex as P1; I'd rate it lower than P1 given the queue-level backpressure, but the point stands.)
|
|
||
| while (!stopRequested && (runDeadline === undefined || Date.now() < runDeadline)) { | ||
| const availableSegments = FOLLOW_SEGMENTS | ||
| ? await readReplaySegments(replayDirectory, false, segmentCache) |
There was a problem hiding this comment.
[suggestion] segmentCache is unbounded and every parsed segment stays resident for the life of the process. Eviction in readReplaySegments only happens when the underlying file disappears, and cleanupSegments requires both FOLLOW_SEGMENTS and CLEANUP_CONSUMED_SEGMENTS — the latter defaults to false for replay:run (it is only true by default in loadBufferedConfig, and runContinuousReplay forwards it). So a standalone FOLLOW_SEGMENTS=1 run without cleanup accumulates every 200-block segment (with trace payloads) in memory for the whole RUN_DURATION_HOURS, and additionally re-flatMaps all of them into pendingBlocks on each poll.
A size/age cap on the cache, or dropping cache entries for segments below nextSourceBlock, would make the non-buffered follow path safe for long runs.
| if (cached) return { file, segment: cached }; | ||
| try { | ||
| const parsed = JSON.parse( | ||
| await fs.readFile(path.join(directory, file), 'utf8'), |
There was a problem hiding this comment.
[nit] There's a latent TOCTOU here: readdir and readFile are separate steps, and when allowMissing is false an ENOENT from a segment removed in between propagates out. runReplay's follow loop calls this with allowMissing = false, so a concurrent deleter would take the whole replay down via main().catch → process.exit(1).
Today the only deleter is cleanupConsumedReplaySegments running in that same process (the buffered supervisor reads with allowMissing = true), so this isn't reachable as wired. It becomes reachable the moment two processes share a REPLAY_DIR. Treating a mid-read ENOENT as "file vanished, skip it" regardless of allowMissing would remove the sharp edge — the allowMissing flag would then only govern a missing directory.
| const evm = entry.evm; | ||
| metrics.offered++; | ||
| if (evm) { | ||
| const built = buildEvmReplay(evm, { |
There was a problem hiding this comment.
[nit] Unlike executeEvm/executeCosmos, which wrap the adapter call in try/catch and record an adapterError bucket, the dry-run path has no per-transaction isolation: a single throw from buildEvmReplay (e.g. fitCalldata when natural semantic calldata exceeds MAX_CALLDATA_BYTES, or encodeCallGraphHarness) aborts the entire classification pass with Fatal:.
Not reachable with current defaults — natural semantic calldata is ≤ ~68 bytes and MAX_CALLDATA_BYTES has a floor of 260 via minimumInteger — but since dry-run classification is the primary triage tool for a new corpus, losing the whole report to one anomalous source tx is a poor failure mode. A try/catch that counts an adapterError and continues would match the execute path.
| const isPrivileged = types.some(type => | ||
| PRIVILEGED_PATTERNS.some(pattern => type.includes(pattern)), | ||
| ); | ||
| if (isPrivileged && context.privilegedMode !== 'shape') { |
There was a problem hiding this comment.
[nit] privilegedMode is optional and this comparison makes an omitted value behave as skip, while loadReplayConfig's PRIVILEGED_REPLAY_MODE default is shape. The two defaults disagree.
Harmless today (runReplay always passes it explicitly, and the tests exercise both modes deliberately), but it's the kind of divergence that produces a confusing corpus diff if a future caller forgets the field. Making the property required on CosmosAdapterContext, or defaulting it to 'shape' here, would keep one source of truth.
| BUFFER_START_MODE=latest | ||
| INITIAL_BUFFER_BLOCKS=200 | ||
| # Buffered mode deletes consumed capture segments and keeps the newest completed one. | ||
| CLEANUP_CONSUMED_SEGMENTS=1 |
There was a problem hiding this comment.
[nit] The comment scopes this to "Buffered mode", but .env is loaded by every command via dotenv/config, and loadReplayConfig reads the same key. A user who copies this template and later runs replay:run directly with FOLLOW_SEGMENTS=1 gets segment deletion too — which reads as contradicting the README's "Direct replay:run leaves segments untouched unless cleanup is explicitly enabled."
Suggest wording it as "Deletes consumed capture segments in any follow-mode run; keeps the newest completed one."
| const lane: ReplayLane = evm ? 'evm' : 'cosmos'; | ||
| liveMetrics.recordOffered(lane); | ||
| const currentSequence = sequence++; | ||
| if (evm) { | ||
| const worker = workers[evmCursor++ % workers.length]; | ||
| if (worker.evmPending >= MAX_PENDING_PER_LANE) { | ||
| recordSkip(metrics, 'EVM worker queue full', liveMetrics, 'evm'); | ||
| await bucketAudit.record( | ||
| evmAuditRecord( | ||
| evm, | ||
| entry.sourceCosmosHash, |
There was a problem hiding this comment.
🟡 The queue-full admission check compares a per-worker counter (worker.evmPending/worker.cosmosPending) against MAX_PENDING_PER_LANE at runReplay.ts:405 and :452, but work is dispatched round-robin across all WORKER_COUNT workers, so the number of transactions the lane will admit/queue before back-pressuring is MAX_PENDING_PER_LANE * WORKER_COUNT, not MAX_PENDING_PER_LANE as the name and README imply. The genuinely lane-wide pendingFor(lane) aggregate (lines 292-296) is computed but only ever fed to metrics, never consulted for admission — tuning this knob down does not tighten the backlog bound the way an operator would expect.
Extended reasoning...
The mismatch: runReplay.ts maintains both a per-worker pending counter (worker.evmPending/worker.cosmosPending, declared at lines 90-91) and a true per-lane aggregator, pendingFor(lane) (lines 292-296), which sums that counter across every worker. The admission checks at lines 405 and 452 compare the per-worker counter against MAX_PENDING_PER_LANE, while pendingFor(lane) is only ever passed to liveMetrics.setPending (lines 423, 446, 469, 489) — it never gates admission. Because evmCursor++ % workers.length (line 404) and cosmosCursor++ % workers.length (line 451) dispatch round-robin, consecutive entries land on distinct workers, each independently allowed up to MAX_PENDING_PER_LANE admitted jobs. The result is that the lane-wide admission ceiling is MAX_PENDING_PER_LANE * WORKER_COUNT (2 * 20 = 40 with the .env.example defaults), not MAX_PENDING_PER_LANE — a 20x looser bound than the name and the README's listing of MAX_PENDING_PER_LANE as one of the 'important bounds' for target-chain load would suggest to an operator tuning it down.\n\nWhy this is a backlog/admission bound, not a chain-concurrency bound (addressing the refutation): one verifier correctly pointed out that each worker's evmQueue/cosmosQueue is a serial promise chain (job = worker.evmQueue.then(() => executeEvm(...)), line 424; similarly line 470 for Cosmos) — a queued job does not begin broadcasting until the prior job on that worker fully resolves (including its receipt wait). So the number of transactions actively broadcast and awaiting on-chain confirmation at any instant is bounded by WORKER_COUNT (one per worker), not MAX_PENDING_PER_LANE * WORKER_COUNT. The original bug description's framing of 'up to 40 concurrent unconfirmed EVM transactions' overstates this specific claim, and I don't think that particular consequence is accurate as stated.\n\nWhat is accurate, and is the actual defect: the admitted-but-not-yet-settled job count per lane — i.e. how many transactions the loop will accept into a worker's queue (running + waiting their turn) before it starts dropping ('worker queue full', lines 406/453) — is bounded by MAX_PENDING_PER_LANE * WORKER_COUNT, not MAX_PENDING_PER_LANE. Within a single block, up to 40 transactions can be admitted and queued (even though at most 20 are ever broadcasting simultaneously) before the per-block barrier at line 497 (await Promise.allSettled([...active])) forces the loop to wait for all of them to settle. So the practical effect of the bug is that the backpressure/skip threshold — the thing an operator is actually trying to control by setting MAX_PENDING_PER_LANE low — is 20x looser than its name suggests, even though the peak instantaneous on-chain broadcast concurrency is separately capped by WORKER_COUNT regardless of this bug.\n\nConcrete walkthrough: with MAX_PENDING_PER_LANE=2 and WORKER_COUNT=20 (the .env.example defaults), suppose a block contains 40 EVM-lane entries. Cursor round-robin assigns exactly 2 entries to each of the 20 workers. For every worker: entry 1 increments evmPending to 1 and starts executing immediately (worker.evmQueue was empty); entry 2 increments evmPending to 2 and queues behind it. All 40 entries are admitted — none hit the worker.evmPending >= MAX_PENDING_PER_LANE skip path — because the check only ever sees a per-worker count of at most 2, never the lane-wide total of 40 that pendingFor('evm') would report. Only when a 41st entry would land on an already-saturated worker does a skip occur. Had the check used pendingFor('evm') >= MAX_PENDING_PER_LANE instead, admission would have stopped after the 2nd entry lane-wide, matching the documented intent of the knob.\n\nFix: either replace the per-worker check with pendingFor(lane) >= MAX_PENDING_PER_LANE (true per-lane enforcement, matching the name), or rename the constant/README wording to reflect that it is actually a per-worker queue-depth limit (e.g. MAX_PENDING_PER_WORKER) so the semantics match what's enforced.\n\nSeverity: this is a naming/semantics mismatch in a standalone testnet load-generation tool (integration_test/load_generator, per the PR description "No prod code change") — nothing crashes, corrupts data, or causes incorrect on-chain behavior, and per-worker queue-depth bounding is itself a defensible design. Marking as nit, consistent with all three confirming verifiers.
4 participants
Describe your changes and provide context
A replayer for arctic-1 and atlantic-2. Replays pacific 1 load onto the testnets.
Testing performed to validate your change
No prod code change.