Using gas value provided by estimateGas triggers error even though tx succeeds

[cgewecke]

Description

In 1.2.2, perfect gas throws an error. This problem was introduced in #3123. It looks like the modified logic is meant to handle pre-byzantium chain behavior where gasUsed === gasSent is a signal that the transaction was reverted. Post-byzantium, the status field on the receipt is a more definitive check. There are also gas refunds which may make these estimates less useful overall.

Perhaps there should be some community discussion about how to address this. Is there agreement that the estimates are imperfect and estimate + something should always be provided? If so, perhaps a docs clarification is all that is required.

Conversely, the change could be see as de-facto breaking and reverted on those grounds.

@gabmontes What is your view of this? Could you provide more background on what execution contexts this gas check is necessary for?

Actual behavior

var estimate = await instance
  .methods
  .setValue('1')
  .estimateGas();

var receipt = await instance
  .methods
  .setValue('1')
  .send({from: accounts[0], gas: estimate});


> Error: Transaction ran out of gas. Please provide more gas:
{
  "transactionHash": "0x5ac5cd5c927fb5fc379ef08ab76d901e04ff39d479c626574cfefe36ba88cbd0",
  "transactionIndex": 0,
  "blockHash": "0x9b7bd2b531e94862ba99ed69771bbe4098f87ca729e1209f9eacef30c6aa3857",
  "blockNumber": 2,
  "from": "0x27ca018a8ab7e7661a596077633e4d5026a571b1",
  "to": "0xe63685170cd157a1d4dad3e3dd65df9fd44ad6a9",
  "gasUsed": 41728,
  "cumulativeGasUsed": 41728,
  "contractAddress": null,
  "status": true,
  "logsBloom": "0x000..00",
  "v": "0x1c",
  "r": "0x23d539bb02df7fbbc99c4ee8901dd6149fd5c812176990fbba868bc2b5e0dfee",
  "s": "0x7aef125f4f79a66f8253733c306c333bec9fcbe38d1a2623192bf0c702d44674",
  "events": {}
}

Versions

• web3.js: 1.2.2
• nodejs:
• browser:
• ethereum node:

[nivida]

@cgewecke Thanks for opening this issue! The gas check got completely fixed in 2.x and does only slightly differ from the gas check we have in 1.x.

1.x does throw the Transaction ran out of gas error if the gasProvided and gasUsed value from the Method class on line 351 is equal and if the status property is set totrue. The correct and also implemented logic in 2.x is that it first checks if the status property is set to false and if the gasProvided and gasUsed property are equals. (2.x PR)

Btw.: I think to cover the gas validation logic would it require some smaller additional test cases.

[gabmontes]

@cgewecke @nivida actually #3123 fixes a comparison that had been broken for a long time. As a side-effect, it looks like it also revealed another issue with the gas-checking logic.

In order to handle both pre and post-byzantimum fork in a better way, additional checks have to be added. This is what we are currently using in our projects:

--- a/node_modules/web3-core-method/src/index.js
+++ b/node_modules/web3-core-method/src/index.js
@@ -203,6 +203,7 @@ Method.prototype._confirmTransaction = function(defer, result, payload) {
         lastBlock = null,
         receiptJSON = '',
         gasProvided = (_.isObject(payload.params[0]) && payload.params[0].gas) ? payload.params[0].gas : null,
+        isContractCall = _.isObject(payload.params[0]) && !!payload.params[0].data,
         isContractDeployment = _.isObject(payload.params[0]) &&
             payload.params[0].data &&
             payload.params[0].from &&
@@ -394,8 +395,8 @@ Method.prototype._confirmTransaction = function(defer, result, payload) {
                 .then(function(receipt) {
                     if (!isContractDeployment && !promiseResolved) {
                         if (!receipt.outOfGas &&
-                            (!gasProvided || gasProvided !== utils.numberToHex(receipt.gasUsed)) &&
-                            (receipt.status === true || receipt.status === '0x1' || typeof receipt.status === 'undefined')) {
+                            (!gasProvided || gasProvided !== utils.numberToHex(receipt.gasUsed) || !isContractCall || (isContractCall && receipt.events)) &&
+                            (receipt.status === true || receipt.status === '0x1' || receipt.status === null || typeof receipt.status === 'undefined')) {
                             defer.eventEmitter.emit('receipt', receipt);
                             defer.resolve(receipt);

I have not sent a PR with this because it needs to be tested thoroughly. In summary, if you have status as true or false, you are post-byzantinum and should use that. Otherwise, if it is null or undefined you are pre-byzantinum and can only try to infer if the transaction was successful or was reverted. In order to do that, we added more checks so even when the gas used is equal to the gas price, if it is a contract call and you have logs, you know the transaction was successful.

Is there a good test suite for this in 2.x? In that case we can port the tests back and work on fixing this logic properly.

[cgewecke]

Thanks @nivida @gabmontes!

[cgewecke]

Does anyone know what client or fork actually attaches this field to the receipt?

receipt.outOfGas

[nivida]

@cgewecke

receipt.outOfGas

This property doesn't exist officially only the ganache client does have it.

[nivida]

The correct and also implemented logic in 2.x is that it first checks if the status property is set to false and if the gasProvided and gasUsed property are equals.

This logic doesn't reject successfully mined transactions who exactly used the provided gas.
What the 2.x code is missing is returning of the contract error messages in a human-readable way but this already reported as an issue here.

The rules behind the status property are:

• If the status property is false and providedGas !== gasUsed then the transaction got reverted because of the logic implemented of the current contract (require, throw).
• If the status property is false and providedGas === gasUsed then the transaction got reverted by a non-contract-logic related problem (not enough gas).

[H34D]

Well, this was unexpected. It behaved differently in version v1.2.1. (https://github.com/oceanprotocol/squid-js/pull/331/files#diff-b9cfc7f2cdf78a7f4b91a753d10865a2L62)

I was not expecting such a change in a patch release.