ethereumjs-monorepo
ethereumjs-monorepo copied to clipboard
Blockchain -> Tree Shaking Refactor: Consensus Integration
Part of #3446
Earliest start together with official breaking release work!
This is somewhat of a no-brainer tree shaking refactor I just discovered: for Blockchain the second largest (22 KB uncompressed) code part of the code base is the clique consensus code:
Currently integration in the blockchain constructor is done like this:
switch (this.common.consensusAlgorithm()) {
case ConsensusAlgorithm.Casper:
this.consensus = new CasperConsensus()
break
case ConsensusAlgorithm.Clique:
this.consensus = new CliqueConsensus()
break
case ConsensusAlgorithm.Ethash:
this.consensus = new EthashConsensus()
break
default:
throw new Error(`consensus algorithm ${this.common.consensusAlgorithm()} not supported`)
}
This is not tree shakeable.
I first thought this would a pretty straight-forward refactor would be to have the consensus object being passed in to blockchain (consensus: new CliqueConsensus()), eventually together with removing consensusAlgorithm from Common. This would be a larger refactor though, since we have if (this.common.consensusAlgorithm()) { ... } else { ... } all over the code base, and I am really not sure if we want to open this can right now. Also there is still this Ethash -> Casper (PoS) switch logic deeply in the code base, and for blockchain there is new CasperConsensus() consensus object creation within the checkAndTransitionHardForkByNumber() method. Not totally against giving this more thought, but we should also be careful to not waste/spend too much ressources here which are then missing somewhere else.
For this case I have an easy independent refactoring suggestion:
So what we can do is to create a small subclass CliqueBlockchain.
This basically needs to overwrite in three methods:
public static async create(opts: BlockchainOptions = {}) {
const blockchain = new CliqueBlockchain(opts)
await Blockchain.create(opts, blockchain) // new optional parameter
}
protected constructor(opts: BlockchainOptions = {}) {
this.consensus = new CliqueConsensus()
super(opts)
}
async checkAndTransitionHardForkByNumber(...) {
this.consensus = new CliqueConsensus()
super.checkAndTransitionHardForkByNumber(...)
}
Then we can remove the clique consensus support from the Blockchain class code and that's it. I would think this solution should be bearable, this is somewhat a half-deprecation (or let's call it: downgrade) of clique consensus without fully remove. So if someone wants to dynamically switch to Clique this might need some extra check now:
if (common.consensusAlgorith === Clique) {
await CliqueBlockchain.create()
} else {
await Blockchain.create()
}
This should be bearable though since this will be somewhat rarely used code.
We can alternatively discuss to fully deprecate clique, I would vote against though on first iteration. I do like the clique code, this is a very worked out and fine grained consensus mechanism and generally PoA consensus has some unique properties (especially not having this tight coupling with the CL) which we otherwise would loose and which might have its usages also in the future.
Update: already for removing etash bunding it is highly recommended to deal with ethash (+ PoW) in this regard as well.
The ethash bigint-crypto-utils dependency also has unlucky top-level-await side effects.
Regarding the team call, one of the questions was if the blockchain class can switch consensus mechanism on the fly. The answer is: yes, this happened at the merge, where Ethash had to be disabled and it was swapped with Casper (merge consensus algo, super simple, it only validates if the difficulty is 0).
I think we can swap this out, especially this feature (multiple consensus mechanisms sound good to me, if we want to support other chains, these might have different consensus algorithms), but not on-the-fly. See: checkAndTransitionHardForkByNumber this method in blockchain.ts.
In fact, I would even go as far to say we should deprecate Ethash at this point. PoW is now almost 2 years old!
Closed by #3504