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

ethereum: Add verifyCurrentQuorum method to core bridge #3540

Draft
wants to merge 6 commits into
base: ethereum/optimizations
Choose a base branch
from

Conversation

djb15
Copy link
Collaborator

@djb15 djb15 commented Nov 16, 2023

TODO:

Summary

Added a new method to the core bridge that makes it easier to verify the validity of a message against the current guardian set. This is useful for cases like CCQ since using this new method will reduce the number of external calls from QueryResponse.sol from 4 to 1 which will save gas and therefore make an even stronger case for the message pull pattern.

Gas analysis

Previous

The current verification looks like it does in https://github.com/wormhole-foundation/wormhole-solidity-sdk/blob/b9e129e65d34827d92fceeed8c87d3ecdfc801d0/src/QueryResponse.sol#L631-L663, with 4 unique external calls into the core bridge. This gas usage for this was tested with a sample test case:

  function testFuzz_verifyCurrentQuorumWithoutHelper(bytes memory encoded) public {
    vm.assume(encoded.length > 0);

    // Set the initial guardian set
    address[] memory initialGuardians = new address[](1);
    initialGuardians[0] = testGuardianPub;

    // Create a guardian set
    Structs.GuardianSet memory initialGuardianSet = Structs.GuardianSet({
      keys: initialGuardians,
      expirationTime: 0
    });

    messages.storeGuardianSetPub(initialGuardianSet, uint32(0));

    bytes32 message = keccak256(encoded);

    // Generate legitimate signature.
    Structs.Signature memory goodSignature;
    (goodSignature.v, goodSignature.r, goodSignature.s) = vm.sign(testGuardian, message);
    assertEq(ecrecover(message, goodSignature.v, goodSignature.r, goodSignature.s), vm.addr(testGuardian));
    goodSignature.guardianIndex = 0;

    // Attempt to verify signatures.
    Structs.Signature[] memory sigs = new Structs.Signature[](1);
    sigs[0] = goodSignature;

        // It might be worth adding a verifyCurrentQuorum call on the core bridge so that there is only 1 cross call instead of 4.
    uint32 gsi = messages.getCurrentGuardianSetIndex();
    Structs.GuardianSet memory fetchedGuardianSet = messages.getGuardianSet(gsi);

    /**
    * @dev Checks whether the guardianSet has zero keys
    * WARNING: This keys check is critical to ensure the guardianSet has keys present AND to ensure
    * that guardianSet key size doesn't fall to zero and negatively impact quorum assessment.  If guardianSet
    * key length is 0 and vm.signatures length is 0, this could compromise the integrity of both vm and
    * signature verification.
    */
    if(fetchedGuardianSet.keys.length == 0){
        revert("invalid guardian set");
    }

    /**
    * @dev We're using a fixed point number transformation with 1 decimal to deal with rounding.
    *   WARNING: This quorum check is critical to assessing whether we have enough Guardian signatures to validate a VM
    *   if making any changes to this, obtain additional peer review. If guardianSet key length is 0 and
    *   vm.signatures length is 0, this could compromise the integrity of both vm and signature verification.
    */
    if (sigs.length < messages.quorum(fetchedGuardianSet.keys.length)){
        revert("no quorum");
    }

    /// @dev Verify the proposed vm.signatures against the guardianSet
    (bool valid, string memory reason) = messages.verifySignatures(message, sigs, fetchedGuardianSet);

    assertEq(valid, true);
    assertEq(bytes(reason).length, 0);
  }
❯ forge test --match-test testFuzz_verifyCurrentQuorumWithoutHelper --gas-report
[⠊] Compiling...
No files changed, compilation skipped

Ran 1 test for forge-test/Messages.t.sol:TestMessages
[PASS] testFuzz_verifyCurrentQuorumWithoutHelper(bytes) (runs: 256, μ: 106115, ~: 106107)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 131.91ms (130.80ms CPU time)
| forge-test/Messages.t.sol:ExportedMessages contract |                 |       |        |       |         |
|-----------------------------------------------------|-----------------|-------|--------|-------|---------|
| Deployment Cost                                     | Deployment Size |       |        |       |         |
| 1668761                                             | 7513            |       |        |       |         |
| Function Name                                       | min             | avg   | median | max   | # calls |
| getCurrentGuardianSetIndex                          | 1042            | 1042  | 1042   | 1042  | 256     |
| getGuardianSet                                      | 3612            | 3612  | 3612   | 3612  | 256     |
| quorum                                              | 594             | 594   | 594    | 594   | 257     |
| storeGuardianSetPub                                 | 67461           | 67461 | 67461  | 67461 | 256     |
| verifySignatures                                    | 7046            | 7046  | 7046   | 7046  | 256     |




Ran 1 test suite in 144.28ms (131.91ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

We exclude the call to storeGuardianSetPub, and the total gas for this sums to 12,284.

New verifyCurrentQuorum method

❯ forge test --match-test "testFuzz_verifyCurrentQuorum\\b" --gas-report
[⠊] Compiling...
No files changed, compilation skipped

Ran 1 test for forge-test/Messages.t.sol:TestMessages
[PASS] testFuzz_verifyCurrentQuorum(bytes) (runs: 256, μ: 95136, ~: 95129)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 136.61ms (129.51ms CPU time)
| forge-test/Messages.t.sol:ExportedMessages contract |                 |       |        |       |         |
|-----------------------------------------------------|-----------------|-------|--------|-------|---------|
| Deployment Cost                                     | Deployment Size |       |        |       |         |
| 1668761                                             | 7513            |       |        |       |         |
| Function Name                                       | min             | avg   | median | max   | # calls |
| quorum                                              | 594             | 594   | 594    | 594   | 1       |
| storeGuardianSetPub                                 | 67461           | 67461 | 67461  | 67461 | 256     |
| verifyCurrentQuorum                                 | 10313           | 10313 | 10313  | 10313 | 256     |




Ran 1 test suite in 154.74ms (136.61ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

The gas to call this new method is 10,313.

Summary

This helper method saves 12,284 - 10,313 = 1971 gas

@djb15 djb15 changed the title ethereum: ore validate sigs ethereum: Add verifyCurrentQuorum method to core bridge Nov 16, 2023
@djb15 djb15 requested review from SEJeff and evan-gray November 16, 2023 17:43
@djb15 djb15 force-pushed the ethereum/core-validate-sigs branch from f4c0079 to 23552c1 Compare November 16, 2023 17:56
@evan-gray evan-gray added the evm label Mar 14, 2024
@djb15 djb15 force-pushed the ethereum/core-validate-sigs branch from 23552c1 to 4e959a0 Compare April 23, 2024 14:59
@djb15 djb15 force-pushed the ethereum/core-validate-sigs branch 2 times, most recently from 81fe11d to 2b9a104 Compare May 8, 2024 16:43
@djb15 djb15 changed the base branch from main to ethereum/optimizations May 8, 2024 16:43
@djb15 djb15 force-pushed the ethereum/core-validate-sigs branch from 4463803 to 50289c0 Compare July 10, 2024 20:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants