[ETCM-1058] Replace block import by consensus#1078
Conversation
leo-bogastry
force-pushed
the
feature/ETCM-1058-replace-BlockImport-by-Consensus
branch
2 times, most recently
from
c41b1fe to
13bf68d
Compare
jvdp
left a comment
jvdp
left a comment
There was a problem hiding this comment.
Do have some questions but wouldn't want to block further work on this.
| * - [[BlockImportFailed]] - block failed to execute (when importing to top or reorganising the chain) | ||
| */ | ||
| def importBlock( | ||
| override def evaluateBranchBlock( |
There was a problem hiding this comment.
Why is it called evaluate, the term for this elsewhere is execute, right?
Also, the comments and result type still talk of "importing", while we should probably drop that term everywhere?
There was a problem hiding this comment.
The term elsewhere is execute, right? No. Where did you get that idea from? Actually me and Aurelién are struggling to give this method a proper name, because it can have several outcomes. I removed the name import exactly because as said in the description it Tries to import
There was a problem hiding this comment.
There is the BlockExecution class for example. So this could be considered "branch execution"?
There was a problem hiding this comment.
It is not guaranteed that there will be a block or branch execution, the block may end up by just being added to the BlockQueue
| ) | ||
| } | ||
|
|
||
| private def returnNoBestBlock(): Task[BlockImportFailed] = { |
There was a problem hiding this comment.
Why is this split out into a method? Doesn't seem to do much.
(If anything should be split out: BlockImportFailed("Couldn't find the current best block") could be a val.)
Perhaps we can reuse these error descriptions as log messages as well?
There was a problem hiding this comment.
It was mainly to make the main method evaluateBranchBlock more readable, by moving the detail of logging the error and creating the actual error description
There was a problem hiding this comment.
So I would argue it is less readable now since those methods actually do so little.
| import io.iohk.ethereum.utils.Logger | ||
|
|
||
| class BlockImport( | ||
| class ConsensusImpl( |
There was a problem hiding this comment.
Does it need to be split into Consensus and ConsensusImpl?
There was a problem hiding this comment.
One is a trait the other a class. It will help with testing
There was a problem hiding this comment.
Does it though? It didn't get in the way with BlockImport before.
This way does make code navigation a lot more annoying.
There was a problem hiding this comment.
Just look at the private[ledger] val blockExecution: BlockExecution (previous line 28). That was being accessed directly in the tests.
Hopefully we can improve the tests much more in the future
leo-bogastry
force-pushed
the
feature/ETCM-1058-replace-BlockImport-by-Consensus
branch
2 times, most recently
from
1ac7040 to
ae639d1
Compare
leo-bogastry
force-pushed
the
feature/ETCM-1058-replace-BlockImport-by-Consensus
branch
from
ae639d1 to
b8a4882
Compare
leo-bogastry
deleted the
feature/ETCM-1058-replace-BlockImport-by-Consensus
branch
4 participants
Description
Replace class BlockImport by a Consensus class, reusing current code.
Important Changes Introduced
Added a new trait Consensus and class ConsensusImpl
ConsensusImpl reuses existing code (with some refactoring)
The refactoring is done is on separate commits to make it easier to evaluate
Testing
Regular Sync was tested with mainnet
Regular Sync / mining still to be tested in staging