Change how we handle Sorted JSON encoding

[ValarDragon]

Currently what we do is inefficient. We marshal a JSON string, and then run MustSortJSON on it. MustSortJSON JSON decodes the input, and then re-JSON encodes, but using the stdlib instead of amino.

Either we should make amino provide a method to JSON sort its input, or we should make a method to just sort the lines of a JSON encoded blob. In a 30 block simulation I ran, it was responsible for about 10% of the time. (A little bit skewed though, since the simulation didn't really hit anything that actually makes the chain slow down. This simulation did however have 525 signature verifications)

---

For Admin Use

• Not duplicate issue

[alexanderbez]

Either we should make amino provide a method to JSON sort its input...

What is the input here? A byte string or an interface?

[ValarDragon]

The sort JSON method takes a JSON encoded byte string.

Amino takes in a concrete struct and returns a JSON encoded byte string. The order currently getting called is effectively:

amino_bz           := amino_json_encode(msg_struct)
decoded_bytes      := stdlib_json_decode(amino_bz)
return_val         := stdlib_json_encode(decoded_bytes)

example: https://github.com/cosmos/cosmos-sdk/blob/develop/x/gov/msgs.go#L132

[tac0turtle]

@alexanderbez is this still applicable with the client encoding changes that will be done in the coming weeks

[alexanderbez]

I imagine this will still be useful for clients that still which to use amino. But we should fix the SDK's (Must)SortJSON to not decode.

[tac0turtle]

reopen if still applicable

[ValarDragon]

This is still relevant.

Most messages sign bytes are implemented of the form

func (msg MsgSetWithdrawAddress) GetSignBytes() []byte {
	bz := ModuleCdc.MustMarshalJSON(&msg)
	return sdk.MustSortJSON(bz)
}

The implementation now does

unsorted_json_bz           := module_cdc_encode(msg_struct)
decoded_bytes      := stdlib_json_decode(unsorted_json_bz)
return_val         := stdlib_json_encode(decoded_bytes)

See

cosmos-sdk/types/utils.go

Lines 29 to 35 in ab24d0e

	func SortJSON(toSortJSON []byte) ([]byte, error) {
	var c interface{}
	err := json.Unmarshal(toSortJSON, &c)
	if err != nil {
	return nil, err
	}
	js, err := json.Marshal(c)

I propose we add a ModuleCDC.MustMarshalSortedJSON method, and switch to using this everywhere applicable. Then for the moduleCDC, we can just make sorted json directly via the stdlib.

[odeke-em]

@ValarDragon thanks for filing this issue! So if I follow closely, this issue is asking for go-amino's JSON encoder to be eliminated entirely, correct?

If so, that means that we are going to entirely use encoding/json which means, we are going to violate the unsupported types per https://github.com/tendermint/go-amino#unsupported-types that will now be supported:

• floating points
• enums
• maps

this can potentially cause security issues given that JSON is not bound to any schema and would be unexpected for Cosmos ecosystem code. Also Amino has an advantage of understanding interfaces.

Suggested remedies

• If cdc.Unsafe is set then we can use the stdlib encoding/json immediately
• If !cdc.Unsafe, use a helper function that traverses a struct's fields and panics the way amino would when it encounters floating points, enums and maps

CURRENT DIRECTION/EXPERIMENT
I am currently writing code to walk a reflect.Value to check if go-amino would reject it, without invoking .encodeReflectJSON and if it passes, we can invoke stdlib's encoding/json to speed things up.

/cc @elias-orijtech

[odeke-em]

Great results from my 1.5 hour long experiment in which I wrote a package aminocompat with a function AllClear per #16092 which in the scenario that !cdc.Unsafe (the common case), will walk through a value and if it doesn't have any fields that are amino-incompatible, returns a nil error and we can then directly invoke encoding/json.Marshal once. The results of doing this on x/gov/types/v1.MsgDeposit.GetSignBytes are start

$ benchstat before.txt after.txt 
name          old time/op    new time/op    delta
MsgDeposit-8    10.8µs ± 1%     1.3µs ± 1%  -87.55%  (p=0.000 n=10+8)

name          old alloc/op   new alloc/op   delta
MsgDeposit-8    3.75kB ± 0%    0.26kB ± 0%  -92.96%  (p=0.000 n=10+10)

name          old allocs/op  new allocs/op  delta
MsgDeposit-8      99.0 ± 0%      13.0 ± 0%  -86.87%  (p=0.000 n=10+10)

[aaronc]

Signing using go-amino and GetSignBytes is deprecated and I believe the code paths calling these have mostly been removed on main. The new implementation is in x/tx and we can optimize sorting there addressing this issue. @kocubinski can you comment.

[kocubinski]

Correct, main no longer uses go-amino for amino JSON marshaling in production code paths.

The current implementation uses a round trip to sort JSON same as the legacy encoder.

cosmos-sdk/x/tx/signing/aminojson/aminojson.go

Line 115 in 4cb71ce

return sortJSON(bz)

We only do this once at the end of encoding, in contrast to the legacy implementation which roundtrips once per message then again at the end, so there should be a slight performance improvement.

That said we should just render JSON fields in order. The sort should happen after we fetch all field names from the protoreflect.Message, somewhere the vicinity of the line linked below.

cosmos-sdk/x/tx/signing/aminojson/json_marshal.go

Line 171 in 4cb71ce

fields := msg.Descriptor().Fields()

[aaronc]

@odeke-em would you be interested in working on the improvement to x/tx/signing/aminojson that @kocubinski outlined above?

[odeke-em]

Gladly @aaronc! I'll jump onto it in the morning as it is almost midnight here.