Block: support uncle validation #935

jochem-brouwer · 2020-10-31T15:37:19Z

Closes #933

This PR fixes the following TODO in block:

    // TODO: Validate that the uncle header hasn't been included in the blockchain yet.
    // This is not possible in ethereumjs-blockchain since this PR was merged:
    // https://github.com/ethereumjs/ethereumjs-blockchain/pull/47

codecov · 2020-10-31T15:38:26Z

Codecov Report

Merging #935 (6286888) into master (9b86316) will increase coverage by 0.95%.
The diff coverage is 79.48%.

Flag	Coverage Δ
block	`77.69% <79.48%> (+2.65%)`	⬆️
blockchain	`77.39% <ø> (ø)`
common	`91.87% <ø> (ø)`
ethash	`82.08% <ø> (ø)`
tx	`86.04% <ø> (-0.22%)`	⬇️
vm	`87.21% <ø> (ø)`

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

holgerd77 · 2020-11-09T13:23:57Z

Ok, actually 2nd last "biggie" I am getting aware. 🙃 Is this intended to go into v5 as well? Likely, right?

jochem-brouwer · 2020-11-09T13:39:25Z

Hi @holgerd77, I have created a seperate PR which handles the requirements for this specific PR: this is #942. The uncle handling logic is rather complex; the actual logic itself depends on changing the current interface of blocks' validate method to make the blockchain argument mandatory instead of optional; this is thus in #942. I have marked those as "P2 very important" to indicate those should be in V5.

holgerd77 · 2020-11-09T13:42:04Z

Ok, great, than please rebase #942 as soon as you'll have some time.

jochem-brouwer · 2020-11-09T13:43:07Z

Ah yes good point, will do now.

jochem-brouwer · 2020-11-09T22:11:58Z

Wrote a massive amount of uncle tests here, which I think are also great because of the educational value. This should cover all rules regarding uncles 😄

I hope the VM tests pass now.

jochem-brouwer · 2020-11-09T22:32:07Z

VM tests fail but this is due to the linter? I haven't touched VM here so the linter should pass? Does maybe ESLint here have a different version or something? @evertonfraga

Anyways this is ready for review 👍 😄

jochem-brouwer · 2020-11-10T13:36:03Z

It would be great if we can get this in the RC release, as this completes the validation logic of block 😄

Will rebase.

block: fix validate uncles

block: fix option carry-over bug in uncle headers block: remove comment, add changes to changelog

jochem-brouwer · 2020-11-10T21:32:41Z

OK, ready for review again!

packages/block/src/block.ts

holgerd77

This seems to be a complex topic which needs to be handled with some care, not sure if this can still make it into the RC release, we'll see.

Have some starter questions.

holgerd77 · 2020-11-11T08:25:10Z

packages/block/src/block.ts

+
+    // validate the header
+    for (let i = 0; i < uncleHeaders.length; i++) {
+      const header = uncleHeaders[i]


Also not a blocker, but I find for (const uncleHeader of uncleHeaders) more elegant and simple.

I did all the others without a loop and instead with a map, don't know why I used a for loop here. Note that for(const x of y) is something that ESLint does not like (we have ignored that rule). I don't know why ESLint doesn't like that syntax.

Ah right, I can't directly use map because it calls an async function.

Have rewritten this using map and Promise.all, personally, I like it 😄

holgerd77 · 2020-11-11T08:33:02Z

packages/block/src/block.ts

+      parentHash = parentBlock.header.parentHash
+    }
+
+    canonicalBlockMap.map((block) => {


It would be good if these different validation sections in _validateUncleHeaders get some short comment headers (maybe analogue to the already commented validateUncles starting section) to structure this a bit and grasp what is going on.

Yes good point, will do this.

Have squashed this logic using some helpers, should be easier to read now 😄

packages/block/src/block.ts

holgerd77 · 2020-11-11T08:35:32Z

packages/block/src/block.ts

+      }
+    })
+
+    const getBlocks = this.header.number.clone().sub(lowestUncleNumber).addn(1).toNumber()


If the lowestUncleNumber here is e.g. 1 for whatever reason the following loop would go and re-read the whole blockchain. This can't be right I would assume? 🤔

I am not seeing why this would read the whole blockchain. Please note that in validating the headers, it is checked that the uncle is at most 8 blocks older, so this should put an upper bound of the amount of blocks being read.

Thanks, I wasn't aware of this check in uncle header validation.

jochem-brouwer · 2020-11-11T15:04:32Z

Okay, upon re-reading the code after your comments @holgerd77 I think I can simplify this code a bit more by introducing some helper variables. I am using map and filter in this validation function but I think I can just make this a bit simpler by creating some lookup tables which indicate if certain hashes match certain properties or not. Will update soon including moving some of the comments around / writing some more comments.

jochem-brouwer · 2020-11-11T21:09:20Z

Okay, just updated this one, moved some comments around and made the validation logic better readable (I hope) 😄 . Ready for re-review.

holgerd77

Thanks @jochem-brouwer, this code is now super-clean and really a great read! 👍

One structural suggestion on combining the methods but we can also directly proceed if you don't have time for the change right now and would rather like to go on.

holgerd77 · 2020-11-16T10:22:36Z

packages/block/src/block.ts

-    for (const uh of this.uncleHeaders) {
-      await this._validateUncleHeader(uh, blockchain)
-    }
+    await this._validateUncleHeaders(this.uncleHeaders, blockchain)


On a second look I don't think it makes any sense to keep this double public/private method structure with validateUncles() and _validateUncleHeaders(), these double this.uncleHeaders.length checks (one on > 2, the other on === 0) rather makes this in the current split form more inconsistent, do we want to throw this into one function? 🤔 This double documentation is also a bit "redundancy for nothing" for my taste.

holgerd77 · 2020-11-16T10:24:49Z

packages/block/src/block.ts

+      }
+    })
+
+    const getBlocks = this.header.number.clone().sub(lowestUncleNumber).addn(1).toNumber()


Thanks, I wasn't aware of this check in uncle header validation.

holgerd77 · 2020-11-16T10:26:11Z

packages/block/src/block.ts

+    // Helper variable: set hash to `true` if uncle hash is included in any canonical block
+    const includedUncles: { [key: string]: boolean } = {}
+
+    // Due to the header validation check above, we know that `getBlocks` it between 1 and 8 inclusive.


Super nit-pick: "it" -> "is"

holgerd77

On a second tought: this method combination question is such an internal thing, have added this to the tiny-fixes-issue and will merge here now, so we keep a blank slate for the releases.

jochem-brouwer added PR state: WIP PR state: do-not-merge package: block labels Oct 31, 2020

jochem-brouwer force-pushed the blockchain-concurrency-and-safety-improvements branch 5 times, most recently from 4db67e9 to 12f0dcb Compare November 8, 2020 13:11

Base automatically changed from blockchain-concurrency-and-safety-improvements to master November 9, 2020 13:21

jochem-brouwer force-pushed the validate-uncles branch from e435940 to cbec91f Compare November 9, 2020 22:10

jochem-brouwer changed the base branch from master to block-blockchain-validation-mandatory November 9, 2020 22:11

jochem-brouwer changed the title ~~block: preliminary work on validating uncles~~ Block: support uncle validation Nov 9, 2020

jochem-brouwer marked this pull request as ready for review November 9, 2020 22:32

jochem-brouwer added PR state: needs review and removed PR state: WIP labels Nov 9, 2020

Base automatically changed from block-blockchain-validation-mandatory to master November 10, 2020 08:18

jochem-brouwer removed the PR state: do-not-merge label Nov 10, 2020

holgerd77 mentioned this pull request Nov 10, 2020

Tx Constructor Restructuring #944

Merged

jochem-brouwer added 2 commits November 10, 2020 22:06

block: validate uncle headers

c9b624a

block: fix validate uncles

block: add uncle validation tests

ecb1064

block: fix option carry-over bug in uncle headers block: remove comment, add changes to changelog

jochem-brouwer force-pushed the validate-uncles branch from cbec91f to ecb1064 Compare November 10, 2020 21:06

jochem-brouwer requested review from holgerd77 and ryanio November 10, 2020 21:32

holgerd77 reviewed Nov 11, 2020

View reviewed changes

packages/block/src/block.ts Show resolved Hide resolved

holgerd77 reviewed Nov 11, 2020

View reviewed changes

packages/block/src/block.ts Outdated Show resolved Hide resolved

holgerd77 reviewed Nov 11, 2020

View reviewed changes

jochem-brouwer mentioned this pull request Nov 11, 2020

Blockchain/Block: move Block validation methods to Blockchain #947

Closed

block: introduce helpers in validate uncles, add comments

397bb29

Merge branch 'master' into validate-uncles

67f9719

holgerd77 reviewed Nov 16, 2020

View reviewed changes

Merge branch 'master' into validate-uncles

6286888

holgerd77 mentioned this pull request Nov 16, 2020

Tiny Fixes Meta Issue (old) #905

Closed

18 tasks

holgerd77 approved these changes Nov 16, 2020

View reviewed changes

holgerd77 merged commit 81fc88b into master Nov 16, 2020

holgerd77 deleted the validate-uncles branch November 16, 2020 11:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Block: support uncle validation #935

Block: support uncle validation #935

jochem-brouwer commented Oct 31, 2020 •

edited

codecov bot commented Oct 31, 2020 •

edited

holgerd77 commented Nov 9, 2020

jochem-brouwer commented Nov 9, 2020

holgerd77 commented Nov 9, 2020

jochem-brouwer commented Nov 9, 2020

jochem-brouwer commented Nov 9, 2020

jochem-brouwer commented Nov 9, 2020

jochem-brouwer commented Nov 10, 2020

jochem-brouwer commented Nov 10, 2020

holgerd77 left a comment

holgerd77 Nov 11, 2020

jochem-brouwer Nov 11, 2020

jochem-brouwer Nov 11, 2020

jochem-brouwer Nov 11, 2020

holgerd77 Nov 11, 2020

jochem-brouwer Nov 11, 2020

jochem-brouwer Nov 11, 2020

holgerd77 Nov 11, 2020

jochem-brouwer Nov 11, 2020

holgerd77 Nov 16, 2020

jochem-brouwer commented Nov 11, 2020

jochem-brouwer commented Nov 11, 2020

holgerd77 left a comment

holgerd77 Nov 16, 2020

holgerd77 Nov 16, 2020

holgerd77 Nov 16, 2020

holgerd77 left a comment

Block: support uncle validation #935

Block: support uncle validation #935

Conversation

jochem-brouwer commented Oct 31, 2020 • edited

codecov bot commented Oct 31, 2020 • edited

Codecov Report

holgerd77 commented Nov 9, 2020

jochem-brouwer commented Nov 9, 2020

holgerd77 commented Nov 9, 2020

jochem-brouwer commented Nov 9, 2020

jochem-brouwer commented Nov 9, 2020

jochem-brouwer commented Nov 9, 2020

jochem-brouwer commented Nov 10, 2020

jochem-brouwer commented Nov 10, 2020

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

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

Choose a reason for hiding this comment

jochem-brouwer commented Nov 11, 2020

jochem-brouwer commented Nov 11, 2020

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

holgerd77 left a comment

Choose a reason for hiding this comment

jochem-brouwer commented Oct 31, 2020 •

edited

codecov bot commented Oct 31, 2020 •

edited