Send canonicaljson in response to federation requests

[jevolk]

This reverts commit 120c238.

Signed-off-by: Jason Volk [email protected]

[matrixbot]

Can one of the admins verify this patch?

[matrixbot]

Can one of the admins verify this patch?

[jevolk]

Closes #2951

[richvdh]

For the record, the only reason for not merging this has been concerns over the performance of canonicaljson as compared to ujson. However, I think that with the move to simplejson across the board (and the conclusions of matrix-org/matrix-spec-proposals#3009), and the performance improvements in canonicaljson 1.1.0, we'll be able to merge this once we bump to canonicaljson 1.1.0.

[richvdh]

Wait, this changes the response for all federation APIs. matrix-org/matrix-doc#1013 only discussed the format of events on the wire. The two seem only loosely related - what am I missing?

[jevolk]

The two seem only loosely related - what am I missing?

If the message has to be signed and verified as canonical JSON then the wire message should be canonical JSON. If there's an expectation of non-canonical JSON for any message, well, wouldn't you think this whole exercise is kind of pointless then?

Perhaps @ara4n thought events should be canonical without considering X-Matrix verification when coming up with the title for https://github.com/matrix-org/matrix-doc/issues/1013 but signing JSON indeed happens in other places....

[richvdh]

but signing JSON indeed happens in other places....

Right, but the responses to federation API requests isn't one of those places. Perhaps I'm missing something, but I don't see the need for canonicaljson here.

[jevolk]

So pretty much what you're saying is to go around and make sure every PDU being sent is sorted JSON but then somehow dumps() the final response output preserving the carried PDU's order but without caring about that final outer dumps() object order...

Is that even possible for you? I don't think it is. And this is to avoid the sorting of, as a common example, two pystring references "pdus" and "auth_chain" in most responses?

I'm pretty sure you want a simple and uniform method of output without trying to grotesquely optimize it by sorting some parts and not others.

Otherwise, you'd be me.

[richvdh]

So I guess the answer to "Perhaps I'm missing something" is simply "this seems the simplest way to make sure that the events returned by /federation/v1/event and friends are sent as canonical json".

However:

Is that even possible for you?

Well, in theory we could use simplejson.RawJSON (simplejson/simplejson@da17e35, etc). I agree that is probably excessive, though.

And this is to avoid the sorting of, as a common example, two pystring references "pdus" and "auth_chain" in most responses?

Well, my concern is more about responses which contain a whole tree of objects, all of which need sorting. And irrespective of sorting, encode_canonical_json remains much slower than a default simplejson.dumps, thanks to having to work around simplejson/simplejson#207.

[llebout]

@richvdh canonicaljson was updated to 1.1.x, can this now be merged? I'm unsure how bad the performance of canonicaljson is, but if need be, I can reimplement it in C so we'll be done thinking about performance on that matter.

[llebout]

Also, I created a proposal to enforce this in the spec, matrix-org/matrix-spec-proposals#2102

[clokep]

Due to inactivity I'm going to close this PR. Is it still necessary after the changes to ujson, simplejson, etc.? If it is, please rebase and reopen this PR!