msgp: Add canonical validation to msgp unmarshalling

[iansuvak]

Summary

Adds UnmarshalValidateMsg() functions to the set of generated methods. This function will error out if non-canonical encoding of a message is detected. This should only be used in places where we care about canonicity, specifically any messages that end up going on-chain such as transactions and certs.

Adds code generated by algorand/msgp#25 as well as supporting functions -- various LessFns passed into msgp:sort directives.

• Add the new method to /protocol/codec.go to call UnmarshallValidate and handle the response.

Test Plan

Existing tests should pass, other than codegen test because it's running off of a specific commit in the msgp repo.

• Add explicit tests that check that map key and struct field inversions are caught

Benchmarks

After running the benchmarks this does seem to increase the time required to decode a transaction as per the Benchmark randomized Transaction test added. The differences are:

master vs new (Just Unmarshal no validation):

and new (no validation) vs validation:

[codecov]

Codecov Report

Attention: Patch coverage is 17.64706% with 28 lines in your changes missing coverage. Please review.

Project coverage is 55.10%. Comparing base (4ff2bf3) to head (e1203f4).
Report is 411 commits behind head on master.

Files with missing lines	Patch %	Lines
protocol/codec.go	0.00%	9 Missing ⚠️
agreement/sort.go	42.85%	8 Missing ⚠️
protocol/codec_tester.go	0.00%	7 Missing ⚠️
data/basics/sort.go	0.00%	3 Missing ⚠️
protocol/stateproof.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5605      +/-   ##
==========================================
- Coverage   55.13%   55.10%   -0.04%     
==========================================
  Files         465      465              
  Lines       65022    65051      +29     
==========================================
- Hits        35852    35847       -5     
- Misses      26780    26810      +30     
- Partials     2390     2394       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

---

🚨 Try these New Features:

• Flaky Tests Detection - Detect and resolve failed and flaky tests

[iansuvak]

Just added benchstats to the PR description above. There is a 10% slowdown (.1usec) even for no validation path and an additional .1usec for the validation path. The absolute values of this seem reasonable to me but let me know if anyone disagrees or if anyone wants anything else tested.

[zeldovich]

Out of curiosity, what's the goal of this change? It doesn't seem like anything is invoking protocol.DecodeCanonicalMsg(), so it's hard for me to guess precisely what problem you're trying to solve.

Perhaps related to the above question, it seems like the UnmarshalValidateMsg methods that were added recently in msgp only check the sort order of keys in maps, though the commit message for that change suggests the intent was to check for canonical encoding.

Was there some problem with sort order specifically that prompted this change? Or is the goal to try to enforce canonical encodings for good measure? (The latter seems like a reasonable idea, but then we should probably enforce all of the canonical encoding rules, which are specified in https://github.com/algorandfoundation/specs/blob/master/dev/crypto.md)

[iansuvak]

Thank you @zeldovich the responses are inlined:

Out of curiosity, what's the goal of this change? It doesn't seem like anything is invoking protocol.DecodeCanonicalMsg(), so it's hard for me to guess precisely what problem you're trying to solve.

You are right this PR doesn't do anything other than expose the functionality. The goal was to use this for transaction decoding for two main reasons:

1. To be able to determine canonicity of a transaction message at decode time to save the re-encodes that we do when forwarding the message.
2. This would allow us to use TxID as msgID in libp2p.Gossipsub which would give us their implementation of caching/deduplication.

Perhaps related to the above question, it seems like the UnmarshalValidateMsg methods that were added recently in msgp only check the sort order of keys in maps, though the commit message for that change suggests the intent was to check for canonical encoding.
Was there some problem with sort order specifically that prompted this change? Or is the goal to try to enforce canonical encodings for good measure? (The latter seems like a reasonable idea, but then we should probably enforce all of the canonical encoding rules, which are specified in https://github.com/algorandfoundation/specs/blob/master/dev/crypto.md)

This is my mistake. I was under the impression that canonicity only enforced the sort and not omit-empty and others. Thank you for sharing the spec document. There were no problems with sort order and the idea was indeed to enforce all canonicity rules.

[zeldovich]

The goal was to use this for transaction decoding for two main reasons:

1. To be able to determine canonicity of a transaction message at decode time to save the re-encodes that we do when forwarding the message.

Just to double-check: why is canonicality important here? That is, we could forward a transaction even if the encoding is not canonical, if the other peer doesn't check for canonicality on the receiving side (which we don't).

It might be that checking canonicality is a good idea to defend against some kinds of DoS attacks, but this seems unrelated to being able to save on the re-encoding cost..

2. This would allow us to use TxID as msgID in libp2p.Gossipsub which would give us their implementation of caching/deduplication.

Interesting; I don't really know the internal details of Gossipsub. Does it require the msgID to be a hash of the transaction being gossipped?

This is my mistake. I was under the impression that canonicity only enforced the sort and not omit-empty and others. Thank you for sharing the spec document. There were no problems with sort order and the idea was indeed to enforce all canonicity rules.

Sounds good. Yeah, this is probably worth fixing then (assuming there is indeed a strong requirement for checking canonical encoding, as we are discussing above).

[zeldovich]

You might want to use fuzzing to check that you get canonical decoding right; I expect it's pretty tricky.

Here's a rough fuzzer I sketched out, just to make it more concrete what I'm talking about: zeldovich@6256ef3 (run it using, say, go test -fuzz CanonicalDecode)

As one example of non-canonicality that the fuzzer finds, which we didn't explicitly write up in the spec document, is that you might have trailing garbage at the end, which our current codec doesn't flag as an error.

[iansuvak]

@zeldovich thank you for the feedback and the fuzzer example. I will go ahead and make these changes soon and converting the PR into draft form in the meantime.