Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider adding byte packing utils #435

Open
cam-schultz opened this issue Jul 22, 2024 · 5 comments
Open

Consider adding byte packing utils #435

cam-schultz opened this issue Jul 22, 2024 · 5 comments
Assignees
Labels
audit scope Issues required as part of an upcoming audit scope enhancement New feature or request L1 Staking Contract

Comments

@cam-schultz
Copy link
Contributor

cam-schultz commented Jul 22, 2024

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 #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
Copy link
Contributor

iansuvak commented Oct 7, 2024

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
Copy link
Contributor

iansuvak commented Oct 7, 2024

Confirmed that this is a fundamental issue therefore closing.

@iansuvak iansuvak closed this as not planned Won't fix, can't repro, duplicate, stale Oct 7, 2024
@github-project-automation github-project-automation bot moved this from Backlog 🗄️ to Done ✅ in Platform Engineering Group Oct 7, 2024
@iansuvak iansuvak reopened this Oct 15, 2024
@ARR4N
Copy link

ARR4N commented Oct 15, 2024

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
Copy link
Contributor

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
Copy link

ARR4N commented Oct 28, 2024

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
audit scope Issues required as part of an upcoming audit scope enhancement New feature or request L1 Staking Contract
Projects
Archived in project
Development

No branches or pull requests

5 participants