Ethereum: add optimized verification endpoint to the core bridge #3366
Conversation
gator-boi
force-pushed
the
ethereum/optimizations
branch
from
4a4ced1 to
8294537
Compare
ethereum/forge-test/Messages.t.sol
Outdated
| uint32 guardianSetIndex | ||
| ) internal view returns (bytes memory signedTransfer) { | ||
| // construct `TransferWithPayload` Wormhole message | ||
| Structs.VM memory vm; |
There was a problem hiding this comment.
If you're ok with it, would you mind renaming vm --> vm_? It just makes me cringe a bit to shadow the forge test vm cheatcode god object.
ethereum/forge-test/Messages.t.sol
Outdated
| vm.consistencyLevel = 15; | ||
| vm.payload = payload; | ||
|
|
||
| // encode the bservation |
There was a problem hiding this comment.
| // encode the bservation | |
| // encode the observation |
ethereum/forge-test/Messages.t.sol
Outdated
| @@ -161,4 +256,69 @@ contract TestMessages is Test { | |||
| assertEq(valid, false); | |||
| assertEq(reason, "vm.hash doesn't match body"); | |||
| } | |||
|
|
|||
| function testParseGuardianSetOptimized(uint8 guardianCount) public view { | |||
| vm.assume(guardianCount > 0 && guardianCount <= 19); | |||
There was a problem hiding this comment.
| vm.assume(guardianCount > 0 && guardianCount <= 19); | |
| guardianCount = bound(guardianCount, 0, 19); |
Per the forge docs for the assume cheatcode, for integers, it is best to use the bound cheatcode when fuzzing.
The way assume works with the fuzzer is that it tosses every fuzz test where the assume condition is false. Alternatively, bound simply constrains the integer within the given range. At high fuzz runs, more assumes will cause the fuzz test run to fail with max rejects.
gator-boi
force-pushed
the
ethereum/optimizations
branch
4 times, most recently
from
3122859 to
5bd07f9
Compare
ethereum/contracts/Messages.sol
Outdated
| function parseGuardianSet(bytes calldata guardianSetData) public pure returns (Structs.GuardianSet memory guardianSet) { | ||
| // Fetch the guardian set length. | ||
| uint256 endGuardianKeyIndex = guardianSetData.length - 4; | ||
| uint256 guardianCount = endGuardianKeyIndex / 20; |
There was a problem hiding this comment.
I think it would be nice to specify that this 20 is the size of an address, perhaps with an ADDRESS_SIZE constant or something equivalent.
| /// @dev Obtain the current guardianSet for the guardianSetIndex provided | ||
| Structs.GuardianSet memory guardianSet = getGuardianSet(vm.guardianSetIndex); | ||
|
|
||
| function verifyVMInternal(Structs.VM memory vm, Structs.GuardianSet memory guardianSet, bool checkHash) internal view returns (bool valid, string memory reason) { |
There was a problem hiding this comment.
I wonder, wouldn't it be better to actually have observer functions that let you parse a guardian key just in time instead of reading them all into an unpacked struct array like this?
djb15
force-pushed
the
ethereum/optimizations
branch
from
71e5682 to
5b94fb0
Compare
ethereum/contracts/Setters.sol
Outdated
| } | ||
| _state.guardianSets[index] = set; | ||
| _state.guardianSets[index] = set; |
The purpose of this PR is to add an optimized
parseAndVerifyendpoint toMessages.solalong with various other optimizations. See the following table for the estimated gas savings: