Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug]: incorrect gas estimation on BSC when gasPrice is not explicitly supplied by the website #20699

Closed
Snugface opened this issue Sep 2, 2023 · 14 comments
Assignees
Labels
Sev2-normal Normal severity; minor loss of service or inconvenience. stale issues and PRs marked as stale team-confirmations-planning (only for internal use within Confirmations team) type-bug

Comments

@Snugface
Copy link

Snugface commented Sep 2, 2023

Describe the bug

MetaMask incorrectly estimates gas settings on BSC when no gas price is explicitly supplied by the website.
Likely the bug is only specific to BSC, though I haven't tried reproducing it on other chains.

Steps to reproduce

  1. Initiate transaction with a contract on BSC using web3.js without explicitly supplying any gas settings, like so:
    contract.methods.function(args).send({ from: address });
  2. Review gas settings automatically picked by Metamask, and observe that they are falsely reported as "suggested by site", while also being too low for the tx to go through
2. Screenshots

MM gas bug bad1 Screenshot from 2023-09-02 14-43-20
MM gas bug bad2 Screenshot from 2023-09-02 15-14-43

  1. Send transaction. It will fail with "transaction undepriced" error
  2. Now, to avoid this bug one must manually supply gasPrice, for example: contract.methods.function(args).send({ from: address, gasPrice: '3000000000' });
  3. Review gas settings again. Same contract, same function, same arguments. Same report from Metamask, that gas settings are supplied by the site. But this time it is true, and those gas settings are adequate
5. Screenshots

MM gas bug good1 Screenshot from 2023-09-02 15-35-27
MM gas bug good2 Screenshot from 2023-09-02 15-34-53

  1. Send transaction. Transaction succeeds

Error messages or log output

MetaMask - RPC Error: [ethjs-query] while formatting outputs from RPC '{"value":
{"code":-32603,
"data":{
"code":-32000,
"message":"transaction underpriced"
}}}'

Version

10.34.3

Build type

None

Browser

Chrome, Firefox

Operating system

Windows, Linux

Hardware wallet

No response

Additional context

web3.js version 1.6.1
RPC in use: https://bsc-dataseed2.binance.org:443 on website; https://bsc-dataseed1.binance.org:443 in wallet. Likely the same behavior is present on others, but I tested with these.
Same behavior is observed with payable and non-payable contract functions. Behavior only first appeared about 3-5 days back. So far every user who reported this behavior is a Metamask user.
No changes were made to the website codebase recently(for many weeks). So the bug is being caused either by updates in Metamask or in BSC nodes.

@Snugface
Copy link
Author

Snugface commented Sep 2, 2023

Also just confirmed that the same bug is happening on other websites(which don't explicitly set gas price), not just the site I'm the maintainer of.

@qq919006380
Copy link

I had the same issue as you, but it got resolved after switching to a different RPC. Free RPCs can become unstable occasionally, especially during network congestion. You can consider adding a few backup RPCs or opting for paid ones.

@Snugface
Copy link
Author

Snugface commented Sep 2, 2023

Well, the truth is: RPC is reporting the correct gas price.
web3.eth.getGasPrice() reports 3 Gwei, as it should. And if I manually add that to transaction like so:

rpcGasPrice = await web3.eth.getGasPrice()
contract.methods.function(args).send({ from: address, gasPrice: rpcGasPrice })

it works as expected. And transaction goes through. So the problem isn't with the RPC reporting a wrong value; or at least it reports it correctly to web3.js.

Besides, MM receives 2.5 Gwei from somewhere instead. Not 0, but a specific value. And then also it says to the users that this is what the website suggested to it, which isn't true. It also calculates the gas limit correctly, as you can see from my screenshots.
Moreover, I've never-ever-ever before encountered this issue on BSC, where gas prices are fixed and never change. And no one ever reported it. Until a few days ago.
I can also consistently reproduce it. And other users are reporting it, too. And they, so far, were only MetaMask users. So I believe it is a recently-introduced bug, and not an RPC issue

@arti210
Copy link

arti210 commented Sep 5, 2023

Describe the bug

MetaMask incorrectly estimates gas settings on BSC when no gas price is explicitly supplied by the website.
Likely the bug is only specific to BSC, though I haven't tried reproducing it on other chains.

Steps to reproduce

  1. Initiate transaction with a contract on BSC using web3.js without explicitly supplying any gas settings, like so:
    contract.methods.function(args).send({ from: address });
  2. Review gas settings automatically picked by Metamask, and observe that they are falsely reported as "suggested by site", while also being too low for the tx to go through
2. Screenshots

MM gas bug bad1 Screenshot from 2023-09-02 14-43-20
MM gas bug bad2 Screenshot from 2023-09-02 15-14-43

  1. Send transaction. It will fail with "transaction undepriced" error
  2. Now, to avoid this bug one must manually supply gasPrice, for example: contract.methods.function(args).send({ from: address, gasPrice: '3000000000' });
  3. Review gas settings again. Same contract, same function, same arguments. Same report from Metamask, that gas settings are supplied by the site. But this time it is true, and those gas settings are adequate
5. Screenshots

MM gas bug good1 Screenshot from 2023-09-02 15-35-27
MM gas bug good2 Screenshot from 2023-09-02 15-34-53

  1. Send transaction. Transaction succeeds

Error messages or log output

MetaMask - RPC Error: [ethjs-query] while formatting outputs from RPC '{"value":
{"code":-32603,
"data":{
"code":-32000,
"message":"transaction underpriced"
}}}'

Version

10.34.3

Build type

None

Browser

Chrome, Firefox

Operating system

Windows, Linux

Hardware wallet

No response

Additional context

web3.js version 1.6.1
RPC in use: https://bsc-dataseed2.binance.org:443 on website; https://bsc-dataseed1.binance.org:443 in wallet. Likely the same behavior is present on others, but I tested with these.
Same behavior is observed with payable and non-payable contract functions. Behavior only first appeared about 3-5 days back. So far every user who reported this behavior is a Metamask user.
No changes were made to the website codebase recently(for many weeks). So the bug is being caused either by updates in Metamask or in BSC nodes.

@Snugface
Copy link
Author

Snugface commented Sep 8, 2023

"Hey, guys. Metamask team here. We just wanted to thank you for reporting this bug. It's not too serious, but it definitely is a bug that we introduced recently. And it also misleads users, by telling them that gas-estimation was done incorrectly on the side of a web3 app, while it is being done incorrectly by Metamask.
We wanted to show you how much we care about this, that's why for a week we haven't even acknowledged whether this is indeed a bug or not."

@Snugface
Copy link
Author

Snugface commented Sep 8, 2023

Opened a duplicate issue, against the best practices, but in the hope that it does get some acknowledgement from the maintainers.
#20797

@Snugface
Copy link
Author

Updates in the duplicate issue. Comment from a MetaMask representative:
#20797 (comment)

Duplicate of #20699 , opening multiple issues of the same content is not the intended way to get our attention...If anything it undermines it as it creates more noise.

We're still no further with the issue itself though. But that's something, definitely.

@anaamolnar anaamolnar added Sev2-normal Normal severity; minor loss of service or inconvenience. team-wallet-framework team-confirmations-secure-ux DEPRECATED: please use "team-confirmations" label instead and removed team-wallet-framework labels Sep 12, 2023
@bschorchit
Copy link

Due to timing, this seems related with the BNB chain upgrade to use EIP1559. We'll look into it.

@Snugface
Copy link
Author

Due to timing, this seems related with the BNB chain upgrade to use EIP1559. We'll look into it.

Thank you!
I was always so confused with baseFee and maxPriorityFee before this, being a BSC dev. Googled the EIP, and found a good explanation
https://www.quicknode.com/guides/ethereum-development/transactions/how-to-send-an-eip-1559-transaction
Will bear in mind that this has also been rolled out to BSC. Really, thank you. This is informative.

@brad-decker
Copy link
Contributor

@Snugface i'm opening a PR that references this issue, but it only helps with the formatting of the error thrown not the error itself. Just wanted to make sure I don't provide any false hope for a prompt resolution ☹️ BUT hopefully this will allow us to get actual stack traces in our error reporting tool that will further our efforts to resolve this (and other) issues.

repost cause the duplicate issue I worked with confused me :P

@Snugface
Copy link
Author

@brad-decker Much appreciated! Thank you for continuing to dig into this. Better stack traces are always good.

brad-decker added a commit that referenced this issue Sep 26, 2023
## **Description**
Our fork of ethjs-query has been namespaced to the `@metamask/`
namespace, and updated with mostly development only fixes. There is one
exception which is the reason for this pull request which is the removal
of a try/catch that was catching too broadly and wrapping legitimate
errors in a new Error object that claimed the issue was with the
formatting of the output. In most cases this is incorrect and results in
a wide swath of errors being lumped together inside of sentry. This
change will result in the real errors being surfaced, after which we can
decide where to prioritize efforts to resolve RPC issues.

This PR progresses, and is expected to change the stack trace for the
following issues:
#9317 
#10519 
#10619
#11488 
#11974 
#13395 
#14298 
#14365 
#15250 
#17073 
#17803 
#19697 
#20699 

We are closing these issues opened automatically by sentry-io which are
not fully resolved but should result in better errors and stack traces:

fixes #10552 
fixes #14660 
fixes #14676 
fixes #14730 
fixes #14801
fixes #15065 

## **Manual testing steps**
1. Attempt to reproduce any of the bugs listed, 17073 was the easiest to
reproduce for me. This involves getting test currency from the wemix
faucet as listed in the issue and initiating a transaction between two
accounts you own.
2. On develop you'll see an 'error formatting outputs' error text
similar to what is reported in the issue.
3. On this branch you'll get the original error, without the wrapped
'formatting' error. including a different stack trace.

## **Pre-merge author checklist**

- [x] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've clearly explained:
  - [x] What problem this PR is solving.
  - [x] How this problem was solved.
  - [x] How reviewers can test my changes.
- [x] I’ve indicated what issue this PR is linked to: Fixes #???
- [x] I’ve included tests if applicable.
- [x] I’ve documented any added code.
- [x] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
- [x] I’ve properly set the pull request status:
  - [ ] In case it's not yet "ready for review", I've set it to "draft".
- [x] In case it's "ready for review", I've changed it from "draft" to
"non-draft".

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.

---------

Co-authored-by: MetaMask Bot <[email protected]>
@segun
Copy link
Contributor

segun commented Oct 19, 2023

I can't seem to replicate the invalid gas issue, I can see that the label says Site Suggested even though I didn't provide gas, but the gas suggested is enough for the transaction. Asides the wrong labelling, everything seems ok.

Please see attached screen capture.

Here's my source code

   const jsonInterface = [
        {
            "inputs": [],
            "name": "retrieve",
            "outputs": [
                {
                    "internalType": "uint256",
                    "name": "",
                    "type": "uint256"
                }
            ],
            "stateMutability": "view",
            "type": "function"
        },
        {
            "inputs": [
                {
                    "internalType": "uint256",
                    "name": "num",
                    "type": "uint256"
                }
            ],
            "name": "store",
            "outputs": [],
            "stateMutability": "nonpayable",
            "type": "function"
        }
    ];


    if (window.ethereum) {
        await window.ethereum.request({ method: 'eth_requestAccounts' });
        window.web3 = new Web3(window.ethereum);
        console.log('web3 enabled', window.web3);

        const accounts = await window.web3.eth.getAccounts();

        const contract = new web3.eth.Contract(jsonInterface, "0xd4f6d842Bc1526EFB3be6f01d3E6EBDd6A733d0b");
        let result = await contract.methods.retrieve().call();
        console.log('result', result);
        // await contract.methods.store(100).send({ from: accounts[0], gasPrice: '3000000000' });
        await contract.methods.store(100).send({ from: accounts[0]});
        result = await contract.methods.retrieve().call();
        console.log('result', result);
    } else {
        console.error('No Metamask');
    }

and here's the screen capture

Screen.Recording.2023-10-19.at.17.04.01.mov

cc @seaona @bschorchit

@segun segun self-assigned this Nov 1, 2023
@bschorchit bschorchit added team-confirmations-planning (only for internal use within Confirmations team) and removed team-confirmations-secure-ux DEPRECATED: please use "team-confirmations" label instead labels Nov 17, 2023
Copy link
Contributor

This issue has been automatically marked as stale because it has not had recent activity in the last 90 days. It will be closed in 45 days if there is no further activity. The MetaMask team intends on reviewing this issue before close, and removing the stale label if it is still a bug. We welcome new comments on this issue. We do not intend on closing issues if they report bugs that are still reproducible. Thank you for your contributions.

@github-actions github-actions bot added the stale issues and PRs marked as stale label Feb 15, 2024
@github-project-automation github-project-automation bot moved this to To be fixed in Bugs by severity Feb 19, 2024
Copy link
Contributor

This issue was closed because there has been no follow up activity in the last 45 days. If you feel this was closed in error, please reopen and provide evidence on the latest release of the extension. Thank you for your contributions.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Mar 31, 2024
@github-project-automation github-project-automation bot moved this from To be fixed to Fixed in Bugs by severity Mar 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Sev2-normal Normal severity; minor loss of service or inconvenience. stale issues and PRs marked as stale team-confirmations-planning (only for internal use within Confirmations team) type-bug
Projects
Archived in project
Development

No branches or pull requests

7 participants