refactor: replace dynamic subxt calls in clients/ with storage-subxt - #318
refactor: replace dynamic subxt calls in clients/ with storage-subxt#318danielbui12 wants to merge 9 commits into
Conversation
- Move client/ to clients/storage, storage-interfaces/file-system/client to clients/file-system, and storage-interfaces/s3/client to clients/s3 - Remove storage-interfaces/: interface READMEs move next to their crates (clients/s3/README.md, clients/file-system/INTERFACE.md); delete five orphaned examples that were never registered as cargo targets - Update workspace members and path dependencies in the root Cargo.toml - Replace client/ and storage-interfaces/ license-header globs with clients/**/*.rs and rewrite the coverage ignore regex for the new paths - Update path references and relative links across docs
- Rewrite all extrinsic builders, storage reads, constants, and event decoding in storage-client, file-system-client, and s3-client against the generated storage-subxt bindings; zero subxt::dynamic/scale_value usage remains under clients/ - Add clients/storage/src/convert.rs: conversions between sp_runtime/ storage_primitives types and the generated runtime types, shared by all three crates - Fix CheckpointManager on-chain submission: it targeted a nonexistent submit_commitment extrinsic; now submits the real checkpoint call, with the provider nonce threaded into CommitmentCollection and same-root signatures grouped by (start_seq, leaf_count, nonce) - Fix Challenges reads: it is a (deadline, index) double map, not deadline -> Vec; indices now come from typed storage keys instead of vec positions (list_my_challenges, list_active_challenges, check_and_claim_reward) - Fix agreement-terms encoding dropping ReplicaTerms::sync_price and provider challenge listing reading leaf/chunk indices at the wrong nesting level - Make checkpoint, challenge_offchain, and confirm_replica_sync builders fallible (validate 64-byte sr25519 signatures) and s3 submit_and_finalize generic over any payload, keeping the stale-nonce retry - Delete the dynamic storage helper modules, hand-rolled scale_value decoders, raw twox_128 key builders, and heuristic SCALE parsers; replace key-byte-offset slicing with typed key decoding - Update INTEGRATION.md to document the static bindings and the just subxt-codegen regeneration flow
- Fix checkpoint submission end to end: the client now picks the nonce (current anchor block) and passes it to GET /commitment, which requires and echoes it. The nonce-less request was rejected with 400, so every provider was classed unreachable and no checkpoint was ever submitted - Validate each provider response before it can join a batch: drop those echoing a different nonce, carrying an undecodable or non-64-byte signature, or reporting a range behind the on-chain snapshot. Previously one bad signature aborted the whole checkpoint, and a provider could install a shorter committed range to shrink its slashing exposure - Gate the consensus threshold on the signatures actually submitted and report only those accounts as signers - Resolve real provider accounts from /info for manually configured endpoints; the placeholder accounts could never pass ProviderNotInSnapshot - Replace the SCALE round-trip in convert::multisig with an explicit variant match, removing a panic path and the codegen derive dependency - Add convert::account_hex and convert::unbounded and route all account rendering and BoundedVec unwrapping through them; rename conversions after their destination type - Stream provider iteration with early termination instead of collecting the whole Providers map, and score challenge candidates from one map scan rather than a point fetch per provider - Hoist storage entries out of loops, fetch S3 bucket infos concurrently, and memoize the immutable drive to bucket mapping - Add SubstrateClient::at_current_block and decoded_key helpers, replacing ~26 duplicated blocks - Return S3ClientError from every s3 SubstrateClient method, expose FileSystemClient::delete_drive, and update EXECUTION_FLOWS.md to the checkpoint extrinsic and nonce-bearing commitment request
ilchu
left a comment
ilchu
left a comment
There was a problem hiding this comment.
My biggest concerns here is that we're silently changing the semantics, and once we tackle that, the mechanical move is perfectly justified.
| assert_eq!(cloned.nonce, 42); | ||
| } | ||
|
|
||
| fn payload_key(start_seq: u64, leaf_count: u64, nonce: u64) -> PayloadKey { |
There was a problem hiding this comment.
Looks like PayloadKey::new() to me.
| } | ||
| } | ||
|
|
||
| fn commitment_response( |
| Note over SC: nonce = current anchor block (client-chosen, one per round) | ||
|
|
||
| loop For each primary provider | ||
| SC->>PN: GET /commitment?bucket_id=X | ||
| PN->>PN: Sign CommitmentPayload | ||
| PN-->>SC: { mmr_root, start_seq, provider_signature } | ||
| SC->>PN: GET /commitment?bucket_id=X&nonce=N | ||
| PN->>PN: Sign CommitmentPayload over the given nonce | ||
| PN-->>SC: { mmr_root, start_seq, leaf_count, provider_signature, nonce } |
There was a problem hiding this comment.
As mentioned by @eskimor in Matrix, we need to be getting rid of nonces in commitments. I understand that this and all the nonce retreading around the checkpoint.rs specifically is meant to fix what was silently broken previously and that nonces themselves are not a new addition of this PR. Still, writing logic around a feature that should not be there sounds pretty wasteful to me. Maybe we could land a cleanup PR for de-noncing before merging this?
| /// Only providers whose response matched the modal | ||
| /// `(start_seq, leaf_count, nonce)` payload are included. | ||
| pub signatures: Vec<(AccountId32, Vec<u8>)>, |
There was a problem hiding this comment.
I believe we could use sp_core::sr25519::Signature instead of opaque bytes as it's already a dependency. And that would remove the need of having length checks or almost any other validation except at HTTP boundary.
| }, | ||
| ); | ||
| } | ||
| Err(e) => tracing::warn!("Failed to decode provider {}: {e}", account.0[0]), |
There was a problem hiding this comment.
| Err(e) => tracing::warn!("Failed to decode provider {}: {e}", account.0[0]), | |
| Err(e) => tracing::warn!("Failed to decode provider ID {}: {e}", account.0), |
Otherwise it's just a single ID byte AFAICT.
|
|
||
| match Self::validated_signature(bucket_id, &account, &commitment, nonce, floor) { | ||
| Some(sig_bytes) => { | ||
| let mmr_root = self.parse_h256(&commitment.mmr_root)?; |
There was a problem hiding this comment.
I know it's not in the diff, but ? operator here means one malicious operator could fail the whole signature collection by malforming the MMR root. So maybe we should push all parsing of the response (i.e. not just the sig, but also the root - pretty much the whole commitment) outside, making it Self::validated_commitment or something. Just keeping it neatly at the boundary.
| // A shrinking range would narrow what the provider can be challenged | ||
| // over — never accept it, whatever the majority says. | ||
| if let Some((floor_start_seq, floor_leaf_count)) = floor { |
There was a problem hiding this comment.
I don't think we have it stipulated anywhere in the design that the client ignores received checkpoints that are lower than what is on chain despite the majority. Does it sound like a reasonable default @eskimor?
| .or_default() | ||
| .push((id.clone(), sig.clone())); | ||
| } | ||
| let (payload, signatures) = modal_payload_group(by_payload); |
There was a problem hiding this comment.
Previously we only grouped responses by mmr_root, but now it's also considering start_seq and leaf_count when choosing the representative payload for checkpoint signature collection. cc: @eskimor
| nonce: payload.nonce, | ||
| signatures, | ||
| agreeing_providers: agreeing.iter().map(|(id, _)| id.clone()).collect(), | ||
| agreeing_providers: agreeing.iter().map(|(id, _, _)| id.clone()).collect(), |
There was a problem hiding this comment.
I think this is a good finding by my assistant with a worked example. Basically the meaning of the agreeing_providers field shifted in this diff and now it could have some providers who didn't sign the checkpoint but because they're included in this collection they could be unreasonably slashed downstream:
Providers A, B, C, D all report the same mmr_root. A, B, C say the range is (start_seq=0, leaf_count=100); D says (0, 120). Payload grouping picks the A/B/C group. The resulting CommitmentCollection now says:
agreeing_providers = [A, B, C, D]— everyone who matched the rootsignatures = [(A,…), (B,…), (C,…)]— who's actually in the on-chain checkpoint
D is in the first list but not the second. check_and_submit internally uses the right set (it thresholds and reports signers from signatures), so the happy path is correct. The footgun is the public struct: anything downstream — persistence, health tracking, monitoring, a UI — that reads agreeing_providers as "who backed this checkpoint" will count D as covered when D has no on-chain liability for that snapshot. Two fields whose names suggest the same set, holding different sets, distinguished only by a doc comment. A rename (root_agreeing_providers vs signers) or folding the dropped ones into their own field would close it.
| // pallet counts signatures, not opinions. | ||
| if collection.signatures.len() < required { | ||
| return CheckpointResult::InsufficientConsensus { | ||
| agreeing: collection.agreeing_providers.len(), |
There was a problem hiding this comment.
This is also a barely noticeable semantic change. The checkpoint threshold used to be a number of providers who submitted just the same root, but now it's same root and range.
So it's once again a question for @eskimor, but mostly circling around the same thing: should we think about the case where we have two honest providers reporting the same root, but somehow their (start_seq, leaf_count) is different? It feels that it should be more or less impossible as a range mismatch under an identical root implies at least one of them is buggy or lying. So what would be the most correct policy for checkpoint signature collection? Is it "pick the biggest sub-group and tie-break toward the larger range" (as currently implied by the changes of this PR) or is it "same root + conflicting range -> drop the minority as misbehaving or maybe surface it as a ProviderConflict"?
2 participants
No description provided.