refactor(provider-challenge): replace stringly-typed ChallengeError with cause-based variants - #367
Open
danielbui12 wants to merge 40 commits into
Open
refactor(provider-challenge): replace stringly-typed ChallengeError with cause-based variants#367danielbui12 wants to merge 40 commits into
danielbui12 wants to merge 40 commits into
Conversation
- Move provider-node/src/challenge_responder.rs to crates/providers/challenge (Apache-2.0, workspace-inherited deps)
- Decouple from provider-node internals: new ChallengeError enum and narrow ChallengeProofSource trait replace crate::{Error, ProviderState}; ProviderState implements the trait by delegating to its storage backend
- Re-export the crate from provider-node under the old challenge_responder module path so existing imports and tests keep working
- Switch SubxtChainClient's ChallengeChainClient impl and the coordinator test mock to ChallengeError
- Classify provider-challenge in scripts/coverage.sh COV_PACKAGES so the coverage gate keeps passing
- Move `chain_connection` and `chain_events` from provider-node into the new Apache-licensed `provider-rpc` crate under `crates/providers/` - Give the crate its own error enum (`Connection`, `NotConnected`) in place of the node's HTTP-coupled `Error::Internal`, mapped back via `From` in provider-node so error messages are unchanged - Rewire all call sites (auth, command, coordinators, subxt client, tests) to the new crate directly, with no re-export shims left behind - Classify `provider-rpc` in `COV_PACKAGES` and list all three provider crates in the README structure tree
- Move `crates/providers/rpc` to `crates/providers/chain` and rename the package to `provider-chain` - Retarget every `provider_rpc` import in provider-node (auth, command, coordinators, subxt client, error mapping, tests) - Update the workspace member list, `[workspace.dependencies]`, the provider-node dependency, and `COV_PACKAGES` in `scripts/coverage.sh` - Name the crate for its contents (chain connection plus event decoding) rather than the transport, since a light-client variant is planned behind `ChainTransport` and `RpcParams` already means the node's own HTTP endpoints
…lenge_refactoring
- Declare the provider-chain and sp-runtime deps the crate already imports
- Drop the stale `use crate::{Error, ProviderState}` left by the merge
- Add `provider_id()` to `ChallengeProofSource` and route the ChallengeCreated
filter through it, replacing the removed `self.state.provider_id`
- Return `ChallengeError` from the subxt client's `fetch_challenge`, and map
`api()` / `submit_and_finalize` failures to `ChallengeError::Chain`
- Switch the merged event_fanout and challenge mocks to `ChallengeError`
Bumps the github-actions-deps group with 1 update: [taiki-e/install-action](https://github.com/taiki-e/install-action). Updates `taiki-e/install-action` from 2.85.0 to 2.85.5 - [Release notes](https://github.com/taiki-e/install-action/releases) - [Changelog](https://github.com/taiki-e/install-action/blob/main/CHANGELOG.md) - [Commits](taiki-e/install-action@7572810...6a1bd70) --- updated-dependencies: - dependency-name: taiki-e/install-action dependency-version: 2.85.5 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: github-actions-deps ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…dates (#346) Bumps the npm-security group with 1 update in the / directory: [postcss](https://github.com/postcss/postcss). Updates `postcss` from 8.5.22 to 8.5.23 - [Release notes](https://github.com/postcss/postcss/releases) - [Changelog](https://github.com/postcss/postcss/blob/main/CHANGELOG.md) - [Commits](postcss/postcss@8.5.22...8.5.23) Updates `nanoid` from 3.3.16 to 3.3.18 - [Release notes](https://github.com/ai/nanoid/releases) - [Changelog](https://github.com/ai/nanoid/blob/3.3.18/CHANGELOG.md) - [Commits](ai/nanoid@3.3.16...3.3.18) --- updated-dependencies: - dependency-name: postcss dependency-version: 8.5.23 dependency-type: direct:development dependency-group: npm-security - dependency-name: nanoid dependency-version: 3.3.18 dependency-type: indirect dependency-group: npm-security ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…cal docs (#343) * chore(skills): add design-alignment and reference-docs review skills Add two skills enforcing that docs/design and docs/reference stay canonical and true to the code: - design-alignment: changes must conform to the core design docs; any deviation is flagged explicitly as a DESIGN DEVIATION and requires either conforming or updating the design doc with rationale in the same PR - reference-docs: changes that alter documented flows or invalidate reference docs (EXTRINSICS_REFERENCE, PAYMENT_CALCULATOR) must update them in the same change; missing updates are blocking Wire both into /review as criteria sections 8 and 9. * chore: add CODEOWNERS for canonical docs folders docs/design and docs/reference changes now require review from @eskimor, @mudigal, and @bkontur. * chore: keep @eskimor as sole owner of docs/design Restores the mandatory-review semantics for the design docs: with a single listed owner, GitHub requires that person's approval specifically. docs/reference keeps the three-owner rule (any one satisfies it). * fix(skills): correct reference-docs detection paths, add EXECUTION_FLOWS The step-1 trigger keyed on `pallet/` and `storage-interfaces/file-system/pallet-registry/`, neither of which exists in this repo, so detection never matched anything. Every `#[pallet::call]` lives under `crates/pallets/**`; the glob also survives new pallets. `EXECUTION_FLOWS.md` is in `docs/reference/` but was absent from the doc list, so nothing checked it. PR #339 changed extrinsic signatures and had to update it alongside `EXTRINSICS_REFERENCE.md`. * fix(skills): default the no-args diff to `dev`, not `main` The repository's default branch is `dev` and PRs target it, so reviewing the local diff against `main` compared against the wrong base. Affected the no-argument path in all three review skills; passing a PR number was never impacted. * docs: reconcile `docs/reference/` gating between CLAUDE.md and /review CLAUDE.md described `docs/reference/` as "not gated" while the new /review section 9 called it "canonical" — a direct contradiction once CODEOWNERS started gating the directory. Both now say the same thing: derived, but review-gated and required to stay true to the code. * chore: clarify the `/**` glob comment in CODEOWNERS Rewording the design-docs comment dropped the "in docs/design/" anchor, so the note read as a claim about every file in the repo rather than about the directory each pattern names. * fix(skills): make /review actually invoke the two docs-governance skills Sections 8 and 9 only mentioned the skills parenthetically, so a /review run applied the five-line summaries and never loaded either procedure. Both now instruct the reviewer to invoke the skill and state that the bullets are a summary rather than a substitute. * fix(skills): correct the design-alignment document map The map listed eight targets, but `docs/design/` holds two. Six pointed into `docs/drafts/` — which CLAUDE.md marks unratified — one pointed at `EXECUTION_FLOWS.md` in `docs/reference/`, and `storage-db-decision-notes.md` does not exist. `provider-initiated-checkpoints.md` is the archive of code deleted in #306, so a checkpoint PR would have been measured against removed functionality under a rule that forbids letting deviations pass silently. Step 1 now names the two actual design docs. The topic mapping survives as step 2, scoped to `docs/drafts/` and framed as optional background — drafts often explain reasoning the design docs cover only briefly — with a reminder to check each one's header for supersede/removal notices. `⚠️ DESIGN DEVIATION` is reserved for contradictions with `docs/design/` itself. --------- Co-authored-by: Branislav Kontur <bkontur@gmail.com>
…tiation (1) (#319) * refactor: move provider-negotiation crate to crates/providers/ - Move provider/negotiation to crates/providers/negotiation (pure rename, no code changes) - Update workspace member and provider-negotiation dependency paths in root Cargo.toml - Swap the Apache license header glob from provider/**/*.rs to crates/providers/**/*.rs - Move the crate's line under crates/ in the README project-structure tree * refactor: extract provider HTTP auth into provider-auth crate - Add crates/providers/auth (package provider-auth, Apache-2.0): moves provider-node/src/auth.rs (membership cache/resolvers, signature verification, role enforcement) and negotiation's http_auth.rs (header building) into one shared crate - Decouple from provider-node: new AuthError enum replaces the four Error variants; require_role takes &MembershipCache instead of &ProviderState; From<AuthError> in provider-node preserves the exact HTTP status mapping (401/401/403/500) - Rewire provider-node and client imports to provider_auth; remove http_auth module and re-exports from provider-negotiation - Drop unused CallerIdentity struct - Update stale path references in READMEs, TS doc comments, and the design doc's wire-format pointer * chore: update coverage * chore: update README.md * refactor: take a closure for the chain API in ChainMembershipResolver - Replace the `ChainApiSource` trait with a `Fn() -> Result<OnlineClient, String>` closure passed to `ChainMembershipResolver::new` - Drop the `WatchApiSource` newtype from the provider node; callers now close over the watch receiver directly - Rewrite the role-enforcement tests to drive `require_role` end to end over a full (granted, required) matrix instead of asserting on `matches!` patterns - Add tests covering non-member signers, missing headers, and a header replayed against a different bucket * chore: tidy up leftovers from the provider-auth extraction Follow-ups on the `provider-auth` crate extraction, all cosmetic or hygiene — no behaviour change: - drop `dashmap` from provider-node: the moved `auth.rs` was its only user in the crate (Cargo.lock updated accordingly) - keep the "Membership lookup failed" wording in `AuthError`'s `Display` instead of re-spelling it in the `From` impl, so the two crates cannot drift apart - `check_role`: `.await.map_err(Into::into)` rather than `.await?; Ok(())` - restore the `use dashmap::DashMap;` / `use crate::http_auth::{..};` imports the move left fully qualified at their call sites - only build the `<Bytes>`-wrapped payload when the raw signature check has already failed - README: `crates/providers/` also holds provider-storage * refactor: fold AuthError into Error via #[from] `Error::AuthRequired`, `Error::TimestampExpired` and `Error::InsufficientRole` had no producer left other than the hand-written `From<AuthError>` impl — they only restated an error space `provider-auth` already owns. Replace the three with a single `Auth(#[from] AuthError)` and let thiserror derive the conversion. `IntoResponse` now matches the inner `AuthError` directly, which makes it the one exhaustive mapping point: a new `AuthError` variant stops compiling until it is given a status code, instead of silently needing a node variant, a `From` arm and a response arm. `MembershipLookup` gets its own arm rather than being folded into `Internal`. The `auth_required` / `insufficient_role` response codes are unchanged. The `details.message` for an expired timestamp now reads "Request timestamp expired" (AuthError's wording) instead of "Timestamp expired or too far in the future". * refactor: make provider-auth chain-agnostic, drop its subxt dependency `MembershipResolver` was already the right seam — the chain-backed implementation was just sitting on the wrong side of it. Move `ChainMembershipResolver` and the dynamic `Buckets` decoding helpers into `provider-node::membership`, leaving `provider-auth` with the trait, the TTL cache and the static test resolver. `provider-auth` no longer depends on subxt, so client-side consumers that only want `build_auth_header` (clients/storage today, a wasm or CLI client tomorrow) stop pulling in a chain client and a membership cache to get a 25-line header formatter. Back on the node's side of the trait the resolver can hold the `ChainWatch` receiver directly again, which removes the `CurrentApi` boxed closure and the closure-plumbing at its two construction sites. Reconnect behaviour is unchanged: the connection is still resolved per lookup, not pinned at construction. The `collect_members` nesting regression test moves with the decoder it covers, and `chain_resolver_fails_cleanly_before_first_connect` moves next to the resolver it exercises. * fix: drop needless borrow flagged by clippy in verify_signature `wrap_bytes` returns an owned `Vec<u8>`, which already satisfies the `AsRef<[u8]>` bound on `Pair::verify`, so borrowing the temporary trips `clippy::needless_borrows_for_generic_args` under `-D warnings`. Both verify calls now pass their message argument the same way. Fixes CI on 71a3151, where inlining the wrapped payload turned a borrow of a local binding into a borrow of a temporary. * refactor: give provider-auth a role module, fold find_role into the cache entry Two related tidies in provider-auth. `RequiredRole` and the granted-vs-required matrix sat in `verify.rs` between signature parsing and role enforcement. Move them to their own `role` module so the policy is one small, directly testable thing, and match over the `(required, granted)` pair rather than on `required` alone. Previously `RequiredRole::Reader => true` covered every granted role implicitly, so a new variant on `storage_primitives::Role` would compile clean and be answered by accident; now it fails to compile until given an explicit answer. `find_role` was a free function taking the member slice and an account — `CachedMembership` already owns that slice, so make it `role_of` on the entry, alongside `is_fresh` for the TTL check. The fetch path builds the entry first and reads the role from it, so no call site needs the loose helper. No free functions remain in the module. `RequiredRole` is still re-exported from the crate root, so no call site changes. * refactor: give membership resolution a typed signature `fetch_members` returned `Result<Vec<(AccountId32, Role)>, String>`, so a transient chain outage and a decode bug arrived at the HTTP layer as the same opaque string and both came back as 500. Replace the error with `MembershipError`, split by what the caller should do: `Unavailable` is worth retrying and now maps to 503 `membership_unavailable`, `Decode` stays a 500. `AuthError::MembershipLookup` takes it via `#[from]`, so `require_role` is just `.await?`. Replace the tuple with a named `Member { account, role }` — the name the pallet, the generated bindings and the design doc already use. Construction goes through `From<(AccountId32, Role)>`, so call sites keep passing tuples. Also fold the `role` module back into `membership`: once `Member` lived there both modules were about roles, and the exhaustive `(required, granted)` match that module was for is unaffected by where it sits. * docs: prefer keeping crates/providers free of subxt Records the guidance from PR #319 review: express what a `crates/providers` crate needs as a trait and let provider-node supply the subxt-backed implementation, so swapping the chain client stays a provider-node change. Adds a matching (non-blocking) crate-boundary criterion to the review skill. * refactor: fold auth config into an Authenticator `require_role` took six arguments, mixing per-request values with settings that never change: `membership` and `max_skew` both came from `ProviderState` on every call. Introduce `Authenticator`, which owns those two, and make `require_role` a method taking only the request (`auth_header`, `method`, `bucket_id`, `required`). `ProviderDeps` and `ProviderState` each lose a field, so the ~27 fixtures that built a cache and a skew separately now build one thing. Constructors take `impl MembershipResolver + 'static` rather than `Box<dyn MembershipResolver>`, so callers stop writing `Box::new` — the boxing is the cache's business. A forwarding impl on `Box<T>` keeps an already-erased resolver acceptable, which is the only way to choose one at runtime; there is a test for exactly that. Narrow the crate's public API to what a consumer can use: `verify_signature` and `MembershipCache` have no callers outside `provider-auth` and there is no way to hand a cache to an `Authenticator`, so both are now internal. Adding them back later is non-breaking; removing them later would not be. * fix: import Authenticator in provider-node's lib tests The six fixtures in `mod tests` referred to `Authenticator` unqualified while the rest of the file spells `provider_auth::` out in full, so the lib test target failed to build (E0433). Also stop the dynamic decode from papering over shapes it cannot read. `MembershipError::Decode` was only produced by the SCALE decode itself, while the hand-rolled tree walk degraded silently: an unrecognised `Role` variant became `Role::Reader`, and a member set we could not walk became "no members", i.e. a 403. Both now return `Decode`. A bucket that does not exist still returns an empty set, since that is a genuine "not a member" — `create_bucket` seeds an admin and `remove_member` refuses to drop the last one, so an existing bucket always has members. Add rustdoc to the public items that lacked it: `Member`'s fields, `RequiredRole`'s variants, and `MembershipResolver::fetch_members`. --------- Co-authored-by: Branislav Kontur <bkontur@gmail.com>
…tiation (2) (#324) * refactor: move provider-negotiation crate to crates/providers/ - Move provider/negotiation to crates/providers/negotiation (pure rename, no code changes) - Update workspace member and provider-negotiation dependency paths in root Cargo.toml - Swap the Apache license header glob from provider/**/*.rs to crates/providers/**/*.rs - Move the crate's line under crates/ in the README project-structure tree * refactor: extract provider HTTP auth into provider-auth crate - Add crates/providers/auth (package provider-auth, Apache-2.0): moves provider-node/src/auth.rs (membership cache/resolvers, signature verification, role enforcement) and negotiation's http_auth.rs (header building) into one shared crate - Decouple from provider-node: new AuthError enum replaces the four Error variants; require_role takes &MembershipCache instead of &ProviderState; From<AuthError> in provider-node preserves the exact HTTP status mapping (401/401/403/500) - Rewire provider-node and client imports to provider_auth; remove http_auth module and re-exports from provider-negotiation - Drop unused CallerIdentity struct - Update stale path references in READMEs, TS doc comments, and the design doc's wire-format pointer * chore: update coverage * chore: update README.md * refactor: replace dynamic subxt calls in provider-auth with storage-subxt - Rewrite ChainMembershipResolver::fetch_members to use the typed storage().storage_provider().buckets() address (unvalidated, since the bindings are paseo-generated and the local runtime hash differs) - Pass the bucket id as the native u64 key instead of a coerced u128 Value - Convert generated Member fields inline: AccountId32 from raw bytes and a match mapping the runtime Role to storage_primitives::Role - Delete the scale_value tree-walking helpers and their nesting-shape test, now dead with the dynamic decode gone - Add storage-subxt to provider-auth dependencies * chore: improve coverage * refactor: move generated Role conversion into storage-subxt - Add `impl From<runtime Role> for storage_primitives::Role` in the new storage-subxt `primitive_conversions` module, where the generated type is local and the orphan rule permits the impl - Depend on storage-primitives from storage-subxt - Drop provider-auth's local `from_runtime_role` in favour of `.into()` - Rename `members_from_bucket` to `member_roles_from_bucket`, since it returns flattened (account, role) pairs rather than on-chain `Member` structs * refactor: take a closure for the chain API in ChainMembershipResolver - Replace the `ChainApiSource` trait with a `Fn() -> Result<OnlineClient, String>` closure passed to `ChainMembershipResolver::new` - Drop the `WatchApiSource` newtype from the provider node; callers now close over the watch receiver directly - Rewrite the role-enforcement tests to drive `require_role` end to end over a full (granted, required) matrix instead of asserting on `matches!` patterns - Add tests covering non-member signers, missing headers, and a header replayed against a different bucket * chore: tidy up leftovers from the provider-auth extraction Follow-ups on the `provider-auth` crate extraction, all cosmetic or hygiene — no behaviour change: - drop `dashmap` from provider-node: the moved `auth.rs` was its only user in the crate (Cargo.lock updated accordingly) - keep the "Membership lookup failed" wording in `AuthError`'s `Display` instead of re-spelling it in the `From` impl, so the two crates cannot drift apart - `check_role`: `.await.map_err(Into::into)` rather than `.await?; Ok(())` - restore the `use dashmap::DashMap;` / `use crate::http_auth::{..};` imports the move left fully qualified at their call sites - only build the `<Bytes>`-wrapped payload when the raw signature check has already failed - README: `crates/providers/` also holds provider-storage * refactor: fold AuthError into Error via #[from] `Error::AuthRequired`, `Error::TimestampExpired` and `Error::InsufficientRole` had no producer left other than the hand-written `From<AuthError>` impl — they only restated an error space `provider-auth` already owns. Replace the three with a single `Auth(#[from] AuthError)` and let thiserror derive the conversion. `IntoResponse` now matches the inner `AuthError` directly, which makes it the one exhaustive mapping point: a new `AuthError` variant stops compiling until it is given a status code, instead of silently needing a node variant, a `From` arm and a response arm. `MembershipLookup` gets its own arm rather than being folded into `Internal`. The `auth_required` / `insufficient_role` response codes are unchanged. The `details.message` for an expired timestamp now reads "Request timestamp expired" (AuthError's wording) instead of "Timestamp expired or too far in the future". * refactor: make provider-auth chain-agnostic, drop its subxt dependency `MembershipResolver` was already the right seam — the chain-backed implementation was just sitting on the wrong side of it. Move `ChainMembershipResolver` and the dynamic `Buckets` decoding helpers into `provider-node::membership`, leaving `provider-auth` with the trait, the TTL cache and the static test resolver. `provider-auth` no longer depends on subxt, so client-side consumers that only want `build_auth_header` (clients/storage today, a wasm or CLI client tomorrow) stop pulling in a chain client and a membership cache to get a 25-line header formatter. Back on the node's side of the trait the resolver can hold the `ChainWatch` receiver directly again, which removes the `CurrentApi` boxed closure and the closure-plumbing at its two construction sites. Reconnect behaviour is unchanged: the connection is still resolved per lookup, not pinned at construction. The `collect_members` nesting regression test moves with the decoder it covers, and `chain_resolver_fails_cleanly_before_first_connect` moves next to the resolver it exercises. * fix: drop needless borrow flagged by clippy in verify_signature `wrap_bytes` returns an owned `Vec<u8>`, which already satisfies the `AsRef<[u8]>` bound on `Pair::verify`, so borrowing the temporary trips `clippy::needless_borrows_for_generic_args` under `-D warnings`. Both verify calls now pass their message argument the same way. Fixes CI on 71a3151, where inlining the wrapped payload turned a borrow of a local binding into a borrow of a temporary. * refactor: give provider-auth a role module, fold find_role into the cache entry Two related tidies in provider-auth. `RequiredRole` and the granted-vs-required matrix sat in `verify.rs` between signature parsing and role enforcement. Move them to their own `role` module so the policy is one small, directly testable thing, and match over the `(required, granted)` pair rather than on `required` alone. Previously `RequiredRole::Reader => true` covered every granted role implicitly, so a new variant on `storage_primitives::Role` would compile clean and be answered by accident; now it fails to compile until given an explicit answer. `find_role` was a free function taking the member slice and an account — `CachedMembership` already owns that slice, so make it `role_of` on the entry, alongside `is_fresh` for the TTL check. The fetch path builds the entry first and reads the role from it, so no call site needs the loose helper. No free functions remain in the module. `RequiredRole` is still re-exported from the crate root, so no call site changes. * refactor: give membership resolution a typed signature `fetch_members` returned `Result<Vec<(AccountId32, Role)>, String>`, so a transient chain outage and a decode bug arrived at the HTTP layer as the same opaque string and both came back as 500. Replace the error with `MembershipError`, split by what the caller should do: `Unavailable` is worth retrying and now maps to 503 `membership_unavailable`, `Decode` stays a 500. `AuthError::MembershipLookup` takes it via `#[from]`, so `require_role` is just `.await?`. Replace the tuple with a named `Member { account, role }` — the name the pallet, the generated bindings and the design doc already use. Construction goes through `From<(AccountId32, Role)>`, so call sites keep passing tuples. Also fold the `role` module back into `membership`: once `Member` lived there both modules were about roles, and the exhaustive `(required, granted)` match that module was for is unaffected by where it sits. * docs: prefer keeping crates/providers free of subxt Records the guidance from PR #319 review: express what a `crates/providers` crate needs as a trait and let provider-node supply the subxt-backed implementation, so swapping the chain client stays a provider-node change. Adds a matching (non-blocking) crate-boundary criterion to the review skill. * refactor: fold auth config into an Authenticator `require_role` took six arguments, mixing per-request values with settings that never change: `membership` and `max_skew` both came from `ProviderState` on every call. Introduce `Authenticator`, which owns those two, and make `require_role` a method taking only the request (`auth_header`, `method`, `bucket_id`, `required`). `ProviderDeps` and `ProviderState` each lose a field, so the ~27 fixtures that built a cache and a skew separately now build one thing. Constructors take `impl MembershipResolver + 'static` rather than `Box<dyn MembershipResolver>`, so callers stop writing `Box::new` — the boxing is the cache's business. A forwarding impl on `Box<T>` keeps an already-erased resolver acceptable, which is the only way to choose one at runtime; there is a test for exactly that. Narrow the crate's public API to what a consumer can use: `verify_signature` and `MembershipCache` have no callers outside `provider-auth` and there is no way to hand a cache to an `Authenticator`, so both are now internal. Adding them back later is non-breaking; removing them later would not be. * fix: import Authenticator in provider-node's lib tests The six fixtures in `mod tests` referred to `Authenticator` unqualified while the rest of the file spells `provider_auth::` out in full, so the lib test target failed to build (E0433). Also stop the dynamic decode from papering over shapes it cannot read. `MembershipError::Decode` was only produced by the SCALE decode itself, while the hand-rolled tree walk degraded silently: an unrecognised `Role` variant became `Role::Reader`, and a member set we could not walk became "no members", i.e. a 403. Both now return `Decode`. A bucket that does not exist still returns an empty set, since that is a genuine "not a member" — `create_bucket` seeds an admin and `remove_member` refuses to drop the last one, so an existing bucket always has members. Add rustdoc to the public items that lacked it: `Member`'s fields, `RequiredRole`'s variants, and `MembershipResolver::fetch_members`. * refactor: read bucket membership through the typed storage-subxt bindings Carries PR #324's intent to where #319 moved the chain-backed resolver: `provider-node`'s `ChainMembershipResolver` now reads `StorageProvider.Buckets` through the static `storage-subxt` address (`unvalidated`, as elsewhere in the node) with the native `u64` key, instead of a dynamic query plus a scale_value tree walk. - Convert the decoded members inline via `storage-subxt`'s generated `Role` -> `storage_primitives::Role` conversion. - Drop the scale_value walking helpers and their nesting-shape tests, dead now that decoding is typed. - A zero-member bucket only warns: with typed decoding there is no shape left to misread, so it is a chain-side surprise, not a decode failure, and the caller already reads an empty set as "not a member". * chore: trim the membership docs and unduplicate the unvalidated rationale - State the `unvalidated()` policy once, as `storage-subxt` crate docs, and cut both call sites down to a pointer. - `member_roles` takes the member vec instead of the whole `Bucket`, so its test no longer spells out six unrelated bucket fields to reach `members`. - Shorten the comments the typed read left behind: the empty-bucket note no longer explains the decoding it replaced, and the helper doc no longer restates its signature. - Widen the `MembershipResolver::fetch_members` contract: an empty set now also means an existing bucket with no members. * docs: scope the unvalidated() rationale to storage reads The policy note read as though it covered extrinsic submission too, but every call still goes through `subxt::dynamic::tx` and the "a mismatch fails at decode" argument does not hold there: a shifted index encodes a different call instead of failing. --------- Co-authored-by: Branislav Kontur <bkontur@gmail.com>
…ted as 503 - Change Error::Connection to wrap subxt::error::OnlineClientError via #[from] instead of stringifying it - Map provider_chain::Error::NotConnected to a new retryable Error::ChainUnavailable (503) in provider-node, keeping other chain errors as internal - Update chain_connection tests to assert on error variants instead of message substrings
- Replace Ok(chain_connection::current_api(&self.chain_rx)?) with .map_err(Into::into), matching the conversion style already used at api.rs:169
…lenge_refactoring
* Bump subxt 0.44.3 -> 0.50.2 across all chain-facing crates
Mechanical migration to the redesigned subxt 0.50 API, no behavior
change intended:
- blocks().subscribe_finalized() -> stream_blocks(); block-level
events via block.at().events().fetch()
- storage().at_latest().fetch/iter -> at_current_block() +
storage().try_fetch/iter; addresses are now <KeyParts, Value>
generic and keys are passed at call time
- constants().at(..) -> at_current_block().constants().entry(..)
- tx() -> at_current_block().transactions(); dynamic tx payload
construction is unchanged
- event decoding: variant_name() -> event_name(), field_values() ->
decode_fields_unchecked_as; decoded values drop the u32 type-id
context (Value<u32> -> Value)
- fetch_raw missing values surface as StorageError::NoValueFound
instead of None; call sites map it back to preserve error strings
Verified: 424 tests pass, clippy -D warnings clean, and the live
zombienet flows (just demo, fs-demo-ci, s3-demo-ci) all pass against
a running relay + parachain + provider.
Known semantic change inherited from subxt 0.50: transaction nonces
are read from state at the anchored finalized block rather than the
pool-aware system_accountNextIndex, so a transaction submitted within
one finality lag of a previous same-account transaction (across
processes) can see a stale nonce. In-process flows are unaffected
because every submission waits for finalization.
* Make provider coordinators event-driven off a single chain connection
One finalized-block follower (the chain-state coordinator) now owns the
chain connection and fans out decoded events; the polling coordinators
react to events instead of scanning storage maps on fixed intervals.
- New chain_connection module: the single construction site for the
chain client, published to all consumers through a watch channel. The
provider's three independent sockets collapse into one connection.
- New chain_events module: per-block BlockEvent fan-out (broadcast
channel) decoded once by the follower - the block clock, challenge
creation, replica agreements, checkpoint updates, and a Resubscribed
marker after every (re)connect.
- chain-state coordinator: adds a 60s finality-stall watchdog (a stream
that yields no finalized block gets torn down and rebuilt - a stalled
backend otherwise hangs forever with no error) and republishes the
connection on every rebuild.
- challenge responder: reacts to ChallengeCreated events with a
targeted point read of the challenge (the event carries the id, not
the proof parameters). The full Challenges scan now runs only at
bootstrap/resubscribe and on a slow safety net (default 300s, was
every 6s; 0 disables) - a missed challenge means getting slashed, so
the event path is backstopped rather than trusted blindly.
- replica-sync coordinator: duty passes run on relevant agreement and
checkpoint events plus the same bootstrap/safety-net pattern (default
600s, was every 12s).
- checkpoint coordinator: clocked by finalized blocks instead of wall
time (checkpoint windows are a function of block height); the duty
source itself remains the pre-existing stub.
- SubxtChainClient and the auth membership resolver borrow the shared
watch connection; extrinsic submission gains a bounded retry that
treats pallet duplicate-rejections (ChallengeNotFound,
CheckpointAlreadySubmitted, SyncTooFrequent) on the retry as proof
the first attempt landed.
Verified: 282 provider-node tests pass including new
tests/event_fanout_integration.rs; clippy clean; live zombienet check
confirmed a challenge autonomously defended through the event path
alone (safety-net scan disabled, client never responding).
* Add embedded smoldot light-client transport to the provider node
The provider can now follow the chain without any operated RPC
infrastructure: --chain-transport light boots an embedded smoldot
instance that warp-syncs the relay chain and derives parachain
finality from it. The RPC transport remains the default; the switch
is config-level because all chain access already flows through the
chain_connection seam introduced with the event-driven coordinators.
- ChainTransport::Light { relay_spec, para_spec } with SpecSource
being either a vetted spec file (production: trust-preserving) or
fetched from a node's RPC at startup (dev convenience for zombienet,
which regenerates genesis every run; logged loudly as trust-defeating).
- ChainHandle keeps the LightClient alive; the follower's stall
watchdog rebuilding the connection therefore boots a fresh smoldot,
recovering even from a wedged smoldot background task (the silent-
hang failure mode tracked upstream as subxt#1536).
- The first finalized block after a (re)connect gets a 300s budget
(warp sync + peer discovery) before the 60s stall watchdog applies.
- scripts/test-light-client.sh: manual e2e against a running zombienet.
It synthesizes the light-client parachain spec from zombienet's
network directory - convert-to-raw genesis (smoldot cannot execute
runtimeGenesis), inject the collator's boot nodes, and align
relay_chain with the relay spec's exact id (smoldot matches
parachain to relay by exact id; omni-node tolerates the westend-local
vs westend_local_testnet mismatch, smoldot does not).
Verified live: the provider ran with --chain-transport light against
a local zombienet, synced the relay + parachain via smoldot, followed
finalized blocks, and served its on-chain registration on /info.
282 provider-node tests pass; clippy clean.
* Cover the migrated chain-read paths with mock-RPC-backed tests
The patch-coverage gate flagged the subxt 0.50 migration's rewritten
chain reads (RealChainStateClient, the finalized-block follow loop) as
untested - previously they were only exercised against a live chain.
Back a real OnlineClient (legacy backend) with subxt-rpcs'
MockRpcClient and the repo's tracked runtime metadata snapshot, so the
tests run the actual dynamic storage/constants/events machinery:
- RequestTimeout constant lookup, asserted self-consistent against the
raw constant bytes in the same metadata
- Providers reads: absent -> None, and a full round-trip where the
storage value is scale_value-encoded against the real runtime
ProviderInfo type and decoded back (this immediately caught that the
minimal decoder fixture lacks the runtime's public_key field)
- the whole follow() pipeline over one finalized block: header
subscription, System.Events decode, ProviderRegistered recognition,
and the resulting provider-state refresh and nonce bootstrap
connect_and_follow is split into connect + follow(api) so the tests
can drive the full loop over the mock connection; behavior unchanged.
* Classify transaction failures by typed error variants
Replace the substring matching in submit_and_finalize with structural
matching on subxt 0.50's split error types, following the same style
polkadot-sdk adopted in its 0.50 migration (paritytech/polkadot-sdk#12096):
- a dispatch failure is TransactionFinalizedSuccessError::SuccessError(
TransactionEventsError::ExtrinsicFailed(_)) - anything else means the
watch or transport died before a verdict, which is the retryable class
- a duplicate rejection is a DispatchError::Module whose metadata-resolved
pallet is StorageProvider and whose error variant is one of
ALREADY_DONE_ERRORS, instead of a substring of the formatted message
* Construct ChainHandle through a from_api helper
The merge from the base branch brought mock-connection tests that
built ChainHandle with struct-literal syntax; the _light keep-alive
field added on this branch is private outside chain_connection, so the
RPC arm and those tests now construct handles via from_api.
* Cover the merge's new lines: From impls, connection plumbing, loop arms
The patch-coverage gate flagged the static-bindings merge additions:
- the three untested BlockEvent From impls get direct construction
tests, and the mock-RPC follower fixture now emits a ChallengeCreated
alongside ProviderRegistered, so the static decode path runs against
real runtime metadata and the fan-out broadcast is asserted end to end
- chain_connection: unreachable-chain connect and pre-connect
current_api error paths
- auth: the membership resolver's pre-connect lookup error path
- checkpoint coordinator: block-tick arms (irrelevant event skipped,
NewBlock/Resubscribed duty check, paused skip) via a live channel
- replica-sync: BucketCheckpointUpdated relevance both ways (bucket
held locally vs not) and a full duty pass through sync_and_confirm
to PrimaryUnavailable (new root, no reachable primaries, no network);
the checkpoint mock is reused crate-wide instead of duplicated
* Allow smoldot's license for the light client; cover Light error paths
Enabling subxt's light-client feature put smoldot and smoldot-light
into cargo-deny's feature-resolved graph for the first time, and their
GPL-3.0-or-later WITH Classpath-exception-2.0 expression was not in
the allow list. Allowed via exceptions scoped to those two crates -
compatible with the GPL-3.0-only provider node, and the exception only
grants additional linking permissions - rather than allowing the
expression workspace-wide.
Also covers the Light transport's failure paths for the patch gate:
missing spec file, invalid spec contents rejected by smoldot, and an
unreachable spec-fetch node.
* Use expect_err in chain_connection tests so no panic line stays uncovered
The else { panic! } idiom left the panic call as an executable line that
never runs when the test passes, dragging the file's patch coverage below
the gate. .map(|_| ()).expect_err() asserts the same thing without an
uncovered branch; chain_connection patch coverage rises to 82%.
* Use O(1) get_bucket lookup in replica-sync event relevance check
* Remove dead poll_interval from CheckpointCoordinatorConfig
The coordinator is clocked by finalized-block events, not wall time; the
field was never read.
* Serialize transaction submission across coordinator clients
All coordinators sign with the same account, and subxt 0.50 reads the
nonce from finalized state rather than the pool-aware account-next-index,
so concurrent submissions would pick the same nonce and one would be
rejected. A shared async mutex held across submit-and-finalize removes
the race.
* Add BlockEventTx/BlockEventRx aliases for the event fan-out channel
Keeps the tokio broadcast plumbing out of coordinator signatures.
* Note event-decode mapping revisit in a TODO
* Queue events while paused instead of dropping them
Both coordinators drained the event channel while paused and discarded
what they read, so a challenge or replica duty announced during a pause was
recoverable only by the safety-net scan — and with the scan disabled
(poll_interval = 0), not at all.
Gate the select arm on !paused so events stay in the broadcast buffer. Replay
on resume is safe: a challenge is point-read against live chain state before
responding, and a duty pass is an idempotent reconciliation. A pause longer
than the channel's capacity surfaces as Lagged, which already reconciles with
a full scan. auto_respond / auto_confirm keep draining, since no later state
change makes those events actionable.
* Replace the submit retry loop with two explicit attempts
The 0..2 loop encoded "retry once" in a loop bound and needed an unreachable!
to convince the compiler it terminated. Lift one submit-and-watch pass into
try_submit, returning Landed / Retryable / Rejected, and have
submit_and_finalize call it twice. Same behaviour, retry count visible in the
control flow, and totality proven by the match.
* fix: bound each submit-and-watch pass so submit_lock can't wedge
submit_lock is held across both try_submit calls and the retry delay
between them, and nothing bounded the passes: a transaction watch that
neither resolves nor errors would block every later duty submission for
good.
Wrap each pass in a 120s timeout — generous next to normal finalization,
so a merely slow chain isn't mistaken for a stuck one. A timed-out pass
is Retryable, the same classification a dropped watch already got, so
the resubmit plus duplicate-rejection path still recognises a
transaction that landed while the watch hung. Bounding per pass rather
than the whole critical section keeps the retry alive; the total lock
hold is now at most 2 * SUBMIT_TIMEOUT + RETRY_DELAY.
* refactor: drop the unused BlockEvent::NewBlock variant
It was only ever sent by the block follower and asserted in one test; no
coordinator matched on it. They take their clock from the
current_anchor_block atomic plus their own safety-net interval, so the
per-block broadcast served nobody.
* Drop the auth-bypass flag from the light-client e2e script
dev's #320 removed --disable-auth-i-know-what-i-am-doing; auth is now
always on and tuned via --auth-cache-ttl/--auth-max-skew. The script's
only authenticated-endpoint need was /info, which takes no auth
extractor, so dropping the flag is the whole fix.
* Address review: bound chain bootstrap, run light-client e2e in CI
- Wrap connect (smoldot peer discovery + warp sync) in a 300s budget and
the subscribe/bootstrap reads in a 60s budget so a wedged connection
returns Err to the reconnect loop instead of hanging forever with the
stale handle still published; the block loop now uses the flat 60s
stall timeout from the first block.
- Run scripts/test-light-client.sh as the last step of the zombienet
smoke-test job, after the demos, since it shares the //Alice key.
- Rename ChainTransport::Light to ChainTransport::LightClient.
* Fold --relay-rpc into the spec flags as path-or-URL values
--relay-chain-spec and --para-chain-spec now accept either a spec file
path or a ws:// / wss:// node URL to fetch the spec from (dev only),
mapping 1:1 onto SpecSource. This drops the --relay-rpc flag and the
four-way fallback match; the para spec still defaults to fetching from
--chain-rpc.
* Extract with_timeout so the reconnect budgets are testable
The timeout-expiry map_err closures were unexecutable in tests and
failed the patch-coverage gate. One shared helper maps expiry to an
Error; tests cover expiry (paused time), result passthrough, and
connect_and_follow's error path.
* test: derive the light-client para spec from the genesis state root
Replace the chain-spec-builder convert-to-raw + bootnode merge with a
minimal para spec: the live spec's identity and boot nodes plus a
stateRootHash-only genesis, fetched from the para node's RPC. smoldot
never executes parachain genesis (it derives the head from the relay),
so the genesis state root alone identifies the chain.
This is also the spec shape a production deployment would vet and ship
(polkadot-bulletin-chain#716 checks in exactly this form), and the
script no longer needs .bin/chain-spec-builder.
Verified against a live zombienet: smoldot derives the correct para
genesis hash from the trimmed spec and the provider syncs its on-chain
registration through the light client.
* fix: make FetchFromRpc spec sources actually work
subxt's fetch_chainspec_from_rpc_node calls sync_state_genSyncSpec,
which omni-node does not expose at all (the para spec fetch always
failed with "Method not found") and which on relay nodes returns a
spec with an empty bootNodes list (smoldot had no peers and hung until
the connect timeout). The dev fetch path therefore worked in no
environment.
Replace it:
- relay: sync_state_genSyncSpec, with the lightSyncState checkpoint
dropped (vetted spec files don't carry one, and a checkpoint start
leaves smoldot waiting minutes for the next GrandPa commit on quiet
dev relays) and the node's own listen addresses injected as boot
nodes when the generated spec has none.
- para: assembled from ordinary RPC calls — genesis state root,
boot-node addresses, para id — since a parachain spec needs no
genesis state or checkpoint; smoldot derives the para head from the
relay, and the relay_chain id comes from the loaded relay spec.
connect() now also waits for the first finalized block on the light
transport, so the chain-state follower's tight bootstrap budget starts
on a synced connection instead of rebuilding (and thereby resetting) a
still-syncing smoldot.
test-light-client.sh now exercises this path end to end: no spec
files, no chain-spec-builder — both specs come from the running
zombienet's RPCs (verified locally: PASSED in 45s).
---------
Co-authored-by: Branislav Kontur <bkontur@gmail.com>
Co-authored-by: Tung Bui <79790753+danielbui12@users.noreply.github.com>
* refactor(provider-node): transparent Backend variant for storage errors The node enum mirrored four provider-storage variants (ChildrenMissing, QuotaExceeded, InvalidStartSeq, RootNotFound) that nothing in the node constructs directly, plus a field-for-field From impl to carry them over. Replace the mirrors with a single #[error(transparent)] Backend(#[from] ...) variant, mapped by an exhaustive (no wildcard) storage_response(): every new storage variant still fails compilation until it gets an explicit HTTP status, but without declaring and converting it twice. Variants the handlers build directly (NodeNotFound, BucketNotFound, InvalidHash, Storage, Serialization) stay node-native. JSON shapes and status codes are unchanged. * refactor(provider-node): fold storage lookup misses into the Backend path NodeNotFound and BucketNotFound existed twice: as storage-crate error variants flowing through Backend, and as node variants the handlers built by hand when a get_node/get_bucket Option came back None. Those misses are storage-domain facts, so the six handler sites now construct the storage-crate error (converted by the derived From via ?) and the two node variants are gone — one representation and one response shape per failure kind. Deliberately NOT done by flipping get_node/get_bucket to Result: 50+ call sites use the Option form, several intentionally (existence probes in coordinators, missing-node-tolerant tree walks), so the signature change would ripple far for no behavioral gain. JSON and status codes unchanged.
* Wire provider auth membership cache to chain-state coordinator
Closes the auth revocation staleness window: MemberSet/MemberRemoved/
BucketDeleted events are now surfaced as typed StorageEvent variants and
invalidate the affected bucket's MembershipCache entry on every finalized
block, instead of only expiring on TTL.
Also fixes a lost-update race in MembershipCache::get_role where a chain
fetch already in flight could overwrite a concurrent invalidate() with
stale membership, silently reopening the revocation window it was meant
to close.
* fix: guard membership cache against in-flight-fetch resurrection
- Add a generation counter to each MembershipCache slot, bumped on
invalidate(), so a fetch already in flight when an invalidation lands
cannot overwrite the cache with stale pre-invalidation members
- Add Authenticator::invalidate_bucket to drop a bucket's cached
membership so the next request re-resolves it instead of waiting out
the TTL
* feat: drive membership cache invalidation from real-time chain events
- Remove Arc<Authenticator> from ChainStateCoordinator; it now broadcasts
BlockEvent::BucketMembershipChanged / Resubscribed on the existing
per-block fan-out instead of pushing into the authenticator directly
- Add MembershipInvalidations trait + Invalidation enum to provider-auth,
and have MembershipCache drain the feed at the top of every lookup
- Add BlockEventInvalidations in provider-node, translating the chain-state
coordinator's broadcast into that feed; wired via Authenticator::with_invalidations
- Delete Authenticator::invalidate_bucket and invalidate_membership_for_buckets,
which are no longer needed now that the cache pulls invalidations itself
- Replace the per-bucket CacheSlot.generation guard with a single cache-wide
AtomicU64 epoch: invalidate() now removes the entry outright instead of
leaving a blanked slot behind, fixing an unbounded leak where every
chain-wide membership event permanently grew the map by one entry for
buckets this provider never serves
- Add regression coverage for in-flight fetch races (per-bucket invalidate,
wholesale invalidate_all, and invalidation of a never-cached bucket) and
for the fixed leak
* fix: react to BucketCreated in chain-driven membership invalidation
- Include BucketCreated alongside MemberSet/MemberRemoved/BucketDeleted so newly created buckets invalidate the membership cache
- Reorder the membership cache write to take the shard lock before reading the epoch, closing the invalidation race
- Update coordinator tests to cover the new bucket-created event and use test_chain_state() for on-disk backed ChainState
* feat: bound stale-on-error membership with --auth-max-stale
- Add a max_stale ceiling to MembershipCache's stale-on-error path: past it,
a lookup refuses the request instead of serving unboundedly stale membership
- Default max_stale to a multiple of the TTL, overridable via
Authenticator::with_max_stale and surfaced as --auth-max-stale (default 300s)
- Add CachedMembership::age() and express is_fresh in terms of it
- Replace Authenticator::new and MembershipCache::new's positional ttl/max_skew
arguments with with_ttl/with_max_skew/with_max_stale builders, closing a
silent-swap hazard between two adjacent Duration parameters
- Update call sites in provider-node and clients/storage for the new builder API
* docs: correct membership caching docs for the max_stale bound
- Replace the stale-while-revalidate design-doc bullet with one naming all
three bounds (chain-event invalidation, auth-cache-ttl, auth-max-stale)
and the 503 refusal, plus a bullet on undecodable Role denying access
- Rewrite the provider-node README caching paragraph: a lagged or
reconnected feed invalidates cached buckets immediately rather than
falling back to the TTL, and document the auth-max-stale bound and its
503 refusal past it
* refactor: decode BucketMembershipChanged via the static event bindings
- Move membership-event decoding (BucketCreated, MemberSet, MemberRemoved,
BucketDeleted) into chain_events.rs as From impls on BlockEvent, using
the same static storage-subxt bindings as every other coordinator event
- Delete parse_membership_changes and its dynamic Value/field-lookup
decoding from chain_state_coordinator.rs, along with its now-redundant
broadcast loop in follow()
- Drop the "one exception" carve-out from chain_events.rs's module doc
now that BucketMembershipChanged decodes the same way as everything else
- Retarget the coordinator's membership tests onto decode_block_events and
add direct unit tests for the four new event conversions
* refactor: simplify max_stale to a flat default instead of a ttl-derived one
- Replace MembershipCache's Option<Duration> max_stale field, its
MAX_STALE_TTL_MULTIPLE derivation, and the max_stale() accessor with a
single plain Duration field defaulting to DEFAULT_MAX_STALE (300s)
- with_max_stale now overwrites the field directly instead of wrapping it
in Some(..)
- Update with_max_stale's rustdoc to state the flat default instead of
"a multiple of the TTL"
- Fix the one test that relied on with_ttl(ZERO) implicitly deriving
max_stale to zero, by setting with_max_stale(ZERO) explicitly
* refactor: fix stale rustdoc, derive Eq on Invalidation, escalate undecodable membership events
- Rewrite start_chain_state_coordinator's rustdoc to describe the actual
broadcast/drain direction instead of the old push model
- Derive Debug/Clone/PartialEq/Eq on Invalidation and switch the feed-drain
tests from matches! to assert_eq! for real diagnostic output on failure
- Consolidate the duplicated FlakyResolver and QueuedInvalidations test
doubles into a shared test_support module in the auth crate
- Log the block number and escalate to a wholesale Resubscribed re-scan when
a membership-changing event fails to decode, instead of silently dropping
it for up to the auth cache TTL
* fix: close the undrained-invalidation window on the error path
- Re-drain the invalidation feed in get_role's error arm before falling
back to a cached entry, so a change for this bucket that arrived
during the in-flight refetch (and that no other request drained)
is applied before the stale-if-error check runs
- Fix auth_max_stale/auth_cache_ttl incoherence warning to trigger on
equal values too, not just strictly-below
- Add coverage for an unrelated bucket's invalidation still allowing
stale-if-error, and for an undrained same-bucket event refusing it
* test: add provider HTTP auth e2e workflow
- Add workflow 12: exercises the provider node's signed-request guard
end to end over real HTTP - role ladder enforcement, rejection of
unsigned/tampered/expired signatures, and that on-chain membership
changes (set_member/remove_member) are honoured by the provider
* fix: bound the membership cache with a moka store
- Replace the unbounded DashMap membership cache with moka::future::Cache,
capped by a new max_entries (default 10,000, W-TinyLFU eviction)
- Add a per-entry expiry policy: non-empty entries expire at max_stale,
empty ones at ttl, so a nonexistent-bucket lookup can no longer grow the
map without limit
- Re-derive the lost-update guard against concurrent invalidation without
DashMap's shard lock: check-then-insert-then-verify against a monotone
epoch counter
- Make invalidate/invalidate_all async to match moka's API; invalidate_all
now uses moka's own O(1) invalidation instead of a full map clear
- Add --auth-cache-max-entries / AUTH_CACHE_MAX_ENTRIES to the provider CLI
and log it alongside the existing auth timings at startup
* fix: repair membership cache refresh and invalidation gaps
- Re-derive residency on cache refresh (expire_after_update), fixing
stale-if-error grace that silently shrank toward zero for any
bucket looked up more than once
- Drop the now-redundant manual max_stale age check in get_role; the
cache's own expiry is the sole owner of residency
- Add BlockEvent::MembershipScopeUnknown so a decode failure or
unreadable block invalidates only the membership cache instead of
reusing Resubscribed and triggering full reconciliation scans in
unrelated coordinators
- Escalate transient block-read failures in the chain-state
coordinator to MembershipScopeUnknown, closing a window where a
revoked member kept authorizing on an otherwise healthy node
- Add e2e coverage (workflow 12.13): scan 12,000 bucket ids past the
cache's default entry ceiling and assert the scan is refused while
a real member stays authorized
- Document the cache's entry-count bound and eviction behavior in
provider-node/README.md
* fix: bound the e2e bucket-id scan so it cannot exceed the CI timeout
- Shrink workflow 12's bucket-id scan from 12,000 to 200 ids; the old
size was sized to cross the cache's 10,000-entry default and could
take long enough on a loaded runner to exceed the job timeout
- Add an auth-cache-max-entries input to the start-provider action so
a job can size the cache down instead of scanning past a large default
- Set auth-cache-max-entries to 64 for the e2e job's provider, so the
200-id scan still crosses the ceiling and exercises real eviction
- Add a per-request timeout to the workflow's fetch helper so a hung
provider call fails fast instead of stalling the whole suite
* refactor: use signProviderRequest's timestamp param for the skew test
- Add a time parameter to signProviderRequest so callers can sign for an
arbitrary timestamp instead of always the current one
- Remove the hand-rolled staleAuthHeader helper in the e2e auth workflow
and build the skewed header via signProviderRequest directly
- Shorten the membership-change comment in the e2e auth workflow
* refactor: consolidate gated resolver test doubles into Gated<R>
- Move CountingResolver into test_support.rs so it's shared rather than
redefined per test module
- Replace GatedResolver and GatedFlakyResolver with a generic Gated<R>
wrapper that gates any inner resolver's answer behind a proceed flag
- Update membership.rs call sites to compose Gated with CountingResolver
or FlakyResolver, adjusting proceed's initial state where the gate now
also covers the seeding call
* fix: update stale ChainHandle test constructor after dev merge
- Switch the remaining ChainHandle { api } struct literal in
chain_state_coordinator's tests to ChainHandle::from_api(api), matching
the constructor introduced upstream now that the field is private
---------
Co-authored-by: Ilia Churin <ilia@parity.io>
- Replace Error::ChainUnavailable with a transparent Chain(#[from] provider_chain::Error) variant - Map chain sub-errors to status codes exhaustively in into_response, matching the existing Backend pattern - Update tests to construct chain errors via .into() and cover both the retryable and internal status codes
…derState - Trim ChallengeProofSource to its two storage methods and drop provider_id() - Add StorageProofSource, a newtype over Arc<dyn StorageBackend> in provider-node that implements ChallengeProofSource, plus unit tests exercising it directly - Make ChallengeResponderConfig.provider_account a required AccountId32 field (replaces the Default impl with ChallengeResponderConfig::new) - Parse the provider account at responder startup the same way the chain-state coordinator does, skipping the responder on an invalid SS58 - Update coordinator integration tests to the new config and proof-source API
…ith cause-based variants - Replace ChallengeError's three String buckets (Chain/Storage/Internal) with ProofDataMissing, StorageUnavailable, ChainUnavailable, ChainRejected, and Shutdown, each naming a distinct condition and the response it implies - Add ProofTarget to identify what proof data a failure was about (MMR leaf or chunk), and is_retryable()/risks_slashing() accessors on ChallengeError - Mark ChallengeError non_exhaustive with no catch-all variant - Change StorageBackend::get_node/get_bucket from Option<T> to Result<Option<T>, Error> so a backend failure (RocksDB error, undecodable record) can no longer be reported the same way as genuine absence - Make collect_chunk_hashes propagate read failures instead of silently skipping unreadable nodes, which used to shorten the chunk list and make a backend fault indistinguishable from a missing chunk - Update StorageProofSource to classify storage errors into the new ChallengeError variants instead of stringifying them - Update respond_to_challenge to classify by cause rather than by which proof-gathering step failed, and log slashing-risk failures at error level with structured fields - Update all call sites in provider-node and the challenge coordinator tests to the new signatures
…or tests - Replace ChallengeError variant tests (which only re-asserted matches! against their own definitions) with tests against respond_to_challenge and the responder run loop - Add mock ChallengeProofSource and ChallengeChainClient to drive respond_to_challenge through success, missing-data, backend-failure, and chain-rejection paths, asserting the chunk lookup uses the data root from the MMR leaf and that submissions match - Add run loop tests covering provider filtering, stale challenges, auto_respond disabled, Resubscribed/Lagged reconciliation scans, pause/resume event queuing, the safety-net interval, and a closed event channel not starving the scan - Add tokio test-util dev-dependency for paused-time tests
danielbui12
marked this pull request as ready for review
September 9, 2026 11:17
Collaborator
|
/aireview |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🤖 AI-generated review — advisory only, not a substitute for human review.
Summary
- Replaces string-based ChallengeError variants with structured, cause-based variants indicating retryability and slashing risk.
- Updates StorageBackend read methods to return Result<Option, Error> to distinguish absent entries from storage backend failures.
- Identified a bug in collect_chunk_hashes where missing tree nodes are still silently skipped rather than returning an error.
Correctness
- Missing nodes are silently skipped in collect_chunk_hashes
- Impact: If an intermediate or leaf node is absent from storage, collect_chunk_hashes silently skips it and returns a truncated list of chunk hashes. This causes chunks after the missing node to be misindexed, produces invalid Merkle proofs for existing chunks, and obscures the missing node.
- Fix: In crates/providers/storage/src/backend/mod.rs within collect_chunk_hashes, replace the
if let Some(node) = self.get_node(&hash)?check with an explicit error return such as.ok_or_else(|| Error::NodeNotFound(...))when a node is missing.
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
4 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.
Follow-up #333 (comment)