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.

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

mining: Split into separate files. #2431

Merged
merged 5 commits into from Oct 16, 2020

Conversation

rstaudt2
Copy link
Member

This splits the mining package into a few additional files to make the package easier to navigate. It also cleans up some tests and comments along the way.

This is split into separate commits so that it is easy to verify that things did not change as they were moved.

This moves txPriorityQueue and the related logic to a separate file,
mining/txpriorityqueue.go.  Additionally, it moves the txPriorityQueue
tests to mining/txpriorityqueue_tests.go.

This is part of an effort to split mining into several files to make the
package easier to navigate, and in preparation of adding additional test
coverage.
This includes some minor cleanup for the txPriorityQueue tests and
additionally improves the test coverage to 100% for txpriorityqueue.go.
This moves the interfaces that are defined by mining to a separate file,
mining/interface.go.

This is part of an effort to split mining into several files to make the
package easier to navigate, and in preparation of adding additional test
coverage.
Copy link
Member

@dnldd dnldd left a comment

Choose a reason for hiding this comment

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

Nice 👍

Copy link
Member

@davecgh davecgh left a comment

Choose a reason for hiding this comment

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

Thanks for the PR and, as I often mention, I really appreciate that you split the changes up into well crafted and easier to review commits. It makes the review significantly easier and speeds the process up as a result.

Nice change to the priority queue tests to only create the random items once!

One thing I noticed is that the blockManagerFacade is really a pretty poorly named interface and has at least one method on it that probably should either be its own interface or reworked entirely. Namely, the NotifyWork(templateNtfn *TemplateNtfn). If it is to be kept, it probably should be part of something like type WorkNotifier interface { ... }. However, looking at its purpose, all it really does is call the NotifyWork func on the block manager which in turn forwards it to the rpcserver.NotifyWork method if the rpcserver is active, so I suspect the RPC server should just subscribe for work notifications like everything else and then there would be no need for the delegation at all.

I know it was obviously already named it and hence I don't think it's appropriate to change it this PR, but I figured I'd mention it while here.

@davecgh davecgh added this to the 1.7.0 milestone Oct 16, 2020
This moves BgBlkTmplGenerator and the related logic to a separate file,
mining/bgblktmplgenerator.go.

This is part of an effort to split mining into several files to make the
package easier to navigate, and in preparation of adding additional test
coverage.
@rstaudt2 rstaudt2 force-pushed the spilt-mining-to-separate-files branch from 44cea34 to 0977e2b Compare October 16, 2020 19:48
@rstaudt2
Copy link
Member Author

Thanks for the reviews. I updated the copyright as suggested in bgblktmplgenerator.go. Agreed regarding the blockManagerFacade interface - I'll consider adjusting that interface in a future PR since some mining interfaces are going to be needed to added/adjusted to support testing as well.

@davecgh davecgh merged commit cbb4dcb into decred:master Oct 16, 2020
@rstaudt2 rstaudt2 deleted the spilt-mining-to-separate-files branch January 15, 2021 12:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants