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: Move txMiningView from mempool package. #2467

Merged
merged 1 commit into from Nov 18, 2020

Conversation

rstaudt2
Copy link
Member

Requires #2458

This moves txMiningView and txDescGraph to the mining package. This reduces the coupling between the mempool and mining packages and allows mining tests to use the actual txMiningView and txDescGraph implementations.


The more immediate reason I am looking to bring this in is that I am working toward adding a mining test harness. In its current state, most/all of the txMiningView and txDescGraph functionality would need to be duplicated in the test code.

Summary of the changes (unfortunately, this couldn't really be split into multiple commits since the intermediate steps would be in a non-working state):

  • Moved the txMiningView type and methods to mining/mining_view.go.
    • Removed mining.* everywhere in this file.
  • Moved the txDescGraph type and methods to mining/tx_desc_graph.go.
    • Removed mining.* everywhere in this file.
    • Nothing is exported within this file, txDescGraph is only used internally by txMiningView.
  • Updated TxMiningView to be exported and updated all references accordingly.
  • Updated AddTransaction to be exported and updated all references accordingly.
  • Removed TxMiningView interface and updated mempool to use the exported mining.TxMiningView type and methods.
    • I didn't see a need for this interface at this point. If mempool wanted to use an alternative mining view implementation (for testing or other purposes), it could make sense to have a mining view interface defined in mempool.
    • mempool is only using AddTransaction, Remove, and Clone from TxMiningView. However, mempool_test uses quite a few other TxMiningView methods since it is validating that the internals of TxMiningView are set correctly. Testing these internals could be moved to mining in the future, and then those other methods could be unexported.
  • Added mining.NewTxMiningView and mining.TxMiningView Clone methods for mempool to use instead of handling that logic directly.

@davecgh davecgh added this to the 1.7.0 milestone Nov 17, 2020
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. Looks good overall. Excellent PR description as well!

internal/mining/mining_view.go Outdated Show resolved Hide resolved
internal/mining/mining_view.go Show resolved Hide resolved
internal/mining/mining_view.go Outdated Show resolved Hide resolved
internal/mining/mining_view.go Show resolved Hide resolved
internal/mining/mining_view.go Outdated Show resolved Hide resolved
internal/mining/mining_view.go Outdated Show resolved Hide resolved
internal/mining/mining_view.go Outdated Show resolved Hide resolved
internal/mining/tx_desc_graph.go Outdated Show resolved Hide resolved
internal/mining/mining_view.go Show resolved Hide resolved
internal/mining/mining_view.go Outdated Show resolved Hide resolved
@rstaudt2
Copy link
Member Author

@davecgh Thanks for the review. Good catch on some of the comments that needed to be corrected as well - those have been updated, and I noticed a couple of others that were still mentioning mempool as well and fixed those.

@davecgh
Copy link
Member

davecgh commented Nov 17, 2020

I approved this since it looks good to go now, but it obviously has to wait until #2458 is in.

@sefbkn Is going to be squashing that down and this will need to be rebased after that is done, but that shouldn't cause any conflicts.

@davecgh
Copy link
Member

davecgh commented Nov 18, 2020

Alright, #2458 has landed. This just needs a rebase and I'll get it merged.

This moves txMiningView and txDescGraph to the mining package. This
reduces the coupling between the mempool and mining packages and allows
mining tests to use the actual txMiningView and txDescGraph
implementations.
@davecgh davecgh changed the title Move txMiningView to the mining package. mining: Move txMiningView from mempool package. Nov 18, 2020
@davecgh davecgh merged commit ed85046 into decred:master Nov 18, 2020
@rstaudt2 rstaudt2 deleted the move-mining-view 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

2 participants