ava-labs / teleporter

EVM cross-chain messaging protocol built on top of Avalanche Warp Messaging
Other
49 stars 23 forks source link

Consider adding byte packing utils #435

Open cam-schultz opened 3 months ago

cam-schultz commented 3 months ago

Context and scope The Staking Manager contracts pack and upack Warp messages to/from byte strings. This involves bit manipulation which can likely be extracted into libraries.

@ARR4N 's POC here https://github.com/ava-labs/teleporter/pull/482 is the desired approach but it needs to be extended to support our encoding schema.

It i's impossible to decode a message with more than one dynamic length field without knowing the lengths ahead of time. We encode lengths for all dynamic length fields right now in the field prior to the dynamic length field except for the blsPublicKeys which are packed straight to 48 bytes but are still dynamic bytes field on the Solidity side.

To use the approach above we need to account for both cases. One way of handling it might be to add an annotation to the blsPublicKey field that explicitly sets it's length to 48 and assume that all other dynamic length fields are directly preceeded by their encoded length.

iansuvak commented 1 month ago

Existing implementation for more efficient unpacking by @ARR4N is limited to at most one dynamic field. We should verify if a similar solution is possible with multiple dynamic length fields and close if not

iansuvak commented 1 month ago

Confirmed that this is a fundamental issue therefore closing.

ARR4N commented 1 month ago

Adding the buffer length immediately before the packed bytes provides an elegant way to decode it because that mirrors the way Solidity encodes arrays in memory. It means that you can just point some output variable at the word in memory that encodes the length.

My (jet-lagged) first thought is that this should still work even if the length word isn't on a regular memory-word boundary, but there's a small chance you'll either have to copy memory for decoding or have some empty bytes in the encoding.

iansuvak commented 4 weeks ago

Changing the encoding in general to either account for blsPubKey or to add empty bytes to do word padding would be another ACP-77 change so I doubt that we would go for it. I think that copying some memory for decoding would be an acceptable tradeoff.

@ARR4N do you think that doing some sort of annotation or special handling of blsPubKey length fields to assume that it's 48 bytes is possible? It looks like the intention of your PR is to call unpackgen generator from a manual script with byte_sizes flag argument. It looks like manual handling for blsPubKey would just involve allowing for constant length bytes > 32 and decoding them as bytes memory of constant length and then passing the length 48 to the generator.

ARR4N commented 2 weeks ago

Sorry, I'm not sure why I wasn't notified about your reply.

It looks like manual handling for blsPubKey would just involve allowing for constant length bytes > 32 and decoding them as bytes memory of constant length and then passing the length 48 to the generator.

Yup. There will be some nuance around the ability to unpack in place vs having to use MCOPY (which we don't have yet), but that's an implementation detail (granted, one worth flagging early).