feat: make base fee computation parameters configurable via chain spec by roberto-bayardo · Pull Request #3992 · paradigmxyz/reth

roberto-bayardo · 2023-07-30T17:01:49Z

Making these parameters configurable will, for example, allow chains to have a different gas target per block.

rkrasiuk

got some minor comments, lgtm otherwise

crates/consensus/auto-seal/src/task.rs

crates/transaction-pool/src/maintain.rs

crates/rpc/rpc/src/eth/api/fees.rs

rkrasiuk · 2023-07-30T18:08:38Z

Context: ported from op-rs#32

codecov · 2023-07-30T18:18:26Z

Codecov Report

Merging #3992 (6723a03) into main (e3457b8) will decrease coverage by 52.32%.
Report is 3 commits behind head on main.
The diff coverage is 19.04%.

❗ Current head 6723a03 differs from pull request most recent head 6365d49. Consider uploading reports for the commit 6365d49 to get more accurate results

Files Changed	Coverage Δ
bin/reth/src/args/rpc_server_args.rs	`0.00% <0.00%> (-53.99%)`	⬇️
bin/reth/src/init.rs	`0.00% <0.00%> (-97.21%)`	⬇️
crates/consensus/auto-seal/src/lib.rs	`0.00% <0.00%> (ø)`
crates/consensus/auto-seal/src/task.rs	`0.00% <0.00%> (ø)`
crates/consensus/common/src/validation.rs	`0.00% <0.00%> (-72.53%)`	⬇️
crates/payload/builder/src/payload.rs	`0.00% <0.00%> (ø)`
crates/primitives/src/basefee.rs	`0.00% <0.00%> (-97.83%)`	⬇️
crates/primitives/src/constants/mod.rs	`0.00% <ø> (-100.00%)`	⬇️
crates/primitives/src/hardfork.rs	`4.42% <0.00%> (-95.58%)`	⬇️
crates/primitives/src/header.rs	`36.27% <0.00%> (-59.29%)`	⬇️
... and 12 more

... and 374 files with indirect coverage changes

Flag	Coverage Δ
integration-tests	`16.38% <19.04%> (-0.16%)`	⬇️
unit-tests	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
reth binary	`5.96% <0.00%> (-19.72%)`	⬇️
blockchain tree	`0.00% <ø> (-83.05%)`	⬇️
pipeline	`0.00% <ø> (-90.08%)`	⬇️
storage (db)	`18.22% <66.66%> (-56.20%)`	⬇️
trie	`0.00% <ø> (-94.71%)`	⬇️
txpool	`14.94% <0.00%> (-34.03%)`	⬇️
networking	`27.45% <ø> (-50.00%)`	⬇️
rpc	`23.77% <12.50%> (-33.69%)`	⬇️
consensus	`1.00% <0.00%> (-63.08%)`	⬇️
revm	`1.42% <ø> (-31.12%)`	⬇️
payload builder	`6.53% <0.00%> (-0.06%)`	⬇️
primitives	`22.62% <32.55%> (-65.40%)`	⬇️

rkrasiuk

one more fix, otherwise lgtm! @mattsse PTAL

crates/rpc/rpc/src/eth/api/mod.rs

crates/transaction-pool/src/maintain.rs

rkrasiuk · 2023-07-31T07:58:59Z

@roberto-bayardo sanity_check test which calls full_validation seems to be failing

crates/primitives/src/header.rs

crates/consensus/common/src/validation.rs

roberto-bayardo · 2023-07-31T15:00:06Z

I tried to make everything consistent (with change_denomiator always preceeding elasticity) in my last commit but apparently missed updating next_block_base_fee ... thanks for the catch. fixed now.

rkrasiuk

one last thing, otherwise lgtm

crates/rpc/rpc/src/eth/api/mod.rs

crates/primitives/src/chain/spec.rs

mattsse

supportive, but I'd like to make this a bit more ergonomic,

we can also add the inverse of the next_base_fee via ChainSpec::next_base_fee(&block) that could also be convenient

crates/primitives/src/header.rs

crates/transaction-pool/src/maintain.rs

merklefruit · 2023-08-03T17:28:53Z

Closes #3750

mattsse

nice, only two pedantic nits

mattsse · 2023-08-07T12:49:51Z

crates/primitives/src/basefee.rs

they can be passed as copy

Suggested change

base_fee_params: &crate::BaseFeeParams,

base_fee_params: crate::BaseFeeParams,

done, also changed next_block_base_fee to accept copy instead of reference.

crates/primitives/src/basefee.rs

mattsse · 2023-08-07T12:50:16Z

crates/primitives/src/chain/spec.rs

Suggested change

#[derive(Serialize, Deserialize, Debug, Clone)]

#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq)]

thanks, also added "Copy" since we're now passing copies

crates/primitives/src/chain/spec.rs

mattsse

lgtm

roberto-bayardo marked this pull request as ready for review July 30, 2023 17:08

roberto-bayardo requested review from Rjected, gakonst, joshieDo, mattsse, onbjerg and rakita as code owners July 30, 2023 17:08

rkrasiuk added the A-op-reth Related to Optimism and op-reth label Jul 30, 2023

rkrasiuk suggested changes Jul 30, 2023

View reviewed changes

crates/consensus/auto-seal/src/task.rs Outdated Show resolved Hide resolved

crates/transaction-pool/src/maintain.rs Outdated Show resolved Hide resolved

crates/rpc/rpc/src/eth/api/fees.rs Outdated Show resolved Hide resolved

rkrasiuk added the C-enhancement New feature or request label Jul 30, 2023

rkrasiuk approved these changes Jul 31, 2023

View reviewed changes

crates/rpc/rpc/src/eth/api/mod.rs Outdated Show resolved Hide resolved

rkrasiuk reviewed Jul 31, 2023

View reviewed changes

crates/transaction-pool/src/maintain.rs Outdated Show resolved Hide resolved

rkrasiuk suggested changes Jul 31, 2023

View reviewed changes

crates/primitives/src/header.rs Outdated Show resolved Hide resolved

rkrasiuk reviewed Jul 31, 2023

View reviewed changes

crates/consensus/common/src/validation.rs Outdated Show resolved Hide resolved

onbjerg changed the title ~~make the base fee computation parameters configurable via chain spec~~ feat: make base fee computation parameters configurable via chain spec Jul 31, 2023

onbjerg assigned rkrasiuk Jul 31, 2023

onbjerg added the M-changelog This change should be included in the changelog label Jul 31, 2023

rkrasiuk approved these changes Jul 31, 2023

View reviewed changes

crates/rpc/rpc/src/eth/api/mod.rs Outdated Show resolved Hide resolved

roberto-bayardo requested a review from rkrasiuk July 31, 2023 23:42

joshieDo approved these changes Aug 1, 2023

View reviewed changes

crates/primitives/src/chain/spec.rs Outdated Show resolved Hide resolved

mattsse requested changes Aug 1, 2023

View reviewed changes

crates/primitives/src/header.rs Outdated Show resolved Hide resolved

crates/transaction-pool/src/maintain.rs Outdated Show resolved Hide resolved

roberto-bayardo mentioned this pull request Aug 2, 2023

use the parameters from the optimism config in 1559 base fee computation op-rs/op-reth#32

Merged

roberto-bayardo requested review from joshieDo and mattsse August 3, 2023 01:30

roberto-bayardo mentioned this pull request Aug 3, 2023

Revert "use the parameters from the optimism config in 1559 base fee computation" op-rs/op-reth#40

Merged

mattsse requested changes Aug 7, 2023

View reviewed changes

roberto-bayardo requested a review from mattsse August 7, 2023 14:22

make the base fee computation parameters configurable via chain spec

6365d49

mattsse approved these changes Aug 7, 2023

View reviewed changes

mattsse added this pull request to the merge queue Aug 7, 2023

Merged via the queue into paradigmxyz:main with commit 9569deb Aug 7, 2023

	base_fee_params: &crate::BaseFeeParams,
	base_fee_params: crate::BaseFeeParams,

	#[derive(Serialize, Deserialize, Debug, Clone)]
	#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq)]

Conversation

roberto-bayardo commented Jul 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rkrasiuk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rkrasiuk commented Jul 30, 2023

Uh oh!

codecov bot commented Jul 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

rkrasiuk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rkrasiuk commented Jul 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

roberto-bayardo commented Jul 31, 2023

Uh oh!

rkrasiuk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mattsse left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

merklefruit commented Aug 3, 2023

Uh oh!

mattsse left a comment

Choose a reason for hiding this comment

Uh oh!

mattsse Aug 7, 2023

Choose a reason for hiding this comment

Uh oh!

roberto-bayardo Aug 7, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mattsse Aug 7, 2023

Choose a reason for hiding this comment

Uh oh!

roberto-bayardo Aug 7, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mattsse left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

roberto-bayardo commented Jul 30, 2023 •

edited

Loading

codecov bot commented Jul 30, 2023 •

edited

Loading

rkrasiuk commented Jul 31, 2023 •

edited

Loading