Skip to content
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

refactor: chunk KOS extend message #98

Merged
merged 8 commits into from
Feb 13, 2024
Merged

refactor: chunk KOS extend message #98

merged 8 commits into from
Feb 13, 2024

Conversation

sinui0
Copy link
Collaborator

@sinui0 sinui0 commented Feb 1, 2024

This PR refactors KOS impl to chunk the extension message sent from the Receiver to the Sender.

Closes #97 and tlsnotary/tlsn-utils#10

@sinui0 sinui0 requested review from themighty1 and th4s February 1, 2024 20:46
@sinui0 sinui0 mentioned this pull request Feb 1, 2024
Copy link
Member

@th4s th4s left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be cleaner to handle this in a general way in our messaging layer? Then this change would apply to every sink and stream we are using, while at the same time not mixing the protocol code with IO stuff.

I am thinking of something like wrapper functions in our IoSink and IoStream traits, which break down the payload if needed.

@sinui0
Copy link
Collaborator Author

sinui0 commented Feb 2, 2024

That thought certainly crossed my mind, but it didn't seem to be quite as straight forward.

  1. The IO layer already breaks down the message into chunks. The limit we are running into is mostly regarding deserialization and how much memory a received message is allowed to utilize. This limit needs to be specific to the application context and message type. We could add additional bounds to the IO traits to specify this, but that would make them harder to implement and use. This would be less of a problem with specialization, but that's not in the foreseeable future.
  2. A less flexible middleground is to fork tokio_util::codec::length_delimited::LengthDelimitedCodec and make it aware of variant-level size limits, so we could specify a limit specifically for Extend at the codec level rather than baking this into the IO traits. I don't want to block increasing the TLSNotary protocol limit on this work.
  3. I think at the end of the day the chunking will get baked into the callsite control flow anyhow. The optimal state of this code would be to generate the matrix in chunks, streaming them, and processing the chunks concurrently. That would require "chunk" message types anyhow. Given our deprecation plans, this is a simpler short-term remedy even if its not actually doing stream processing.

@th4s
Copy link
Member

th4s commented Feb 6, 2024

Makes sense 👍, I haven't put that much thought into it when I wrote that.

Copy link
Member

@th4s th4s left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

ot/mpz-ot-core/src/kos/msgs.rs Outdated Show resolved Hide resolved
ot/mpz-ot-core/src/kos/mod.rs Outdated Show resolved Hide resolved
ot/mpz-ot/src/kos/sender.rs Show resolved Hide resolved
@sinui0 sinui0 requested a review from themighty1 February 9, 2024 23:50
@sinui0 sinui0 force-pushed the fix/chunk-kos-extend-msg branch from 0cddc17 to efb5f30 Compare February 10, 2024 00:30
ot/mpz-ot/src/kos/receiver.rs Show resolved Hide resolved
ot/mpz-ot/src/kos/receiver.rs Outdated Show resolved Hide resolved
@sinui0 sinui0 requested a review from themighty1 February 12, 2024 19:44
Copy link
Collaborator

@themighty1 themighty1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gw

@sinui0 sinui0 merged commit 850636f into dev Feb 13, 2024
3 checks passed
@sinui0 sinui0 deleted the fix/chunk-kos-extend-msg branch February 13, 2024 14:51
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.

chunk KOS extension message
3 participants