perf: run transaction batches in the worker instead of the main thread - #313
Open
WiktorStarczewski wants to merge 22 commits into
Open
perf: run transaction batches in the worker instead of the main thread#313WiktorStarczewski wants to merge 22 commits into
WiktorStarczewski wants to merge 22 commits into
Conversation
F-002 P1 mock batch override lost the lock the Proxy used to provide F-003 P1 mock client was published before it finished initializing F-001 P1 blockNumber assertion would fail: fresh mock chain starts at 0 F-006 P2 batch.browser.test.ts ran in no CI shard F-011 P3 proveBlock must be awaited; it bypasses the serializing wrapper
F-013 P0 a pending batch is not part of the serialized mock chain, so the
worker round trip silently discarded it while the shared store had
already applied the transactions
F-014 P1 no longer rebuilds the mock client, so a seeded RNG is not rewound
F-020 P1 the rayon claim was false for MT and vacuous for ST F-021 P2 batched transactions prove sequentially by construction F-023 P3 requests were copied again in the worker for no reason
F-030 P1 nothing failed if worker forwarding was reverted; spy on postMessage F-025 P1 mock docstring implied a loss it only defers; scope it F-026 P2 @returns described the wrong block number F-029 P3 argument coercion differed between the worker and fallback paths
F-032 P2 the mock override bypassed argument normalization entirely F-033 P2 mock spy had no positive control and matched names too narrowly F-034 P2 three public copies of the corrected block-number claim were stale F-035 P3 worker comment still claimed all mock proving happens in the worker F-037 P3 dropped a tautological assertion about a constant that never shipped
…alizer F-043 P2 batches always prove locally; the docs claimed proverUrl applied F-047 P2 sparse arrays skipped validation and reached WASM as undefined F-048 P2 a view was cloned with its whole backing buffer, not just its range F-044 P3 the benefit does not reach useWorker:false or non-isolated pages F-046 P3 the lock docblock misdescribed when the chain slot is released F-050 P3 the mock spy could miss a per-transaction forwarding regression Moves the normalizer to utils.js so vitest can reach it; 10 tests added.
F-059 P2 the validation's stated reason was wrong; wasm-bindgen type-checks F-055 P2 api-types advertised a prover option that cannot exist F-058 P2 same-account batching is our limit, not the Rust client's F-057 P2 README and docs still said the batch "landed in" the returned block F-060 P2 not every worker-forwarded mock method round-trips the chain F-066 P3 mock override duplicated the base in-thread branch verbatim Trims six rounds of accreted comments and files #314 for waitForConfirmation.
…e prose F-072 P1 the validation entry claimed to narrow inputs wasm-bindgen already rejected F-073 P1 README and docs prose still described waitForConfirmation as awaiting commit F-074 P2 initializeWorker still claimed every mock prove happens in the worker F-075 P2 rejection message named a type narrower than the check accepts F-076 P2 DataView was excluded for a reason that was not true of DataView F-077 P3 cross-realm acceptance, the main widening, had no test
…prose pass F-081 P1 worker "error"/"messageerror" went unhandled, hanging every later call F-082 P2 same-account comment contradicted the types this PR corrected F-083 P2 docs claimed mixing accounts throws; the value is ignored F-084 P2 trimmed comments and changelog that had accreted over nine rounds
…med truths F-090 P1 the worker fix only released in-flight calls; reverted to issue #315 F-091 P2 trim widened "reads and syncs" to "other calls", making it false F-092 P2 trim dropped a true caveat about main-thread request building F-093 P2 local-only batch proving was missing from both public guides F-094 P2 a prefix view now pins the half of the copy condition nothing tested
F-098 P2 456 lines of markdown churn in files .prettierignore excludes; rebuilt to 25 F-099 P2 the 30s keystore ceiling on batch signatures was only in the changelog F-100 P3 the corrected proving claim still deferred to an uncorrected one F-101 P3 mock rationale credited sync with a chain round trip it does not do
…ndling F-106 P3 the PR body said local proving was undocumented; the docs said the opposite F-107 P3 README could still imply mixing accounts throws F-108 P3 a sentence named the caveat where it should name the value
F-111 P2 waitForConfirmation calls a method that does not exist; docs said it worked F-112 P2 BatchOperation drops every per-submission option, not just account F-113 P2 mock batching does not cost nothing; it costs a mandatory proveBlock() F-114 P2 a node-accepted batch can still fail locally, where retry double-submits F-115 P3 a TransactionRequest carries no account, so none is "ignored"
…wo of five F-118 P1 types, changelog and PR body still described waitForConfirmation as working F-119 P2 the documented error names never reach JS; quote the real message F-120 P2 the external guide lagged the README, and neither showed the alternative F-121 P3 the hedge invented an auto-sync the web client does not have F-123 P3 returnNote is a request-building switch, not a submission option
F-129 P1 six public copies promised chain-level batch atomicity; upstream
fans a batch out into one validator submission per transaction and
documents atomicity only for the local store update
F-126 P2 batch() @PARAM still framed waitForConfirmation as a working setting
F-127 P2 lastAuthError() doc claimed keystore callbacks are unreachable under
the worker; they are proxied back to the main thread
F-125 P2 three test names asserted the retracted waitForConfirmation behaviour
F-132 P3 in-thread note named useWorker:false but not a missing Worker
…ode proto
F-133 P0 round 15 withdrew the all-or-nothing batch guarantee on the strength
of per-transaction SEALING language; the node's own RPC contract
(miden-node-proto-build 0.16.0-rc.1, rpc.proto:61) states batches are
"committed together or not all". Reverted and cited the source.
F-135 P2 prover rationale named the API shape, not the wiring that makes it
true: per-tx proofs use the Rust client's prover, which this crate
never sets
F-137 P3 two JSDoc caveats referenced syncStateWithTimeout, which does not exist
…316 F-138 P1 a keystore callback that throws a non-Error is reported as a successful signature; the 30s ceiling is fixed and a batch commits atomically, so one slow approval fails all of it. Both live in the shared bridge used by every forwarded method, so scoping a fix to batching would leave ~40 others broken — filed as #316 and documented. F-140 P3 the sync caveat named acquireSyncLock, which does not exist either
…nism F-141 P2 the changelog credited lastAuthError() to #316 and omitted #316's worse defect; lastAuthError is a dual-instance issue, not a bridge one F-142 P3 "because a batch commits atomically" named the wrong cause — signing happens at push time, so a timeout aborts the builder before submit F-143 P3 the #316 caveat was missing from the published .d.ts F-145 P3 attributed the mempool precondition; matched the same-account wording
F-146 P2 said a non-Error throw is reported as a SUCCESSFUL signature; Rust
rejects the undefined with "sign callback must return a Uint8Array",
so the rejection is not forged — its reason is lost behind a
misleading error. Corrected in all four copies and in #316.
F-149 P2 lastAuthError regression was only in the changelog, not on the API
F-150 P2 mock batching is the one mock path doing real proofs; documented
F-151 P3 both batch error logs were byte-identical; Changes section restored
F-152 P2 "the one mock path that produces real proofs" — the dummy shortcut
is gated on prover.is_none(), so an explicit prover on a mock proves
for real too; batching is only the path with no way around it
F-153 P2 "the builder aborts on the first failed signature" — upstream says a
failed push leaves the batch intact; the abort is this wrapper's
F-154 P2 "a callback that throws a non-Error" — what matters is a falsy
.message; {message: "..."} propagates fine
F-155 P3 the JS request validation is browser-only; Node forwards unchanged
F-156 P2 "no truthy .message" also covered throw null/undefined, which instead
throws inside the bridge and hangs to the timeout — scoped and noted
F-157 P3 "without a rayon pool" is false on an MT build where the consumer
called initThreadPool on the main thread; dropped the reason
F-158 P3 waitForIdle's method list omitted the batch method it now serializes
F-159 P3 syncLock test comment still claimed the fallback runs fn directly
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #312.
Batching proved every transaction on the main thread while single submits offloaded to the Web Worker, so a large batch froze the UI for its full duration. This routes the batch call through the worker like every other write.
What changed
submitNewTransactionBatchreached the worker shim only throughcreateClientProxy's fallback, which runs the call on the main-thread WASM instance. It is now an explicit method onWebClientthat forwards to the worker, with aMethodNameentry and a handler on the worker side. One batch call executes and proves every transaction plus the batch proof, so this is the single longest-running WASM call the SDK makes.MockWebClientdeliberately keeps batching on the main thread. Mock submits round-trip the serialized mock chain through the worker, butMockChain's serializer does not writepending_batchesand its deserializer resets the field to empty — so forwarding a mock batch would discard it while the shared store had already recorded its per-transaction updates, advancing the nonce on a chain that never saw the batch. Mock clients exist for tests, where a blocked main thread costs nothing.Request bytes are normalized once on the main thread, ahead of the worker/in-thread branch, so both paths accept exactly the same input.
normalizeSerializedRequestslives injs/utils.jsand is unit-tested.Behaviour notes
_serializeWasmCallfor the whole round trip, so reads, syncs, and other forwarded methods still queue behind it. What changes is that the page keeps painting.SYNC_METHODS.useWorker: falsebatching still runs in-thread, exactly as before.lastAuthError()no longer reports a failed batch's auth error.Documentation corrections
Review turned up several public claims about batching that were simply wrong, all corrected here:
waitForConfirmationpolls against it — so it confirms only that the client caught up to the submission point. Behaviour is unchanged; the docs were wrong. Filed as batch waitForConfirmation is broken two ways: wrong target height, and it calls a method that does not exist #314.ClientOptions.proverUrlapplies tosubmit()but not to batching. The public docs said the opposite: the resource JSDoc claimed the batch used the client's configured prover, and thesubmitBatchJSDoc listed aproversetting that was never declared.BatchBuilder::pushtakes an account per request.TransactionRequestcarries no account at all, so there is nothing per-operation to conflict with the batch-level account.miden-client's per-transaction sealing language that a batch is not all-or-nothing on chain; the node's own RPC contract settles it the other way (miden-node-proto-build 0.16.0-rc.1,rpc.proto:61: "All transactions in this batch will be considered atomic, and be committed together or not all"). The docs now quote it, so the claim stops being re-derived from the wrong layer.Tests
normalizeSerializedRequests, including cross-realm views and a prefix view that pins the half of the copy condition nothing else covered (verified by applying the mutant).batch.browser.test.tsspies onWorker.prototype.postMessageand asserts the batch is forwarded. It was in no shard, so CI never ran it; it is now inci-shard-1-tx-flows.miden_client_api.test.tsasserts the mock batch is not forwarded, withsyncStateMockas a positive control so the assertion cannot pass vacuously.Unit suite: 399 passing.
Deliberately not fixed here
.message— a bare string, or a wallet-style{ code: 4001 }— loses its reason: the bridge sendserror.message, the worker reads the resultingundefinedas success and resolvesundefined, and Rust then rejects it with the genericsign callback must return a Uint8Array— so a user rejection surfaces as what looks like a bug in the consumer's callback. And the 30s per-callback ceiling is fixed, which batching sharpens: signing happens at push time and this wrapper treats a failed push as fatal, so one slow hardware-wallet approval fails the whole batch. Both live in the bridge shared by every worker-forwarded method, so a batch-scoped fix would leave the other ~40 broken — same reasoning as A dead or erroring Web Worker leaves the client permanently wedged #315.waitForConfirmationis broken on batches (batch waitForConfirmation is broken two ways: wrong target height, and it calls a method that does not exist #314). Two separate defects: its poll callssyncStateWithTimeout, which does not exist anywhere in the crate, so it cannot advance the sync height and times out; and even once that is fixed it would only confirm the client reached the submission tip, not that the batch committed. Review also found that the unit test covering it invents the missing method on its mock and then asserts it was called, which is why it passed. All of that is reported on batch waitForConfirmation is broken two ways: wrong target height, and it calls a method that does not exist #314. This PR documents the option as broken across every public surface and drops it from both examples, but does not fix the code — it is pre-existing and#waitForBlockis not on the batching path this PR changes.