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: Encapsulate Consensus Mechanism #1756

Closed
holgerd77 opened this issue Mar 1, 2022 · 1 comment
Closed

Blockchain: Encapsulate Consensus Mechanism #1756

holgerd77 opened this issue Mar 1, 2022 · 1 comment

Comments

@holgerd77
Copy link
Member

holgerd77 commented Mar 1, 2022

Ok, I thought a bit longer about this topic today and I think I now have got the pieces together, so that we can remove all consensus logic out of the blockchain class and encapsulate so that this can be easily overridden (and leads to a much cleaner core-functionality code base). 😄

Here is the rough proposal:

We add a new folder consensus, following files:

consensus/ethash.ts
consensus/clique.ts

All - especially clique, but also ethash - functionality from index.ts will move into these files. These files contain a dedicated CliqueConsensus respectively EthashConsensus class adhering to a generic Consensus interface which should look roughly as follows:

export interface Consensus {
  genesisInit(genesisBlock: Block): Promise<void> 
  setup(): Promise<void>
  validate(block: Block): Promise<void>
  newBlock(bock: Block, ancientHeaders): Promise<void>
}
  • additionally, the constructor should take in a DB object.

There might be some tweaks necessary (changed return type or something), but this should be generally it and it should be possible to encompass both the ethash and clique (and hopefully other consensus mechanisms) into this simple interface. If no functional executions on one specific method for a consensus mechanism is needed (e.g. for genesisInit for ethash) the function body just remains empty (or we make a respective function optional? might also be a possibility).

Within Blockchain we can then default-instantiate one of the two consensus classes we implement ourself along a this._common.consensusAlgorithm() === ConsensusAlgorithm.Ethash (or: clique) switch in the constructor and can otherwise eliminate all other such switches in the main class.

On top we add a consensus option to the blockchain options dict (which also needs to adhere to the interface from above). If this is passed in, the consensus from this object is taken.

Some more concrete notes on the respective replacements, since this is a bit tricky and needs some close looks into the code base.

setup(): Promise

Called after genesisInit().

  • Ethash: new Ethash(this.db) initialization from here (setup() should be called later though to unify with clique call), guess this (the Ethash object) can always be initialized (no overhead, just looked) and not only when _validateConsensus is activated, that should make things easier
  • Clique: The setup stuff from here, so referenced code location is also the place for the generic await this.consensus.setup() call.

genesisInit()

  • Ethash: nothing to do
  • Clique: the call from here
  • Other: might need to do something or might not, genesisInit(genesisBlock) should generally be a sufficient API

validate()

  • Ethash: the ethash.verifyPOW(block) check from here
  • Clique: the signature verification check from here, I would cautiously say that both the following this.cliqueCheckRecentlySigned(header) check and the epoch transition signer checks can be included to unify in the consensus.validate() call (and the latter one (the epoch transition checks) can then also included under the _validateConsensus flag along, guess - again, cautiously - that should be ok).

TD check

Side note: I am not 100% sure yet about the TD check from here, we can leave for now, maybe we'll find a generic solution, not completely seeing through.

newBlock()

So this is basically: conensus mechanism update tasks needed to be done once a new block has been added.

  • Ethash: nothing to do
  • Clique: basically everything from here

This might be the most tricky one and the one which needs the most explanation. So following thoughts: This _findAncient(newHeader) function is - in the current code base - only called in a Clique context. However this is a very generic functionality to find a common ancestor of a new block and the latest canonical (?) head. So I would say that we should keep this functionality in the main Blockchain class, rename to findCommonAncestor() and along not only returning the single header found at the end but a whole array with the all the headers found on the way (so: this is a super-set of the previous result). This can then be fed to the generic newBlock(bock: Block, ancientHeaders): Promise<void> function. My guts say that this should likely be generally the important information needed in this context also for other consensus mechanisms.

Special note on the Clique DB functionality

There is some clique specific DB functionality in db/manager.ts. This let me scratch my head a bit at first but on a second look: this can just be taken out from there and included into the new consensus module. The DB manager is generally a bit over-engineered and this is not much functionality related specifically to Clique (side note: and this is also not relevant regarding the DB cache, not much data).

Other Notes

  • the consensus property from Blockchain should also be publicly exposed by a getter or something
  • I guess it's ok if a consensus class implementation contains some (very) limited set of additional functionality (1-2 more methods) beyond the interface if necessary. Would be optimal if be completely avoided but might be handy for 1-2 things.

Ok, this should basically be it. 😄

Sound a lot, but I guess this is doable and we gain a substantial round of flexibility and code cleanliness by this.

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

3 participants
@holgerd77 @emersonmacro and others