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

intrinsic gas check #4867

Merged
merged 10 commits into from
Oct 4, 2023
Merged

intrinsic gas check #4867

merged 10 commits into from
Oct 4, 2023

Conversation

supernovahs
Copy link
Contributor

Ref #4504

@codecov
Copy link

codecov bot commented Oct 1, 2023

Codecov Report

Merging #4867 (7a96533) into main (d3cc4cc) will decrease coverage by 0.10%.
Report is 1 commits behind head on main.
The diff coverage is 7.35%.

Impacted file tree graph

Files Coverage Δ
crates/rpc/rpc/src/eth/revm_utils.rs 44.89% <100.00%> (ø)
crates/rpc/rpc/src/eth/error.rs 9.34% <0.00%> (-0.06%) ⬇️
crates/transaction-pool/src/error.rs 0.00% <0.00%> (ø)
crates/transaction-pool/src/test_utils/mock.rs 55.81% <0.00%> (-5.46%) ⬇️
crates/transaction-pool/src/traits.rs 12.07% <0.00%> (-0.28%) ⬇️
crates/primitives/src/transaction/access_list.rs 37.83% <30.76%> (-16.01%) ⬇️
crates/revm/revm-primitives/src/compat.rs 47.05% <0.00%> (-19.61%) ⬇️
crates/primitives/src/transaction/mod.rs 74.97% <0.00%> (-1.14%) ⬇️
crates/transaction-pool/src/validate/eth.rs 7.85% <0.00%> (-0.37%) ⬇️

... and 8 files with indirect coverage changes

Flag Coverage Δ
integration-tests 15.47% <7.35%> (-0.03%) ⬇️
unit-tests 62.62% <0.00%> (-0.09%) ⬇️

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

Components Coverage Δ
reth binary 32.58% <ø> (ø)
blockchain tree 80.64% <ø> (ø)
pipeline 88.45% <ø> (ø)
storage (db) 73.32% <ø> (ø)
trie 94.48% <ø> (ø)
txpool 48.90% <0.00%> (-0.73%) ⬇️
networking 76.07% <ø> (-0.09%) ⬇️
rpc 57.72% <50.00%> (-0.01%) ⬇️
consensus 61.05% <ø> (ø)
revm 28.47% <0.00%> (-0.07%) ⬇️
payload builder 8.16% <ø> (ø)
primitives 85.34% <15.38%> (-0.19%) ⬇️

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

thanks,

I want the check as separate function that does not use generics but bools for enabled hardforks instead

Comment on lines 159 to 180
let input = tx.input().as_ref();

let access_list: AccessList = match transaction.tx_type() {
LEGACY_TX_TYPE_ID => AccessList::default(),
EIP2930_TX_TYPE_ID => {
if let Some(access_list_tx) = tx.as_eip2930() {
access_list_tx.access_list.clone()
} else {
AccessList::default()
}
}
EIP1559_TX_TYPE_ID => {
if let Some(dynamic_fee_tx) = transaction.to_recovered_transaction().as_eip1559() {
dynamic_fee_tx.access_list.clone()
} else {
AccessList::default()
}
}
EIP4844_TX_TYPE_ID => {
if let Some(blob_tx) = transaction.to_recovered_transaction().as_eip4844() {
blob_tx.access_list.clone()
} else {
Copy link
Collaborator

Choose a reason for hiding this comment

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

we want this check as a new function in reth-revm-primitives

@@ -51,6 +51,21 @@ impl AccessList {
self.flatten().collect()
}

/// Converts the ['AccessList'] into a Vec of (Address, `Vec<U256>`), as expected by revm
pub fn to_ref_slice(&self) -> Vec<(Address, Vec<U256>)> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this does not return a slice

maybe fn as_flattened?

Copy link
Member

Choose a reason for hiding this comment

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

agree w/ as_flattened, I would also remain the other function to into_flattened and ensure as_flattened returns an iterator instead of collecting

@mattsse mattsse added the A-tx-pool Related to the transaction mempool label Oct 2, 2023
@supernovahs
Copy link
Contributor Author

To ensure I understand it correctly, we want the entire check to be a different function right? Or owe want the access list extraction as a separate function?
thanks

@mattsse
Copy link
Collaborator

mattsse commented Oct 2, 2023

the intrinsic gas check as separate function yes

@supernovahs
Copy link
Contributor Author

Looks like there is cyclic dependency between transaction-pool and revm-primitives. Maybe move the check to revm-inspectors instead? wdyt

@mattsse
Copy link
Collaborator

mattsse commented Oct 3, 2023

I had another look at this,

this can be simplified a lot by adding a new function in reth-revm-primitives that

does not use generics but bools for the enabled hardforks instead

and calls:

https://github.com/bluealloy/revm/blob/4e78fbe86ef4d030968d00f461926abbcb813943/crates/interpreter/src/gas/calc.rs#L344

we also want a new trait function of PoolTransaction that returns the access list to get rid of the match

@supernovahs
Copy link
Contributor Author

Realised that we didn't need the new flattened function since B160 was deprecated. So deleted it and using the existing functions.

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

this is great,

I made a few small changes

@mattsse mattsse enabled auto-merge October 4, 2023 12:23
@mattsse mattsse added this pull request to the merge queue Oct 4, 2023
@supernovahs
Copy link
Contributor Author

Amazed to see the modularity of the code

Merged via the queue into paradigmxyz:main with commit afebb2b Oct 4, 2023
24 checks passed
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants