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

Calculate EIP1559 gas price in JSON-RPC #1783

Merged
merged 2 commits into from
Aug 8, 2023

Conversation

mchmatt
Copy link
Contributor

@mchmatt mchmatt commented Aug 6, 2023

Description

Tools like https://github.com/blockscout/blockscout operate under the premise that field "gasPrice" will be present even if the TX is an EIP 1559 type, and thus don't work correctly when connected to Polygon-Edge's RPC.
This PR calculates the gas price on the fly for requests done to the RPC, through the existing function GetGasPrice.

Changes include

  • Bugfix (non-breaking change that solves an issue)
  • Hotfix (change that solves an urgent issue, and requires immediate attention)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (change that is not backwards-compatible and/or changes current functionality)

Additional comments

This fixes issues blockscout/blockscout#8128 and blockscout/blockscout#8130. All other RPCs I have tested (Llama RPC, Alchemy, Ankr) include the gasPrice field for EIP1559 transactions.

@github-actions
Copy link

github-actions bot commented Aug 6, 2023

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@mchmatt
Copy link
Contributor Author

mchmatt commented Aug 6, 2023

I have read the CLA Document and I hereby sign the CLA

1 similar comment
@xxMattewxx
Copy link
Contributor

I have read the CLA Document and I hereby sign the CLA

Copy link
Collaborator

@Stefan-Ethernal Stefan-Ethernal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @mchmatt thanks for the PR, generally looks good to me.

However, WDYT about setting a gas price in the txpool_content JSON RPC function as well (https://github.com/mchmatt/polygon-edge/blob/develop/jsonrpc/txpool_endpoint.go#L54)? I was thinking about adding this line there as well tx.GasPrice = tx.GetGasPrice(t.store.GetBaseFee()).

Also, pay attention to the linting error and fix it, so we can merge it eventually.

github-actions bot added a commit that referenced this pull request Aug 7, 2023
@mchmatt
Copy link
Contributor Author

mchmatt commented Aug 7, 2023

However, WDYT about setting a gas price in the txpool_content JSON RPC function as well (https://github.com/mchmatt/polygon-edge/blob/develop/jsonrpc/txpool_endpoint.go#L54)? I was thinking about adding this line there as well tx.GasPrice = tx.GetGasPrice(t.store.GetBaseFee()).

I'm not sure how that would work as my understanding is that EIP 1559 TXs in the pool aren't assigned to a block yet, but maybe I'm missing something? Not really familiar with blockchain dev, sorry.

Also, pay attention to the linting error and fix it, so we can merge it eventually.

Should be fixed now, really appreciate the quick reply.

@Stefan-Ethernal
Copy link
Collaborator

I'm not sure how that would work as my understanding is that EIP 1559 TXs in the pool aren't assigned to a block yet, but maybe I'm missing something?

Yes, that is correct, in the tx pool, tx is not yet included in the block. And since the Blockscout indexes only blocks, then we should be ok with your approach. You have my approval. 🙂

@Stefan-Ethernal Stefan-Ethernal requested a review from a team August 7, 2023 07:00
@polygomic
Copy link

Please accept this pull request we have big problems with blockscout

@Stefan-Ethernal
Copy link
Collaborator

We are going to merge this PR, although we have noticed that the GetGasPrice function does not calculate gas prices the same way as Geth does. Geth for instance just returns a gas fee cap for dynamic fee transactions, whereas Edge has some logic, which does not seem entirely correct.

@Stefan-Ethernal Stefan-Ethernal merged commit c4f9ec1 into 0xPolygon:develop Aug 8, 2023
6 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Aug 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants