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

vm: Discard blob txs with missing blobs for block building #2765

Merged
merged 4 commits into from
Jun 10, 2023

Conversation

g11tech
Copy link
Contributor

@g11tech g11tech commented Jun 8, 2023

makes sure that a block is not build with missing blobs (i.e. from non network wrapper tx)

TODO:

  • reject tx with missing blob in build block
    - [ ] ensure same in gossip TO be dealt in a different PR after discussion
  • recheck in rpc submission
  • enhance coverage

@codecov
Copy link

codecov bot commented Jun 8, 2023

Codecov Report

Merging #2765 (1bda1bf) into master (4fe13f4) will increase coverage by 0.09%.
The diff coverage is 94.73%.

Additional details and impacted files

Impacted file tree graph

Flag Coverage Δ
block 91.08% <ø> (ø)
blockchain 90.72% <ø> (ø)
client 87.48% <97.77%> (+0.22%) ⬆️
common 97.00% <ø> (ø)
devp2p 89.45% <ø> (+0.14%) ⬆️
ethash ∅ <ø> (∅)
evm 79.98% <ø> (ø)
rlp ?
statemanager 86.28% <ø> (ø)
trie 89.92% <ø> (ø)
tx 95.69% <ø> (ø)
util 81.13% <ø> (ø)
vm 82.89% <40.00%> (-0.12%) ⬇️

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

@g11tech g11tech force-pushed the remove-non-networkblobtx branch from dda32d4 to 1bda1bf Compare June 10, 2023 15:51
@@ -325,18 +325,6 @@ export class Miner {
`Miner: Assembled block full (gasLeft: ${gasLimit - blockBuilder.gasUsed})`
)
}
} else if ((error as Error).message.includes('tx has a different hardfork than the vm')) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is a case no longer required as we stopped throwing on tx have different hardfork than vm, but instead set it to vm's if vm's and block's hf match

@gabrocheleau
Copy link
Contributor

Looks good to me, can be merged once tests pass.

@g11tech g11tech merged commit cac6694 into master Jun 10, 2023
@holgerd77 holgerd77 deleted the remove-non-networkblobtx branch July 11, 2024 08:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants