Blockchain concurrency and safety improvements #930

jochem-brouwer · 2020-10-28T21:49:18Z

WIP. Cherry-picked from #895 and builds upon #927

Closes #690

This PR aims to:

Fix concurrency issues when using the Blockchain (getters/setters now all wait)
Document the library
Make variable names more correct
Make the library more readable (more docs, some (unnecessary?) recursion removed)

Also security:

Genesis block should be put in the Blockchain constructor to prevent accidentally wiping the Genesis block from the DB / Tracking a completely other network (!)
This also means that putGenesis is now not possible anymore. You set Genesis in constructor and it cannot change after that.
Safe static constructor which throws if the init method throws

codecov · 2020-10-28T21:50:22Z

Codecov Report

Merging #930 (00ef9cf) into master (7371fdf) will increase coverage by 0.55%.
The diff coverage is 79.31%.

Flag	Coverage Δ
block	`74.71% <79.20%> (+1.26%)`	⬆️
blockchain	`78.04% <79.20%> (+2.70%)`	⬆️
common	`92.03% <ø> (ø)`
ethash	`82.08% <ø> (ø)`
tx	`88.37% <ø> (-0.18%)`	⬇️
vm	`87.21% <100.00%> (+0.03%)`	⬆️

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

packages/blockchain/src/index.ts

jochem-brouwer · 2020-10-28T22:58:10Z

There is a problem here when creating a DB and then trying to instantiate the Blockchain which should initialize, reading from this DB. Either the order is wrong in _init() or we are actually testing something which we do not want to test. Will investigate ASAP.

jochem-brouwer · 2020-10-30T22:39:45Z

packages/blockchain/src/index.ts

-    }
+    // Q: is it safe to make this not wait for a lock?
+    // this is called from `runBlockchain` in case `runBlock` throws (i.e. the block is invalid). But is this the way to go?
+    // If we know this is called from the iterator/runBlockchain we are safe, but if this is called from anywhere else then this might lead to a concurrency problem?


delBlock has no lock because VM calls it for deletion... do we want this? The alternative here is to delete the block and halt the iterator if the onBlock function in the iterator throws. We can thus internally call _delBlock and can put a lock on this one, which I think is safer. Then the only function without a lock is getBlock, which should be fine as the iterator blocks any put operation until it is done (so BLOCKHASH will read from current canonical chain)

jochem-brouwer · 2020-11-05T21:13:37Z

Rebased this upon the latest changes of #927

jochem-brouwer · 2020-11-06T19:05:51Z

OK - at this point the logic of this PR is done. I will update a bit more with some tests, which besides verifying the code is correct also serves a more explanatory role.

At this point the logic of this PR can already be reviewed. Since I've moved a lot of code around (without changing it) the diff looks rather big here: it is easier to review this per commit. If the commit is rather large there's a big chance there's code moved around (but might still change the code).

packages/vm/lib/index.ts

holgerd77 · 2020-11-06T21:30:26Z

packages/blockchain/src/index.ts

-   * Fetches the meta info about the blockchain from the db. Meta info contains
-   * hashes of the headerchain head, blockchain head, genesis block and iterator
-   * heads.
+   * This method is called in the constructor and either setups the DB or reads values from the DB and makes these available to the consumers of Blockchain.


Please keep the length of comment lines roughly to the line length of the code blocks (I am actually wondering why this is not done by linting automatically?). This is otherwise not very well readable, on my MacBook Air it e.g. completely goes out of screen.

I will manually lint those, thought it indeed would be done by linter as well

(this applies to most of the comments added)

So printWidth setting in the linting config is set to 100, for orientation

I will manually lint those, thought it indeed would be done by linter as well

??? Can't you then just run the linter or is it not working for you locally?

Just checked, if I run npm run lint:fix doesn't change anything here, I'd assume it would break up those comments.

Seems like linter does not fix it. I have installed this Rewrap extension in VSCode which helps wrapping the comments. Wrapped comments now in 4db67e9

holgerd77 · 2020-11-06T21:34:18Z

packages/blockchain/src/index.ts

+    }
+
+    // eslint-disable-next-line @typescript-eslint/no-floating-promises
+    this.initPromise = this._init(opts.genesisBlock)


I think I find the pattern we have in the VM now with this init() method to be called from outside the constructor more intuitive than this solution, do we eventually want to apply this here as well also for consistency reasons? 🤔

I can apply this here, I just think it makes more sense to just let this Promise run in advance and not call this Promise at the point we actually need it to be initialized. But for consistency we can instead use the pattern from VM - let me know what you think 😄

I think it's ok as it is now on reconsideration. I think I was just irritated by the external usage of initPromise within the VM, but this was before I realized that this is somewhat of a hack due to the Blockchain initialization in the VM constructor.

packages/blockchain/src/index.ts

holgerd77 · 2020-11-06T21:50:08Z

packages/vm/lib/index.ts

@@ -225,6 +232,8 @@ export default class VM extends AsyncEventEmitter {
      return
    }

+    await this.blockchain.initPromise


Can't you just use your static constructor here?

Update: ah, got it, Blockchain is created in the constructor.

I will remove this since all public blockchain methods await this one internally; just bloats the logic here.

Never mind; if you don't await this Promise then after init VM might be initialized, but Blockchain might not be initialized.

packages/blockchain/src/db/helpers.ts

…te, fix tests

vm, blockchain: lint

…rsive calls

…tead of recursive calls

blockchain: fix concurrency issues blockchain: fix test blockchain: fix infinite while loop blockchain: delete `putGenesis` blockchain: more comments blockchain: rename/remove some unused/confusing variables blockchain: fix reorg test

blockchain: fix rebase, fix docs vm: fix example blockchain: fix tests

blockchain: import bn from ethereumjs-util vm: await initPromise of blockchain to ensure blockchain is initialized

jochem-brouwer · 2020-11-08T22:35:30Z

So just to recap here, the logic of this PR changes: constructing the DB and the concurrency things. I have written the tests for the constructor.

Note that if you pass a custom genesis block, it is mandatory to also pass this again when re-creating the DB (but now from a given database). This ensures that the user is aware that they are using a custom genesis block. I think this is much safer than "just reading the genesis from the DB": users might take a long time to figure out they are on the wrong chain.

The concurrency is a bit hard to test and I'd need some suggestions here, because I find myself writing rather complex tests with a lot of Promises in them. I do think though that at this point this is correct, but it should be tested anyways.

In general I think at this point the PR is ready for review.

holgerd77 · 2020-11-09T10:54:50Z

… inlined _getCanonicalGenesisBlock() function

holgerd77

I am now through with this, hanks Jochem, this really looks fantastic and really is a milestone regarding the readability of this library and will give us a super-solid base for further client work! 👍 😄 Would nevertheless also assume that this library will further evolve.

I've done a per-commit review and pushed a small interface fix, a new CHANGELOG entry and some doc improvements and I think the review was thorough enough to justify a merge, also considering our timeline for the releases. I was a bit loose on the last commits, particularly f6498a0, 87b069b and 24f2d54. If someone wants to do a post-merge-review here this would be encouraged.

Ok: last "biggie". 😄

jochem-brouwer · 2020-11-09T13:30:02Z

Great, thanks a lot @holgerd77, thanks for the interface fix and also for writing the changelog 😄 👍 Very happy to see this merged!

jochem-brouwer changed the base branch from master to remove-db-coupling-blockchain October 28, 2020 21:50

jochem-brouwer added package: blockchain PR state: WIP labels Oct 28, 2020

jochem-brouwer marked this pull request as draft October 28, 2020 21:50

jochem-brouwer mentioned this pull request Oct 28, 2020

Declare fields and methods starting with _ as private #690

Closed

jochem-brouwer added the PR state: do-not-merge label Oct 28, 2020

jochem-brouwer commented Oct 28, 2020

View reviewed changes

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

jochem-brouwer mentioned this pull request Oct 30, 2020

Block: validate if uncle not included in canonical chain #933

Closed

jochem-brouwer commented Oct 30, 2020

View reviewed changes

jochem-brouwer mentioned this pull request Oct 31, 2020

Block: support uncle validation #935

Merged

holgerd77 force-pushed the remove-db-coupling-blockchain branch from 07c5f70 to 9f8ff31 Compare November 3, 2020 11:08

jochem-brouwer force-pushed the remove-db-coupling-blockchain branch from 9f8ff31 to d747941 Compare November 5, 2020 19:36

jochem-brouwer force-pushed the blockchain-concurrency-and-safety-improvements branch from 652d9a1 to e2f60e4 Compare November 5, 2020 21:12

jochem-brouwer force-pushed the remove-db-coupling-blockchain branch from d747941 to a673fdc Compare November 5, 2020 21:14

holgerd77 force-pushed the remove-db-coupling-blockchain branch from a673fdc to 57bcd88 Compare November 6, 2020 09:13

Base automatically changed from remove-db-coupling-blockchain to master November 6, 2020 09:36

jochem-brouwer force-pushed the blockchain-concurrency-and-safety-improvements branch 3 times, most recently from 4757523 to 8296f13 Compare November 6, 2020 18:59

holgerd77 reviewed Nov 6, 2020

View reviewed changes

packages/vm/lib/index.ts Outdated Show resolved Hide resolved

holgerd77 reviewed Nov 6, 2020

View reviewed changes

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

holgerd77 reviewed Nov 6, 2020

View reviewed changes

ryanio reviewed Nov 6, 2020

View reviewed changes

packages/blockchain/src/db/helpers.ts Outdated Show resolved Hide resolved

jochem-brouwer added the prio: P2 very important label Nov 8, 2020

jochem-brouwer added 13 commits November 8, 2020 14:04

blockchain: throw early on errors

c25ca77

blockchain: add docs, rename internal functions, make functions priva…

07aee69

…te, fix tests

blockchain: add helper functions for initializing and locks

131e411

blockchain: if non-canonical genesis block, provide this is constructor

5f959ed

vm, blockchain: lint

blockchain: genesis block options checks, static factory

1e35eb5

blockchain: extra DB helper functions

c331ca8

blockchain: make rebuildCanonical use a do-while loop instead of recu…

f6498a0

…rsive calls

blockchain: make _deleteCanonicalChainReferences use a while loop ins…

87b069b

…tead of recursive calls

blockchain: add safeNumberToHash helper method

e93b8ff

blockchain: move reorg / helper functions to bottom

e69b8f1

blockchain: make all private fns private

c3d4d6b

blockchain: fix rebase, fix docs vm: fix example blockchain: fix tests

blockchain: update docs

12f0dcb

blockchain: import bn from ethereumjs-util vm: await initPromise of blockchain to ensure blockchain is initialized

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

blockchain: add initialization tests

43ef512

jochem-brouwer added PR state: needs review and removed PR state: do-not-merge PR state: WIP labels Nov 8, 2020

holgerd77 marked this pull request as ready for review November 9, 2020 10:38

holgerd77 added 2 commits November 9, 2020 12:26

blockchain: removed isGenesis option from Blockchain interface

0339b06

blockchain: added v5.0.0-beta.2 CHANGELOG, some comment streamlining,…

00ef9cf

… inlined _getCanonicalGenesisBlock() function

holgerd77 approved these changes Nov 9, 2020

View reviewed changes

holgerd77 merged commit 84aaf81 into master Nov 9, 2020

holgerd77 deleted the blockchain-concurrency-and-safety-improvements branch November 9, 2020 13:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Blockchain concurrency and safety improvements #930

Blockchain concurrency and safety improvements #930

jochem-brouwer commented Oct 28, 2020 •

edited

Loading

codecov bot commented Oct 28, 2020 •

edited

Loading

jochem-brouwer commented Oct 28, 2020

jochem-brouwer Oct 30, 2020

jochem-brouwer commented Nov 5, 2020

jochem-brouwer commented Nov 6, 2020

holgerd77 Nov 6, 2020

jochem-brouwer Nov 6, 2020

holgerd77 Nov 6, 2020

holgerd77 Nov 6, 2020

holgerd77 Nov 6, 2020

jochem-brouwer Nov 6, 2020

jochem-brouwer Nov 8, 2020

holgerd77 Nov 6, 2020

jochem-brouwer Nov 6, 2020

holgerd77 Nov 6, 2020

holgerd77 Nov 6, 2020

holgerd77 Nov 6, 2020

jochem-brouwer Nov 6, 2020

jochem-brouwer Nov 8, 2020

jochem-brouwer commented Nov 8, 2020

holgerd77 commented Nov 9, 2020 •

edited

Loading

holgerd77 left a comment

jochem-brouwer commented Nov 9, 2020

Blockchain concurrency and safety improvements #930

Blockchain concurrency and safety improvements #930

Conversation

jochem-brouwer commented Oct 28, 2020 • edited Loading

codecov bot commented Oct 28, 2020 • edited Loading

Codecov Report

jochem-brouwer commented Oct 28, 2020

Choose a reason for hiding this comment

jochem-brouwer commented Nov 5, 2020

jochem-brouwer commented Nov 6, 2020

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

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 8, 2020

holgerd77 commented Nov 9, 2020 • edited Loading

holgerd77 left a comment

Choose a reason for hiding this comment

jochem-brouwer commented Nov 9, 2020

jochem-brouwer commented Oct 28, 2020 •

edited

Loading

codecov bot commented Oct 28, 2020 •

edited

Loading

holgerd77 commented Nov 9, 2020 •

edited

Loading