Add UnmarshallValidateMsg to generated functions

[iansuvak]

Summary

Existing UnmarshallMsg will currently decode any valid messagepack message that corresponds to a given type. For on-chain things such as Transactions and Certs.

This PR generates an additional method called UnmarshallValidateMsg that fails if:

• A sort inversion is detected for either a struct field name or a map key
• A struct is encoded as an array instead of map.

It changes the interface for the msgp:sort directive adding LessFn as the last argument which defines a method of comparing two map fields.

Generated code example is in accompanying PR in go-algorand repo: algorand/go-algorand#5605

Testing

Perhaps add explicit tests in repo confirming that canonicity is correctly tested

[algorandskiy]

The comparison algorithm looks OK. I'd like to see a test here or in go-algorand. (upd: found a test in go-algorand)
My main concerns are:

• allowing UnmarshalValidateMsg without giving the less function silently disables the feature.
• performance penalty with additional branches for types not required validating. Need to benchmark to ensure if they are negligible.

[algorandskiy]

add a code that errs if LessFunction not set for this type?

[iansuvak]

LessFunction missing is a compile time issue not runtime. Should I just emit a panic inside the code if LessFn is expected and missing?

[iansuvak]

Ah I see yeah I can just return a nil slice and error since I already have error interface.

[algorandskiy]

maybe do not require less func for all types? since numbers, strings, arrays are comparable, byte slices can be generated with bytes.Compare if m.Key.TypeName() == []bytes - i.e. to make this feature more usable by requiring less work by a user.

Btw, it looks like I can call UnmarshalValidateMsg on a type without providing LessFunction, this code would not be generated and I might get no err even there is an ordering issue.

I guess we need to go one of the roads:

1. allow UnmarshalValidateMsg only for types with LessFunction provided.
2. have UnmarshalValidateMsg to generate own code by propagating validate=true/false flag down in generator methods: UnmarshalMsg would generate regular stuff as now without if validate overhead, and UnmarshalValidateMsg would generate code with if validate + err if less function not provided but needed.

[iansuvak]

maybe do not require less func for all types? since numbers, strings, arrays are comparable, byte slices can be generated with bytes.Compare if m.Key.TypeName() == []bytes - i.e. to make this feature more usable by requiring less work by a user.

I agree -- I can make it smarter

I guess we need to go one of the roads:

1. allow UnmarshalValidateMsg only for types with LessFunction provided.
2. have UnmarshalValidateMsg to generate own code by propagating validate=true/false flag down in generator methods: UnmarshalMsg would generate regular stuff as now without if validate overhead, and UnmarshalValidateMsg would generate code with if validate + err if less function not provided but needed.

These two don't seem mutually exclusive to me. The first one is a correctness issue and the second seems like a performance concern.

I could fix the issue by including the if validate statement as separate and then inside that block do another if check if we have the LessFn and return an error uncoditionally if LessFn is missing.

[algorandskiy]

My main concern is still correctness/robustness of the code. If LessFunction not set then calling calling UnmarshalValidateMsg could return OK because the entire branch if m.Key.LessFunction() and if validate was not generated.

[iansuvak]

My main concern is still correctness/robustness of the code. If LessFunction not set then calling calling UnmarshalValidateMsg could return OK because the entire branch if m.Key.LessFunction() and if validate was not generated.

If validate should now be generated regardless of whether LessFn is set and error is hardcoded for cases when lessFn is not available. Do you see a case where I missed it?

[algorandskiy]

If validate should now be generated regardless of whether LessFn is set and error is hardcoded for cases when lessFn is not available. Do you see a case where I missed it?

How? the code is still there and I can't see preconditions like if validate and not LessFunction then error. Maybe it was not pushed? Btw, GH says "This branch cannot be rebased due to conflicts".

	if m.Key.LessFunction() != "" {
		u.p.printf("\nif validate && %s && %s(%s, %s) {", lastSet, m.Key.LessFunction(), m.Keyidx, last)
		u.p.printf("\nerr = &msgp.ErrNonCanonical{}")
		u.p.printf("\nreturn")
		u.p.printf("\n}")
	}

[iansuvak]

How? the code is still there and I can't see preconditions like if validate and not LessFunction then error. Maybe it was not pushed? Btw, GH says "This branch cannot be rebased due to conflicts".

Apologies, I had pushed the branch to upstream as well since I needed access to msgp.Uint64Less functions for code in go-algorand repo to compile but didn't push to this one which the PR is based off of. Fixed now.

Re: not being to merge do you mean the go-algorand branch? Yeah it's due to other files getting msgp changes, I was going to do it once I have a go-ahead on the approach/perf tradeoffs since I will need to change the go.mod and autogen checker versions anyway