Conversation
The worker crate defined WorkerError/WorkerResult but several crate-owned
functions still returned anyhow::Result, erasing the typed error at the
crate's own API. Callers then had to inspect strings to tell a missing
genesis state from a transient RPC failure.
Return WorkerResult from the crate-owned entry points (AsmWorkerBuilder::launch,
AsmWorkerHandle::submit_block/submit_block_async). anyhow stays only at the
external strata-service trait seam (Service::process_input), where the trait
signature forces it and typed errors convert via `?`.
Keep the error source chain intact rather than flattening causes to strings, so
Error::source() walks all the way down and concrete causes stay recoverable via
downcast_ref:
- DbError carries the underlying storage error (#[source] anyhow::Error) instead
of being a payload-less marker that dropped the cause entirely.
- BtcRpc carries the underlying error as a #[source]; the context impl attaches
which-call/which-block context before wrapping. The worker is generic over its
WorkerContext, so it deliberately does not name a concrete RPC client type.
- Replace the Unexpected(String) fallback in convert_service_error with a typed
Service(#[source] ServiceError) variant, and drop the now-unused Unexpected.
- ServiceLaunch(#[source] anyhow::Error) wraps the framework's open-ended launch
failure at the launch_sync seam.
The source-carrying variants (BtcRpc/DbError/Service) also render the source in
their Display string ("...: {0}"), matching the AsmError/AnchorMismatch/
ServiceLaunch convention already in the enum. strata-service logs worker errors
with plain Display (%e), which prints only the outermost message and not the
.source() chain, so without this the real cause would be dropped from logs.
The worker's builder-launch seam still returned anyhow::Result, leaking an untyped error out of otherwise-typed crate logic. Only the strata-service trait signatures we implement force anyhow; the builder's own launch does not, so type it. launch() now returns MohoWorkerResult. The one anyhow that genuinely originates here — the framework's launch_async — is flattened at that seam into a new ServiceLaunch variant that carries the anyhow::Error as a #[source], so its full chain stays reachable rather than being stringified. Storage previously held a String, discarding the concrete backend error type. It now boxes the underlying std::error::Error as a #[source]; the backends behind the four concern traits (sled, the Bitcoin client) have distinct error types, hence the boxed source. Callers convert via .into() instead of .to_string(), preserving each error's chain.
The prover worker leaned on anyhow throughout its crate-owned code, so every failure collapsed into an opaque string: callers could not match on what went wrong (missing prerequisite vs. storage fault vs. remote-prover failure) and the underlying error's type and cause chain were lost at the first `.context(...)`. Expand ProverError into specific variants (Storage, NotFound, Decode, RemoteStatus/RemoteRetrieve/RemoteSubmit, RemoteIdDecode, Backend, BackendUnavailable, Launch) and thread ProverResult through proof_store, input, backend, service helpers, and the crate-owned reader traits. Each variant that wraps a real failure keeps it as an Error::source: concrete, always-available errors (ssz::DecodeError, zkaleido::ZkVmError) are carried by their own type; the heterogeneous storage/chain-source backends and the feature-gated SP1/native construction errors — whose types vary per implementor or build feature and so cannot be named as a concrete field — are boxed as dyn Error, which still chains through source(). Nothing is re-stringified. anyhow remains only where the external strata-service trait fixes it: Service::process_input's signature, and the framework builder's anyhow::Error carried as the source of ProverError::Launch. The runner's AsmProverContext is updated to the typed trait signatures, boxing the sled layer's anyhow errors into the Storage variant's source.
🔒 AI Security ReviewReviewed the full diff (mechanical refactor replacing No new attack surface is introduced: no new parsing/deserialization of untrusted input, no new I/O, auth, or crypto logic — only error-type plumbing. Underlying error causes are preserved as No security issues found in this diff. |
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
2 participants
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.
Description
Replaces
anyhowwith typed, per-crate error enums across the three worker crates, so crate logic surfaces specific error variants and only the binary usesanyhow.anyhowis retained solely at the externalstrata-servicetrait seam (process_input/on_launch/recv_next/executor spawn closures), whose signatures are fixed and can't change; typed errors convert there via?.This PR consolidates work that was originally split across three PRs (previously alpenlabs#168 for moho and alpenlabs#170 for the prover), one commit per crate:
refactor(worker)—strata-asm-worker: crate-ownedsubmit_block/submit_block_async/launchreturnWorkerResult;WorkerErrorde-erased (DbErrorwent from a payload-less unit variant to carrying its cause,BtcRpcfromStringto a source), addedService(#[source] ServiceError), removed theUnexpected(String)catch-all.refactor(moho)—strata-asm-moho-worker:launchreturnsMohoWorkerResult;Storage(String)→ boxed concrete#[source]; addedServiceLaunch(#[source] anyhow::Error).refactor(prover)—strata-asm-prover-worker: removed the#[from] anyhow::Errorpassthrough in favor of specific variants (Decode,Storage,Remote*,Backend*,Launch,NotFound, …), carryingssz::DecodeError/ZkVmErrorby concrete type and heterogeneous/feature-gated backends as boxeddyn Errorsources; crate-owned reader traits now returnProverResult.No type erasure: every wrapping variant keeps the underlying error via
#[source]/#[from]soError::source()chains to the real cause. Each source-carrying variant also embeds the source in itsDisplaystring, becausestrata-servicelogs with%e(plainDisplay, which does not walk the source chain) — without this the cause would be dropped from logs.Type of Change
Notes to Reviewers
Breaking API change: public return types and error-variant payloads change in all three crates. Each crate is its own atomic commit, so the diff can be reviewed commit-by-commit.
Follow-up left out of scope: in the moho worker,
MohoWorkerContextImpl::get_parent_blockmaps a retry-exhaustedClientErrorintoMissingParentBlock, dropping the client error. Preserving it would reshapeMissingParentBlock(also raised by the test mock with no underlying cause) and its test matcher.Checklist