fix: Address PR #1 review comments #10
Open
Conversation
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
Change threshold default from hardcoded 1 (minimum) to maximum fault tolerance formula (n-1)/3, matching CLI behavior. JUSTIFICATION: The threshold determines Byzantine fault tolerance - how many malicious or crashed parties the protocol can tolerate. Defaulting to maximum provides the strongest security guarantees by default. Changes: - Add `threshold: Option<usize>` field to StoffelServerBuilder - Add `.with_threshold(t)` builder method for explicit override - Calculate default threshold as (n-1)/3 for n >= 4, otherwise 1 - Add validation: n >= 3t+1 (HoneyBadger constraint) - Add comprehensive unit tests for threshold calculation Trade-off: Higher threshold increases preprocessing cost, but security should not be silently compromised for performance. Users needing faster preprocessing for development can explicitly opt-in via with_threshold(1). Addresses PR #1 review comment 2 (line 338). Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Incoming connections are always from clients (peer connections are established outbound), so the ConnectionType enum and match statement were unnecessary complexity. Changes: - Remove ConnectionType enum (dead Server variant was never used) - Remove match wrapper in handle_incoming_connection - Update function documentation to clarify it handles client connections - Simplify code by removing extra indentation levels Addresses PR #1 review comment (line 1020). Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The hardcoded 30-second timeout for clients waiting for preprocessing
was arbitrary. Changed to a configurable timeout with a more reasonable
default of 60 seconds.
Changes:
- Add `preprocessing_wait_timeout` field to StoffelServerBuilder (default: 60s)
- Add `.with_preprocessing_wait_timeout(Duration)` builder method
- Pass timeout through StoffelServer to handle_incoming_connection
- Replace hardcoded 30s with configurable value
Example usage:
```rust
let server = Stoffel::server(0)
.with_preprocessing_wait_timeout(Duration::from_secs(120))
.build()?;
```
Addresses PR #1 review comment (line 1091).
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
OutputMessage now serves as both output delivery and completion signal. This simplifies the protocol by removing an unnecessary message type. Changes: - Remove ComputationComplete variant from MPCaaSMessage enum - Update protocol documentation - Remove ComputationComplete sending code in server.rs - Client now triggers reconstruction when enough output shares received Addresses PR #1 review comment 12. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
PeerManager was acting as a container without actual connection logic. Since STO-404 will unify networks using QuicNetworkManager directly, remove PeerManager and store peer addresses as Vec<(usize, SocketAddr)>. Changes: - Delete src/mpcaas/peer_manager.rs - Update server.rs to use peer_addresses directly - Remove PeerManager exports from mod.rs and prelude.rs - Update CLAUDE.md module structure Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
cadaniluk
reviewed
Jan 21, 2026
| // | ||
| // Formula: For HoneyBadger, n >= 3t + 1, so max t = (n-1)/3 | ||
| let threshold = self.threshold.unwrap_or_else(|| { | ||
| if n_parties < 4 { |
There was a problem hiding this comment.
If the no. of parties is lower than 4, then all parties must be honest, so t=0, not t=1.
|
Changes are fine, but comments
have not been addressed. |
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.
Summary
This PR addresses review comments from PR #1 (MPCaaS implementation):
with_preprocessing_timeout()Vec<(usize, SocketAddr)>Commits
73f9a9d- Default to maximum threshold for stronger securityf3b144c- Remove unnecessary ConnectionType enum79a4f43- Make preprocessing wait timeout configurablec35f0be- Remove ComputationComplete message68ec11a- Remove PeerManager, use direct peer addressesTest plan
cargo test)cargo clippy)🤖 Generated with Claude Code