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

block: add new consensusFormatValidation flag #2139

Merged
merged 9 commits into from Aug 17, 2022
Merged

Conversation

acolytec3
Copy link
Contributor

Fixes #2135

@codecov
Copy link

codecov bot commented Aug 15, 2022

Codecov Report

Merging #2139 (243164e) into master (d9b021d) will increase coverage by 84.33%.
The diff coverage is 100.00%.

Impacted file tree graph

Flag Coverage Δ
block 93.07% <100.00%> (?)
blockchain 88.82% <ø> (?)
client 86.67% <ø> (?)
common 98.09% <ø> (?)
devp2p 92.25% <ø> (?)
ethash ∅ <ø> (?)
evm 62.96% <ø> (?)
rlp ∅ <ø> (?)
statemanager 88.66% <ø> (?)
trie 89.27% <ø> (?)
tx 97.98% <ø> (?)
util 93.14% <ø> (?)
vm 86.17% <ø> (?)

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

@acolytec3 acolytec3 mentioned this pull request Aug 15, 2022
13 tasks
gabrocheleau
gabrocheleau previously approved these changes Aug 17, 2022
@@ -26,6 +26,7 @@ import { CLIQUE_EXTRA_SEAL, CLIQUE_EXTRA_VANITY } from './clique'
import type { BlockHeaderBuffer, BlockOptions, HeaderData, JsonHeader } from './types'
import type { CliqueConfig } from '@ethereumjs/common'


Copy link
Contributor

Choose a reason for hiding this comment

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

Extra space

@@ -1,3 +1,4 @@

Copy link
Contributor

Choose a reason for hiding this comment

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

Extra space

@@ -69,6 +69,10 @@ export interface BlockOptions {
* Will throw if provided on a non-PoA chain.
*/
cliqueSigner?: Buffer
/**
* Perform consensus validation checks on header if set. Defaults to true.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Perform consensus validation checks on header if set. Defaults to true.
* Perform consensus validation checks on header if set. Defaults to true.

(err: any) => err.message.includes('transactionsTrie must be 32 bytes'),
'throws on invalid transactionsTrie root hash length'
)
let nonce = Buffer.alloc(5)
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a big deal, but I find the nonce reassignment a bit weird. Could we just either scope it differently or directly assign fromHeaderData({nonce: Buffer.alloc(5)}) ?

@holgerd77
Copy link
Member

Just a short additional LGTM from my side as well, @acolytec3 thanks for addressing! 🙂

@acolytec3 acolytec3 merged commit f9a0471 into master Aug 17, 2022
@holgerd77 holgerd77 deleted the consensus-validation branch March 2, 2023 09:48
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.

Block: Add flag for consensus-specific format validation to Block Header
3 participants