-
Notifications
You must be signed in to change notification settings - Fork 383
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
MSC4033: Explicit ordering of events for receipts #4033
Open
andybalaam
wants to merge
51
commits into
main
Choose a base branch
from
andybalaam/event-thread-and-order
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+386
−0
Open
Changes from 1 commit
Commits
Show all changes
51 commits
Select commit
Hold shift + click to select a range
3c50306
Initial draft
andybalaam e7983f2
Update number to MSC4033
andybalaam a75a8e5
Fix mis-spelling
andybalaam 6841e75
Fix another mis-spelling
andybalaam 2521212
Add some more alternatives
andybalaam fe83ad9
Add TODO list
andybalaam 3ae4995
Add missing bracket
andybalaam 8414c2e
Add an acknowledgements section
andybalaam 7e26924
Make thread roots not in their thread
andybalaam ee19047
Add a changelog
andybalaam 0e17420
Clarify stream order and handle redactions
andybalaam a32a3f9
Small clarifications
andybalaam 58102cf
Include updated defintion of read/unread event
andybalaam 8ecb0bb
Provide JSON examples
andybalaam c452342
Remove TODO list
andybalaam 2d6a03e
Wrap code blocks in pre
andybalaam fba3bbe
Format code with pre only
andybalaam f4899a0
Fix misplaced space
andybalaam afa9629
Add missing "in"
andybalaam 90e5798
Explain what the spec issue is about
andybalaam 76217a0
Remove redundant part of main thread definition
andybalaam c9bdac8
Reflect the spec saying thread roots are not in main
andybalaam 501bfac
Restrict to just redacted events being always read
andybalaam 17bfe74
Add missing "a"
andybalaam c02285b
Servers probably also think thread roots are in main
andybalaam a48c1d0
Move thread_id into content thanks to conversation with Nico
andybalaam aeb0650
Mention 3051 as an alternative to thread_id
andybalaam d58758f
Move thread_id to cleartext of content
andybalaam bbf1c94
Cut down to just ordering
andybalaam 36a28e5
Small brevity improvements
andybalaam be77a58
Note that order should be inserted by servers
andybalaam 04c4606
Move order to be a sibling of ts in receipts
andybalaam 6e027e2
Reword motivation paragraph
andybalaam 5499b04
Formatting
andybalaam 7493391
Mention the specific API used to get older messages
andybalaam 7d3df44
Make clear the order only needs to increase relative to messages in t…
andybalaam c45a5e4
Formatting
andybalaam 1b668ce
Clarify the consistency guarantees and meaning of order
andybalaam d2fe0f4
Include order in redacted events too
andybalaam 694317e
Emphasise the room level when talking about server calculation
andybalaam be0f7ac
Add a note that this applies to other users' receipts too
andybalaam e650a11
Acknowledge that we actually are changing the definition of read/unread
andybalaam 7d1728e
Fix typo
andybalaam 12cde91
Fix missing e in mxid
andybalaam 6950a59
Make order on a receipt mandatory
andybalaam 2543428
Document the idea of just never sending receipts that don't have orde…
andybalaam 605eadd
Add a note about fully-read markers being out of scope
andybalaam d749fb1
Add notes about negative event order
andybalaam 6859b8d
Add a note about negative order meaning event is read
andybalaam d2cc49d
Add a note about order not being identical
andybalaam c7a8192
Fix spelling error
andybalaam File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Add a note about fully-read markers being out of scope
commit 605eaddf12f8487d5d3c1e8bcad442816754ce8a
There are no files selected for viewing
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
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.
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 problem with numbers is that you can't fill gaps between them, so if you already have 15 and 16, there's no place inbetween them. Instead, you could treat the order as a string of digits instead with the property that
15 < 151 < 16
For easier understanding, you can compare this to decimal numbers: 0.15 < 0.151 < 0.16
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.
if you need to be able to insert a new value between any existing pair, you can use Dyadic Rationals, basically a value of the form
a * 2^b
wherea
andb
are integers.e.g. if you have
23
and24
, you can use47 * 2^-1 = 23.5
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.
Using an array as the index is also a concept that can be found in CRDT's.
[1]
,[1,1]
,[1,2]
,[1,2,1]
,[2]
(very similar to what timokoesters is proposing but strings bring more typing ambiguity/issues.)
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.
In terms of storage/memory used per event (thus bandwidth usage), dyadic rationals sound optimal: sufficiently large space, only require two u32 or two u64. Strings or array representations would require bigger allocations. (Nobody really proposed it, but FP numbers would be a waste in terms of storage space b/o all the
NaN
s + IEEE754 is hard to get right.)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.
dyadic rationals only require one byte for the exponent, unless you want to support a value with >256 bits (imo excessive), so they only need 5 bytes (32-bit mantissa) or 9 bytes (64-bit mantissa), though you can definitely use more for alignment or convenience.
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.
Note that we will need to be able to handle edge cases where we run out of exponents, e.g. you have an event A and you keep inserting events just after it (which may very well end up a common case). If you only have a one byte exponent then you quickly run out of room.
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.
if you run out of exponent bits, that also means the mantissa has to be at least 256-bits wide (or something like that), so you will need big-integer arithmetic for that...i think that's likely excessive.
maybe a better idea is to maintain a binary tree with one node per message and have the server send the list of changed nodes (if the message's contents doesn't change, all that needs to be sent is the node ids of the tree's children, since that's all that changes during tree balancing), this allows tree balancing to avoid the tree getting too deep.
messages would then be ordered by their position in an in-order traversal of that tree.