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

Block: Encapsulate Consensus Mechanism #1044

Closed
holgerd77 opened this issue Jan 15, 2021 · 2 comments
Closed

Block: Encapsulate Consensus Mechanism #1044

holgerd77 opened this issue Jan 15, 2021 · 2 comments

Comments

@holgerd77
Copy link
Member

holgerd77 commented Jan 15, 2021

Now with PoA clique added to the Block class in #1032 it becomes apparent that the consensus functionality in the Block class is too tightly coupled with the structural and further generic functionality and - especially on Clique - new functions like cliqueIsEpochTransition() or similar somewhat pollute the namespace of the main API.

It would be therefore desirable to better encapsulate the consensus logic, tow possibilities - subclassing and a dedicated consensus module - have been discussed in the chat, here is an extract:


From @holgerd77 (somewhat summarizing the previous posts):

"@jochem-brouwer @ryanio yeah, I was thinking about this too a bit and wanted to raise this question about the clique* methods along the PR code. I decided to keep it simple but a bit unelegant at the end for backwards-compatibility reasons.

To add on...

to not crowd the block class with PoA concepts

Actually the class was "crowded" with ethash concepts already before. Now it just becomes more apparent with the two concepts included.

During my work I briefly thought about both concepts - so the subclassing with PoABlock (or something similar) as well as the consensus capsulation - didn't dig much further with my thoughts though here.

Subclassing I mentally discarded relatively quickly because I wasn't a fan of having code like:

if (PoA) {
  new PoABlock()
} else {
  new PoWBlock()
}

throughout the codebase. Not sure if this is a valid argument though.

Regarding a consensus class I was on first thought somewhat sceptical as well if this can be encapsulated (easily enough) since there is a lot of code - like e.g. the extraData checks - really deeply in the generic functions code base. On second thought though I am slowly moving over that this might just be a question of giving this some more design thinking.

I am still sceptical if we completely manage to capture a generic API with an abstract class or an interface since e.g. on clique there is very specific logic in these extra functions with getEpochTransitionBlock() or whatever. We might nevertheless be able to get to good common denominator which already encapsulates a lot.
Not the end of the world if there is still some type-specific functionality in there at the end like - to imagine something:

block.consensus.isEpochTransition()

or something. These kind of calls should be encapsulated well enough in the other libraries by the Common switches (if (common.consensusType() === 'poa') { ... } .

Not sure, maybe it's a way to introduce such a new consensus class along with the PoA PR (or a subsequent one, as what we think is more practical) where we can already integrate ALL of the new PoA logic and where we even might be able to move over some of the PoW logic and mark the remaining PoW functions as deprecated so that we might be able to do all this in a non-breaking way?

Having a way for people to plug in their own consensus engines would be really great (we shouldn't be too overly ambitious here though on first round to not get toooo side-tracked). Eventually (likely (?)) this is also already some good preparation for the Eth 2.0 transition, can very well imagine that.
Generally I am bit fan if more people continue to work on the PoA PR, I think this can only improve the quality of the PR if we reiterate here from various perspectives."

@holgerd77
Copy link
Member Author

Update: not sure if we want to follow up here or if we are satisfied on the current level of abstraction.

@holgerd77
Copy link
Member Author

Closed by #1756

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

2 participants
@holgerd77 and others