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.
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
MSC2716: Incrementally importing history into existing rooms #2716
base: old_master
Are you sure you want to change the base?
MSC2716: Incrementally importing history into existing rooms #2716
Changes from 4 commits
8c8d5e3
3a03172
5e6b7b9
9451439
8668e3a
d40f3b9
5854ca2
b766be5
b448452
8a4d136
92f87ed
bb44a63
3367f56
a4c474e
9df8b6e
b3b7903
38bebb1
7df80fe
3f28588
80e68bc
1678282
c6a60b1
65e5f7b
d7cf789
d016b7d
2544a3f
7258f64
a828de3
b2b5b54
a7920fb
73f4143
92a7658
1cf7395
2433dfa
efbee43
b4ba8c4
7cde5cd
8a50cbf
8286ca4
850e2f1
3332ca8
3668193
c936c7b
1d3f562
e593c20
2c46547
f60c233
b081ec7
d20455f
a8313bd
9d96c5c
991bd84
55551fc
1ee23d4
e7e435d
775d3d3
6ccecd7
af24a5f
10599cb
a6b5d8f
0642d88
b18c214
02b5f4b
69bd287
5412e80
16a6a40
4a8f834
e4193ff
1fc8b6b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Backfilling via
/messages
works by walking back upprev_events
. If the DAG looks like this, we'll never hit different eras so/messages
will return 0 events.EDIT: Actually it uses depth which will interleave instead.
/get_missing_events
will however walk up prev events, so all these lovely eras will never make it to other federated servers.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 don't think this works because the
/messages
endpoint has no idea when to jump to a different era. That endpoint topologically walks the DAG (in Dendrite it does this bydepth
), meaning if you actually did this you would get interleaved events as each era's events start producing the samedepth
values. This at least returns the events in the forks, but not where you want them.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 addition, doing this would produce forwards extremities at the end of each era, which servers will attempt to merge.
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.
so my expectation is that a homeserver should calculate the an appropriate depth when importing history like this, probably by tiebreaking based on origin_server_ts. Where does Dendrite get its depth param from? As it certainly shouldn't be trusting the one it receives over federation, because of https://github.com/matrix-org/matrix-doc/issues/1229.
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.
also, the fact that we create forward extremities at the end of each era which then get merged by the next message sent in the room was intended to be a feature, not a bug.
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.
From our meeting, the depth is assumed from the stream ID and can be spoofed. I may not have the details correct but we did discuss fudging it.
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.
So having rediscussed this IRL: Dendrite (and Synapse) currently get their depth parameters used for ordering from the wire. Ideally, we'd calculate the depth parameter instead - which could be easy, if we mandate that blocks of old history are always loaded contiguously in reverse chronological order. As a quick fudge to test the approach however we could set depth=1 for these events, and hopefully the default ordering will be sufficient (we think it is on synapse, but dendrite might need a tweak).
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.
It might be useful to have an unstable feature flag to check if the homeserver supports this