-
Notifications
You must be signed in to change notification settings - Fork 716
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
[Miner] Rewrite miner code with proper encapsulation + test coverage #2155
Conversation
4932fc5
to
e59b572
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice stuff. First comments/nits...
e59b572
to
2994d88
Compare
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.
2994d88
to
0d55bcf
Compare
Just rebased it locally and there were no conflicts. Git is playing with your heart @random-zebra. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK 0d55bcf after rebase
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Been running this on testnet all day and looking great!
ACK 0d55bcf
Have rewritten the miner code sources, dividing the miner thread from the block assembling process, encapsulating the block creation state inside a new
BlockAssembler
class (adapting bitcoin#7598).Cleaned several mempool redundant checks inside the assembly process (adaptation of bitcoin#6898):
Another point added here is the rework of the previously non-functional
miner_tests
unit 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.