You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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){newPoABlock()}else{newPoWBlock()}
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."
The text was updated successfully, but these errors were encountered:
Now with PoA clique added to the
Block
class in #1032 it becomes apparent that the consensus functionality in theBlock
class is too tightly coupled with the structural and further generic functionality and - especially on Clique - new functions likecliqueIsEpochTransition()
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...
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:
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:
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."
The text was updated successfully, but these errors were encountered: