-
Notifications
You must be signed in to change notification settings - Fork 477
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
JsonRpc standardization: change MinerPremiumNegative error to FeeCapTooLow #8128
base: master
Are you sure you want to change the base?
Conversation
@@ -462,7 +462,7 @@ protected virtual TransactionResult BuyGas(Transaction tx, BlockHeader header, I | |||
if (!tx.TryCalculatePremiumPerGas(header.BaseFeePerGas, out premiumPerGas)) | |||
{ | |||
TraceLogInvalidTx(tx, "MINER_PREMIUM_IS_NEGATIVE"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should change MINER_PREMIUM_IS_NEGATIVE
log also?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does geth show same "max fee per gas less than block base fee..." error message in logs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, it is:
WARN [02-03|17:35:21.985] Served eth_call conn=127.0.0.1:51596 reqid=1 duration="395.958µs" err="err: max fee per gas less than block base fee: address 0x32E4E4c7c5d1CEa5db5F9202a9E4D99E56c91a24, maxFeePerGas: 0, baseFee: 0 (supplied gas 50000000)"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My suggestion would be to log the same or similar message
@@ -84,7 +84,7 @@ protected override TransactionResult BuyGas(Transaction tx, BlockHeader header, | |||
if (!tx.TryCalculatePremiumPerGas(header.BaseFeePerGas, out premiumPerGas)) | |||
{ | |||
TraceLogInvalidTx(tx, "MINER_PREMIUM_IS_NEGATIVE"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should change MINER_PREMIUM_IS_NEGATIVE
log also?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, we can change it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think about covering by test
@@ -84,7 +84,7 @@ protected override TransactionResult BuyGas(Transaction tx, BlockHeader header, | |||
if (!tx.TryCalculatePremiumPerGas(header.BaseFeePerGas, out premiumPerGas)) | |||
{ | |||
TraceLogInvalidTx(tx, "MINER_PREMIUM_IS_NEGATIVE"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, we can change it
Currently
eth_call
,eth_simulateV1
,eth_estimateGas
,eth_sendRawTransaction
,eth_sendTransaction
returns in Geth and Nethermind different error message for the same error:Geth link to code:
Nethermind:
Changes
Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Notes on testing
Optional. Remove if not applicable.
Documentation
Requires documentation update
If yes, link the PR to the docs update or the issue with the details labeled
docs
. Remove if not applicable.Requires explanation in Release Notes
If yes, fill in the details here. Remove if not applicable.
Remarks
Optional. Remove if not applicable.