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

Fix possible state reset issue #419

Merged
merged 6 commits into from Oct 23, 2023
Merged

Fix possible state reset issue #419

merged 6 commits into from Oct 23, 2023

Conversation

S7evinK
Copy link
Contributor

@S7evinK S7evinK commented Oct 6, 2023

The spec says:

Iterative auth checks. The iterative auth checks algorithm takes as input an initial room state and a sorted list of state events, and constructs a new room state by iterating through the event list and applying the state event to the room state if the state event is allowed by the authorization rules. If the state event is not allowed by the authorization rules, then the event is ignored. If a (event_type, state_key) key that is required for checking the authorization rules is not present in the state, then the appropriate state event from the event’s auth_events is used if the auth event is not rejected.

The part we didn't do, given the membership event may not be resolved (e.g. it is conflicted):

If a (event_type, state_key) key that is required for checking the authorization rules is not present in the state, then the appropriate state event from the event’s auth_events is used if the auth event is not rejected.

@codecov
Copy link

codecov bot commented Oct 6, 2023

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (77f02e6) 58.76% compared to head (2c8d23d) 58.90%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #419      +/-   ##
==========================================
+ Coverage   58.76%   58.90%   +0.13%     
==========================================
  Files          51       51              
  Lines        7098     7105       +7     
==========================================
+ Hits         4171     4185      +14     
+ Misses       2536     2531       -5     
+ Partials      391      389       -2     
Files Coverage Δ
stateresolutionv2.go 89.88% <80.00%> (-0.25%) ⬇️

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@devonh devonh left a comment

Choose a reason for hiding this comment

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

Can you add a comment to the test outlining which prev_events are wrong?
It's impossible to tell what the event_ids are just from looking at the code.

Otherwise this looks like it should meet the rules outlined in the spec.

@S7evinK
Copy link
Contributor Author

S7evinK commented Oct 23, 2023

There isn't anything wrong with the prev_events in this test. The problem is mainly that Dendrite is "wrongly" calculating the conflicted events. See this.

@S7evinK S7evinK marked this pull request as ready for review October 23, 2023 11:47
@S7evinK S7evinK merged commit 16e7431 into main Oct 23, 2023
7 checks passed
@S7evinK S7evinK deleted the s7evink/state-resets branch October 23, 2023 12:15
S7evinK added a commit to matrix-org/dendrite that referenced this pull request Oct 23, 2023
S7evinK added a commit that referenced this pull request Oct 24, 2023
To be even more spec compliant, this adds a `IsRejected` type/function
when resolving state.
(Part 2 of #419)
danpe pushed a commit to globekeeper/dendrite that referenced this pull request Oct 30, 2023
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.

None yet

2 participants