[eth]: Fix gas benchmark to generate useful gas snapshot (#372)

* [eth]: Fix incorrect gas usage problem

* Make gas report more accurate

* Update readme

* Address Jayant comment

ethereum/README.md

@@ -59,17 +59,19 @@ A gas report should have a couple of tables like this:
 ├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
 │ .............                                                                             ┆ .....           ┆ .....  ┆ .....  ┆ .....   ┆ ..      │
 ├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
-│ parseAndVerifyVM                                                                          ┆ 90292           ┆ 91262  ┆ 90292  ┆ 138792  ┆ 50      │
+│ updatePriceFeeds                                                                          ┆ 383169          ┆ 724277 ┆ 187385 ┆ 1065385 ┆ 2       │
-├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
-│ updatePriceFeeds                                                                          ┆ 187385          ┆ 206005 ┆ 187385 ┆ 1118385 ┆ 50      │
 ├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
 │ .............                                                                             ┆ .....           ┆ .....  ┆ .....  ┆ .....   ┆ ...     │
 ╰───────────────────────────────────────────────────────────────────────────────────────────┴─────────────────┴────────┴────────┴─────────┴─────────╯
 ```
 
-For most of the methods, the median gas usage is an indication of our desired gas usage. Because the calls that store something in the storage
+For most of the methods, the minimum gas usage is an indication of our desired gas usage. Because the calls that store something in the storage
-for the first time use significantly more gas.
+for the first time in `setUp` use significantly more gas. For example, in the above table, there are two calls to `updatePriceFeeds`. The first
+call has happend in the `setUp` method and costed over a million gas and is not intended for our Benchmark. So our desired value is the
+minimum value which is around 380k gas.
 
 If you like to optimize the contract and measure the gas optimization you can get gas snapshots using `forge snapshot` and evaluate your
 optimization with it. For more information, please refer to [Gas Snapshots documentation](https://book.getfoundry.sh/forge/gas-snapshots).
 Once you optimized the code, please share the snapshot difference (generated using `forge snapshot --diff <old-snapshot>`) in the PR too.
+This snapshot gas value also includes an initial transaction cost as well as reading from the contract storage itself. You can get the
+most accurate result by looking at the gas report or the gas shown in the call trace with `-vvvv` argument to `forge test`.

ethereum/forge-test/GasBenchmark.t.sol

@@ -21,12 +21,23 @@ contract GasBenchmark is Test, WormholeTestUtils, PythTestUtils {
     // We use 5 prices to form a batch of 5 prices, close to our mainnet transactions.
     uint8 constant NUM_PRICES = 5;
 
-    uint constant BENCHMARK_ITERATIONS = 1000;
-
     IPyth public pyth;
     
     bytes32[] priceIds;
-    PythStructs.Price[] prices;
+
+    // Cached prices are populated in the setUp
+    PythStructs.Price[] cachedPrices;
+    bytes[] cachedPricesUpdateData;
+    uint cachedPricesUpdateFee;
+    uint64[] cachedPricesPublishTimes;
+
+    // Fresh prices are different prices that can be used
+    // as a fresh price to update the prices
+    PythStructs.Price[] freshPrices;
+    bytes[] freshPricesUpdateData;
+    uint freshPricesUpdateFee;
+    uint64[] freshPricesPublishTimes;
+
     uint64 sequence;
     uint randSeed;
 
@@ -40,13 +51,32 @@ contract GasBenchmark is Test, WormholeTestUtils, PythTestUtils {
         }
 
         for (uint i = 0; i < NUM_PRICES; ++i) {
-            prices.push(PythStructs.Price(
+            
+            uint64 publishTime = uint64(getRand() % 10);
+    
+            cachedPrices.push(PythStructs.Price(
                 int64(uint64(getRand() % 1000)), // Price
                 uint64(getRand() % 100), // Confidence
                 -5, // Expo
-                getRand() % 10 // publishTime
+                publishTime
             ));
+            cachedPricesPublishTimes.push(publishTime);
+
+            publishTime += uint64(getRand() % 10);
+            freshPrices.push(PythStructs.Price(
+                int64(uint64(getRand() % 1000)), // Price
+                uint64(getRand() % 100), // Confidence
+                -5, // Expo
+                publishTime
+            ));
+            freshPricesPublishTimes.push(publishTime);
         }
+
+        // Populate the contract with the initial prices
+        (cachedPricesUpdateData, cachedPricesUpdateFee) = generateUpdateDataAndFee(cachedPrices);
+        pyth.updatePriceFeeds{value: cachedPricesUpdateFee}(cachedPricesUpdateData);
+
+        (freshPricesUpdateData, freshPricesUpdateFee) = generateUpdateDataAndFee(freshPrices);
     }
 
     function getRand() internal returns (uint val) {
@@ -54,15 +84,7 @@ contract GasBenchmark is Test, WormholeTestUtils, PythTestUtils {
         val = uint(keccak256(abi.encode(randSeed)));
     }
 
-    function advancePrices() internal {
+    function generateUpdateDataAndFee(PythStructs.Price[] memory prices) internal returns (bytes[] memory updateData, uint updateFee) {
-        for (uint i = 0; i < NUM_PRICES; ++i) {
-            prices[i].price = int64(uint64(getRand() % 1000));
-            prices[i].conf = uint64(getRand() % 100);
-            prices[i].publishTime += getRand() % 10;
-        }
-    }
-
-    function generateUpdateDataAndFee() internal returns (bytes[] memory updateData, uint updateFee) {
         bytes memory vaa = generatePriceFeedUpdateVAA(
             priceIds,
             prices,
@@ -79,68 +101,36 @@ contract GasBenchmark is Test, WormholeTestUtils, PythTestUtils {
     }
 
     function testBenchmarkUpdatePriceFeedsFresh() public {
-        for (uint i = 0; i < BENCHMARK_ITERATIONS; ++i) {
+        pyth.updatePriceFeeds{value: freshPricesUpdateFee}(freshPricesUpdateData);
-            advancePrices();
-
-            (bytes[] memory updateData, uint updateFee) = generateUpdateDataAndFee();
-            pyth.updatePriceFeeds{value: updateFee}(updateData);
-        }
     }
 
     function testBenchmarkUpdatePriceFeedsNotFresh() public {
-        for (uint i = 0; i < BENCHMARK_ITERATIONS; ++i) {
+        pyth.updatePriceFeeds{value: cachedPricesUpdateFee}(cachedPricesUpdateData);
-            (bytes[] memory updateData, uint updateFee) = generateUpdateDataAndFee();
-            pyth.updatePriceFeeds{value: updateFee}(updateData);
-        }
     }
 
     function testBenchmarkUpdatePriceFeedsIfNecessaryFresh() public {
-        for (uint i = 0; i < BENCHMARK_ITERATIONS; ++i) {
+        // Since the prices have advanced, the publishTimes are newer than one in
-            advancePrices();
+        // the contract and hence, the call should succeed.
-
+        pyth.updatePriceFeedsIfNecessary{value: freshPricesUpdateFee}(freshPricesUpdateData, priceIds, freshPricesPublishTimes);
-            uint64[] memory publishTimes = new uint64[](NUM_PRICES);
-            
-            for (uint j = 0; j < NUM_PRICES; ++j) {
-                publishTimes[j] = uint64(prices[j].publishTime);
-            }
-
-            (bytes[] memory updateData, uint updateFee) = generateUpdateDataAndFee();
-
-            // Since the prices have advanced, the publishTimes are newer than one in
-            // the contract and hence, the call should succeed.
-            pyth.updatePriceFeedsIfNecessary{value: updateFee}(updateData, priceIds, publishTimes);
-        }
     }
 
     function testBenchmarkUpdatePriceFeedsIfNecessaryNotFresh() public {
-        for (uint i = 0; i < BENCHMARK_ITERATIONS; ++i) {
+        // Since the price is not advanced, the publishTimes are the same as the
-            uint64[] memory publishTimes = new uint64[](NUM_PRICES);
+        // ones in the contract.
-            
+        vm.expectRevert(bytes("no prices in the submitted batch have fresh prices, so this update will have no effect"));
-            for (uint j = 0; j < NUM_PRICES; ++j) {
-                publishTimes[j] = uint64(prices[j].publishTime);
-            }
 
-            (bytes[] memory updateData, uint updateFee) = generateUpdateDataAndFee();
+        pyth.updatePriceFeedsIfNecessary{value: cachedPricesUpdateFee}(cachedPricesUpdateData, priceIds, cachedPricesPublishTimes);
-
-            // Since the price is not advanced, the publishTimes are the same as the
-            // ones in the contract except the first update.
-            if (i > 0) {
-                vm.expectRevert(bytes("no prices in the submitted batch have fresh prices, so this update will have no effect"));
-            }
-    
-            pyth.updatePriceFeedsIfNecessary{value: updateFee}(updateData, priceIds, publishTimes);
-        }
     }
 
     function testBenchmarkGetPrice() public {
-        (bytes[] memory updateData, uint updateFee) = generateUpdateDataAndFee();
+        // Set the block timestamp to 0. As prices have < 10 timestamp and staleness 
-        pyth.updatePriceFeeds{value: updateFee}(updateData);
+        // is set to 60 seconds, the getPrice should work as expected.
+        vm.warp(0);
 
-        // Set the block timestamp to the publish time, so getPrice work as expected.
+        pyth.getPrice(priceIds[0]);
-        vm.warp(prices[0].publishTime);
+    }
 
-        for (uint i = 0; i < BENCHMARK_ITERATIONS; ++i) {
+    function testBenchmarkGetUpdateFee() public view {
-            pyth.getPrice(priceIds[getRand() % NUM_PRICES]);
+        pyth.getUpdateFee(freshPricesUpdateData);
-        }
     }
 }