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

refactor(txpool): consider below proto fee cap an error #71

Merged
merged 1 commit into from
Oct 14, 2022

Conversation

mattsse
Copy link
Collaborator

@mattsse mattsse commented Oct 14, 2022

instead of tracking the feeCap_tx > proto_feecap requirement as part of the state, treat it as an error.

This is sound, since a tx that can't satisfy this condition will never become valid, since the lower bound of the protocol feecap is constant (7 WEI).

@onbjerg onbjerg added C-enhancement New feature or request A-tx-pool Related to the transaction mempool labels Oct 14, 2022
if fee > self.minimal_protocol_basefee {
state.insert(TxState::ENOUGH_FEE_CAP_PROTOCOL);
if fee_cap >= self.pending_basefee {
state.insert(TxState::ENOUGH_FEE_CAP_BLOCK);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit for later. We can reasonably add check for tx.gas_limit < block.target_gas_limit or <block.target_gas_limit/2 (as min gas limit ). This is not consensus restricted as target_gas_limit is something set by validators as configuration but it should be fine to have it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

right, I haven't implemented this check yet, but there's already a block_gas_limit field that will hold this value.

Copy link
Collaborator

Choose a reason for hiding this comment

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

There is block.gas_limit but it changes over time (by max +-12.5% I think) as variable block size is introduced with eip1559, config.target_gas_limit is what is set and expected to be the same with the majority of nodes but it is config value.

Copy link
Collaborator

@rakita rakita left a comment

Choose a reason for hiding this comment

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

lgtm

@mattsse mattsse merged commit a7cf915 into main Oct 14, 2022
@mattsse mattsse deleted the matt/refactor-min-proto-fee branch October 14, 2022 15:10
yutianwu pushed a commit to yutianwu/reth that referenced this pull request Jul 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tx-pool Related to the transaction mempool C-enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants