Block validation methods, take 3 #1959

acolytec3 · 2022-06-14T18:52:24Z

Removes block.validate and blockheader.validate and moves the format/basic consensus checks into helper methods that are called in the respective constructors. These helpers throw if invalid fields are found
Creates two new methods in blockchain, validateBlock and validateHeader that are called when a block is inserted into the chain. All of the checks from the removed validate methods that require a blockchain context are now done in these methods.
Move validateUncles to `blockchain
Rewrite block tests to remove blockchain dependency where possible and moved to blockchain where not
Some test data was modified (notably modifying the extradata fields to be PoA compliant) to make it work with the tests
fixes qheap import issue related to Monorepo: Remove esModuleInterop and allowSyntheticDefaultImports Options (cherry-picked) #1975

acolytec3 · 2022-06-14T18:54:28Z

@holgerd77 here's my WIP on the block validation methods. block.spec.ts tests don't pass at present since I need to address the changes made with moving the header validation methods around but I've taken the first step of moving header validation to blockchain and am going to focus on the difficulty checks next. Blockchain tests are all currently passing so hopefully won't have to adjust them too much more once I finish the difficulty check updates.

acolytec3 · 2022-06-14T19:05:19Z

Correction: all the blockchain tests are passing locally. CI is failing because I haven't addressed the broader monorepo yet and was focused on getting things working between block and blockchain and would then address other places where build/tests are failing.

codecov · 2022-06-15T18:21:22Z

Codecov Report

Merging #1959 (9e08602) into master (c5fb316) will increase coverage by 1.78%.
The diff coverage is 82.32%.

Flag	Coverage Δ
block	`84.30% <85.41%> (-1.86%)`	⬇️
blockchain	`81.01% <78.35%> (-0.56%)`	⬇️
client	`78.34% <100.00%> (?)`
common	`95.01% <ø> (ø)`
devp2p	`82.78% <ø> (ø)`
ethash	`90.81% <ø> (ø)`
evm	`42.62% <ø> (ø)`
rlp	`?`
statemanager	`84.55% <ø> (ø)`
trie	`73.72% <ø> (ø)`
tx	`92.17% <ø> (ø)`
util	`87.03% <ø> (ø)`
vm	`76.87% <100.00%> (+0.03%)`	⬆️

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

holgerd77 · 2022-06-16T10:13:36Z

packages/block/src/block.ts

-    await this.header.validate(blockchain)
-    await this.validateUncles(blockchain)
-    await this.validateData(onlyHeader)
-  }


I think I wrote otherwise in the original issue but I guess we really want to keep this combined version (which would also solve a related client-TODO) in a public Blockchain.validateBlock() method.

The thought being we just want to be able to validate a new block in it's entirety without having to actually put it in the chain?

Ah no, my thought was actually only to have an atomic (and also: more convenient) validate()method (as before) since this already appeared as a use case in client (where you wrote your TODO).

Does this otherwise have any affects on the "put it on the chain or not" question?

Without having had a look into the current state of the code: it would generally be good for these methods if we can call in for a child block (so: the block to be validated) which not necessarily would need to be in the chain and only the parent block being expected to be in the chain.

Does this make sense?

(or even better in the cases where this is possible: both doesn't need to be in the chain, so these would be the cases where a method could even be called in a static context)

I've added a public blockchain.validateBlock method that wraps up those three validation checks. I don't think we can make it a static method without breaking it into further pieces since some the checks are heavily dependent on the existence of the chain.

acolytec3 · 2022-06-18T18:33:14Z

@holgerd77 This PR is now ready for review. I've revised the initial description to reflect the changes made.

Thanks @g11tech for helping track down some of the trickier client test failures!

g11tech

LGTM!

holgerd77

Thanks, looks already pretty great, made some comments and change requests on things like structure, (method) order, naming and visibility of things.

packages/block/src/header.ts

packages/blockchain/src/consensus/clique.ts

packages/block/src/header.ts

packages/block/src/block.ts

packages/blockchain/src/consensus/casper.ts

holgerd77 · 2022-06-20T11:20:49Z

packages/blockchain/src/consensus/clique.ts

+// Block difficulty for in-turn signatures
+export const CLIQUE_DIFF_INTURN = BigInt(2)
+// Block difficulty for out-of-turn signatures
+export const CLIQUE_DIFF_NOTURN = BigInt(1)


Can we change the order of constants and types here (or is it just me who feel that constants would belong to the top?) and then also put all exported constants grouped together first?

holgerd77 · 2022-06-20T11:28:56Z

packages/blockchain/src/index.ts

+
+    if (header._common.consensusType() === 'pow') {
+      await this.consensus.validateDifficulty(header)
+    }


I guess we can (and should) remove this PoW check here, to make sure this works in a generic context.

This one is super tricky I've discovered. Because this.consensus is the blockchain's current consensus mechanism, this fails on merge transition blocks where the header is now PoS but the blockchain is still PoW (since the blockchain hasn't switched to PoS so blockchain.consensus is ethashConsensus. We should skip this check on the merge block because the PoS difficulty check (i.e. PoS blocks must always have difficulty 0) is done in the block constructor via `block._consensusValidation) and it will always fail if you pass in a new header that's a PoS block but the current chain is either still PoA or PoW. I'll check for PoS instead and skip if so.

holgerd77 · 2022-06-20T11:31:37Z

packages/blockchain/src/index.ts

+      }
+      // Validate clique difficulty
+      await this.consensus.validateDifficulty(header)
+    }


Now we have this one consensus-related check back in here which is unlucky. Wonder if there is a good place in consensus we can safely move.

Maybe in the clique.validate() method as well? 🤔 Might need a double check if the semantic result would be somewhat matching/equivalent.

But we should really target to remove here.

packages/blockchain/src/index.ts

packages/vm/examples/run-solidity-contract.ts

packages/vm/tests/api/runTx.spec.ts

gabrocheleau

Left some minor comments, look good to me overall.

acolytec3 · 2022-06-20T19:25:23Z

Left some minor comments, look good to me overall.

Thanks for the detailed review! Will address all this cleanup.

acolytec3 · 2022-06-20T19:46:12Z

Please don't merge this. I've got to fix the client tests, again.

holgerd77 · 2022-06-21T07:51:22Z

@acolytec3 thanks a lot for addressing all the feedback, this looks really great! 👍

Decide whether block.validateTransactions

Yes, I guess we should not throw here.

Decide if we should add a flag to the block/blockchain constructors to allow the validation checks to be bypassed (like the validateBlocks parameter on the blockchain constructor

Maybe I am missing something but there is a validateBlocks parameter for Blockchain? 🤔

There is also a validateConsensus flag, looking at that part I realized that we should rename the consensus.validataBlockData() methods to consensus.validateConsensus(), this makes things a lot more clear.

acolytec3 · 2022-06-21T10:25:15Z

Maybe I am missing something but there is a validateBlocks parameter for Blockchain? thinking

There is also a validateConsensus flag, looking at that part I realized that we should rename the consensus.validataBlockData() methods to consensus.validateConsensus(), this makes things a lot more clear.

The reason I ask the question is we have format level consensus checks in the blockheader constructor that require things like PoA blocks have to have minimum 97 bytes of extraData which in turn means we have to populate these fields when we're instantiating blocks. Do we want to have a validateConsensus flag as part of the blockOpts so these checks can be bypassed the way we support it at the blockchain level. At the moment, these checks are enforced unless you stub them out in the test itself (with occasionally frustrating results trying to triage why a test is breaking)

holgerd77 · 2022-06-21T10:35:47Z

The reason I ask the question is we have format level consensus checks in the blockheader constructor that require things like PoA blocks have to have minimum 97 bytes of extraData which in turn means we have to populate these fields when we're instantiating blocks. Do we want to have a validateConsensus flag as part of the blockOpts so these checks can be bypassed the way we support it at the blockchain level. At the moment, these checks are enforced unless you stub them out in the test itself (with occasionally frustrating results trying to triage why a test is breaking)

Yes, I guess we want but can you leave it out here and instead open a short issue, since this is not breaking and we can add later (if I am not mistaken).

I will merge in #1974 once CI passes. Can you prioritize on bringing this branch up to date then so that we can merge in here as well? I would want to have the big PRs out of the way.

ugh. 🙂

Thanks (I am getting a bit stressed 😋).

…thods

holgerd77

LGTM, great work 🎉, will merge.

holgerd77 · 2022-06-22T09:06:27Z

packages/block/src/header.ts

   *
   * @param msg Base error message
   * @hidden
   */
-  protected _errorMsg(msg: string) {
+  private _errorMsg(msg: string) {


Small nit here, this should remain protected to allow for sub-classing.

Got it. Adding this to #1982

holgerd77 · 2022-06-22T09:08:07Z

packages/block/src/block.ts

@@ -524,7 +389,7 @@ export class Block {
   * @param msg Base error message
   * @hidden
   */
-  protected _errorMsg(msg: string) {
+  private _errorMsg(msg: string) {


acolytec3 marked this pull request as draft June 14, 2022 18:52

acolytec3 added package: block package: blockchain labels Jun 14, 2022

acolytec3 linked an issue Jun 14, 2022 that may be closed by this pull request

Block: Directly throw in validation functions #1889

Closed

acolytec3 added package: vm package: ethash package: client PR state: WIP labels Jun 15, 2022

holgerd77 reviewed Jun 16, 2022

View reviewed changes

acolytec3 force-pushed the block-validation-methods branch 2 times, most recently from 674919b to a268fb2 Compare June 16, 2022 20:44

acolytec3 added PR state: needs review and removed PR state: WIP labels Jun 18, 2022

acolytec3 marked this pull request as ready for review June 18, 2022 18:32

acolytec3 force-pushed the block-validation-methods branch from 1273a87 to 9214ce4 Compare June 19, 2022 02:12

g11tech previously approved these changes Jun 19, 2022

View reviewed changes

holgerd77 requested changes Jun 20, 2022

View reviewed changes

acolytec3 dismissed g11tech’s stale review via ec3f96b June 20, 2022 14:05

acolytec3 added 8 commits June 20, 2022 10:50

rename _validateHeaderFields

5785f2f

Move static consensus checks

44431b2

block: fix header consensus validation tests

410a3f4

Move header.validate to blockhain

6aea854

blockchain: fix tests related to headerValidate

cb83caa

move validateDifficulty to blockchain

0469fe7

block: fix difficulty tests

f12d9bd

move blockchain dependendent validateUncle checks

a30c462

gabrocheleau reviewed Jun 20, 2022

View reviewed changes

packages/blockchain/src/index.ts Show resolved Hide resolved

gabrocheleau reviewed Jun 20, 2022

View reviewed changes

packages/blockchain/src/index.ts Show resolved Hide resolved

gabrocheleau reviewed Jun 20, 2022

View reviewed changes

packages/vm/examples/run-solidity-contract.ts Show resolved Hide resolved

gabrocheleau reviewed Jun 20, 2022

View reviewed changes

packages/vm/tests/api/runTx.spec.ts Show resolved Hide resolved

gabrocheleau previously approved these changes Jun 20, 2022

View reviewed changes

acolytec3 dismissed gabrocheleau’s stale review via 337c0a0 June 20, 2022 19:45

acolytec3 added the PR state: do-not-merge label Jun 20, 2022

acolytec3 added 2 commits June 20, 2022 20:26

clean up commented stubs

13171b4

remove lint rule

ddeaa8b

acolytec3 force-pushed the block-validation-methods branch from 337c0a0 to ddeaa8b Compare June 21, 2022 00:36

client: fix integration test

4d17ef9

blockchain: rename validation method

570843e

acolytec3 force-pushed the block-validation-methods branch from c0f3b1f to 570843e Compare June 21, 2022 12:22

holgerd77 mentioned this pull request Jun 21, 2022

Block: low-level validation methods refactor #1954

Closed

acolytec3 added 2 commits June 21, 2022 10:07

Merge remote-tracking branch 'origin/master' into block-validation-me…

b479b4f

…thods

Update imports, fix qheap

9e08602

acolytec3 removed the PR state: do-not-merge label Jun 21, 2022

acolytec3 mentioned this pull request Jun 21, 2022

Blockchain structure reorg #1978

Closed

holgerd77 approved these changes Jun 21, 2022

View reviewed changes

holgerd77 merged commit 1bd5c4f into master Jun 21, 2022

holgerd77 deleted the block-validation-methods branch June 21, 2022 16:09

holgerd77 reviewed Jun 22, 2022

View reviewed changes

acolytec3 mentioned this pull request Jun 24, 2022

Uncle Validation Test #1998

Closed

acolytec3 mentioned this pull request Jul 9, 2022

Fix consensus validation bugs #2031

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Block validation methods, take 3 #1959

Block validation methods, take 3 #1959

acolytec3 commented Jun 14, 2022 •

edited

Loading

acolytec3 commented Jun 14, 2022

acolytec3 commented Jun 14, 2022

codecov bot commented Jun 15, 2022 •

edited

Loading

holgerd77 Jun 16, 2022

acolytec3 Jun 16, 2022

holgerd77 Jun 16, 2022

holgerd77 Jun 16, 2022

acolytec3 Jun 16, 2022

acolytec3 commented Jun 18, 2022

g11tech left a comment

holgerd77 left a comment

holgerd77 Jun 20, 2022

holgerd77 Jun 20, 2022

acolytec3 Jun 20, 2022

holgerd77 Jun 20, 2022

gabrocheleau left a comment

acolytec3 commented Jun 20, 2022

acolytec3 commented Jun 20, 2022

holgerd77 commented Jun 21, 2022

acolytec3 commented Jun 21, 2022

holgerd77 commented Jun 21, 2022

holgerd77 left a comment

holgerd77 Jun 22, 2022

acolytec3 Jun 22, 2022

holgerd77 Jun 22, 2022

Block validation methods, take 3 #1959

Block validation methods, take 3 #1959

Conversation

acolytec3 commented Jun 14, 2022 • edited Loading

acolytec3 commented Jun 14, 2022

acolytec3 commented Jun 14, 2022

codecov bot commented Jun 15, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acolytec3 commented Jun 18, 2022

g11tech left a comment

Choose a reason for hiding this comment

holgerd77 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrocheleau left a comment

Choose a reason for hiding this comment

acolytec3 commented Jun 20, 2022

acolytec3 commented Jun 20, 2022

holgerd77 commented Jun 21, 2022

acolytec3 commented Jun 21, 2022

holgerd77 commented Jun 21, 2022

holgerd77 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acolytec3 commented Jun 14, 2022 •

edited

Loading

codecov bot commented Jun 15, 2022 •

edited

Loading