Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

In Geth chain-mode, logic to accept/reject transactions based on gas price/limit should match Geth #2176

Open
MicaiahReid opened this issue Jan 24, 2022 · 2 comments

Comments

@MicaiahReid
Copy link
Contributor

If a transaction gets into the miner's runTx function, and it fails in the VM, that catch block should probably add the transaction back into the pool.

Check against geth if:

  1. a transaction like this: {"from" : "0x90F8bf6A479f320ead074411a4B0e7944Ea8c9C1", "gas": "0x5b8d80", "maxFeePerGas": "0x1" } (too low maxFeePerGas) immediately returns an error like this VM Exception while processing transaction: Transaction's maxFeePerGas (1) is less than the block's baseFeePerGas, and if
  2. the transaction is then available in txpool_content
@MicaiahReid MicaiahReid added this to the 7.0.x milestone Jan 24, 2022
@jeffsmale90 jeffsmale90 self-assigned this Apr 6, 2022
@jeffsmale90
Copy link
Contributor

To paraphrase @MicaiahReid: "The best course of action is to add our check in the transaction-pool.ts prepareTransaction function to validate that a transaction's maxFeePerGas >= baseFeePerGas, and we'll do the rest in #2831"

For reference, the same behaviour is described in Geth here: https://github.com/ethereum/go-ethereum/blob/06540146524222f648d8b19c5bcb00a8fd3dd339/core/state_transition.go#L235

jeffsmale90 added a commit that referenced this issue Apr 7, 2022
@jeffsmale90 jeffsmale90 changed the title A rejected transaction should possibly be added back into the pool A transaction with maxFeePerGas less than the next block's baseFeePerGas should be rejected Apr 7, 2022
jeffsmale90 added a commit that referenced this issue Apr 18, 2022
jeffsmale90 added a commit that referenced this issue Apr 27, 2022
@davidmurdoch davidmurdoch removed this from the 7.0.x milestone May 20, 2022
@jeffsmale90
Copy link
Contributor

jeffsmale90 commented May 24, 2022

As per comment from the geth team here: ethereum/go-ethereum#24661 (comment), and @MicaiahReid 's (excellent) summary here: #2840 (review), Geth's behaviour is:

Gas Limit Gas Price Result Notes
Unspecified Unspecified No error Geth chooses a good gas price, then uses that price to estimate gas limit.
Specified < baseFeePerGas No error Geth blindly accepts the gas price because the transaction has a gas limit.
Unspecified < baseFeePerGas Error: maxFeePerGas is less than block base fee. Because no gas limit was specified, Geth needs to estimate it. To estimate it, it runs the transaction against the next block. Because the gas price is too low, it's rejected.

(Quoting @MicaiahReid)
The key difference between us and Geth in this case is that our eth_estimateGas function does not enforce a baseFeePerGas. This can be seen when we make the RuntimeBlock in eth_estimateGas:

const block = new RuntimeBlock(
  Quantity.from((parentHeader.number.toBigInt() || 0n) + 1n),
  parentHeader.parentHash,
  parentHeader.miner,
  tx.gas.toBuffer(),
  parentHeader.gasUsed.toBuffer(),
  parentHeader.timestamp,
  options.miner.difficulty,
  parentHeader.totalDifficulty,
  0n // no baseFeePerGas for estimates
);

So rather than this current change, we may just want to pass in the next block's baseFeePerGas rather than 0n.

@jeffsmale90 jeffsmale90 changed the title A transaction with maxFeePerGas less than the next block's baseFeePerGas should be rejected If chain-mode is Geth, logic to accept/reject transactions based on gas price/limit should match Geth May 24, 2022
@jeffsmale90 jeffsmale90 changed the title If chain-mode is Geth, logic to accept/reject transactions based on gas price/limit should match Geth In Geth chain-mode, logic to accept/reject transactions based on gas price/limit should match Geth May 24, 2022
@davidmurdoch davidmurdoch moved this to Inbox in Ganache Jul 19, 2022
@davidmurdoch davidmurdoch moved this from Inbox to Backlog in Ganache Jul 19, 2022
@davidmurdoch davidmurdoch moved this from Backlog to In Progress in Ganache Jul 19, 2022
@jeffsmale90 jeffsmale90 moved this from In Progress to Backlog in Ganache Jul 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Status: Backlog
4 participants