Skip to content

refactor: simplifying storage by giving it the client directly#3719

Open
SimonRastikian wants to merge 4 commits into
3164-adding-ReconstructionThreshold-in-nodefrom
1645-storage-takes-client
Open

refactor: simplifying storage by giving it the client directly#3719
SimonRastikian wants to merge 4 commits into
3164-adding-ReconstructionThreshold-in-nodefrom
1645-storage-takes-client

Conversation

@SimonRastikian

Copy link
Copy Markdown
Contributor

Fixes #1645 by simplifying the triple generation part.

@claude

claude Bot commented Jul 1, 2026

Copy link
Copy Markdown

PR title type suggestion: This PR restructures the storage API to take a client parameter. The type prefix should probably be refactor: to indicate code restructuring.

Suggested title: refactor: storage takes client

@SimonRastikian SimonRastikian changed the title 1645 storage takes client refactor: simplifying storage by giving it the client directlyh Jul 1, 2026
@SimonRastikian SimonRastikian changed the title refactor: simplifying storage by giving it the client directlyh refactor: simplifying storage by giving it the client directly Jul 1, 2026
@claude

claude Bot commented Jul 1, 2026

Copy link
Copy Markdown

Pull request overview

Simplifies TripleStorage::new to accept &Arc<MeshNetworkClient> directly instead of a separate my_participant_id + alive_participant_ids_query closure — matching the pre-existing PresignatureStorage::new shape. Adds a network::testing::new_test_client helper so unit tests can construct a MeshNetworkClient synchronously with all participants reported alive. Also drops a 4-line explanatory comment in robust_ecdsa::run_key_generation_client that appears unrelated to the storage-signature change.

Changes:

  • TripleStorage::new signature: (clock, db, &client, threshold) — derives my_participant_id and the active-participants query from the client.
  • EcdsaSignatureProvider::new and three cleanup tests updated to the new signature.
  • New network::testing::new_test_client(participants, my_participant_id) returning a light-weight Arc<MeshNetworkClient> for unit tests (no live senders, no real connectivity).
  • Removed the "reconstruction lower bound = t" comment in RobustEcdsaSignatureProvider::run_key_generation_client.

Reviewed changes

Per-file summary
File Description
crates/node/src/providers/ecdsa/triple.rs TripleStorage::new now takes &Arc<MeshNetworkClient>; the internal DistributedAssetStorage gets client.my_participant_id() and ecdsa_common::active_participants_query(client). Test helper new_triple_store updated.
crates/node/src/providers/ecdsa.rs Call site simplified — the two closure arguments collapsed to &client.
crates/node/src/network.rs Added pub fn new_test_client(...) in mod testing that wires up a TestMeshTransportSender with an empty senders map, a fresh NetworkTaskChannelManager, and an IndexerHeightTracker seeded from participants.
crates/node/src/assets/cleanup.rs Three TripleStorage::new call sites in tests switched from Arc<Mutex<Vec<ParticipantId>>> closures to new_test_client.
crates/node/src/providers/robust_ecdsa.rs Removes the 4-line "MaxMalicious = t − 1" comment from run_key_generation_client.

Findings

Non-blocking (nits, follow-ups, suggestions):

  • crates/node/src/providers/robust_ecdsa.rs:127 — The removed comment on run_key_generation_client explained why robust-ECDSA can reuse cait-sith's internal keygen (reconstruction lower bound = MaxMalicious + 1 = t). This is exactly the kind of non-obvious "why" that project engineering standards say to keep. The sister function run_key_resharing_client still carries a similar remark at robust_ecdsa.rs:138, so the deletion also leaves the file inconsistent. This change is also unrelated to the stated PR goal ("simplifying the triple generation part") — consider restoring the comment or splitting this into its own PR with justification.
  • crates/node/src/network.rs:924new_test_client builds the underlying TestMeshTransport with senders: HashMap::new(). Any test that reaches for send/new_channel_for_task on this client will fail with "Unknown recipient" at runtime. Worth calling out in the doc-comment (e.g. "no working transport — do not use for message sending; suitable only for read-only queries like all_alive_participant_ids") so future test authors don't reach for it in the wrong context.
  • crates/node/src/network.rs:937-938IndexerHeightTracker::new(&participants) only seeds heights for the passed-in participants slice. If a caller passes a list that omits my_participant_id, set_height(my_participant_id, ...) would panic (heights.get(...).unwrap()), and all_alive_participant_ids would still push my_participant_id via its final result.push(...), silently masking the inconsistency. All current call sites include my_participant_id, so this is only a latent footgun — a debug_assert!(participants.contains(&my_participant_id)) inside new_test_client would make the contract explicit.

✅ Approved

@SimonRastikian SimonRastikian self-assigned this Jul 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant