Track when the pulled event signature fails

[MadLittleMods]

Track when the pulled event signature fails. Because we're doing the recording in _check_sigs_and_hash_for_pulled_events_and_fetch (previously named _check_sigs_and_hash_and_fetch), this means we will track signature failures for backfill, get_room_state, get_event_auth, and get_missing_events (all pulled event scenarios). And we also record signature failures from get_pdu.

Part of #13700

Part of #13676 and #13356

This PR will be especially important for #13816 so we can avoid the costly _get_state_ids_after_missing_prev_event down the line when /messages calls backfill.

Dev notes

backfill (federation_client)
_check_sigs_and_hash_and_fetch
_check_sigs_and_hash_and_fetch_one
	_check_sigs_and_hash
	get_pdu

get_pdu
get_pdu_from_destination_raw
_check_sigs_and_hash

Other usages of _check_sigs_and_hash. It looks like it's all processing events that come from another homeserver.

on_PUT (/send/<transaction_id>/)
on_incoming_transaction
_on_incoming_transaction_inner
_handle_incoming_transaction
_handle_pdus_in_txn
process_pdus_for_room
process_pdu
_handle_received_pdu
_check_sigs_and_hash


on_send_join_request (on_PUT /send_join) or on_send_leave_request (on_PUT /send_leave) or on_send_knock_request (on_PUT /send_knock)
_on_send_membership_event
_check_sigs_and_hash


on_invite_request (on_PUT /invite)
_check_sigs_and_hash


handle_new_client_event or _handle_request
_persist_event
persist_and_notify_client_event
send_invite
send_invite
_check_sigs_and_hash

send_join
_check_sigs_and_hash_and_fetch_one
_check_sigs_and_hash

backfill or get_room_state or get_event_auth or get_missing_events
_check_sigs_and_hash_and_fetch
_check_sigs_and_hash_and_fetch_one
_check_sigs_and_hash

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.
  • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
• Pull request includes a sign off
• Code style is correct
  (run the linters)

[MadLittleMods]

In the future, we will potentially record a failed attempt when the spam checker soft-fails an event. Part of #13700

[MadLittleMods]

Renamed this so we stick to using this only for pulled event scenarios

[MadLittleMods]

For context, see #13815 (comment) for why we're using a callback pattern here.

[reivilibre]

This except clause seems too broad; I'd worry it might swallow errors arising from bugs rather than solely invalid signatures.

Can we narrow down the interesting types of Exceptions that we care about here?

[reivilibre]

e.g. this will catch CancelledErrors which sounds dodgy (I don't know if cancellation is enabled for backfill, but it doesn't sound like a good reason to record a failed pull attempt)

[MadLittleMods]

It looks like we can just worry about InvalidEventSignatureError here 👍

But all of the downstream places in _check_sigs_on_pdu do the same except Exception as e: 🤷

[MadLittleMods]

What we decide here, we should also apply to #13814

Is there a way we can make the errors we care about opt-out instead of opt-in? It seems like it's going to suck to keep some of these error lists up to date whenever the downstream functions change and raise different errors.

Maybe it's better to be opt-in to handle just what you know but as an example at the following spot, we probably need to look at FederationError, RuntimeError, InvalidResponseError from a quick glance at some of the downstream code and this list seems like it will morph under us since there is so much going on in that block. Is there a good way to get a general sense of all exception types that a code path can raise (doesn't need to be comprehensive, maybe info just from docstrings)?

synapse/synapse/handlers/federation_event.py

Lines 916 to 920 in 6f0c3e6

	except Exception as exc:
	await self._store.record_event_failed_pull_attempt(
	event.room_id, event_id, str(exc)
	)
	raise exc

(created a draft PR to track this point, #13969, and will update with what we think about here)

[reivilibre]

I can't imagine opt-out is easier — I don't think RuntimeError, ValueError, AssertionError, KeyError etc should be included, for example. These represent programming errors rather than the remote not acting according to the way we expect.

Tracking the set of exceptions that some code can throw is difficult in Python, unfortunately.

I would suggest trying some cases of where you want to track a failed pull attempt, perhaps even adding tests that reproduce these cases. I can think of at least these classes of problems:

• DNS errors
• TCP-level Connection errors
• TLS problems such as untrusted self-signed certs
• Homeserver not on federation whitelist (? what should we do here?)
• Bad response code
• Response isn't JSON
• JSON response isn't in an expected format (e.g. doesn't include the right keys)... bearing in mind that our validation is probably rubbish and this might currently manifest itself as a KeyError (IMO this should be fixed so it sounds more like a validation error)

I realise this sounds a bit painful but we ought to figure out what we should do for these cases. Should a network problem actually be tracked against the event at all; won't the existing backoff behaviour be sufficient there? ....

Perhaps, rather than accepting that the code underneath is always going to be messy, try and improve the downstream situation — can we define superclasses for some of these interesting groups of exceptions a e.g. TemporaryNetworkError exception class and make relevant exceptions derived from those or get re-raised as those?
We can then add to the docstrings of the downstream functions what exceptions they're allowed to raise for these kinds of circumstances.

Just some thoughts, realise it may not necessarily be easy..

[MadLittleMods]

I don't think RuntimeError, ValueError, AssertionError, KeyError etc should be included, for example. These represent programming errors rather than the remote not acting according to the way we expect.

Fair enough on RuntimeError and friends, I saw this case and during my quick scan and seemed relevant (it's not) although if it was, would be a good candidate to make it throw the proper error type 👍

I would suggest trying some cases of where you want to track a failed pull attempt, perhaps even adding tests that reproduce these cases

Having these tests would be great. We'd want to apply them to /event/{eventId}, /event_auth/{roomId}/{eventId}, /state/{roomId}?event_id=$xxx, /state_ids/{roomId}?event_id=$xxx (although related and probably covered by the existing backoff and NotRetryingDestination in the next point below). For /backfill and /get_missing_events, we can't track server/network problems because there is no specific event we're requesting but we can track whether the events in their responses are rejected, etc.

Should a network problem actually be tracked against the event at all; won't the existing backoff behaviour be sufficient there? ....

In general, if anything goes wrong with trying to pull an event from another server, we should backoff and do it in the background next time (#13623).

The existing backoff underlying all of the federation requests (matrixfederationclient.py and NotRetryingDestination) seems like it could be sufficient for network related errors. I haven't experienced network problems being the cause of /messages being slow so I haven't put as much thought in this area.

Homeserver not on federation whitelist (? what should we do here?)

Seems like we shouldn't be asking homeservers if they aren't on our whitelist in the first place. I guess we just let FederationDeniedError handle this and continue to the next destination which accomplishes this though.

---

Thanks for the thoughtful response on this @reivilibre! Really helping me form the right thoughts around all of this!

[reivilibre]

It would be good to see a description of at least record_failure_callback — I think the other 2 are fairly self-evident so not fussed about those particularly.

[reivilibre]

(done in a later commit; in future please consider collapsing those into the same commit so that commit by commit review is possible for mere mortals like me who can't track the full thing ;-))

[MadLittleMods]

My PR's are definitely not commit by commit review friendly. I find working that way in the first place is impossible.

And rebasing and splitting multiple changes in the same file is a super chore. I'd rather split stuff by PR which feel free to call out if you see something to split out.

[reivilibre]

If you can make them a bit more CbC friendly, I would really appreciate it to make reviews less daunting if anything. (Maybe I'm the only one though? Definitely not a requirement, but just a preference from me especially when I'm not familiar with the area of code)

Since you seem to have the idea that it's difficult to do: My technique for things like this is to use lazygit (a TUI git frontend but with good mouse support too), which lets you stage changes line-by-line and easily amend (shift+A) them into earlier commits. It also makes it easy to reorder commits etc. I think some other tools support this kind of thing as well, but lazygit is the best I've found for easily crafting the commits into a meaningful arrangement.
I think you may be referring to git's interactive rebasing and interactive staging; you can definitely do this a lot more efficiently than that :).

[reivilibre]

P.S. I find splitting by commits especially useful to avoid conflating 'style' changes (e.g. moving methods, renaming methods) and semantic changes. I think asking you to put these in separate PRs would be too much, though and it would certainly hurt the latency of review.

[MadLittleMods]

What are the team thoughts around rebasing? Anyone annoyed/worried about references changing after they already pulled the branch?

If you're just looking for some of the low hanging fruit of pulling out some style changes or merging some subsequent changes back to the original chunk, that seems a lot more do-able. If we're talking about building up iteratively commit by commit, I have a lot more apprehension to put effort into doing that.

I assume you don't want me to rebase things together from different review cycles so you can easily review the things that have changed.

What about merge commits? I find it a lot harder to rebase around merge commits and would prefer to just rebase my branch than merge develop if I started doing this. But then the rebase would rewrite all of the history including stuff you already reviewed. Do you have a good way to see what's new even though I rebased on the latest develop? (does that even matter to you)

---

A note around rebased history: I really like the GitLab diff versioning so even if you rebase, you can still reference old diff. I used to use this all the time over there.

[DMRobertson]

A note around rebased history: I really like the GitLab diff versioning so even if you rebase, you can still reference old diff. I used to use this all the time over there.

I have similar experiences with GL and reviewboard.

[DMRobertson]

What are the team thoughts around rebasing? Anyone annoyed/worried about references changing after they already pulled the branch?

I believe the rough consensus is: "don't rebase after someone has started to review the changes", because doing so resets the "I have viewed this file" in Github's review UI.

[MadLittleMods]

Thanks for the review @reivilibre 🐹