Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Blockchain -> Tree Shaking Refactor: Consensus Integration #3454

Open
holgerd77 opened this issue Jun 14, 2024 · 1 comment
Open

Blockchain -> Tree Shaking Refactor: Consensus Integration #3454

holgerd77 opened this issue Jun 14, 2024 · 1 comment

Comments

@holgerd77
Copy link
Member

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:

grafik

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.

@holgerd77
Copy link
Member Author

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.

grafik

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant