fix(node): harden transaction inclusion logic to account for fee market #1239

raulk · 2024-12-19T15:11:27Z

This PR improves correctness of tx inclusion logic on two fronts.

Begins rejecting messages with invalid limits at CheckTx time:
- Gas fee cap below minimal base fee.
- Gas limit above maximum block gas limit.

Some type shenanigans via downcasts were necessary to deal with #1241. Tested by extending a materializer test.

Exclude non-includable messages at proposal time.
- I removed the MessagesSelector trait because it was unusable. The vector holding selectors cannot used boxed objects because the trait is not object-safe as it has methods with generic parameters. Also, there's no point over-engineering this prematurely.
- Also removed the decreasing sorting by gas limit; we prefer to randomize for security (otherwise an attacker could drip brutally overestimated transactions just below the block gas limit to DoS the chain).

- gas fee cap below minimal base fee. - gas limit above maximum block gas limit.

LePremierHomme · 2025-01-03T06:44:21Z

fendermint/testing/materializer/tests/docker_tests/standalone.rs

+                // It should be rejected at submission time via CheckTx (which will also reject it at block validation time).
+                let invalid = Eip1559TransactionRequest::new()
+                    .to(to)
+                    .max_fee_per_gas(1)


What about .max_fee_per_gas(min_base_fee - 1)? it will also help to document how that value can be retrieved.

We just don't have access to the min base fee here; nor is it available through an API. We could technically fetch the block header of the genesis block and take that base fee as the min base fee, but I'm sure it would buy us anything.

LePremierHomme · 2025-01-03T10:58:47Z

fendermint/vm/interpreter/src/chain.rs

@@ -228,6 +230,14 @@ where
                    }
                }
                ChainMessage::Signed(signed) => {
+                    // We do not need to check against the minimium base fee because the gas market
+                    // is guaranteed to be capped at it anyway.


Where/how is it guaranteed?

Good point; I think we should check the min base fee is satisfied here since CheckTx won't be invoked for transactions we never saw but that got included by another validator in a proposal.

Sorry, I was mistaken. This is code is correct. The current gas market conditions are determined by our own state transitions. So here we validate message's gas fee cap is above or equal to the current gas market base fee, as determined by our own state. The EIP1559 gas market actor floors the base fee to the minimal base fee here:

ipc/fendermint/actors/gas_market/eip1559/src/lib.rs

Line 150 in 7e4b87c

self.constants.minimal_base_fee.clone()

LePremierHomme · 2025-01-03T11:02:37Z

fendermint/vm/interpreter/src/chain.rs

@@ -228,6 +230,14 @@ where
                    }
                }
                ChainMessage::Signed(signed) => {
+                    // We do not need to check against the minimium base fee because the gas market
+                    // is guaranteed to be capped at it anyway.
+                    if signed.message.gas_fee_cap < gas_market.base_fee {


What about doing that in tx selection for block inclusion (as a leader)? When checking for mempool inclusion only the min fee is verified.

@cryptoAtwill do we not already exclude transactions whose gas fee cap is below the network's base fee at block production time? I was strangely not able to find that logic here: https://github.com/consensus-shipyard/ipc/blob/702d49b619623915772fe935a86e59a89744c3b1/fendermint/vm/interpreter/src/selector.rs

(Do we need a second selector?)

@raulk I dont remember discussing that in prepare. I think we were mostly discussing the cometbft side of things.

What do you mean with "the CometBFT side of things"? We agreed that we'd assign negative priorities to non-includable transactions in CheckTX, to instruct CometBFT to prefer sending us includable transactions, but we'd still have to filter out non-includable transactions in prepare.

I'll fix it in this PR.

if we need to enforce below base fee exclusion, we probably should add the other selector to enforce in fendermint just to be sure. Cometbft should have done one round of selection based on priority.

cryptoAtwill · 2025-01-03T15:12:47Z

fendermint/vm/interpreter/src/chain.rs

+                    .context("inner state not downcastable to FvmExecState")?;
+                let gas_market = fvm_exec_state.block_gas_tracker().current_gas_market();
+                if msg.message.gas_limit > gas_market.block_gas_limit
+                    || msg.message.gas_fee_cap < gas_market.min_base_fee


A quick question, why using min_base_fee here? Currently min_base_fee is not updated so I assume it's updated in a system upgrade? Is it a fixed value that can be smaller than base_fee?

This is to reject transactions whose base fee cap is below the minimum acceptable base fee. Otherwise those transactions would sit forever in the mempool because the base fee would never go low enough to include them.

I misunderstood, should have no issue here.

fix: reject block if it includes messages below the base fee.

7e4b87c

raulk requested a review from a team as a code owner December 19, 2024 15:11

raulk marked this pull request as draft December 19, 2024 15:11

raulk marked this pull request as ready for review January 2, 2025 16:00

reject messages with invalid limits:

cd07cec

- gas fee cap below minimal base fee. - gas limit above maximum block gas limit.

raulk force-pushed the raulk/fix/reject-below-base-fee branch from 05c6790 to cd07cec Compare January 2, 2025 16:35

raulk changed the title ~~fix: reject block if it includes messages below the base fee.~~ fix(node): reject block if it includes messages below the base fee. Jan 2, 2025

Merge branch 'main' into raulk/fix/reject-below-base-fee

49aba08

LePremierHomme reviewed Jan 3, 2025

View reviewed changes

raulk added 2 commits January 3, 2025 14:18

clippy.

2c40148

exclude non-includable messages at proposal time.

02e23db

cryptoAtwill reviewed Jan 3, 2025

View reviewed changes

raulk requested a review from cryptoAtwill January 3, 2025 15:14

raulk changed the title ~~fix(node): reject block if it includes messages below the base fee.~~ fix(node): harden transaction inclusion logic to account for fee market Jan 3, 2025

cryptoAtwill approved these changes Jan 3, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(node): harden transaction inclusion logic to account for fee market #1239

fix(node): harden transaction inclusion logic to account for fee market #1239

raulk commented Dec 19, 2024 •

edited

Loading

LePremierHomme Jan 3, 2025

raulk Jan 3, 2025

LePremierHomme Jan 3, 2025

raulk Jan 3, 2025

raulk Jan 3, 2025

LePremierHomme Jan 3, 2025

raulk Jan 3, 2025

cryptoAtwill Jan 3, 2025

raulk Jan 3, 2025

raulk Jan 3, 2025

cryptoAtwill Jan 3, 2025

cryptoAtwill Jan 3, 2025

raulk Jan 3, 2025

cryptoAtwill Jan 3, 2025

fix(node): harden transaction inclusion logic to account for fee market #1239

Are you sure you want to change the base?

fix(node): harden transaction inclusion logic to account for fee market #1239

Conversation

raulk commented Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raulk commented Dec 19, 2024 •

edited

Loading