-
Notifications
You must be signed in to change notification settings - Fork 78
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make defer-decryption default #573
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
As agreed, I pushed a commit with clarifying comments and a name change with the goal of:
- Emphasize that
deferred decryption
is toggleable. - I tried to avoid leaking the concept of "deferred" into the Transcript-related type.
FromTranscript
s pov there is only data written to it in the online and in the offline phases. I hope this abstraction is correct and I'm not overlooking sth.
lgtm, thank you! Having the inner transcript not know about the concept of deferred decryption makes sense to me. Seems like some test doesn't like the changes, or randomly fails. |
Opened an issue for this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great stuff! just one nit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel we should just remove the defaults entirely. It would be an error to not set one of the recv
sizes. It's not much burden on the user to make sure they think about how much data will be communicated.
crates/common/src/config.rs
Outdated
@@ -31,14 +34,29 @@ pub struct ProtocolConfig { | |||
/// Maximum number of bytes that can be sent. | |||
#[builder(default = "DEFAULT_MAX_SENT_LIMIT")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#[builder(default = "DEFAULT_MAX_SENT_LIMIT")] |
crates/common/src/config.rs
Outdated
max_recv_data_online: usize, | ||
/// Maximum number of bytes for which the decryption can be deferred until after the MPC-TLS | ||
/// connection is closed. | ||
#[builder(default = "self.default_max_deferred_size()")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#[builder(default = "self.default_max_deferred_size()")] |
I am not sure. Users will have to dive into our What is the advantage of having no defaults? |
Users will have to understand this particular configuration anyhow, unless we find an abstraction in higher level tooling. I recall several people running into transcript limit issues. The defaults we have right now are arbitrary. The solution to hiding deferred decryption complexity is to change our approach slightly. It's the default, so we can invert the naming:
Assert |
That sounds good. But then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 change
- `TranscriptConfig` - `MpcTlsLeaderConfig` - `MpcTlsFollowerConfig`
7b228b4
to
38c94a1
Compare
This PR makes defer-decryption default and introduces the necessary config changes.