Prefer receipt status when checking for OOG

[cgewecke]

Description

If receipt has a valid (e.g true or false) value, it cannot be OOG anymore. This is slightly different than what is suggested in #3175 by @nivida, and does not include the additional contract data checks suggested by @gabmontes. Pinging both of you for review...

RE:

• 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).

This PR includes the second case as a test which expects revert. It's possible for someone to use a perfect estimate for a method that calls require for example. I think this is closest the currently expected behavior since the gas check wasn't really being done before.

Fixes #3175

Type of change

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

Checklist:

• I have selected the correct base branch.
• I have performed a self-review of my own code.
• I have commented my code, particularly in hard-to-understand areas.
• I have made corresponding changes to the documentation.
• My changes generate no new warnings.
• Any dependent changes have been merged and published in downstream modules.
• I ran npm run test with success and extended the tests if necessary.
• I ran npm run build and tested the resulting file from dist folder in a browser.
• I have tested my code on the live network.

[coveralls]

Coverage increased (+0.01%) to 84.358% when pulling bafe3e9 on cgewecke:issue/3175 into 22df832 on ethereum:1.x.

[nivida]

Thanks for proposing this PR! I've added a comment with some questions about the extended conditions.

[nivida]

• What exactly are the thoughts behind hasStatus? (Because the status field does always exists in post Byzantium hard fork chains)
• The if conditions who check the status based on the hex value e.g.: receipt.status === '0x1' aren't required because the receipt.status property does get type casted to a boolean value type in the outputTransactionReceiptFormatter of the used getTransactionReceipt method.
  • If the status property is of type undefined or null is no type cast applied.
• Shouldn't isOOG be like this: isOOG = !receipt.status && gasProvided === utils.numberToHex(receipt.gasUsed); ?
• Shouldn't we add here the isOOG condition as well?
  • I would propose those conditions:

      if(receipt.status === true || receipt.status === null || typeof receipt.status === 'undefined' ) {
        // resolve with receipt
      } else {
        if (gasProvided === utils.numberToHex(receipt.gasUsed)) {
          // reject with out of gas
        } else {
          // reject with reverted by EVM
        }
      }

[cgewecke]

What exactly are the thoughts behind hasStatus

My thought is that the reason for the original correction in #3123 is to support non-byzantium chains where gasUsed === gasSent is (almost) logically equivalent to status === false. What is your view of this?

'0x1' aren't required

👍 Will remove

Shouldn't isOOG be like this: isOOG = !receipt.status...

That was my initial thought as well. But it's a problem when people combine estimateGas with tx's that actually revert. In that case receipt status is false and exact gas is consumed. See this added test. My theory is that preferring the status / ignoring the gas is closest to what the 1.2.1 behavior is.

If you look at what GabMontes suggests in #3175, there are other receipt fields they're looking at to disambiguate real OOG from revert-like transaction failure. I'm ambivalent about adding those without being able to add a test against an ETC client here.

[cgewecke]

Ah! I just realized the test has a copy-pasta mistake in it. Let me investigate this further...

[cgewecke]

I've removed the test for using perfect gas to fuel a tx which reverts because it's not possible to get an estimate in that case with the simple contracts here.

• ganache rejects estimateGas with: Error: Returned error: execution error: revert
• geth rejects estimateGas with: exceeds allowance ... or always failing transaction

Tbh this may always happen.

[cgewecke]

@nivida What is receipt.status === null? When does that happen?

[cgewecke]

Another quick note. If these changes need careful review and we'll need to take our time getting this right, maybe #3123 should be reverted as a stop-gap and a hot-fix of uncontroversial changes published early next week. At the moment this very basic, common tx throws an error:

web3.eth.sendTransaction({
  from: accounts[0],
  to: accounts[1],
  value: web3.utils.toWei('1', 'ether'),
  gas: 21000
})

And MEW has had to undo their attempt to upgrade from 1.2.1.

[nivida]

My thought is that the reason for the original correction in #3123 is to support non-byzantium chains where gasUsed === gasSent is (almost) logically equivalent to status === false. What is your view of this?

True that we should support non-byzantium chains as well.

Means this condition would be what we want to have?:

 if(receipt.status === true || ((typeof receipt.status === 'undefined' || receipt.status === null) && gasProvided !== utils.numberToHex(receipt.gasUsed))) {
    // resolve with receipt
  } else {
    if (gasProvided === utils.numberToHex(receipt.gasUsed)) {
      // reject with out of gas
    } else {
      // reject with reverted by EVM
    }
  }

What is receipt.status === null? When does that happen?

I've tried to find the corresponding node who is returning null but can't find it. Let us keep the null case until we know more about.

Another quick note. If these changes need careful review and we'll need to take our time getting this right, maybe #3123 should be reverted as a stop-gap and a hot-fix of uncontroversial changes published early next week. At the moment this very basic, common tx throws an error:

I think we will manage to merge and release this PR this week. Let us see what the state of the open PRs is on Thursday. If those still do take some days will I quickly set up a hotfix PR and release it on Thursday evening (CEST).

[cgewecke]

@nivida I see what you're saying and your approach is simpler / cleaner.

But what about all the other conditions like receipt.outOfGas and !gasProvided? I feel like I don't really understand how this complicated conditional actually works out in the wild.

[nivida]

But what about all the other conditions like receipt.outOfGas and !gasProvided? I feel like I don't really understand how this complicated conditional actually works out in the wild.

We can keep the outOfGas from ganache as additional proof in the condition if existing.

gasProvided = (_.isObject(payload.params[0]) && payload.params[0].gas) ? payload.params[0].gas : null

The gasProvided variable does only exists to have a secure way to get the anyways existing gas property on the transaction options object.

[cgewecke]

The gasProvided variable does only exists to have a secure way to get the anyways existing gas property on the transaction options object.

@nivida Why is !gasProvided currently expressed as a separate check?

Consider this case:

receipt status is false and exact gas is consumed.

How does your re-writing handle that?

[nivida]

Why is !gasProvided currently expressed as a separate check?

There is in my view of point no reason for it but we can keep it just to be sure we do not break anything.

How does your re-writing handle that?

In the else case the first if condition.

[cgewecke]

[How does your rewriting handle the case where] receipt status is false and exact gas is consumed.

In the else case the first if condition.

if(receipt.status === true || ((typeof receipt.status === 'undefined' || receipt.status === null) && gasProvided !== utils.numberToHex(receipt.gasUsed))) {
    // resolve with receipt
  } else {
    if (gasProvided === utils.numberToHex(receipt.gasUsed)) {
      // reject with out of gas
    } else {
      // reject with reverted by EVM
    }
  }

@nivida As I read this when status is false and gas usage is exact it will OOG instead of revert.

[nivida]

As I read this when status is false and gas usage is exact it will OOG instead of revert.

This would be the correct behavior as mentioned in issue #3175:

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).

Edit:
Yes, there is still a small possibility that the status can be false because of a EVM revert and the gas usage is providedGas === gasUsed. But it is currently the best we can achieve without really implementing the transaction reverting process as described in the related EIP.

[cgewecke]

@nivida Ah! Now I see what our disagreement is finally.

If the status property is false and providedGas === gasUsed then the transaction got reverted by a non-contract-logic related problem (not enough gas).

I don't think this is right. Consider the bug we are currently fixing where status is true and gas usage is exact. It should succeed. In the inverse case it should revert, not be OOG. It's not perfect but it's what's closest current behavior.

[nivida]

I don't think this is right. Consider the bug we are currently fixing where status is true and gas usage is exact. It should succeed. In the inverse case it should revert, not be OOG. It's not perfect but it's what's closest current behavior.

This does work because the parent IF condition will be true if the status field is true. Only if the status field is set to false will it check if the provided gas equals the gas used. For non-byzantium chains does it only use the gas values to determine the success or the reverted state of the given receipt.

[cgewecke]

@nivida To me it seems like the behavior for status should be symmetric. (I think it was until 1.2.2):

receipt.status === true --> success even when exact gas is used
receipt.status === false --> revert even when exact gas is used

Why do you feel changing the symmetry is better?

[nivida]

@cgewecke
Thanks for writing this down in pseudo-code. It's way easier to read and discuss.

The logic I have proposed:

Post-Byzantium:
receipt.status === true --> success even when exact gas is used
receipt.status === false --> OOG if exact gas usage otherwise EVM revert (can later be improved with checking of the existence of a revert msg)

Pre-Byzantium:
isPreByzantium = receipt.status === (undefined || null);
gasEquals = gasProvided === receipt.gasUsed;

(isPreByzantium && !gasEquals) === true --> sucess
(isPreByzantium && gasEquals) === true --> OOG

This logic would fix the OOG and revert detection for all post-Byzantium chains. To determine the success or failure of a transaction for pre-Byzantium chains can we use gasUsed and gasProvided. Sadly do we not have any other possibility to have better proof of the Tx status. What we could do to improve this situation would be to add a transaction confirmation property for example called rejectOnOOG which does default to true and can be used for pre-Byzantium chains developers to alter the behavior if gasProvided === gasUsed is true.

Edit:

Sadly do we not have any other possibility to have better proof of the Tx status

I mean we do not have any other possibility for pre-Byzantium chains and the post-Byzantium case can get improved more with implementing the revert reason msg detection and decoding.

[cgewecke]

@nivida Ok yes I think your analysis looks correct 💯

Trying to be quite cautious about changes here - tbh I'm completely paranoid about this because I think people have built their own error handling logic around Web3's existing behavior whether it's "correct/makes sense" or not.

A small thing that might make this +/- non-breaking is to keep the current failure case evaluation order but modify the messages so they're clearer about what happened.

Post-Byzantium Errors: 
gasEquals --> "After using all gas, Transaction has been reverted by the EVM:..."
gasNotEquals --> "Transaction has been reverted by the EVM:..."

Pre-Byzantium Errors:
(other checks? see gabMontes comment in 3175)
gasEquals --> "Transaction ran out of gas. Please provide more gas..."

It also looks like Parity supports pre-byzantium with --chain classic, so maybe it's possible to test this more carefully as part of #3098.

the revert reason msg detection and decoding.

We should talk about how to do this on 1.x, I'm going to open a design discussion issue about it.

[nivida]

@cgewecke I think we have now new possibilities to improve the error handling of a transaction because of the added handleRevert option. We should be able to handle the OOG error nicely as soon as the handleRevert option is enabled (If the status is false and no revert instruction can get detected is it definitely an OOG error).

[cgewecke]

@nivida Agree, I will close here and re-open this as an extension of the new revert behavior.