Blockchain: More Modern and Flexible Consensus Layout / Tree Shaking Optimization #3504

holgerd77 · 2024-07-15T12:06:45Z

This PR moves to a more flexible consensus layout for blockchain allowing for an optional consensusDict to be passed in where all consensus objects which should be used for consensus validation must be present.

This basic setup allows for full tree shaking regarding Clique and Etash code and it will - step 2 - also allow to remove the Ethash dependency.

Since we are now in a Proof-of-Stake world where consensus validation is the exception the PR also moves to having consensus validation deactivated by default. Semantics of the consensusValidation flag is "merged" with the consensusDict object ("consensus" and "consensus validation" is basically the same), this allows for a substantial simplification on otherwise useless "combinations" (this and that object passed, but consensusValidation set to false) and also have a lot cleaner deactivated-by-default structure.

Deactivated-by-default also allows for removing a lot of consensusValidation: false passing, which we lately did more and more on tests since we just didn't want to have/do any more.

Consensus transitions - if wished - still fully work, as a side note (even more flexible than before).

WIP, several tests failing. But on the way.

…allowing to pass in different consensus objects

…block header) difficulty equals 0 check from CasperConsensus

…us* and *consensus validation* is basically the same, allowing for more flexible instantiation and optional consensus object usage

… and custom consensus tests

…rained settings to use the mechanism (e.g. Clique) but skip the respective validation

…chain without need for ethash passing in) and consensus availability validation (now in the validate() call)

…sh object integration

codecov · 2024-07-16T11:40:15Z

Codecov Report

Attention: Patch coverage is 0% with 6 lines in your changes missing coverage. Please review.

Project coverage is 24.24%. Comparing base (ef20930) to head (46df069).
Report is 6 commits behind head on master.

❗ There is a different number of reports uploaded between BASE (ef20930) and HEAD (46df069). Click for more details.

HEAD has 5 uploads less than BASE

Flag BASE (ef20930) HEAD (46df069)

util 1 0

devp2p 1 0

common 1 0

statemanager 1 0

trie 1 0

Additional details and impacted files

Flag	Coverage Δ
block	`73.21% <ø> (?)`
client	`0.00% <0.00%> (?)`
common	`?`
devp2p	`?`
evm	`68.91% <ø> (?)`
statemanager	`?`
trie	`?`
tx	`78.41% <ø> (?)`
util	`?`
vm	`58.63% <ø> (?)`
wallet	`0.00% <ø> (?)`

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

holgerd77 · 2024-07-16T12:25:30Z

Ok, I battled a bit more with this than originally anticipated, but I think I've found a good balance now between usage convenience and strictness needs. 🙂 I had some more radical versions in place in between, removing the validateConsensus parameter and the full `CasperConsensus class (literally doing nothing anyhow), but that had too much interference with test runs and existing code execution. So I went back to some more conservative approach, concentrating more on the core task from here (decoupling, tree shaking, dependencies). We can for sure refactor a bit more in the future, if someone has consistent view of where to go and how.

What I did for convenience is set validateConsensus to false by default. We are in a non-validation world anyhow (CL is doing that now) and this makes it more convenient to instantiate a normal mainnet Blockchain (pointing to block 0 for good reasons and there still have an ethash setting in the back).

If you want to fully follow the thought process I went through here it is best to go commit-by-commit and see what I removed (also read commit messages) and then re-introduced later on. 😂

Everything was a bit delicate, so to get to a setting which worked again, just to say.

Just in train of fixing he last remaining tests (if any).

There is one client CLI test which failed on last run, seemed totally unrelated. 🤔

So generally this is ready for review! 🙂

holgerd77 · 2024-07-16T12:30:00Z

Ah, and to say: so this does complete on its goals. So ethash dependency is fully removed from Blockchain (together with its problematic bigint-crypto-utils dependency and tree shaking is done for EthashConsensus and CliqueConsensus.

Before:

(blockchain size + ethash still present)

After:

jochem-brouwer

This looks super clean, some questions though

jochem-brouwer · 2024-07-16T12:39:31Z

packages/blockchain/src/consensus/casper.ts

-      throw new Error(`${msg} ${header.errorStr()}`)
-    }
-  }
+  public async validateDifficulty(): Promise<void> {}


Where is the difficulty must be 0 check now?

Hey there,
thanks a lot for the review, all good points I guess, will address one by one:

So this one originated from a former more radical version, where I fully deleted the CasperConsensus, and as an in-between step deleted this difficulty check. This is not really a consensus check but just a simple format check and is in fact also done in the BlockHeader: https://github.com/ethereumjs/ethereumjs-monorepo/blob/master/packages/block/src/header.ts#L502

I think it is beneficial to remove it here since it otherwise "tricks" us into thinking we would do some kind of consensus validation here for PoS (must be, there is some code! 😋 ), while in reality this is not the case (PoS consensus validation is done on the CL side).

No super strong opinion, but I would tend to keep this removed.

I should have checked upon reviewing because I suspected this check was in our Block Header as well (this would actually be the more logical place!). I agree it should not be in blockchain. This is actually a great change/removal, let's keep it 😄 👍

jochem-brouwer · 2024-07-16T12:41:15Z

packages/blockchain/src/blockchain.ts

@@ -390,7 +380,13 @@ export class Blockchain implements BlockchainInterface {
        }

        if (this._validateConsensus) {
-          await this.consensus.validateConsensus(block)
+          if (!(this.common.consensusAlgorithm() in this._consensusDict)) {


For cleani-ness should this not use this.consensus()?

You are right, that works as well 🙂, will update!

jochem-brouwer · 2024-07-16T12:46:41Z

packages/blockchain/test/customConsensus.spec.ts

-    blockchain.common.consensusAlgorithm = () => 'fibonacci'
-
-    try {
-      await blockchain.checkAndTransitionHardForkByNumber(5n)


This part of the test got deleted, is this correct?

Could readd by making the checks more consistent.

jochem-brouwer · 2024-07-16T12:47:27Z

packages/blockchain/test/pos.spec.ts

-        {
-          header: {
-            number: 16,
-            difficulty: BigInt(1),


I think this test should still be in, because we test here that we cannot insert PoW blocks (so, blocks with difficulty nonzero)

Ok.

Needed to think three times (or so 😋) about this.

So this is where this actually interflicts with our removed difficulty check.

So: a PoW block for Londond is created: works. Block is added to Blockchain: now (with removed check) works as well.

I wanted to suggest that we instead do a hardfork check on the Commons (block + blockchain).

This gets hairy quickly though (might still be the better thing to do), since we have this _hardforkByHeadBlockNumber setting in Blockchain and this generally reaches deep into the blockchain update code.

I will capitulate here for now and re-add the difficulty check for now, together with a comment that this is not really a consensus check. That should enable to re-activate the test case.

Yes, works (locally), and I guess for this PR it is ok (maybe: better), since this PR is about something else at the end.

But to admit, this difficulty check is just picking out a very selected case and this problem that it should not be possible to add a non-HF matching block (with eventually different formatting or properties) to a blockchain is a lot broader and should likely be solved on a more generic level.

…etter-tree-shakeable-consensus-layout

…nsensus transition

jochem-brouwer

LGTM!

holgerd77 added PR state: WIP type: refactor package: blockchain breaking labels Jul 15, 2024

holgerd77 added 8 commits July 16, 2024 10:34

Switch to a more flexible Blockchain consensusDict options structure …

52f42c8

…allowing to pass in different consensus objects

Remove misplaced (non consensus checkand useless (already checked in …

f033ce7

…block header) difficulty equals 0 check from CasperConsensus

Shift to a new consensus semantics adhering to the fact that *consens…

f0c8b2e

…us* and *consensus validation* is basically the same, allowing for more flexible instantiation and optional consensus object usage

Remove redundant validateConsensus flag, fix block validation, clique…

6cbcbcc

… and custom consensus tests

Readd validateConsensus flag (default: true) to allow for more fine-g…

ae12768

…rained settings to use the mechanism (e.g. Clique) but skip the respective validation

Find a middle ground between convenience (allow mainnet default block…

ed942d7

…chain without need for ethash passing in) and consensus availability validation (now in the validate() call)

Add clique example

9052933

Client fixes

e5c0c0c

holgerd77 force-pushed the blockchain-switch-to-a-more-flexible-and-better-tree-shakeable-consensus-layout branch from e775302 to e5c0c0c Compare July 16, 2024 10:15

holgerd77 added 5 commits July 16, 2024 13:08

Fix VM test

e941110

Minor

5eb17b4

Remove Ethash dependency from Blockchain, adjust EthashConsensus etha…

29eae36

…sh object integration

Re-add CasperConsensus exports

90cf7bd

Rebuild package-lock.json

46df069

holgerd77 added 2 commits July 16, 2024 14:10

EtashConsensus fix

67cc164

Fixes

977d9b6

holgerd77 added PR state: needs review and removed PR state: WIP labels Jul 16, 2024

Fix client CLI tests

cecff0e

jochem-brouwer reviewed Jul 16, 2024

View reviewed changes

holgerd77 added 3 commits July 17, 2024 09:30

Cleaner consensus check on validate call

523e6c5

Merge branch 'master' into blockchain-switch-to-a-more-flexible-and-b…

efd1423

…etter-tree-shakeable-consensus-layout

More consistent check for consensus object, re-add test for custom co…

31e5727

…nsensus transition

Re-add difficulty check for PoS, re-activate removed test

ef06f31

jochem-brouwer approved these changes Jul 17, 2024

View reviewed changes

holgerd77 merged commit 03fa912 into master Jul 17, 2024
34 checks passed

holgerd77 deleted the blockchain-switch-to-a-more-flexible-and-better-tree-shakeable-consensus-layout branch July 17, 2024 09:15

holgerd77 mentioned this pull request Aug 8, 2024

Blockchain -> Tree Shaking Refactor: Consensus Integration #3454

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Blockchain: More Modern and Flexible Consensus Layout / Tree Shaking Optimization #3504

Blockchain: More Modern and Flexible Consensus Layout / Tree Shaking Optimization #3504

holgerd77 commented Jul 15, 2024

codecov bot commented Jul 16, 2024 •

edited

Loading

holgerd77 commented Jul 16, 2024

holgerd77 commented Jul 16, 2024

jochem-brouwer left a comment

jochem-brouwer Jul 16, 2024

holgerd77 Jul 17, 2024

jochem-brouwer Jul 17, 2024

jochem-brouwer Jul 16, 2024

holgerd77 Jul 17, 2024

jochem-brouwer Jul 16, 2024

holgerd77 Jul 17, 2024

jochem-brouwer Jul 16, 2024

holgerd77 Jul 17, 2024

holgerd77 Jul 17, 2024

jochem-brouwer left a comment

Blockchain: More Modern and Flexible Consensus Layout / Tree Shaking Optimization #3504

Blockchain: More Modern and Flexible Consensus Layout / Tree Shaking Optimization #3504

Conversation

holgerd77 commented Jul 15, 2024

codecov bot commented Jul 16, 2024 • edited Loading

Codecov Report

holgerd77 commented Jul 16, 2024

holgerd77 commented Jul 16, 2024

jochem-brouwer 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 left a comment

Choose a reason for hiding this comment

codecov bot commented Jul 16, 2024 •

edited

Loading