Invalid gas check

[gabmontes]

Is there an existing issue for this?

• I have searched the existing issues

Current Behavior

As discussed eons ago in #2381, the gas check done in core-methods is still incorrect. See:

https://github.com/ChainSafe/web3.js/blob/a1c7d71973ec17f9287fbea8939e64a80e589fc6/packages/web3-core-method/src/index.js#L423

While gasProvided is an hex string, receipt.gasUsed is a number so the check will always fail.

The check was fixed in #3123 but later reverted in c3bfcac.

Expected Behavior

The comparison to be done apples-to-apples :)

Steps to Reproduce

The easiest is to just send a transaction, break with the debugger at the mentioned line and check the types.

Web3.js Version

1.5.3

Environment

• Operating System: macOS 11.6.1
• Browser: Chrome 96.0
• Geth: 1.10.8
• Hardhat: 2.6.4

Anything Else?

Wondering if there is any difference between Geth and other implementations, service providers, etc.

[github-actions]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions. If you believe this was a mistake, please comment.

[github-actions]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions. If you believe this was a mistake, please comment.

[github-actions]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions. If you believe this was a mistake, please comment.

[github-actions]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions. If you believe this was a mistake, please comment.

[spacesailor24]

Hey there, apologies it's taken us so long to take a look at this, we've been working on a rewrite of the library (4.x that's currently in alpha)

---

I'm not 100% sure how to handle this issue since the team has decided to only make necessary changes and security patches to 1.x to reduce the likelihood of us breaking user's code unintentionally while we focus on getting 4.x released. Changing the logic on how a transaction is deemed successful or not is a bit risky, and goes against how we've decided to maintain 1.x going forward. That being said, this is a bug so I hope to summarize all the facts in this comment so we can all come to a consensus on how to move forward:

The following check:

(!gasProvided || gasProvided !== receipt.gasUsed)

is part of a series of checks that get executed when a transaction receipt is received from the confirmation polling Web3 does after sending a transaction. gasProvided is what the user set as the gas for the transaction and it gets formatted as a hex string while receipt.gasUsed is formatted as a number. So, at the very least, we can address this issue by updating the above check to:

(!gasProvided || gasProvided !== utils.toHex(receipt.gasUsed))

However, this seems to be a naive approach as #3175 points out that gasProvided and receipt.gasUsed could be equal if the user provides the exact amount of gas a transaction needs

@gabmontes Suggested updating the check to:

(!gasProvided || gasProvided !== utils.numberToHex(receipt.gasUsed) || !isContractCall || (isContractCall && receipt.events))

but this doesn't address the "perfect gas" issue either

Keeping what @nivida mentioned in mind 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).

I think the following refactor is a bit better:

if (!receipt.outOfGas &&
    (
        (receipt.status === true || receipt.status === '0x1') ||
        (
            (receipt.status === null || typeof receipt.status === 'undefined') &&
            (!gasProvided || gasProvided !== utils.toHex(receipt.gasUsed))
        )
    )
) {}

but it still doesn't cover pre-Byzantimum transactions that have "perfect gas". We could add the additional checks @gabmontes mentioned for Contract calls and events: !isContractCall || (isContractCall && receipt.events), but I still don't think this covers all the bases

---

So, the decision to be made: Do we update the the checks how I've proposed (and hope we don't miss an edge case), does someone have a better idea (or see holes in my proposed logic), or do we keep the code as is since it doesn't seem to be affecting other users and @gabmontes seems to have a workaround in place for his use?

[mconnelly8]

Hello @gabmontes, I wanted to let you know that we will not be including this in 1.x, but will investigate further and look to implement in 4.x.

[spacesailor24]

In 4.x the only check from 1.x this explicitly checked for transaction receipts is if transactionReceipt.status === BigInt(0), there is no check for pre-Byzantimum transactions or if gasProvided !== receipt.gasUsed. After discussion, given that the likelihood of a web3.js user submitting transaction to a network that's pre-Byzantimum is slim, we've decided not to handle the edge case