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

Minor drive-by state res clarifications #1042

Merged
merged 9 commits into from
May 30, 2022
Merged

Minor drive-by state res clarifications #1042

merged 9 commits into from
May 30, 2022

Conversation

DMRobertson
Copy link
Contributor

@DMRobertson DMRobertson commented Apr 29, 2022

@DMRobertson DMRobertson marked this pull request as ready for review May 25, 2022 12:03
@DMRobertson DMRobertson requested a review from a team as a code owner May 25, 2022 12:03
@turt2live
Copy link
Member

@DMRobertson is this still WIP? Title and state don't seem to match

@DMRobertson DMRobertson changed the title WIP: state res clarifications Minor drive-by state res clarifications May 26, 2022
@DMRobertson
Copy link
Contributor Author

@DMRobertson is this still WIP? Title and state don't seem to match

No, thanks; title updated.

@richvdh richvdh self-assigned this May 26, 2022
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

definitely an improvement!

unconflicted state map only has one event per `(event_type, state_key)`,
whereas the conflicted state set may have multiple events.
The key-value pairs across all state maps *S<sub>i</sub>* can be divided into
two collections. If a given key *K* is present in every *S<sub>i</sub>* with the
Copy link
Member

Choose a reason for hiding this comment

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

is it obvious that K is a (event_type, state_key) pair, or is there a way we can make that obvious?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be helpful if there was a phrase which meant (event_type, state_key). How about 8ab0634?

*unconflicted state map*. Otherwise (*K*, *V*) belongs to the
*conflicted state set*.

Note that the unconflicted state map only has one event per `(event_type, state_key)`,
Copy link
Member

Choose a reason for hiding this comment

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

... switching from K to (event_type, state_key) here is jarring without that context.


**Auth chain.**
The *auth chain* of an event *E* is the set containing all of *E*'s auth events,
all of *their* authorising events, and so on recursively, stretching back to the
Copy link
Member

Choose a reason for hiding this comment

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

possibly we should stick to the term "auth events" rather than switching to "authorising events" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is 8c49f00

content/rooms/fragments/v2-state-res.md Outdated Show resolved Hide resolved
content/rooms/fragments/v2-state-res.md Outdated Show resolved Hide resolved
@DMRobertson DMRobertson requested a review from richvdh May 30, 2022 13:09
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

thanks!

@richvdh richvdh merged commit 3f7b0e8 into matrix-org:main May 30, 2022
richvdh pushed a commit that referenced this pull request Jun 14, 2022
In #1042 I incorrectly wrote that the conflicted state set is a set of
pairs (K, V). We later take the union of the conflicted state set and
the auth difference. The latter is a set of events (V) only.

Fix this by making the conflicted state set a set of events rather than
a set of pairs. That is, the conflicted state set is a a `Set[Event]`
instead of a `Set[((type, state key), event)]`.
turt2live pushed a commit that referenced this pull request Jul 5, 2022
* Fix unintentional stateres change added in #1042

* Changelog
turt2live pushed a commit that referenced this pull request Aug 3, 2022
* Fix unintentional stateres change added in #1042

* Changelog
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.

State res v2 does not define the term "auth chain"
4 participants