When redacting, keep event fields around that maintain the historical event structure intact (MSC2716)

[MadLittleMods]

When redacting, keep event fields around that maintain the historical event structure intact.

Fix #10521

Part of MSC2716: matrix-org/matrix-spec-proposals#2716

Pull Request Checklist

• Pull request is based on the develop branch
• Pull request includes a changelog file. The entry should:
  • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
  • Use markdown where necessary, mostly for code blocks.
  • End with either a period (.) or an exclamation mark (!).
  • Start with a capital letter.
• Pull request includes a sign off
• Code style is correct (run the linters)

[erikjohnston]

This looks good, though does mean that existing events will end up with different event IDs. I think we should probably update the room version ID to avoid mass confusion?

[MadLittleMods]

@erikjohnston I'm not following 😣. Does redacting an event give it a different event ID?

These redaction checks are already under the experimental MSC2716 room version? Do we need to bump that version again?

[erikjohnston]

@erikjohnston I'm not following . Does redacting an event give it a different event ID?

These redaction checks are already under the experimental MSC2716 room version? Do we need to bump that version again?

Ah, sorry, its non obvious how this relates to event IDs. In non-v1 room versions event IDs are the reference hashes for the event, which are calculated by redacting the event, serialising it to canonical JSON and then calculating the SHA256 hash of those bytes. Hence, if you change the redaction algorithm then the canonical JSON bytes will be different and therefore so would the hash¹.

These redaction checks are already under the experimental MSC2716 room version? Do we need to bump that version again?

I would, just to prevent confusion with any existing test rooms using that experimental version. I hope that's fine and easy?

---

¹ Note that an event also contains a content hash, which is a hash over the unredacted event and is included in the redacted event. This is what protects the unredacted event fields from being changed.

[MadLittleMods]

Ah, sorry, its non obvious how this relates to event IDs. In non-v1 room versions event IDs are the reference hashes for the event, which are calculated by redacting the event, serialising it to canonical JSON and then calculating the SHA256 hash of those bytes. Hence, if you change the redaction algorithm then the canonical JSON bytes will be different and therefore so would the hash1.

Thanks for that context! That makes sense 👍

---

Do we actually verify the event_id hashes after it's created? I assume this only matters for remote federated homeservers trying to verify a received event that was redacted?

Just as a thought, if redacting an event messes up the event verification, that essentially soft_fails the event which has the same effect as redacting 🤔 (not visible for client)

I understand why it matters, just asking a "technically, does it work? 🤓" kind of question 😁

[erikjohnston]

Do we actually verify the event_id hashes after it's created? I assume this only matters for remote federated homeservers trying to verify a received event that was redacted?

Amusingly, yes, as the Event object calculates the event_id on creation, rather than reading it from the DB. The get_events(..) function returns the event as part of an event.event_id -> event mapping, so if you ask for event $A you'd get back a dict with $B in instead, which'd then cause the code to assume the server didn't have $A

Just as a thought, if redacting an event messes up the event verification, that essentially soft_fails the event which has the same effect as redacting (not visible for client)

I understand why it matters, just asking a "technically, does it work? " kind of question

It will reject the event rather than "soft fail", but yes it'd have the same impact as redacting the event. I think not changing the redaction rules would mostly be fine in practice (we have in the past forgotten to change the redaction algorithm when we've changed the auth rules).

[MadLittleMods]

Amusingly, yes, as the Event object calculates the event_id on creation, rather than reading it from the DB. The get_events(..) function returns the event as part of an event.event_id -> event mapping, so if you ask for event $A you'd get back a dict with $B in instead, which'd then cause the code to assume the server didn't have $A

😁 That would be an interesting scenario

---

Thanks for the review @erikjohnston 🐘