Optimize Wormhole Ethereum core contract (and other contracts)

[ali-bahjati]

Hi,

I am a contributor to the Pyth network. We are using Wormhole to send our prices cross-chain and call Wormhole core contract on our target chains to verify the price VAAs. Recently, @njkumr and I have been working on optimizing our Ethereum contract to reduce the price update cost on EVM networks. By profiling the gas usage of our transactions (using Tenderly), we realized that parseAndVerifyVM uses 139k gas on Ethereum and 114k gas on BNB. It is about 30% of our transactions' total gas usage. As our prices will be updated frequently, we would like to save as much gas as possible.
For this reason, we like to discuss optimizing the Wormhole core contract. Not only it benefit us, but it will also benefit the token bridge and all other protocols using Wormhole. If we get positive feedback, we would love to contribute to implementing them.

We created a few benchmark tests to measure our gas usage in a mainnet-like environment and started optimizing our contracts according to the benchmark results. We used a couple of tricks that helped us save substantial gas, and we believe we can also use those tricks in the Wormhole contract. We tried to implement these improvements in our (old) version of Wormhole and could save around 38k gas (measured by our benchmark tests) which is about 30% gas improvement for the Wormhole core contract.

Here are the improvements that we have tried:
Parse and verify a VM (VAA) in place (18.7k gas save): Currently, the Wormhole core contract parses the VM first and stores it in the memory. Then, the contract verifies the parsed VM. Storing ~13 signatures in the memory costs a lot of gas. Instead, we can verify the signatures in place without storing them in the memory. By doing so, we can save 18k gas. This is the modified Message.sol. We made a similar improvement to process our encoded data in place and saved a lot of gas.

Remove out-of-bound checks in BytesLib (8k gas save): Every operation in BytesLib checks whether it's not reading given bytes out of bound. However, we can check the size of a VM only once and remove this check from BytesLib. Making this change will reduce parseAndVerifyVM gas usage by 8k. This PR achieves the same thing on Pyth. Please note that adding an extra check for the size of the VM is necessary.

Make parsing (mathematically) unchecked (5.8k gas save): As solidity 0.8.0, all math operations are checked and will revert on overflow/underflow. However, when parsing a VM there is no complicated math operation, and we can wrap it in unchecked to avoid these checks. Doing the same in Pyth helped us to save ~7.4k gas.

The above changes won't change any contract interface and will not break the interface. There is another change that we tried which is breaking but worth mentioning:
Replace vm.payload with vm.payloadIndex (6k gas save): creating vm.payload actually copies the whole payload into a new memory. It costs a lot of gas, whereas callers could construct it directly from the VM. So it is better to return the payloadIndex instead.

We can implement some of the above suggestions on the other Wormhole contracts. Also, there are probably other possible optimizations that we have not tried. We believe the best way to start is to create benchmark tests (like Pyth benchmark tests) so we can measure the improvements and then go for the significant improvements one by one (in separate PRs). Once we get the benchmark merged in the repo, we can try different optimizations, discuss them here, and implement them if you think it is worth it.

We are keen to hear your opinions and comments. Also, if you have other ideas for optimizing the contracts, feel free to share them here.

P.S: Although the changes are easy, they must be carefully reviewed to ensure they do not introduce any bugs/vulnerabilities.

[tonyjin]

What are contributors' thoughts on the balance of readability vs optimization for Wormhole's EVM contracts? I ask since you could go very deep into gas optimization (see some of the responses to https://twitter.com/RareSkills_io/status/1582492216559370242), but create contracts that are difficult to maintain.

[evan-gray]

For auditability and ease of maintenance / transparency, I prefer more towards the readability side. That said, I'm all for strong unit tests especially around the less readable code to ensure breaking changes (both of the logic and compatibility variety) are avoided.

[kcsongor]

Agreed, the way to go if we devolve into low level optimisations is to keep around a clean reference implementation and use fuzz-testing (or better, the upcoming symbolic execution framework) to guarantee equivalence (up to some observability relation). That being said, these optimisations are worthwhile IMO.

[njk-64]

Definitely agree on the readability front. Optimisations that don't heavily sacrifice readability while also maintaining all the security guarantees of wormhole are a must. Luckily, most of the suggested gas optimisations fit in well with this criteria imo.

[kcsongor]

The above changes won't change any contract interface and will not break the interface.

Technically this is not true of the in-place verify in parse, since that method no longer puts the signatures into the resulting VM struct, which means that it's no longer verifiable again.

[ali-bahjati]

Ah yeah you are right. It's breaking.
I think no downstream consumer of parseAndVerify uses the signatures though, so changing it will be easier.

[kcsongor]

Thanks @ali-bahjati for this detailed writeup, it looks like there's interest in moving forward with adopting some of these optimisations. A good starting point would be to open a PR with the gas profiling setup, so we can start measuring performance. Do you have some spare capacity to do that? Alternatively, I'm also happy to set it up, if you could give some pointers that'd be much appreciated.

Next, any optimisation should be accompanied with unit testing to ensure that the behaviour doesn't change in undesired ways. This is a good opportunity to ramp up foundry-based unit testing, which is beneficial even outside of the optimisation effort.

[ali-bahjati]

@kcsongor nice! I can do it later this week. Is it fine?

[kcsongor]

Perfect!

[ali-bahjati]

Unfortunately I did not get any time to work on this and do not expect to get any free time within a few weeks 😞
So I will share our relevant work here, maybe you @kcsongor or @njkumr can pick it up earlier than me.

This is our Foundry tests directory. In the utils directory there are two files that might be useful to you.

• RandTestUtils. Sometimes there is a need to generate multiple random fuzzy structs and it is not possible to let Foundry handle that. So that's why we created this struct.
• WormholeTestUtils. This contract has two methods:
  • setupWormhole(uint8 numGuardians): initialize Wormhole core contract with the given numGuardians. For benchmarking we use 19 guardians.
  • generateVaa(..., payload, numSigners) generate a VAA from the given payload and signs it with the first numSigners guardians. For benchmarking we use 13 signers because that is the average signers of a VAA on mainnet.

The important file is GasBenchmark that has one method per each important scenario for us that we need to test. Then we use forge snapshot --matchContract GasBenchmark.t.sol to measure the differences of the gas usage and mentioned it in our PRs. We even have this in our CI to print the gas usage of each method separately.

forge test --gas-report is also useful for profiling the gas, but it is not very mature and cannot trace the methods completely.

There is one important tip about writing benchmark tests is that each test execution (from gas perspective) is a single transaction, so:

• The gas usage contains the gas used in the test contract (such as storage access or test generation). Only the initial code that runs in the setUp method of a test is excluded. I found generating our test inputs in setUp and writing them to the test contract storage cheaper than generating them on the test. The tests pays the storage cost to read them and it is a computation vs storage gas tradeoff that you should consider. Sadly, gas profiling using --gas-report will include the setUp phase.
• It means that within your test if you call contract C twice and each time it stores something on the storage slot x, the second time is a warm-write and it's very cheap compared to the first time. So each test scenario should really be an actual user doing the work. To test with multiple runs and randomized inputs (such as payload size in Wormhole core contract) you should use parameters. If you decide to use Foundry fuzzy parameters make sure to pass the --fuzz-seed to your commands otherwise it will be random each time.

I hope it helps!

[njk-64]

@ali-bahjati I can look into this sometime later in the week

[njk-64]

A draft PR for the same: #2211