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]: 429 error on Base. Extension not using supplied rpc api #27099

Open
Yuripetusko opened this issue Sep 12, 2024 · 17 comments
Open

[Bug]: 429 error on Base. Extension not using supplied rpc api #27099

Yuripetusko opened this issue Sep 12, 2024 · 17 comments
Labels
external-contributor regression-prod-12.1.3 Regression bug that was found in production in release 12.1.3 Sev2-normal Normal severity; minor loss of service or inconvenience. team-assets type-bug

Comments

@Yuripetusko
Copy link

Yuripetusko commented Sep 12, 2024

Describe the bug

When using wagmi with configured private rpc api url and metamask connector, metamask still uses default rpc (perhaps for quering gasFee estimation or doing eth_call simulation), which often leads to 429 error. Shouldn't Metamask use the supplied rpc api here somehow to avoid that?

Expected behavior

Metamask to use supplied rpc api for fee estimation, or handle 429 errors better

Screenshots/Recordings

image

Steps to reproduce

Hard to have consistent reproduction as it is dependant on public base (mainner.base.org) rpc requests load.

  • set up wagmi config and supply custom rpc in transports object
  • send a transaction with writeContract and then open dev console of the metamask extension
  • observe that the rpc used by metamask is a default rpc (mainner.base.org), instead of the configured rpc

Error messages or log output

Error: Non-200 status code: '429'

Detection stage

In production (default)

Version

12.1.3

Build type

None

Browser

Brave, Other (please elaborate in the "Additional Context" section)

Operating system

MacOS

Hardware wallet

No response

Additional context

Both Brave and Arc browsers (both latest chromium)

Severity

  • Quite critical as during busy hours, users can't submit a transaction and blame us, and we have no way to handle it
@github-project-automation github-project-automation bot moved this to To be fixed in Bugs by severity Sep 12, 2024
@metamaskbot metamaskbot added external-contributor regression-prod-12.1.3 Regression bug that was found in production in release 12.1.3 labels Sep 12, 2024
@Yuripetusko Yuripetusko changed the title [Bug]: 429 on gas estimation on Base. Extension not using supplied rpc api [Bug]: 429 error on Base. Extension not using supplied rpc api Sep 12, 2024
@Yuripetusko
Copy link
Author

Alternatively, maybe if the dapp provided custom fee values, estimation shouldn't throw the error and prevent user from submitting the transaction?

@DanielTech21 DanielTech21 added team-extension-ux DEPRECATED: please use "team-wallet-ux" label instead Sev2-normal Normal severity; minor loss of service or inconvenience. labels Sep 12, 2024
@DanielTech21
Copy link

Hi @Yuripetusko

Thank you for bringing this to our attention.

Our product team will look into this and provide comments.

@Yuripetusko
Copy link
Author

Updating with some context: As i've been told now, it is normal that wallets don't use the rpc configured by the dapp. So in this case the issue here is that Metamask throws an error for something I didn't ask for and have no control over (gas fee price estimations) and this fails the whole transaction. The 429 should either be handled more gracefully or Metamask should provide a way to opt out of gas fee fetching if I provide it myself

@tingui96
Copy link

i have the error in edge and my friend in chrome, Both in windows. I think that can be general error

@Yuripetusko
Copy link
Author

Some context.
image

@Yuripetusko
Copy link
Author

Still puzzeled that there are not many reports besides mine since we are getting ~100 sentry error instances per day with this error

@bschorchit
Copy link

Base team has adjusted rate limits on their side to minimize this issue. Did it improve the issue on your side, @Yuripetusko ?

@Yuripetusko
Copy link
Author

We had ~6 events today, and around ~100 yesterday. A bit early to say for sure considering it usually starts happening more after 16:00 CET (when folks in USA start to be more active). Let me get back to you again on this tomorrow

image

@bschorchit bschorchit added team-confirmations Push issues to confirmations team and removed team-extension-ux DEPRECATED: please use "team-wallet-ux" label instead labels Sep 17, 2024
@Yuripetusko
Copy link
Author

24 Errors already in the last 5 hours

@Yuripetusko
Copy link
Author

I still think that since I am explicitly passing these gas fields

maxFeePerGas: gasPricePerFeeResult.maxFeePerGasSummed,
maxPriorityFeePerGas: gasPricePerFeeResult.maxPriorityFeePerGasWithExtra,
gas: extraGasLimit ?? 1_300_000n,

metamask shouldn't throw an error when estimating gas. And even if I wasn't providing these fields, I think Metamask should have some fallback defaults to use instead of failing and throwing an error to user, even if these defaults are heavily cached and could be outdated. Why not use some centralised api instead to get latest prices and then fetch from it instead of using rpc here. Or perhaps Metamask can somehow get the rpc that was configured with the connector on the dapp?

@Yuripetusko
Copy link
Author

Also just tried to do a contract call on Base myself too and got the same thing

image

it's fine here, but if this happens at the same time as I click sign, then the error is thrown

@Yuripetusko
Copy link
Author

Ok while debugging this I discovered something that might help you pin down the issue. The 429 always happens immediatly after extension pop-up opens. Once it gets the first gas estimateion response back, from there on it seems to be ok. So it leads me to believe that there's a bug on metamask where it does particulary large number of requests when it just opens up. May or may not be due to some unhandled infinite loop (I haven't looked into MM code that closely).

I am uploading a video right now where you can see that 429 always happens immediatly after popup opens. And the error is thrown usually if you click Sign as quickly as possible

Here's a long and boring video: https://youtu.be/PdJKH8Yqhmo

And here's a short video of the moment the error happens and causes tx to fail:
https://github.com/user-attachments/assets/092d7c73-ad4c-4a86-92fb-5f82fb5c5998

@github-project-automation github-project-automation bot moved this to To be fixed in Bugs by team Sep 19, 2024
@cryptotavares cryptotavares added team-assets and removed team-confirmations Push issues to confirmations team labels Sep 23, 2024
@cryptotavares
Copy link
Contributor

@MetaMask/metamask-assets and @MetaMask/wallet-framework this seems to be a broader issue within the wallet and not just Confirmations.
tldr: we (MetaMask) are triggering to many rpc requests quickly hitting the rate limit of some providers.

In the mean time @Yuripetusko does the issue still exist with other public providers? You can more base public providers here.

@Yuripetusko
Copy link
Author

@MetaMask/metamask-assets and @MetaMask/wallet-framework this seems to be a broader issue within the wallet and not just Confirmations. tldr: we (MetaMask) are triggering to many rpc requests quickly hitting the rate limit of some providers.

In the mean time @Yuripetusko does the issue still exist with other public providers? You can more base public providers here.

It still happens a lot. ~30 errors in our sentry for yesterday. ~15 today. What do you want me to try with the link you sent? Change base public provider in metamask settings? I can do as a test, but this wouldn't be a feasable option to tell to all of our users

@Yuripetusko
Copy link
Author

will try to reproduce with other providers, but yeah, if you inspect extension, it does a lot of calls as it listens to new blocks and then does 3 - 5 requests on each new block to estimate gas (low, medium, aggressive). And since blocktime on base is so short, there are a lot of calls

@Yuripetusko
Copy link
Author

Tried with 2 different public rpcs and it didn't happen yet, but it's understandable if it's not set as a default rpc for base in everyone's metamask's. But also it doesn't happen every single time with base.org rpc. So will try few more times today

@d10r
Copy link

d10r commented Oct 18, 2024

We're having this too with Superfluid Dapps.
I did some digging and concluded that it's caused by Metamask's recently introduced "Estimated Changes" feature.
Here's a log of RPC requests Metamask does when the Dapp superboring.xyz asks it to do a transaction:
mm_sb_requests.txt
The number of resulting RPC requests here is >200 in a few seconds.

Superfluid Dapps are probably a bit more susceptible to run into this issue because of its rather complex transactions. But the problem seems to be a general one.

This is considerably harming the UX of Dapps. The only viable mitigation from our side I can currently see is to switch to EIP-712 and meta-transactions, so that we can avoid using the wallet RPC at all. We already don't use the wallet RPC for anything but transaction broadcast.

Given my superficial understanding of what MM does under the hood, a few suggestions:

  • avoid rate limiting errors becoming aborted transactions. E.g. by waiting for a few seconds and retrying.
  • the number of requests triggered by the change estimation is too high. Assuming that most users are on rate limited public RPCs, MM imo needs to somehow make sure it won't fire hundreds of requests basically all at once. I'm not sure what the best strategy here is. Maybe batching. Maybe put a cap and abort the estimation if hit. Maybe offload to a backend service.

PS: I noticed that when requests fail internally (the http envelope being 200, but the RPC returning an error code), the transaction doesn't fail. Instead MM just shows the estimated changes as failed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
external-contributor regression-prod-12.1.3 Regression bug that was found in production in release 12.1.3 Sev2-normal Normal severity; minor loss of service or inconvenience. team-assets type-bug
Projects
Status: To be fixed
Status: To be fixed
Development

No branches or pull requests

7 participants