[Miner] Rewrite miner code with proper encapsulation + test coverage #2155
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
furszy
force-pushed
the
2020_revamp_miner
branch
3 times, most recently
from
4932fc5 to
e59b572
Compare
random-zebra
left a comment
random-zebra
left a comment
There was a problem hiding this comment.
Nice stuff. First comments/nits...
furszy
force-pushed
the
2020_revamp_miner
branch
from
e59b572 to
2994d88
Compare
Author
|
Feedback tackled. |
|
Mhm... I don't know why GitHub doesn't notify it, as it did in the past, but this PR has conflicts with the current master branch (due to #2105, which rearranged the position of the functions in Aside from this, tested ACK. Working as intended. |
…y work with our blockchain and consensus rules.
…n't need) to be locked for the entire test. Only in the places that the locks are really needed.
…ck, only where are needed.
furszy
force-pushed
the
2020_revamp_miner
branch
from
2994d88 to
0d55bcf
Compare
Author
|
Just rebased it locally and there were no conflicts. Git is playing with your heart @random-zebra. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Have rewritten the miner code sources, dividing the miner thread from the block assembling process, encapsulating the block creation state inside a new
BlockAssemblerclass (adapting bitcoin#7598).Cleaned several mempool redundant checks inside the assembly process (adaptation of dashpay#6898):
Another point added here is the rework of the previously non-functional
miner_testsunit test. Which.. have essentially made it work, adding coverage for the miner process, updating it to a fairly recent point down upstream's path + adapting it to our blockchain and consensus rules.The result is a large speed up in the block assembly process, a much better and cleaner code architecture and enable an easier add of new algorithms for block filling in the future.
A good work path on top of this would be to add unit test coverage for PoS, cold staking and Sapling block creation (among others). At the moment, most of the block creation tests are functional ones which are slow and require to setup entire nodes etc.