-
Notifications
You must be signed in to change notification settings - Fork 385
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
MSC 1659 Proposal: Change Event IDs to Hashes #1659
Conversation
@jevolk is your point that the eventId param may be redundant given it's now determinable from the event? |
@ara4n No. This is just an area of the spec which complicates event_id-as-hashes because the payload is modified by two parties as the target server's signature is added. This has to be considered when specifying the behavior of the invite process. The target server might not be online when the process is initiated. Can an invite be made when the target server is not online? If not, why is it such a big deal to even have the target's signature before persisting the event both locally and broadcasting to the room DAG. Is that signature necessary? These are rhetorical questions at this point but the point is that this area needs to be reconsidered. |
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 note that the reference hash does not currently include the signatures on the event. I assume you're proposing to maintain that behaviour? I don't think it causes any security problems, but it might be a good time to revisit it?
We should probably spell out clearly that signature fields aren't included in the hash. (The protocol assumes in several places that adding signatures don't break hashes). |
Yup. I'm not particularly keen to change that behaviour as it feels reasonable to be able to add signatures without changing the ID of the event tbh. |
950d27a
to
54d9a7e
Compare
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.
looks reasonable, with just a couple minor issues
Co-Authored-By: erikjohnston <erikj@jki.re>
event to clients, however. | ||
|
||
Note that the `auth_events` and `prev_events` fields aren't sent to clients, and | ||
so the changes proposed above won't affect clients. |
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.
unless they use the federation event format, but then they're on their own anyways.
Heads up that I've updated this MSC to include making a room v3 |
🔔 This is now entering its final comment period, as per the review above. 🔔 |
Room v3 should be considered a stable room version - I've made this explicit on the proposal. |
Co-Authored-By: erikjohnston <erikj@jki.re>
The final comment period, with a disposition to merge, as per the review above, is now complete. |
2 similar comments
The final comment period, with a disposition to merge, as per the review above, is now complete. |
The final comment period, with a disposition to merge, as per the review above, is now complete. |
Original proposal: #1659 Implementation proofs (some traversing of the PR tree may be required to get all of them): * matrix-org/synapse#4483 * matrix-org/synapse#4499 This doesn't intentionally change anything from the proposal. **Implementation details**: The simple part of this is the introduction of a rooms/v3.html document. The somewhat unclear part is the stuff done to the s2s definitions. This pulls `unsigned_pdu` out to `unsigned_pdu_base` (all fields except `event_id`) where it can be reused in `pdu` and `pdu_v3` (for rooms v3). These definitions are further moved into the room version specifications where they can highlight the exact schemas in detail. Version 1 has been updated to include the pre-existing event format, however the core principles of the room have not been changed. The same applies to room version 2. Room versions have immutable core principles once in the spec, otherwise these format changes would land in a pre-existing version. The client-server API event formats will need updating, however that is being punted to a different commit to try and keep these changes reviewable.
merged via #1828 🎉 |
PR for #1640MSC has been renumbered to match this PR numberRendered