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.

Sign up for GitHub

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

Jump to bottom

[ETCM-759] refactor mining #979

Merged
merged 24 commits into from
May 20, 2021
Merged

[ETCM-759] refactor mining #979

merged 24 commits into from
May 20, 2021

Conversation

leo-bogastry
Copy link
Contributor

@leo-bogastry leo-bogastry commented May 5, 2021 •
edited
Loading

Description

In ETCM-746 a temporary modification in EthashMiner was introduced to support the new PoW mining algorithm with Keccak mining
In order to keep a separation of concerns, this PR introduces the following refactoring:

  • Introduced a PoWMiningCoordinator that will assert, per each block to be mined, if mining should to be done with ethash or keccak algorithm
  • Simplified EthashMiner to only mine on demand (managed by PoWMiningCoordinator)
  • Extracted from EthashMiner the Keccak specific code and added KeccakMiner (to only mine on demand, managed by PoWMiningCoordinator)

PoWMiningCoordinator protocol contains a placeholder to replace MockedMiner in the future
PoWMiningCoordinator is a typed actor. EthashMiner was modified to not be an actor anymore and just run a Monix task. The same to the KeccakMiner
Reorganized protocols (some is specific the MockedMiner for example)
Reorganized Spec setups for the miners

Testing

Tested in pottery private network using mantis-automation (making sure ethash mining still works as expected)
Currently deployed in Staging (making sure ethash mining still works as expected)

@leo-bogastry leo-bogastry changed the title Feature/etcm 759 refactor mining [ETCM-759] refactor mining May 6, 2021
bsuieric
bsuieric approved these changes May 6, 2021
View reviewed changes
Copy link
Contributor

@bsuieric bsuieric left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, still unclear the use of MockedMiner, but I noticed you added a TODO to another ticket

@leo-bogastry leo-bogastry force-pushed the feature/ETCM-759-refactor-mining branch from 919e9bf to 69b683b Compare May 10, 2021 14:04
dzajkowski
dzajkowski reviewed May 11, 2021
View reviewed changes
src/test/scala/io/iohk/ethereum/consensus/pow/PoWMinerCoordinatorSpec.scala Outdated
coordinator ! StopMining
}

it should "[Recurrent Mining] ProcessMining starts EthashMiner if mineWithKeccak is false" in new TestSetup(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure these tags will scale. maybe we could use a spec that allows nesting (WordSpec, FreeSpec, FunSpec)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FreeSpec is a good fit

dzajkowski
dzajkowski reviewed May 11, 2021
View reviewed changes
src/main/scala/io/iohk/ethereum/consensus/pow/PoWConsensus.scala Outdated
@@ -65,15 +83,30 @@ class PoWConsensus private (
*
* TODO further refactors should focus on extracting two types - one with a miner, one without - based on the config
*/
private[this] def startMiningProcess(node: Node): Unit = {
if (minerRef.isEmpty) {
private[this] def startMiningProcess(node: Node, blocKCreator: PoWBlockCreator): Unit = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: startMiningCoordinator?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that the mockedMiner is supposed to be removed, yes. Otherwise no. What do you think? :)

dzajkowski
dzajkowski requested changes May 11, 2021
View reviewed changes
Copy link
Contributor

@dzajkowski dzajkowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overall looks good. please review my comments.

dzajkowski
dzajkowski approved these changes May 18, 2021
View reviewed changes
Copy link
Contributor

@dzajkowski dzajkowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please verify that the node can synch to sagano with these changes. Would it be possible to verify if ethash mining is still behaving as expected (compatible with sagano)?

@leo-bogastry leo-bogastry force-pushed the feature/ETCM-759-refactor-mining branch 2 times, most recently from b2d11d9 to 26e949b Compare May 18, 2021 12:02
@leo-bogastry
Copy link
Contributor Author

Please verify that the node can synch to sagano with these changes. Would it be possible to verify if ethash mining is still behaving as expected (compatible with sagano)?

A couple of day ago I deployed this branch in staging, and it is mining and synching.
In Sagano I was able to synch, but in order to mine I have to update the miners there

@leo-bogastry
Copy link
Contributor Author

Please verify that the node can synch to sagano with these changes. Would it be possible to verify if ethash mining is still behaving as expected (compatible with sagano)?

It's deployed in Sagano as well

Leonor Boga added 24 commits May 20, 2021 09:33
[ETCM-759] Add actor-typed dependencies
9322186
[ETCM-759] Fix protocol name on pottery.conf to restricted-pow
13dcf4e
[ETCM-759] Update log messsage on BlockFetcher for clarity
c8204d3
[ETCM-759] Rename EthashBlockCreator to PoWBlockCreator
0bebe88
[ETCM-759] Rename MinerResponse to MockedMinerResponse
fa2565a
[ETCM-759] Add PoWMinerCoordinator
b43c6bd
[ETCM-759] Update PowConsensus to use new PoWMinerCoordinator instead…
c5b93ba 
… of EthashMiner. PoWMinerCoordinator is a typed actor and MockedMiner is a classic actor, that's the reason for the extra code added.
[ETCM-759] Modify EthashMiner to only mine on demand, managed by the …
cdeca22 
…PoWMiningManager
[ETCM-759] Extract code to mining with Keccak into an actor called Ke…
93eee49 
…ccakMiner. (Mining on demand, managed by the PoWMiningManager)
[ETCM-759] Reorganize the MinerProtocol
2fbd5fb
[ETCM-759] Add new Patience configuration, for mining in the specs
f949373
[ETCM-759] Adjust MockedMinerSpec
bd89f69
[ETCM-759] Adjust MinerSpecSetup to contain shared setup between mock…
4beda99 
…, keccak, ethash miners and the coordinator
[ETCM-759] Rename PoWMinerCooordinatorSpec to PoWMiningCoordinatorSpec
8110a08
[ETCM-759] Ensure PoWMiningCoordinator continues to coordinate mining…
ad8413a 
… when getBestBlock() returns null. Add tests to cover the scenario
[ETCM-759] Add assertion in Ethash and Keccak miners spec
bce49c8
[ETCM-759] Reorganize miners and coordinator tests setup
c951ffa
[ETCM-759] Fix PoWMiningCoordinatorSpec
49ea230
[ETCM-759] Implement small PR comments
08df24b
[ETCM-759] Renamed DoMining to MineNext in PoWMiningCoordinator
0e8b7d6
[ETCM-759] Rename StartMining(mode: MiningMode) to SetMiningMode((mod…
1ee8800 
…e: MiningMode)) in PoWMiningCoordinator
[ETCM-759] Refactor EthashMiner and KeccakMiner as non actors
202de0b
[ETCM-759] Refactored tests
621d1d8
[ETCM-759] Small refactoring
226c6a3
@leo-bogastry leo-bogastry force-pushed the feature/ETCM-759-refactor-mining branch from 26e949b to 226c6a3 Compare May 20, 2021 07:33
@leo-bogastry leo-bogastry merged commit b81d183 into develop May 20, 2021
@leo-bogastry leo-bogastry deleted the feature/ETCM-759-refactor-mining branch May 20, 2021 08:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AnastasiiaL AnastasiiaL AnastasiiaL left review comments

@bsuieric bsuieric bsuieric approved these changes

@dzajkowski dzajkowski dzajkowski approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@leo-bogastry @dzajkowski @bsuieric @AnastasiiaL