feat: generate inversions of abi.encodePacked() for StakingMessages.unpack*()

[ARR4N]

Why this should be merged

Save significant gas + improve readability so it's easier to find bugs in reviews.

Gas before

Ran 5 tests for contracts/staking/tests/ValidatorMessagesTests.t.sol:ValidatorMessagesTest
[PASS] testRegisterSubnetValidatorMessage() (gas: 127215)
[PASS] testSetSubnetValidatorWeightMessage() (gas: 44491)
[PASS] testSubnetValidatorRegistrationMessage() (gas: 31525)
[PASS] testSubnetValidatorWeightUpdateMessag() (gas: 44578)
[PASS] testValidationUptimeMessage() (gas: 37860)
Suite result: ok. 5 passed; 0 failed; 0 skipped; finished in 563.33µs (757.46µs CPU time)

Gas after

Ran 5 tests for contracts/staking/tests/ValidatorMessagesTests.t.sol:ValidatorMessagesTest
[PASS] testRegisterSubnetValidatorMessage() (gas: 10151)
[PASS] testSetSubnetValidatorWeightMessage() (gas: 4728)
[PASS] testSubnetValidatorRegistrationMessage() (gas: 3782)
[PASS] testSubnetValidatorWeightUpdateMessag() (gas: 4815)
[PASS] testValidationUptimeMessage() (gas: 4133)
Suite result: ok. 5 passed; 0 failed; 0 skipped; finished in 372.38µs (320.00µs CPU time)

How this works

Packing was identical to abi.encodePacked() so it's replaced as such. There is no abi.decodePacked() so there's a code generator to load words directly from memory. Solidity automatically performs the necessary masking for bytes<N> types so only the mload() needs to be explicit.

Warning

There's an option to use mcopy() but we don't have the opcode yet so the generated code defaults to using the original memory space when returning dynamically sized arrays. This is a destructive operation that corrupts the input. If this isn't acceptable I can write a manual mcopy() instead.

How this was tested

The code generator also generates round-trip fuzz tests. To demonstrate equivalence with the previous approach, I originally only changed the pack* methods to allow the existing tests to show that original unpacking worked as expected; only then did I change the unpack* methods to use the generated code.

How is this documented

Comments in line with implementation, if and where required.

Note

Please excuse the force push and chaotic commit history. I honestly have no idea what went wrong when rebasing (then merging) onto the changed upstream branch. This SHOULD be squash merged!

[cam-schultz]

Thanks for putting this together! We'll definitely want to integrate these optimizations once the Warp message definitions in question are stable.

[iansuvak]

Yeah big fan of this method! Thanks!

[minghinmatthewlam]

how strongly do you feel about custom errors? We previously reverted from custom errors back to require statements, because we felt some tooling weren't updated to custom errors and lacked in debugging

[ARR4N]

Which tooling lacks support?

I think they're superior to string errors:

1. Don't need to store the entire string in the contract code
2. Can be parameterised to aid debugging and allow precise path detection in tests
3. Avoid change-detector tests

[minghinmatthewlam]

we reverted to require statements over a year ago, so probably need to revisit this. I think back then some combination of forge errors, block explorers, and e2e testing just showed the hex error signature. This still seems like the case for our e2e test code, but there's likely a workaround we could do.

I am also leaning towards custom errors now, but think we should tackle this in a separate issue, and still conform to require to match the rest of the repo.

[ARR4N]

It's not a hill I'll die on, but something I feel I should push back on and ask "are you sure?" before I implement a toggle between the two.

Independent benchmarking of both deployment and usage gas.

Where is the problematic e2e test code?

[ARR4N]

As it currently stands, these reverts can never occur because I left the original length checks in place for the time being.

[minghinmatthewlam]

This folder holds the e2e tests I'm referring to https://github.com/ava-labs/teleporter/tree/main/tests/flows. This is also not a hill I'll die on, since I'm also leaning towards custom errors at this point, but wasn't sure to defer to a separate issue and changing all the require statements in the repo together so we can keep them consistent for now.

[minghinmatthewlam]

LGTM once licensing is cleared up

[geoff-vball]

Overall this looks so very useful! I think the current generated code was manually run on the current contracts? We should set up some sort of shell script (or add to the existing abi_bindings.sh) to make sure these are regenerated when files are updated.

[geoff-vball]

Just converted this to draft as it's been stale for a while and will need some reworking before it's mergeable again.

[ARR4N]

@geoff-vball would you like me to revive it so it can be merged? At the time there wasn't consensus about its inclusion so I didn't keep up with the base branch, but if the team definitely wants it then I can update it.

[geoff-vball]

@ARR4N I think @iansuvak was the last one that had a good idea of what we wanted to do here, I'll let him chime in :)

[iansuvak]

@ARR4N the issue that cropped up here was that now we have multiple dynamic fields in the message specs. All fields are prefixed with their length except for BLS key which is static at 48 bytes but appears dynamic in the definition because it's greater than 32.

I started hacking at an implementation to support this but ended up deprioritizing. If you have an idea for a clean approach implementation wise I'd love to hear/ see it but it's not high priority