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

Handling of own messages #334

Merged
merged 11 commits into from
Nov 15, 2023
Merged

Handling of own messages #334

merged 11 commits into from
Nov 15, 2023

Conversation

neekolas
Copy link
Contributor

@neekolas neekolas commented Nov 14, 2023

Summary

  • Adds support for matching intents to incoming messages in the receive flow
  • Updates the intent state depending on whether the message matches the group epoch
  • Adds a sync function to encapsulate publishing, receiving, and post-commit flows
  • Refactors message processing to branch out to the new flow if an intent is found

Notes

There are a few error cases that are hard to handle

  1. Retryable vs non-retryable errors. When to abort
  2. What happens if an intent can never be fully fulfilled? We probably need an error intent status for intents that are impossible (trying to add a member that doesn't exist, trying to add a member already present in the group)
  3. Application messages from the wrong epoch. These may actually be fine depending on the max_past_epochs config We now don't reject application messages from the wrong epoch. There will be some follow-up work to try and catch application messages that are from outside the max_past_epochs window.

TODO

  • Add support for writing a transcript of members added/removed
  • Better mapping between commits and payloads. Should include a reference to the staged commit hash or something to make sure that when merge_pending_commit gets called it is actually being called on the correct commit
  • More tests around failure modes and recovery

// intentionally left blank.
ProcessedMessageContent::StagedCommitMessage(staged_commit) => {
println!("[{}] received staged commit", self.client.account_address());
openmls_group.merge_staged_commit(provider, *staged_commit)?;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added this line to make my test case handling conflicting messages produce the right errors

@neekolas neekolas marked this pull request as ready for review November 14, 2023 23:02
@neekolas neekolas requested review from a team, richardhuaaa and insipx November 14, 2023 23:02

self.publish_intents(&mut conn).await?;
// Skipping a full sync here and instead just firing and forgetting
self.publish_intents(conn).await?;
Copy link
Contributor

Choose a reason for hiding this comment

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

To be explicit, what is the reason we can fire-and-forget messages but we can't do the same for other intents?

I'm guessing because OpenMLS only allows us to have one pending commit at a time? Which is not an issue for messages

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's right. Especially with max_past_epochs we should be able to assume that sending a message will "just work" in almost all cases and not need to be re-queued. There are some edge cases where your local state is way out of sync with the network where it would need a retry, but they should be rare enough I think the trade-off is worth it.

xmtp_mls/src/groups/mod.rs Outdated Show resolved Hide resolved
@neekolas neekolas merged commit 2b3e0c3 into main Nov 15, 2023
4 checks passed
@neekolas neekolas deleted the nm/handle-own-commits branch November 15, 2023 18:17
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.

2 participants