Drop invalid PDUs instead of erroring

[clokep]

This is based on a conversation at matrix-org/matrix-spec-proposals#2540 (comment), summarized below:

Currently when an incoming federation event is "bad" for some reason it is rejected by returning a 400 error. This is particularly troublesome in endpoints where multiple events are handled at once, as the entire transaction gets rejected.

Reasons an event might be rejected include:

• events with no type or depth
• events with bad depth values
• events in v{1,2} rooms with no event_id/v3+ rooms with an event_id
• events in v6 rooms that have integers out of range or floats
• events with too many prev_events (We accept stupid events over backfill, causing state res storms #3124)
• etc.

There are three proposed options for this situation:

1. silently drop the event. I would argue that we should not be blackholing events, ever: it feels like we'll end up dropping events unexpectedly.
2. send an error back in the response, against the event's event_id. This, of course, requires the recipient to parse the json, remove a couple of properties, then re-encode it using canonicaljson (which, you recall, is theoretically impossible) to calculate the event id. That sounds like a tautology to me. It also requires servers which don't even support older room versions to still accept transactions with floats in them. So that leaves us with:
3. reject the whole lot.

It is potentially difficult to return a sensible error since (theoretically) you might not even be able to parse the event data and thus it is proposed to silently drop these events for now.

[clokep]

@erikjohnston / @richvdh Does this sound like a reasonable summary of that conversation and what needs to be done?

Open questions from my side include:

• Does this change apply to all places we validate the event, or only locations where a "transaction" would be rejected? If a single event is coming it, then rejecting it seems to be quite reasonable, I'm not sure if any of the endpoints really support single events or not.
• Is this a release blocker for 1.14? (I don't think so since it isn't directly related to the changes in Strictly enforce canonicaljson requirements in a new room version #7381.)
• Does this need any spec work?

[richvdh]

beware that the term "rejected" is fraught with danger: we tend to use it for the class of events which we accept onto the DAG but do not include in the room state or send to clients (that's useful, because we want to preserve the structure of the DAG where possible). I'm not sure we have a term for the class of events which are too mangled to even add to the room DAG, though I tend to refer to them as "invalid".

• The conversation related specifically to /federation/v1/send.
• I don't think it needs to be a blocker, though I would like to see it addressed before time moves on and we forget about it.
• I don't think it needs any spec work, since the spec is very vague here at the moment, and in any case I think this can be left to the impl to some extent - though I think it might be useful if the spec offered recommendations here.

[clokep]

Alright. That clears it up a bit! 👍 I think this would be relatively straightforward then.

[richvdh]

Related: matrix-org/matrix-spec#365