-
Notifications
You must be signed in to change notification settings - Fork 23
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
Add README #284
Add README #284
Conversation
One thing I'm wondering is whether this is more conservative than necessary. For sending application messages, which doesn't advance the epoch, we should very rarely need to re-send them. If we set Syncing the group state before and after adding/removing new members is also a lot. We could just send optimistically and re-send later. There would be a higher rate of re-sends with that approach, but it may be a worthwhile trade-off for speed and efficiency in the base case. |
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 love this proposal - the intent
concept is powerful and gives a foundation for all of our future work, essentially solving the issue of retries for conflicting commits. This is also laid out incredibly clearly and seems very well thought out.
Building on this concept, my main suggestion would be to increase the asynchronicity and robustness of each of these operations using event loops.
Some other thoughts that came up for me, not necessarily addressable here:
- In the event that an update fails due to conflicts, I'm not sure what happens with the cryptographic state, as we have already updated ours when constructing the original payload. I'm not sure if there is forking logic or rewinding logic already built into OpenMLS.
- All retries in this proposal happen automatically without user involvement. It may be worth thinking through any cases where the user should manually retry instead - for example if they sent a sensitive message for a set of group members, but another member was added before the message was committed.
- The event loop I suggested preserves the ordering of all user actions, but I'm not sure if this ordering is preserved if an update fails due to conflicts.
- A malicious group member can add a member to the group, but not send them any welcome messages. This prevents anyone else from adding the member, while keeping the group undiscoverable from the member. It's not the biggest issue in practice, but I don't see anything in the MLS spec for how to address this, curious if @Bren2010 has thoughts
@richardhuaaa great questions
Unlike the demo I had done previously, we don't update our cryptographic state until we read the message from the network in this model. Nothing changes in the local ratchet tree/group state at the time of sending the commit.
It's a good point. Feels like something we can table until post-launch. Retries of application messages should be very rare if we take advantage of
I would expect that if one intent fails due to conflicts all later intents will also fail. That may not be true if we set |
Co-authored-by: Richard Hua <[email protected]>
Co-authored-by: Richard Hua <[email protected]>
xmtp_mls/README.md
Outdated
group_id BLOB NOT NULL, | ||
"data" BLOB NOT NULL, -- Some sort of serializable blob that can be used to re-try the message if the first attempt failed due to conflict | ||
"state" INT NOT NULL, -- INTENT_STATE, | ||
message_hash BLOB -- The hash of the encrypted, concrete, form of the message if it was published. |
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.
You may want to store the epoch in which the message is intended to land in this table. If the message lands in a subsequent epoch, it needs to be retried.
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 was kinda hoping openmls
would handle the logic of deciding "this is from the wrong epoch" rather than us having to do the bookkeeping separately. That way it can transparently handle things like setting max_past_epochs
for the group to a value > 0.
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.
OpenMLS is not going to be able to decrypt messages that you yourself sent. So when you are seeing what order messages got put in on the topic, you will not be able to "black box" run your own messages through OpenMLS to see if they ended up decryptable
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'd love to learn more about that. My understand was that I would be able to validate my own messages, and in the case of commits be able to apply them (because we have a copy of the pending commit staged locally). We should at least be able to do some validation of messages even if they can't be decrypted.
Will try to keep these suggestion PR's small, for this one: 1. Serialize welcome messages to `post_commit_data` field on intent rather than separate table 1. Remove some group sync steps (we can handle conflicts anyway) 1. Drop pending proposals for now 1. Describe how payloads can be synced in a non-duplicative way Other things worth considering: 1. Making some of the steps generic rather than specific to intent type (a lot of it is duplicative/identical) 1. Whether to make commit processing strictly sequential (commit must be fully done before next one begins) or concurrent. If we make it strictly sequential, we need to think about how to handle concurrent syncs (as the application also needs to sync incoming messages, either via pull or subscription)
@@ -0,0 +1,172 @@ | |||
# XMTP MLS |
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.
Thank you for this README, @neekolas! What do you think of updating the title and adding a bit of intro text for context? Some suggested starter text here:
# XMTP MLS | |
# XMTP group chat using MLS | |
This document describes plans for how XMTP can provide group chat using [Messaging Layer Security](https://messaginglayersecurity.rocks/) (MLS). |
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.
The library today only handles group chat, but in future it's gong to include 1:1 as well.
I will add the intro text, which helps set the reader up for the onslaught of technical detail that follows.
1. If no conflicts: Mark the message as committed. | ||
If conflicts: Go back to step 2 and try again (reset the intent's state to `TO_SEND` and clear the `publish_data` and `post_commit_data` fields) | ||
|
||
### Syncing group state |
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.
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.
This makes sense to me. I like it, and it feels straightforward enough to implement. Love removing the locking mechanism from the topic_refresh_state
table.
@richardhuaaa you mind approving this so we can merge it? We can always amend it with more PRs as plans change or details get fleshed out. |
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! Should be good for the next few weeks.
We still need to sketch out how we're going to map between accounts and installations/track group membership, can do that separately
I took a first pass at the database schema and state machine for MLS.
We can rely on the total ordering of the Query endpoint for our network to get to a consistent state on every read of the group topic. All messages are read in ascending order, and any message sent for a previous epoch must be discarded. This flow introduces a new concept "group_intents" to handle cases where you might send a message in one epoch, and then upon subsequently reading the network need to re-send in another epoch.
Intents are not bound to any epoch and can therefore be safely retried in cases where conflicts occur.
Some actions can either be done optimistically (send the message then refresh the group state) or pessimistically (refresh group state, send message, refresh again). I have opted to handle regular message sending optimistically and anything that modifies the ratchet tree (add/remove members and key updates) pessimistically. Conflicts are possible either way, but are less likely with the pessimistic approach.