From 275fb6b369eef7ba760b354cba38c1fa90731ecd Mon Sep 17 00:00:00 2001 From: zajck Date: Wed, 9 Aug 2023 12:40:50 +0200 Subject: [PATCH 1/9] run tests, capture output --- scripts/util/estimate-twin-transfer-limits.js | 57 +++++++++++++++++++ 1 file changed, 57 insertions(+) create mode 100644 scripts/util/estimate-twin-transfer-limits.js diff --git a/scripts/util/estimate-twin-transfer-limits.js b/scripts/util/estimate-twin-transfer-limits.js new file mode 100644 index 000000000..72806bf46 --- /dev/null +++ b/scripts/util/estimate-twin-transfer-limits.js @@ -0,0 +1,57 @@ +var log = console.log; + +// Modify the console.log to capture the contract output +var contractLogs = []; +console.log = function () { + contractLogs.push(arguments[0]); +}; + +const hre = require("hardhat"); + +/** +Estimates the values for SINGLE_TWIN_RESERVED_GAS, MINIMAL_RESIDUAL_GAS + +Use with caution if `transferTwins` or other methods that call it changed a lot. + +This script does the following: +1. Preprocesses the `transferTwins` method to add the `gasLeft()` markers + If it cannot reliably position the markers, it will throw an error +2 Runs the tests that cover the `transferTwins` method +3. Captures the console.log output +4. Analyzes the output and estimates SINGLE_TWIN_RESERVED_GAS and MINIMAL_RESIDUAL_GAS + +*/ +async function estimateTwinTransferLimits() { + // 1. Preprocess the `twinTransfer` to add the `gasLeft()` markers + // hre.config.preprocess = { + // eachLine: () => ({ + // transform: (line) => { + // if (line.match(/^\s*pragma /i)) { + // // + // line = line.replace(/solidity\s+0\.8\.9/i, "solidity 0.8.18"); + // } + // return line; + // }, + // }), + // }; + + // 2. Run tests that cover the bundles and capture the output + hre.config.mocha = { + timeout: 100000, + testFiles: ["./test/protocol/ExchangeHandlerTest.js"], + // grep: "redeemVoucher\\(\\) with bundle", + grep: "uld not decrease twin supplyAvailable if supply is unlimited", + reporter: "min", + }; + hre.config.gasReporter.enabled = false; + + log("Running tests..."); + await hre.run("test"); + + log("Tests complete"); + log(contractLogs); +} + +estimateTwinTransferLimits(); + +exports.estimateTwinTransferLimits = estimateTwinTransferLimits; From 6b6f6288ebed55260d3745bb835b24d68a9fa615 Mon Sep 17 00:00:00 2001 From: zajck Date: Wed, 9 Aug 2023 15:13:41 +0200 Subject: [PATCH 2/9] preprocess start --- .../protocol/facets/ExchangeHandlerFacet.sol | 20 +++++++- scripts/util/estimate-twin-transfer-limits.js | 48 ++++++++++++++----- 2 files changed, 55 insertions(+), 13 deletions(-) diff --git a/contracts/protocol/facets/ExchangeHandlerFacet.sol b/contracts/protocol/facets/ExchangeHandlerFacet.sol index a04473f1a..a85e3a31f 100644 --- a/contracts/protocol/facets/ExchangeHandlerFacet.sol +++ b/contracts/protocol/facets/ExchangeHandlerFacet.sol @@ -16,6 +16,8 @@ import { IERC721 } from "@openzeppelin/contracts/token/ERC721/IERC721.sol"; import { IERC1155 } from "@openzeppelin/contracts/token/ERC1155/IERC1155.sol"; import { Address } from "@openzeppelin/contracts/utils/Address.sol"; +// import "hardhat/console.sol"; + /** * @title ExchangeHandlerFacet * @@ -597,6 +599,8 @@ contract ExchangeHandlerFacet is IBosonExchangeHandler, BuyerBase, DisputeBase { // Notify watchers of state change emit VoucherRedeemed(offerId, _exchangeId, msgSender()); + + // console.log("final event", gasleft()); // add a bit more to toggle reentrancy } /** @@ -830,8 +834,12 @@ contract ExchangeHandlerFacet is IBosonExchangeHandler, BuyerBase, DisputeBase { // Oveflow happens for twinCount ~ 9.6x10^71, which is impossible to achieve uint256 reservedGas = twinCount * SINGLE_TWIN_RESERVED_GAS + MINIMAL_RESIDUAL_GAS; + console.log("before loop", gasleft()); + // Visit the twins for (uint256 i = 0; i < twinCount; ) { + console.log("loop start", i, gasleft()); + // Get the twin (, Twin storage twinS) = fetchTwin(twinIds[i]); @@ -894,12 +902,18 @@ contract ExchangeHandlerFacet is IBosonExchangeHandler, BuyerBase, DisputeBase { // Make call only if there is enough gas and code at address exists. // If not, skip the call and mark the transfer as failed twinM.tokenAddress = twinS.tokenAddress; - if (gasleft() > reservedGas && twinM.tokenAddress.isContract()) { + + console.log("loop constant cost #1", i, gasleft()); + + uint256 gasLeft = gasleft(); + if (gasLeft > reservedGas && twinM.tokenAddress.isContract()) { bytes memory result; - (success, result) = twinM.tokenAddress.call{ gas: gasleft() - reservedGas }(data); + (success, result) = twinM.tokenAddress.call{ gas: gasLeft - reservedGas }(data); success = success && (result.length == 0 || abi.decode(result, (bool))); } + + console.log("loop variable cost", i, gasleft()); } twinM.id = twinS.id; @@ -941,6 +955,7 @@ contract ExchangeHandlerFacet is IBosonExchangeHandler, BuyerBase, DisputeBase { unchecked { i++; } + console.log("loop constant cost #2", i, gasleft()); } } @@ -948,6 +963,7 @@ contract ExchangeHandlerFacet is IBosonExchangeHandler, BuyerBase, DisputeBase { if (transferFailed) { raiseDisputeInternal(_exchange, _voucher, sellerId); } + console.log("redisual cost", gasleft()); } /** diff --git a/scripts/util/estimate-twin-transfer-limits.js b/scripts/util/estimate-twin-transfer-limits.js index 72806bf46..73cebdcbb 100644 --- a/scripts/util/estimate-twin-transfer-limits.js +++ b/scripts/util/estimate-twin-transfer-limits.js @@ -23,17 +23,43 @@ This script does the following: */ async function estimateTwinTransferLimits() { // 1. Preprocess the `twinTransfer` to add the `gasLeft()` markers - // hre.config.preprocess = { - // eachLine: () => ({ - // transform: (line) => { - // if (line.match(/^\s*pragma /i)) { - // // - // line = line.replace(/solidity\s+0\.8\.9/i, "solidity 0.8.18"); - // } - // return line; - // }, - // }), - // }; + let startFound = false; + let invokeFound = false; + const ExchangeHandlerPath = process.cwd() + "/contracts/protocol/facets/ExchangeHandlerFacet.sol"; + hre.config.preprocess = { + eachLine: () => ({ + transform: (line, { absolutePath }) => { + // Filter paths here, since preprocessor's options "files" doesn't work + if (absolutePath !== ExchangeHandlerPath) { + return line; + } + + // 1. add console.sol import + if (line.includes("pragma solidity")) { + line = line + "import 'hardhat/console.sol';"; + console.log("import path"); + console.log(absolutePath); + } + + if (line.includes("transferTwins")) { + if (line.includes("function")) { + startFound = true; + } else { + invokeFound = true; + } + } + + if (startFound && line.includes("{")) { + line = line + 'console.log("start", gasleft());'; + startFound = false; + } else if (invokeFound && line.includes("}")) { + line = 'console.log("final event", gasleft());' + line; + invokeFound = false; + } + return line; + }, + }), + }; // 2. Run tests that cover the bundles and capture the output hre.config.mocha = { From 1475c18f110075a19a5f395f0cd3382d9cb3ce54 Mon Sep 17 00:00:00 2001 From: zajck Date: Thu, 10 Aug 2023 09:17:30 +0200 Subject: [PATCH 3/9] analyze logs --- .../protocol/facets/ExchangeHandlerFacet.sol | 20 +-- scripts/util/estimate-twin-transfer-limits.js | 140 +++++++++++++----- 2 files changed, 108 insertions(+), 52 deletions(-) diff --git a/contracts/protocol/facets/ExchangeHandlerFacet.sol b/contracts/protocol/facets/ExchangeHandlerFacet.sol index a85e3a31f..3448625bd 100644 --- a/contracts/protocol/facets/ExchangeHandlerFacet.sol +++ b/contracts/protocol/facets/ExchangeHandlerFacet.sol @@ -16,7 +16,7 @@ import { IERC721 } from "@openzeppelin/contracts/token/ERC721/IERC721.sol"; import { IERC1155 } from "@openzeppelin/contracts/token/ERC1155/IERC1155.sol"; import { Address } from "@openzeppelin/contracts/utils/Address.sol"; -// import "hardhat/console.sol"; +import "hardhat/console.sol"; /** * @title ExchangeHandlerFacet @@ -600,7 +600,7 @@ contract ExchangeHandlerFacet is IBosonExchangeHandler, BuyerBase, DisputeBase { // Notify watchers of state change emit VoucherRedeemed(offerId, _exchangeId, msgSender()); - // console.log("final event", gasleft()); // add a bit more to toggle reentrancy + console.log("TwinGasEstimate5,0,%s", gasleft()); } /** @@ -834,12 +834,9 @@ contract ExchangeHandlerFacet is IBosonExchangeHandler, BuyerBase, DisputeBase { // Oveflow happens for twinCount ~ 9.6x10^71, which is impossible to achieve uint256 reservedGas = twinCount * SINGLE_TWIN_RESERVED_GAS + MINIMAL_RESIDUAL_GAS; - console.log("before loop", gasleft()); - // Visit the twins - for (uint256 i = 0; i < twinCount; ) { - console.log("loop start", i, gasleft()); - + for (uint256 i = 0; i < twinCount;) { + console.log("TwinGasEstimate1,%s,%s", i, gasleft()); // Get the twin (, Twin storage twinS) = fetchTwin(twinIds[i]); @@ -903,17 +900,15 @@ contract ExchangeHandlerFacet is IBosonExchangeHandler, BuyerBase, DisputeBase { // If not, skip the call and mark the transfer as failed twinM.tokenAddress = twinS.tokenAddress; - console.log("loop constant cost #1", i, gasleft()); - uint256 gasLeft = gasleft(); + console.log("TwinGasEstimate2,%s,%s", i, gasleft()); if (gasLeft > reservedGas && twinM.tokenAddress.isContract()) { bytes memory result; (success, result) = twinM.tokenAddress.call{ gas: gasLeft - reservedGas }(data); success = success && (result.length == 0 || abi.decode(result, (bool))); } - - console.log("loop variable cost", i, gasleft()); + console.log("TwinGasEstimate3,%s,%s", i, gasleft()); } twinM.id = twinS.id; @@ -955,7 +950,7 @@ contract ExchangeHandlerFacet is IBosonExchangeHandler, BuyerBase, DisputeBase { unchecked { i++; } - console.log("loop constant cost #2", i, gasleft()); + console.log("TwinGasEstimate4,%s,%s", i, gasleft()); } } @@ -963,7 +958,6 @@ contract ExchangeHandlerFacet is IBosonExchangeHandler, BuyerBase, DisputeBase { if (transferFailed) { raiseDisputeInternal(_exchange, _voucher, sellerId); } - console.log("redisual cost", gasleft()); } /** diff --git a/scripts/util/estimate-twin-transfer-limits.js b/scripts/util/estimate-twin-transfer-limits.js index 73cebdcbb..f7130650e 100644 --- a/scripts/util/estimate-twin-transfer-limits.js +++ b/scripts/util/estimate-twin-transfer-limits.js @@ -23,50 +23,50 @@ This script does the following: */ async function estimateTwinTransferLimits() { // 1. Preprocess the `twinTransfer` to add the `gasLeft()` markers - let startFound = false; - let invokeFound = false; - const ExchangeHandlerPath = process.cwd() + "/contracts/protocol/facets/ExchangeHandlerFacet.sol"; - hre.config.preprocess = { - eachLine: () => ({ - transform: (line, { absolutePath }) => { - // Filter paths here, since preprocessor's options "files" doesn't work - if (absolutePath !== ExchangeHandlerPath) { - return line; - } + // let startFound = false; + // let invokeFound = false; + // const ExchangeHandlerPath = process.cwd() + "/contracts/protocol/facets/ExchangeHandlerFacet.sol"; + // hre.config.preprocess = { + // eachLine: () => ({ + // transform: (line, { absolutePath }) => { + // // Filter paths here, since preprocessor's options "files" doesn't work + // if (absolutePath !== ExchangeHandlerPath) { + // return line; + // } - // 1. add console.sol import - if (line.includes("pragma solidity")) { - line = line + "import 'hardhat/console.sol';"; - console.log("import path"); - console.log(absolutePath); - } + // // 1. add console.sol import + // if (line.includes("pragma solidity")) { + // line = line + "import 'hardhat/console.sol';"; + // console.log("import path"); + // console.log(absolutePath); + // } - if (line.includes("transferTwins")) { - if (line.includes("function")) { - startFound = true; - } else { - invokeFound = true; - } - } + // if (line.includes("transferTwins")) { + // if (line.includes("function")) { + // startFound = true; + // } else { + // invokeFound = true; + // } + // } - if (startFound && line.includes("{")) { - line = line + 'console.log("start", gasleft());'; - startFound = false; - } else if (invokeFound && line.includes("}")) { - line = 'console.log("final event", gasleft());' + line; - invokeFound = false; - } - return line; - }, - }), - }; + // if (startFound && line.includes("{")) { + // line = line + 'console.log("start", gasleft());'; + // startFound = false; + // } else if (invokeFound && line.includes("}")) { + // line = 'console.log("final event", gasleft());' + line; + // invokeFound = false; + // } + // return line; + // }, + // }), + // }; // 2. Run tests that cover the bundles and capture the output hre.config.mocha = { - timeout: 100000, + ...hre.config.mocha, testFiles: ["./test/protocol/ExchangeHandlerTest.js"], - // grep: "redeemVoucher\\(\\) with bundle", - grep: "uld not decrease twin supplyAvailable if supply is unlimited", + grep: "redeemVoucher\\(\\) with bundle", + // grep: "uld not decrease twin supplyAvailable if supply is unlimited", reporter: "min", }; hre.config.gasReporter.enabled = false; @@ -74,8 +74,70 @@ async function estimateTwinTransferLimits() { log("Running tests..."); await hre.run("test"); - log("Tests complete"); - log(contractLogs); + // 3. Analyze the output + log("Tests complete. Analyzing logs..."); + const [SINGLE_TWIN_RESERVED_GAS, MINIMAL_RESIDUAL_GAS] = analyzeLogs(contractLogs); + + log("SINGLE_TWIN_RESERVED_GAS", SINGLE_TWIN_RESERVED_GAS.toString()); + log("MINIMAL_RESIDUAL_GAS", MINIMAL_RESIDUAL_GAS.toString()); +} + +/** +Analyzes the log output: +1. Parse the logs +2. Calculate the deltas between the logs +3. Find the biggest delta for each measure point +4. Calculate the SINGLE_TWIN_RESERVED_GAS and MINIMAL_RESIDUAL_GAS +*/ +function analyzeLogs(logs) { + // Parse the logs + let deltas = [0n, 0n, 0n, 0n]; // Deltas between the logs + let runningIndex = 0; + let measurePoint = 1; + let previousGasLeft = 0n; + for (const log of logs) { + if (log) { + const logSplit = log.split(","); + if (!logSplit[0].startsWith("TwinGasEstimate")) { + continue; + } + + let gasLeft = BigInt(logSplit[2]); + + if (measurePoint == 5) { + let currentIndex = parseInt(logSplit[1]); + if (currentIndex === runningIndex + 1) { + // in the case of multiple twin transfers, we return to the first measure point + measurePoint = 1; + } + } + + if (measurePoint === 1) { + runningIndex = parseInt(logSplit[1]); + // no delta calculation + } else { + let delta = previousGasLeft - gasLeft; + + // we search for the biggest delta + if (delta > deltas[measurePoint - 2]) { + deltas[measurePoint - 2] = delta; + } + } + + previousGasLeft = gasLeft; + + if (measurePoint == 5) { + measurePoint = 1; + } else { + measurePoint++; + } + } + } + + const SINGLE_TWIN_RESERVED_GAS = deltas[0] + deltas[2]; // deltas[1] represents the external call + const MINIMAL_RESIDUAL_GAS = deltas[2] + deltas[3]; + + return [SINGLE_TWIN_RESERVED_GAS, MINIMAL_RESIDUAL_GAS]; } estimateTwinTransferLimits(); From 4fb7f891b86835df28e6427ecd337e6f2117ac5e Mon Sep 17 00:00:00 2001 From: zajck Date: Thu, 10 Aug 2023 12:06:58 +0200 Subject: [PATCH 4/9] add measure points --- contracts/domain/BosonConstants.sol | 4 +- .../protocol/facets/ExchangeHandlerFacet.sol | 8 - scripts/util/estimate-twin-transfer-limits.js | 147 +++++++++++++----- 3 files changed, 108 insertions(+), 51 deletions(-) diff --git a/contracts/domain/BosonConstants.sol b/contracts/domain/BosonConstants.sol index 7e4ca2390..10b78759d 100644 --- a/contracts/domain/BosonConstants.sol +++ b/contracts/domain/BosonConstants.sol @@ -122,8 +122,8 @@ string constant EXCHANGE_ALREADY_EXISTS = "Exchange already exists"; string constant INVALID_RANGE_LENGTH = "Range length is too large or zero"; // Revert Reasons: Twin related -uint256 constant SINGLE_TWIN_RESERVED_GAS = 140000; -uint256 constant MINIMAL_RESIDUAL_GAS = 180000; +uint256 constant SINGLE_TWIN_RESERVED_GAS = 150000; +uint256 constant MINIMAL_RESIDUAL_GAS = 210000; string constant NO_SUCH_TWIN = "No such twin"; string constant NO_TRANSFER_APPROVED = "No transfer approved"; string constant TWIN_TRANSFER_FAILED = "Twin could not be transferred"; diff --git a/contracts/protocol/facets/ExchangeHandlerFacet.sol b/contracts/protocol/facets/ExchangeHandlerFacet.sol index 3448625bd..18ed8a488 100644 --- a/contracts/protocol/facets/ExchangeHandlerFacet.sol +++ b/contracts/protocol/facets/ExchangeHandlerFacet.sol @@ -16,8 +16,6 @@ import { IERC721 } from "@openzeppelin/contracts/token/ERC721/IERC721.sol"; import { IERC1155 } from "@openzeppelin/contracts/token/ERC1155/IERC1155.sol"; import { Address } from "@openzeppelin/contracts/utils/Address.sol"; -import "hardhat/console.sol"; - /** * @title ExchangeHandlerFacet * @@ -599,8 +597,6 @@ contract ExchangeHandlerFacet is IBosonExchangeHandler, BuyerBase, DisputeBase { // Notify watchers of state change emit VoucherRedeemed(offerId, _exchangeId, msgSender()); - - console.log("TwinGasEstimate5,0,%s", gasleft()); } /** @@ -836,7 +832,6 @@ contract ExchangeHandlerFacet is IBosonExchangeHandler, BuyerBase, DisputeBase { // Visit the twins for (uint256 i = 0; i < twinCount;) { - console.log("TwinGasEstimate1,%s,%s", i, gasleft()); // Get the twin (, Twin storage twinS) = fetchTwin(twinIds[i]); @@ -901,14 +896,12 @@ contract ExchangeHandlerFacet is IBosonExchangeHandler, BuyerBase, DisputeBase { twinM.tokenAddress = twinS.tokenAddress; uint256 gasLeft = gasleft(); - console.log("TwinGasEstimate2,%s,%s", i, gasleft()); if (gasLeft > reservedGas && twinM.tokenAddress.isContract()) { bytes memory result; (success, result) = twinM.tokenAddress.call{ gas: gasLeft - reservedGas }(data); success = success && (result.length == 0 || abi.decode(result, (bool))); } - console.log("TwinGasEstimate3,%s,%s", i, gasleft()); } twinM.id = twinS.id; @@ -950,7 +943,6 @@ contract ExchangeHandlerFacet is IBosonExchangeHandler, BuyerBase, DisputeBase { unchecked { i++; } - console.log("TwinGasEstimate4,%s,%s", i, gasleft()); } } diff --git a/scripts/util/estimate-twin-transfer-limits.js b/scripts/util/estimate-twin-transfer-limits.js index f7130650e..37f1f4740 100644 --- a/scripts/util/estimate-twin-transfer-limits.js +++ b/scripts/util/estimate-twin-transfer-limits.js @@ -11,55 +11,119 @@ const hre = require("hardhat"); /** Estimates the values for SINGLE_TWIN_RESERVED_GAS, MINIMAL_RESIDUAL_GAS -Use with caution if `transferTwins` or other methods that call it changed a lot. +Use with caution if `transferTwins` or other methods that call it changed a lot. Please, refer to docs/twin-transfer-limits.md for more information. This script does the following: -1. Preprocesses the `transferTwins` method to add the `gasLeft()` markers - If it cannot reliably position the markers, it will throw an error +1. Preprocesses the `transferTwins` method to add the `gasLeft()` measure points + If it cannot reliably position the measure points, it will throw an error 2 Runs the tests that cover the `transferTwins` method 3. Captures the console.log output 4. Analyzes the output and estimates SINGLE_TWIN_RESERVED_GAS and MINIMAL_RESIDUAL_GAS */ async function estimateTwinTransferLimits() { - // 1. Preprocess the `twinTransfer` to add the `gasLeft()` markers - // let startFound = false; - // let invokeFound = false; - // const ExchangeHandlerPath = process.cwd() + "/contracts/protocol/facets/ExchangeHandlerFacet.sol"; - // hre.config.preprocess = { - // eachLine: () => ({ - // transform: (line, { absolutePath }) => { - // // Filter paths here, since preprocessor's options "files" doesn't work - // if (absolutePath !== ExchangeHandlerPath) { - // return line; - // } - - // // 1. add console.sol import - // if (line.includes("pragma solidity")) { - // line = line + "import 'hardhat/console.sol';"; - // console.log("import path"); - // console.log(absolutePath); - // } - - // if (line.includes("transferTwins")) { - // if (line.includes("function")) { - // startFound = true; - // } else { - // invokeFound = true; - // } - // } - - // if (startFound && line.includes("{")) { - // line = line + 'console.log("start", gasleft());'; - // startFound = false; - // } else if (invokeFound && line.includes("}")) { - // line = 'console.log("final event", gasleft());' + line; - // invokeFound = false; - // } - // return line; - // }, - // }), - // }; + // 1. Preprocess the `twinTransfer` to add the `gasLeft()` measure points + let invokeFound = false; + let loopFound = false; + let externalCallFound = false; + let paranthesisCount = { + invoke: 0, + loop: 0, + externalCall: 0, + }; + let previousMeasurePoint = ""; + const ExchangeHandlerPath = process.cwd() + "/contracts/protocol/facets/ExchangeHandlerFacet.sol"; + hre.config.preprocess = { + eachLine: () => ({ + transform: (line, { absolutePath }) => { + // Filter paths here, since preprocessor's options "files" doesn't work + if (absolutePath !== ExchangeHandlerPath) { + return line; + } + + // 1. add console.sol import + if (line.includes("pragma solidity")) { + return line + "import 'hardhat/console.sol';"; + } + + // Gas left measure points + // 1. At the beginning of the transfer twin loop + if (line.includes("for (uint256 i = 0; i < twinCount")) { + if (previousMeasurePoint !== "invoke") { + throw new Error("Could not find the invoke measure point"); + } + previousMeasurePoint = "loopStart"; + loopFound = true; + return line + 'console.log("TwinGasEstimate1,%s,%s", i, gasleft());'; + } + + // 2. Before the external call + if (line.includes("uint256 gasLeft = gasleft();")) { + if (previousMeasurePoint !== "loopStart") { + throw new Error("Could not find the loopStart measure point"); + } + previousMeasurePoint = "beforeExternalCall"; + externalCallFound = true; + return line + 'console.log("TwinGasEstimate2,%s,%s", i, gasleft());'; + } + + // 3. After the external call + if (externalCallFound) { + if (line.includes("{")) { + paranthesisCount.externalCall++; + } + if (line.includes("}")) { + if (--paranthesisCount.externalCall == 0) { + if (previousMeasurePoint !== "beforeExternalCall") { + throw new Error("Could not find the beforeExternalCall measure point"); + } + externalCallFound = false; + previousMeasurePoint = "afterExternalCall"; + return line + 'console.log("TwinGasEstimate3,%s,%s", i, gasleft());'; + } + } + } + + // 4. At the end of the transfer twin loop + if (loopFound) { + if (line.includes("{")) { + paranthesisCount.loop++; + } + if (line.includes("}")) { + if (--paranthesisCount.loop == 0) { + if (previousMeasurePoint !== "afterExternalCall") { + throw new Error("Could not find the afterExternalCall measure point"); + } + loopFound = false; + previousMeasurePoint = "loopEnd"; + return 'console.log("TwinGasEstimate4,%s,%s", i, gasleft());' + line; + } + } + } + + // 5. At the end of the function that invokes transferTwins + if (line.includes("transferTwins") && !line.includes("function")) { + invokeFound = true; + paranthesisCount.invoke++; + } + + if (invokeFound) { + if (line.includes("{")) { + paranthesisCount.invoke++; + } + if (line.includes("}")) { + if (--paranthesisCount.invoke == 0) { + invokeFound = false; + previousMeasurePoint = "invoke"; + return 'console.log("TwinGasEstimate5,0,%s", gasleft());' + line; + } + } + } + + return line; + }, + }), + }; // 2. Run tests that cover the bundles and capture the output hre.config.mocha = { @@ -74,6 +138,7 @@ async function estimateTwinTransferLimits() { log("Running tests..."); await hre.run("test"); + // log(contractLogs); // 3. Analyze the output log("Tests complete. Analyzing logs..."); const [SINGLE_TWIN_RESERVED_GAS, MINIMAL_RESIDUAL_GAS] = analyzeLogs(contractLogs); From 718d8121943713edd1f89b9d9a86df50ff1c7a01 Mon Sep 17 00:00:00 2001 From: zajck Date: Thu, 10 Aug 2023 14:56:36 +0200 Subject: [PATCH 5/9] Documentation --- .../protocol/facets/ExchangeHandlerFacet.sol | 2 +- docs/tasks.md | 11 ++- docs/twin-transfer-limits.md | 95 +++++++++++++++++++ scripts/util/estimate-twin-transfer-limits.js | 8 +- 4 files changed, 110 insertions(+), 6 deletions(-) create mode 100644 docs/twin-transfer-limits.md diff --git a/contracts/protocol/facets/ExchangeHandlerFacet.sol b/contracts/protocol/facets/ExchangeHandlerFacet.sol index 18ed8a488..9dddc1df1 100644 --- a/contracts/protocol/facets/ExchangeHandlerFacet.sol +++ b/contracts/protocol/facets/ExchangeHandlerFacet.sol @@ -828,7 +828,7 @@ contract ExchangeHandlerFacet is IBosonExchangeHandler, BuyerBase, DisputeBase { // Handle overall outcome: up to 200,000 gas // Next line would overflow if twinCount > (type(uint256).max - MINIMAL_RESIDUAL_GAS)/SINGLE_TWIN_RESERVED_GAS // Oveflow happens for twinCount ~ 9.6x10^71, which is impossible to achieve - uint256 reservedGas = twinCount * SINGLE_TWIN_RESERVED_GAS + MINIMAL_RESIDUAL_GAS; + uint256 reservedGas = (twinCount - 1) * SINGLE_TWIN_RESERVED_GAS + MINIMAL_RESIDUAL_GAS; // Visit the twins for (uint256 i = 0; i < twinCount;) { diff --git a/docs/tasks.md b/docs/tasks.md index 8b7932b1e..b6ddaa6af 100644 --- a/docs/tasks.md +++ b/docs/tasks.md @@ -264,7 +264,7 @@ npx hardhat detect-changed-contracts v2.1.0 branch-1 // get changes a tag and a Run unit tests and generates chunks of tests with approximately the same execution time in order to run them in parallel on Github Actions. This script must be run wherever we add new unit test files. -Run script with +Run the script with `npx hardhat split-unit-tests-into-chunks ` Parameters: @@ -272,3 +272,12 @@ Parameters: - chunks [required] - Number of chunks to divide the tests into Example: `npx hardhat split-unit-tests-into-chunks 4` + +### Estimate twin transfer limits + +Estimates the values `SINGLE_TWIN_RESERVED_GAS` and `MINIMAL_RESIDUAL_GAS` used during twins transfers. +More information about the procedure is available on a dedicated page: [Estimating the twin transfer limits](twin-transfer-limits.md). + +Run the script with + +`node ./scripts/util/estimate-twin-transfer-limits.js` diff --git a/docs/twin-transfer-limits.md b/docs/twin-transfer-limits.md new file mode 100644 index 000000000..49dbfdf2b --- /dev/null +++ b/docs/twin-transfer-limits.md @@ -0,0 +1,95 @@ +[![banner](images/banner.png)](https://bosonprotocol.io) + +

Boson Protocol V2

+ +### [Intro](../README.md) | [Audits](audits.md) | [Setup](setup.md) | [Tasks](tasks.md) | [Architecture](architecture.md) | [Domain Model](domain.md) | [State Machines](state-machines.md) | [Sequences](sequences.md) + +# Estimating the twin transfer limits + +## Introduction + +An offer can be part of a bundle that contains twins, i.e. tokens (ERC20, ERC721 or ERC1155) which are transferred to the buyer at redemption time. Protocol does not hold the twins in custody, but just tries to transfer them when the buyer redeems the voucher. If one or more twins cannot be transferred to the buyer, the voucher is still redeemed, but the exchange gets automatically disputed. + +The critical part of this process is that twin transfers are calls to external contracts. If they consume too much gas, Boson protocol may not have enough of it to finalize the redemption. Effectively that means that the buyer cannot redeem the voucher, so they can either cancel it or let it expire. In both cases, the seller collects the cancellation penalty without the need to deliver the item. This is a sort of DoS attack that the seller could execute. + +To prevent it, Boson protocol v2.3.0 introduces the gas limit for external calls that facilitate the twin transfers. Even if the external call consumes all provided gas, Boson protocol would still be able to finalize the redemption. + +This document outlines how the limit is estimated and provides the instruction to redo the estimation in the future if the code that transfers the twin changes. + +## Gas consumption during the voucher redemption + +The `redeemVoucher` method can be split into the following parts: +1. operations between twin transfer loop [O1] +2. operations from the beginning of the loop till the external call [O2] +3. an external call that transfers the twin [E1] +4. operations after the external call till the end of the loop [O3] +5. operations after the loop [O4] + +## Minimal gas requirements + +We can describe the minimal gas requirements with the following statement: +> After the external call, contract must have enough gas to finalize the ongoing twin transfer, any other twin transfers and all the operations after the last twin is transferred. + +Let's call this quantity `reservedGas`. Before we make an external call, we make sure that the available gas is higher than reserved gas and forward `availableGas - reservedGas` to the external contract. This ensures that no matter how much gas the external contract consumes, the protocol will still have enough gas after the transfer. + +What is the reserved gas in terms of parts, denoted in the previous section? +1. O1 are not important, since those operations already happened before the transfer and will not be repeated. +2. O2 are important if there are other twins that need to be transferred +3. E1 is not important. This is an external call that can in theory consumes an arbitrary amount of gas. If we wanted to estimate the minimum, where *all* external calls succeed, that is no less than the total block gas limit, which makes no sense as the lower boundary. But even more important than that is, even if don't forward enough gas or if the external call reverts for any reason, the protocol catches it and raises a dispute. But the redemption does not fail, therefore it's not important for the reserved gas. +4. O3 always happen after the transfer, so we must include them +5. O4 always happen after the transfer, so we must include them + +If `n'` is the number of twins that are left to be transferred, and `C(o)` is the maximal cost of a set of operations `o`, we can express the `reservedGas` as +``` +reservedGas = C(O4) + C(O3) + n'*(C(O2)+C(O3)) +``` + +If `N` is the total number of twins to transfer, the `reservedGas` before the first external call should be +``` +reservedGas = C(O4) + C(O3) + (N-1)*(C(O2)+C(O3)) +``` +After each external call, the reserved gas can be reduced for `C(O2)+C(O3)`. + +The equations above suggest that we introduce 2 new protocol constants. +- `SINGLE_TWIN_RESERVED_GAS = C(O2)+C(O3)` +- `MINIMAL_RESIDUAL_GAS = C(O4) + C(O3)` + +## Estimating the constants + +Solidity has a special function `gasLeft()` which can be invoked at any point of execution to get the remaining gas. To get the estimates, one then only needs to call `gasLeft()` before and after a set of operations happens and subtract the second value from the first value. Since the end of one operations set presents the beginning of the next operations set, we need to put 5 measurement points in the code: +- Before O2 +- After O2 +- Before O3 +- After O3 = Before O4 +- After O4 + +To communicate the values outside the chain, one could use hardhat's `console.log` function which outputs the value in the terminal. Since calling this function adds something to the gas consumption, the estimates are not exact, but that is not problematic, since it's sensible to add some buffer on top of the estimates anyway. + +Since different execution paths consume different amounts of gas it's advised to get the estimates in different scenarios (i.e. transfer of a single twin, multiple twins, failing twins, different types of twins etc.). + +Once we get the estimations for individual operations sets (`C(O2)`, `C(O3)` and `C(O4)`) in different scenarios, it's important to take the maximum values (and not their averages). Then we can calculate the values for `SINGLE_TWIN_RESERVED_GAS` and `MINIMAL_RESIDUAL_GAS` as defined in the previous section. To get a final value, we add a sensible buffer and round it to a nice-looking value. + +### Estimation with script + +This repository contains a script that automatically estimates the limits. It does the following: +1. Preprocesses the `transferTwins` method to add the `gasLeft()` measurement points. If it cannot reliably position the measurement points, it throws an error. +2. Runs the unit tests that cover the `transferTwins` method. This way we capture most of the sensible scenarios. +3. Captures the console.log output +4. Analyzes the output and estimates `SINGLE_TWIN_RESERVED_GAS` and `MINIMAL_RESIDUAL_GAS` + +The preprocessing is the most volatile part. If `transferTwins` changes significantly, the script might be unable to reliably place the measurement points. In that case, one should either update the script to work with the new solidity code, or place the measurement points manually and disable the preprocessing. + +To run a script, run the following command from the root folder of the project. It will print out the values for `SINGLE_TWIN_RESERVED_GAS` and `MINIMAL_RESIDUAL_GAS`. + +``` +node ./scripts/util/estimate-twin-transfer-limits.js +``` + +## Results + +Estimation at commit f2a7993 + +| | Estimate | Used value | +|-|-|-| +| `SINGLE_TWIN_RESERVED_GAS` | `147,001` | `150,000` | +| `MINIMAL_RESIDUAL_GAS` | `207,084` | `210,000` | \ No newline at end of file diff --git a/scripts/util/estimate-twin-transfer-limits.js b/scripts/util/estimate-twin-transfer-limits.js index 37f1f4740..651a82a7a 100644 --- a/scripts/util/estimate-twin-transfer-limits.js +++ b/scripts/util/estimate-twin-transfer-limits.js @@ -14,15 +14,15 @@ Estimates the values for SINGLE_TWIN_RESERVED_GAS, MINIMAL_RESIDUAL_GAS Use with caution if `transferTwins` or other methods that call it changed a lot. Please, refer to docs/twin-transfer-limits.md for more information. This script does the following: -1. Preprocesses the `transferTwins` method to add the `gasLeft()` measure points - If it cannot reliably position the measure points, it will throw an error -2 Runs the tests that cover the `transferTwins` method +1. Preprocesses the `transferTwins` method to add the `gasLeft()` measurement points + If it cannot reliably position the measurement points, it throws an error +2. Runs the unit tests that cover the `transferTwins` method 3. Captures the console.log output 4. Analyzes the output and estimates SINGLE_TWIN_RESERVED_GAS and MINIMAL_RESIDUAL_GAS */ async function estimateTwinTransferLimits() { - // 1. Preprocess the `twinTransfer` to add the `gasLeft()` measure points + // 1. Preprocess the `twinTransfer` to add the `gasLeft()` measurement points let invokeFound = false; let loopFound = false; let externalCallFound = false; From b1e839b9b819f8400d36c09344b6341cc02c94c1 Mon Sep 17 00:00:00 2001 From: zajck Date: Mon, 28 Aug 2023 12:57:16 +0200 Subject: [PATCH 6/9] update limits --- contracts/domain/BosonConstants.sol | 4 ++-- docs/twin-transfer-limits.md | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/contracts/domain/BosonConstants.sol b/contracts/domain/BosonConstants.sol index 10b78759d..d7b76dd93 100644 --- a/contracts/domain/BosonConstants.sol +++ b/contracts/domain/BosonConstants.sol @@ -122,8 +122,8 @@ string constant EXCHANGE_ALREADY_EXISTS = "Exchange already exists"; string constant INVALID_RANGE_LENGTH = "Range length is too large or zero"; // Revert Reasons: Twin related -uint256 constant SINGLE_TWIN_RESERVED_GAS = 150000; -uint256 constant MINIMAL_RESIDUAL_GAS = 210000; +uint256 constant SINGLE_TWIN_RESERVED_GAS = 160000; +uint256 constant MINIMAL_RESIDUAL_GAS = 230000; string constant NO_SUCH_TWIN = "No such twin"; string constant NO_TRANSFER_APPROVED = "No transfer approved"; string constant TWIN_TRANSFER_FAILED = "Twin could not be transferred"; diff --git a/docs/twin-transfer-limits.md b/docs/twin-transfer-limits.md index 49dbfdf2b..4f6671efa 100644 --- a/docs/twin-transfer-limits.md +++ b/docs/twin-transfer-limits.md @@ -67,7 +67,7 @@ To communicate the values outside the chain, one could use hardhat's `console.lo Since different execution paths consume different amounts of gas it's advised to get the estimates in different scenarios (i.e. transfer of a single twin, multiple twins, failing twins, different types of twins etc.). -Once we get the estimations for individual operations sets (`C(O2)`, `C(O3)` and `C(O4)`) in different scenarios, it's important to take the maximum values (and not their averages). Then we can calculate the values for `SINGLE_TWIN_RESERVED_GAS` and `MINIMAL_RESIDUAL_GAS` as defined in the previous section. To get a final value, we add a sensible buffer and round it to a nice-looking value. +Once we get the estimations for individual operations sets (`C(O2)`, `C(O3)` and `C(O4)`) in different scenarios, it's important to take the maximum values (and not their averages). Then we can calculate the values for `SINGLE_TWIN_RESERVED_GAS` and `MINIMAL_RESIDUAL_GAS` as defined in the previous section. To get a final value, we add a sensible buffer (e.g. 10%) and round it to a nice-looking value. ### Estimation with script @@ -91,5 +91,5 @@ Estimation at commit f2a7993 | | Estimate | Used value | |-|-|-| -| `SINGLE_TWIN_RESERVED_GAS` | `147,001` | `150,000` | -| `MINIMAL_RESIDUAL_GAS` | `207,084` | `210,000` | \ No newline at end of file +| `SINGLE_TWIN_RESERVED_GAS` | `147,001` | `160,000` | +| `MINIMAL_RESIDUAL_GAS` | `207,084` | `230,000` | \ No newline at end of file From 158074247298bbe8d1d7321542a914fd949c24b7 Mon Sep 17 00:00:00 2001 From: zajck Date: Mon, 28 Aug 2023 13:22:23 +0200 Subject: [PATCH 7/9] lint --- contracts/protocol/facets/ExchangeHandlerFacet.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/protocol/facets/ExchangeHandlerFacet.sol b/contracts/protocol/facets/ExchangeHandlerFacet.sol index 9dddc1df1..ff00469cc 100644 --- a/contracts/protocol/facets/ExchangeHandlerFacet.sol +++ b/contracts/protocol/facets/ExchangeHandlerFacet.sol @@ -831,7 +831,7 @@ contract ExchangeHandlerFacet is IBosonExchangeHandler, BuyerBase, DisputeBase { uint256 reservedGas = (twinCount - 1) * SINGLE_TWIN_RESERVED_GAS + MINIMAL_RESIDUAL_GAS; // Visit the twins - for (uint256 i = 0; i < twinCount;) { + for (uint256 i = 0; i < twinCount; ) { // Get the twin (, Twin storage twinS) = fetchTwin(twinIds[i]); From 6986c49451c2811fee79e6a3061d5a04c7fa49df Mon Sep 17 00:00:00 2001 From: zajck Date: Mon, 28 Aug 2023 15:36:01 +0200 Subject: [PATCH 8/9] rerun estimation --- docs/twin-transfer-limits.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/twin-transfer-limits.md b/docs/twin-transfer-limits.md index 4f6671efa..b73df1c01 100644 --- a/docs/twin-transfer-limits.md +++ b/docs/twin-transfer-limits.md @@ -87,9 +87,9 @@ node ./scripts/util/estimate-twin-transfer-limits.js ## Results -Estimation at commit f2a7993 +Estimation at commit 73e00ca0 | | Estimate | Used value | |-|-|-| -| `SINGLE_TWIN_RESERVED_GAS` | `147,001` | `160,000` | -| `MINIMAL_RESIDUAL_GAS` | `207,084` | `230,000` | \ No newline at end of file +| `SINGLE_TWIN_RESERVED_GAS` | `146,991` | `160,000` | +| `MINIMAL_RESIDUAL_GAS` | `207,000` | `230,000` | \ No newline at end of file From cc586dea09cddafbe993e8e3ed499325d62ce8a0 Mon Sep 17 00:00:00 2001 From: Klemen <64400885+zajck@users.noreply.github.com> Date: Mon, 28 Aug 2023 16:10:34 +0200 Subject: [PATCH 9/9] Apply suggestions from code review Co-authored-by: albertfolch-redeemeum <102516373+albertfolch-redeemeum@users.noreply.github.com> --- scripts/util/estimate-twin-transfer-limits.js | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/scripts/util/estimate-twin-transfer-limits.js b/scripts/util/estimate-twin-transfer-limits.js index 651a82a7a..55c62bbf5 100644 --- a/scripts/util/estimate-twin-transfer-limits.js +++ b/scripts/util/estimate-twin-transfer-limits.js @@ -1,7 +1,7 @@ -var log = console.log; +const log = console.log; // Modify the console.log to capture the contract output -var contractLogs = []; +let contractLogs = []; console.log = function () { contractLogs.push(arguments[0]); }; @@ -130,7 +130,6 @@ async function estimateTwinTransferLimits() { ...hre.config.mocha, testFiles: ["./test/protocol/ExchangeHandlerTest.js"], grep: "redeemVoucher\\(\\) with bundle", - // grep: "uld not decrease twin supplyAvailable if supply is unlimited", reporter: "min", }; hre.config.gasReporter.enabled = false; @@ -138,7 +137,6 @@ async function estimateTwinTransferLimits() { log("Running tests..."); await hre.run("test"); - // log(contractLogs); // 3. Analyze the output log("Tests complete. Analyzing logs..."); const [SINGLE_TWIN_RESERVED_GAS, MINIMAL_RESIDUAL_GAS] = analyzeLogs(contractLogs);