This repository has been archived by the owner on Aug 30, 2022. It is now read-only.
Simplify messages #508
Merged
Simplify messages #508
Conversation
This file contains 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
It turns out that we only use the owned version of the various message types, so let's simplify them by removing the generic parameters.
There is no reason to send it to the coodinator, since it already has it.
little-dude
force-pushed
the
simplify-messages
branch
from
eefaf87
to
d67a61a
Compare
Codecov Report
@@ Coverage Diff @@
## master #508 +/- ##
==========================================
- Coverage 57.33% 57.02% -0.31%
==========================================
Files 68 67 -1
Lines 3117 3048 -69
==========================================
- Hits 1787 1738 -49
+ Misses 1330 1310 -20
Continue to review full report at Codecov.
|
|
For me it looks good👍 |
|
I just started with the multipart support so no need to merge right now. If too many changes queue up, I'll merge, but otherwise I'll wait for Monday. |
Robert-Steiner
approved these changes
Aug 27, 2020
|
Merging because I'm about to open a new PR based on this one. |
little-dude
added a commit
that referenced
this pull request
Aug 31, 2020
This commit contains several major changes. # Re-work the message header ``` 0 1 2 3 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ | | + + | | + + | | + + | | + + | | + + | | + + | | + + | | + signature + | | + + | | + + | | + + | | + + | | + + | | + + | | + + | | +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ | tag | flags | reserved | +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ | | + + | | + + | | + + | | + participant_pk + | | + + | | + + | | + + | | +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ ``` The problem is that: - When reading from a byte stream, we don't know where the message finishes because there is no length field. So far this has not been a problem because we would only receive messages via HTTP. However, if we decide to do encryption in the TLS layer, we might support reading messages from a TLS connection directly. Then, we'll have an encrypted byte stream in input and un-encrypted byte stream as output, and we'll need to be able to find the message boundaries to parse the messages. - The participant public key is used to verify the signature, which is the first thing we do. It's not convenient to have to read this far in the header to get it. Instead it should follow the signature. - The flags field is currently un-used (we removed the certificate field in #508 Therefore we changed the header of the messages as follow: ``` 0 1 2 3 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ | | + + | | + + | | + + | | + + | | + + | | + + | | + + | | + signature + | | + + | | + + | | + + | | + + | | + + | | + + | | + + | | +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ | | + + | | + + | | + + | | + participant_pk + | | + + | | + + | | + + | | +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ | length | +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ | tag | reserved | +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ ``` # Remove the `Header` type To simplify the code, we also removed the `Header` type. Instead, the `Header` is not part of the `Message` itself: ```rust // The length field is omitted because it's compupted // automatically when serializing the message. // // The tag field is omitted because it's encoded in the `Payload` // enum type pub struct Message { pub signature: Option<Signature>, pub participant_pk: PublicSigningKey, pub payload: Payload, } ``` Note that the `Message` type cannot implement the `ToBytes` trait, because the insertion of the signature requires passing a secret signing key as an argument. Instead, `Message` has a `to_bytes` method. We could implement `FromBytes` but to make things consistent, we also implemented it directly as a `from_bytes` methods. # Remove the `Message::new_{sum,update,sum2}` constructors The `Message` type has been greatly simplified. There is no need for these constructors. # Introduce a new `Chunk` message type We introduced a new message type that represents a part, or chunk, of a bigger message: `Chunk`. The message structure is: ``` 0 1 2 3 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ | chunk_id | message_id | +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ | flags | reserved | +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ | payload | ``` We currently only support one flag that indicates whether this is the last chunk of the message or not. The `chunk_id` identifies a chunk, and the `message_id` identifies the biggier message it is part of.
little-dude
added a commit
that referenced
this pull request
Aug 31, 2020
This commit contains several major changes. # Re-work the message header ``` 0 1 2 3 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ | | + + | | + + | | + + | | + + | | + + | | + + | | + + | | + signature + | | + + | | + + | | + + | | + + | | + + | | + + | | + + | | +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ | tag | flags | reserved | +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ | | + + | | + + | | + + | | + participant_pk + | | + + | | + + | | + + | | +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ ``` The problem is that: - When reading from a byte stream, we don't know where the message finishes because there is no length field. So far this has not been a problem because we would only receive messages via HTTP. However, if we decide to do encryption in the TLS layer, we might support reading messages from a TLS connection directly. Then, we'll have an encrypted byte stream in input and un-encrypted byte stream as output, and we'll need to be able to find the message boundaries to parse the messages. - The participant public key is used to verify the signature, which is the first thing we do. It's not convenient to have to read this far in the header to get it. Instead it should follow the signature. - The flags field is currently un-used (we removed the certificate field in #508 Therefore we changed the header of the messages as follow: ``` 0 1 2 3 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ | | + + | | + + | | + + | | + + | | + + | | + + | | + + | | + signature + | | + + | | + + | | + + | | + + | | + + | | + + | | + + | | +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ | | + + | | + + | | + + | | + participant_pk + | | + + | | + + | | + + | | +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ | length | +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ | tag | reserved | +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ ``` # Remove the `Header` type To simplify the code, we also removed the `Header` type. Instead, the `Header` is not part of the `Message` itself: ```rust // The length field is omitted because it's compupted // automatically when serializing the message. // // The tag field is omitted because it's encoded in the `Payload` // enum type pub struct Message { pub signature: Option<Signature>, pub participant_pk: PublicSigningKey, pub payload: Payload, } ``` Note that the `Message` type cannot implement the `ToBytes` trait, because the insertion of the signature requires passing a secret signing key as an argument. Instead, `Message` has a `to_bytes` method. We could implement `FromBytes` but to make things consistent, we also implemented it directly as a `from_bytes` methods. # Remove the `Message::new_{sum,update,sum2}` constructors The `Message` type has been greatly simplified. There is no need for these constructors. # Introduce a new `Chunk` message type We introduced a new message type that represents a part, or chunk, of a bigger message: `Chunk`. The message structure is: ``` 0 1 2 3 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ | chunk_id | message_id | +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ | flags | reserved | +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ | payload | ``` We currently only support one flag that indicates whether this is the last chunk of the message or not. The `chunk_id` identifies a chunk, and the `message_id` identifies the biggier message it is part of.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
This PR simplifies the message types in preparation of the upcoming changes needed to support multipart messages:
Ownedmessage types since they are not used