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

Bugfix/improve gateway contract unit tests #769

Closed
wants to merge 15 commits into from

Conversation

Lohann
Copy link
Contributor

@Lohann Lohann commented Mar 18, 2024

Description

The GMP submitMessage address conversion was wrong, this PR fixes it, and also adds a new contract for accurately calculate the gas cost and base cost.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

@Lohann Lohann requested review from dvc94ch and 4meta5 March 18, 2024 14:00
data "assertGe_b" " Value b"
data "insufficient_funds" "Error: account %s has no sufficient funds, require %s have %s"
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The contract or this check? This is useful for subtract the balance from sender's account before execute the GMP message, this check verify if the account has funds before execute it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meant the entire yul thing. I'm not fluent in reading evm asm.

Copy link
Contributor Author

@Lohann Lohann Mar 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah, my bad, that's mainly because forge gas reporting is misleading, as it doesn't take into account the call overhead.

When you do:

uint256 gasBefore = gasleft();
gateway.execute();
uint256 gasAfter = gasleft();

There's some overhead between gasBefore and gasAfter that forge doesn't take into account.

In most cases that overhead is negligible (100~2600 gas units), but in our case we refund the gas used and we check it in the unit tests, any discrepancy causes the test to fail, so we need a stable and accurate gas cost. I found two alternatives for accurately calculate the Gateway.execute gas cost:
1 - Send the tx to a real node (ex: go-ethereum) and store the gas used.
2 - Subtract the extra overhead from the final gas cost.

The first option requires start a local ethereum node and calls the RPC, forge allow us to call a node from solidity using the RPC Utils, but I didn't want to setup a node just to run the smart contract unit tests.

I decided to go for the second approach, which is subtract the overhead, this would be easy if the overhead was constant, but it isn't 😢 , there's two factors:
1 - If the contract is cold, there's a fixed overhead of 2600, if the contract is warm, the overhead is 100
2 - The number of instructions that solidity inject between gasBefore and gasAfter is not constant, different tests may have different overheads.. damn solidity.

The only way I found to keep the number of instructions between gasBefore and gasAfter constant in any circumstance was by inject an inline call in EVM assembly, for that I needed the verbatim functionality, and unfortunately Solidity doesn't support Verbatim yet, so I had to use yul.

Copy link
Contributor Author

@Lohann Lohann Mar 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only important thing in this yul contract is this line here:

// Execute call
let success, gasUsed := verbatim_7i_2o(
    hex"813f509594939291905a96f15a90910360699003",
    calldataload(68), // gas limit
    calldataload(36), // address
    callvalue(), // value
    add(ptr, 32), // arg offset
    mload(ptr), // arg size
    0,
    0
)

Everything else can be done in solidity.. that line basically tells that this bytecode blob 813f509594939291905a96f15a90910360699003 reads 7 inputs, and returns 2 values, it does the gasBefore and gasAfter but in EVM assembly, so the overhead is always constant.

@Lohann Lohann closed this Apr 5, 2024
@dvc94ch dvc94ch deleted the bugfix/improve-gateway-contract-unit-tests branch April 11, 2024 12:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants