short_vec::decode_len() returns wrong size for aliased values

[t-nelson]

Problem

short_vec::decode_len() always returns the byte size of the most compact encoding, despite the existence of larger aliases. In conjunction with ShortVec this allows for interesting deserialization games to be played.

h/t @svenski123 ❤️

Summary of Changes

Add failing test
Factor byte visitation logic out of ShortU16 deserializer to a helper
Reimplement decode_len() using aforementioned helper

fixes #11628

[garious]

.

[mergify]

automerge label removed due to a CI failure

Pull request has been modified.

[codecov]

Codecov Report

Merging #11624 into master will increase coverage by 0.0%.
The diff coverage is 90.4%.

@@           Coverage Diff           @@
##           master   #11624   +/-   ##
=======================================
  Coverage    82.0%    82.0%           
=======================================
  Files         330      330           
  Lines       76029    76059   +30     
=======================================
+ Hits        62405    62435   +30     
  Misses      13624    13624           

[svenski123]

Given that the maximum size of a transaction on the wire is 1,232 bytes (PACKET_DATA_SIZE in sdk/src/packet.rs) and that the size of a Pubkey is 32 bytes, it's not possible to fit more than 38 account public keys into a transaction.

Along similar lines, it is not possible to fit more than 19 signatures in a single transaction.

Thus I would suggest defining the # number of signatures and # of accounts fields as a single unsigned byte, with the two high bits reserved and set to zero. This allows for values from 0 - 63 which is more than enough given the maximum transaction size, and allows for an easy to introduce / identify a new wire format in the future.

I do not think there would be any existing production transactions with a multi-byte ShortU16 encoding.

I would also suggest considering making a similar change to the number of instructions in Message and the number of account indicies and data bytes in CompiledInstruction.

Of course I note it is possible to encode more than 256 u8 account indices in one instruction and it is possible to encode more than 256 CompiledInstructions (provided they're short) in a Message. However I do not consider there is a reasonable need for more than 255 or (even 127) accounts per instruction. Similarly I would consider a limit of 127 instructions per transaction sufficient.

Replacing variable length encoded ShortU16's with single bytes makes for simpler encoding / decoding logic (especially non-Rust clients) and better security.

[garious]

Good points! Always looking for opportunities to inch closer to a repr(C) format.