multi: Move mining code into mining package #1965

sefbkn · 2019-10-26T04:12:32Z

This PR moves the mining code from the main module to the mining module. This work is part of a larger effort to support prioritizing transactions with dependencies by fee.

#1884 #1556

Expose NewBlkTmplGenerator from mining package.
Expose NewBgBlkTmplGenerator from mining package.
Expose CPUMiner from mining package.
Modify activeNetParams -> g.chainParams in mining code.
Add logger to mining package
Inject blockManager to mining module and create private blockManager type within mining package.
Use chain & chainParams on BlkTmplGenerator rather than from blockManager in main package.

matheusd

Seems a reasonable effort, though there's still more abstracting to do.

davecgh

Thanks for the effort. I started to review this, but I quickly realized that this really needs to be reversed in how it makes the changes (that is to say make the changes first, and then move things to the separate package as the final commit) for at least a couple of reasons:

The intermediate commits do not build and pass the tests. I do not merge intermediate broken states to master.
I can't reasonably review this with any degree of accuracy when there are big changes made when a file is moved since the diff is the entire file.

Hence, the inline comments are only for a portion of the first commit. That said, there are a several changes here that I think we can certainly start with though such as

mining: use chainParams on block template generator rather than from blockManager since they are the same instance.
mining: use chain on block template generator rather than blockManager since they are the same instance.
multi: remove unnecessary methods exposed on blockmanager interface.

Also, I did this refactoring upstream a long time ago which hasn't made it into dcrd yet. I would suggest taking a look at some of the lead up to it as it will be substantially similar and it is already known to work well:

cpuminer: Introduce cpuminerConfig (btcsuite/btcd@6719014)
mining: Break dependency on block manager instance. (btcsuite/btcd@6604672)
mining: Use generator state versus local state. (btcsuite/btcd@a952a3a)
mining: Refactor template code into mining package. (btcsuite/btcd@61ca40e)

Finally, please note that your commits themselves are not following the code contribution guidelines in terms of length and proper formatting.

blockmanager.go

log.go

rpcserver.go

mining.go

dnldd · 2020-04-14T20:39:09Z

mining.go

@@ -1900,6 +1898,12 @@ func (g *BlkTmplGenerator) UpdateBlockTime(header *wire.BlockHeader) error {
 	return nil
 }

+// UpdateBlockTime invokes `UpdateBlockTime` on the underlying
+// BgBlkTmplGenerator.


BgBlkTmplGenerator -> BlkTmplGenerator.

This method is exposed since rpcserver needs to access BlkTmplGenerator.UpdateBlockTime. Where to get a reference to the BlkTmplGenerator in that module without exposing it directly is not standing out at the moment, so I left it.

I misunderstood this feedback when revisiting this PR. You were highlighting that the comment needs to change, not the method signature. Agreed that this should be altered, but given the timing and commits built on this already I think it might be best to handle in a later PR. Sorry I didn't catch and fix that earlier, and thanks for the review.

I already updated it via #2276.

dnldd · 2020-04-15T18:25:25Z

Tested this PR with dcrpool's simnet harness, no errors/issues noted. I think it should be good once the issues identified in the review are resolved.

davecgh · 2020-07-15T05:35:45Z

@sefbkn Very nice job on the updates since the first iteration. Would you mind rebasing this to the latest master to resolve the conflicts so I can merge this? I would very much like to get this in for the 1.6.0 release that is rapidly approaching.

I reviewed it and while there are a few minor things that I see and I also agree with the things that @dnldd pointed out, given this has been sitting here for so long, I think this is good enough to go in as is and I can follow up with another PR to address the minor things I see as well as @dnldd points.

sefbkn · 2020-07-17T01:45:20Z

@davecgh I also agree with what @dnldd highlighted and will rebase and resolve conflicts shortly, sorry for the delay. Thank you for the review and patience.

This removes coupling between the mining code, blockManager, and the main configuration. This change is a step towards moving the mining code into its own package.

This change wraps the CPUMiner's WaitGroup reference in preparation for moving the CPUMiner under the mining package.

davecgh · 2020-07-18T23:54:29Z

Thanks for the update. Looks like it failed CI due to:

+ go mod tidy
+ git status --porcelain go.mod go.sum
+ UPDATED_MOD_STATUS= M mempool/go.sum
+ [  M mempool/go.sum !=  ]
+ go mod tidy
+ echo Running  modified go.mod and/or go.sum
+ exit 1
Running  modified go.mod and/or go.sum

In other words, go mod tidy needs to be run in the mempool module (and really anything that imports the mining module, but that is only mempool I believe).

davecgh · 2020-07-19T00:05:08Z

Hmm, looks like it's still failing due to lack of tidy. I ran it locally and looks like this should do it:

diff --git a/mining/go.sum b/mining/go.sum
index 5f5f1522..a4b22c82 100644
--- a/mining/go.sum
+++ b/mining/go.sum
@@ -6,12 +6,12 @@ github.com/btcsuite/snappy-go v1.0.0 h1:ZxaA6lo2EpxGddsA8JwWOcxlzRybb444sgmeJQMJ
 github.com/btcsuite/snappy-go v1.0.0/go.mod h1:8woku9dyThutzjeg+3xrA5iCpBRH8XEEg3lh6TiUghc=
 github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c=
 github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
+github.com/dchest/siphash v1.2.1 h1:4cLinnzVJDKxTCl9B01807Yiy+W7ZzVHj/KIroQRvT4=
+github.com/dchest/siphash v1.2.1/go.mod h1:q+IRvb2gOSrUnYoPqHiyHXS0FOBBOdl6tONBlVnOnt4=
 github.com/decred/base58 v1.0.1 h1:w5qTcb0hYpKuIBYIn4Ckirkj1aOWrSq8onPQpb3eGg8=
 github.com/decred/base58 v1.0.1/go.mod h1:H2ENcsJjye1G7CbRa67kV9OFaui0LGr56ntKKoY5g9c=
 github.com/decred/base58 v1.0.3 h1:KGZuh8d1WEMIrK0leQRM47W85KqCAdl2N+uagbctdDI=
 github.com/decred/base58 v1.0.3/go.mod h1:pXP9cXCfM2sFLb2viz2FNIdeMWmZDBKG3ZBYbiSM78E=
-github.com/decred/dcrd/chaincfg/chainhash v1.0.2 h1:rt5Vlq/jM3ZawwiacWjPa+smINyLRN07EO0cNBV6DGU=
-github.com/decred/dcrd/chaincfg/chainhash v1.0.2/go.mod h1:BpbrGgrPTr3YJYRN3Bm+D9NuaFd+zGyNeIKgrhCXK60=
 github.com/decred/dcrd/chaincfg/v2 v2.3.0 h1:ItmU+7DeUtyiabrcW+16MJFgY/BBeeYaPfkBLrFLyjo=
 github.com/decred/dcrd/chaincfg/v2 v2.3.0/go.mod h1:7qUJTvn+y/kswSRZ4sT2+EmvlDTDyy2InvNFtX/hxk0=
 github.com/decred/dcrd/crypto/blake256 v1.0.0 h1:/8DMNYp9SGi5f0w7uCm6d6M4OU2rGFK09Y2A4Xv7EE0=
@@ -32,8 +32,6 @@ github.com/decred/dcrd/dcrec/secp256k1/v2 v2.0.0 h1:3GIJYXQDAKpLEFriGFN8SbSffak1
 github.com/decred/dcrd/dcrec/secp256k1/v2 v2.0.0/go.mod h1:3s92l0paYkZoIHuj4X93Teg/HB7eGM9x/zokGw+u4mY=
 github.com/decred/dcrd/dcrutil/v2 v2.0.1 h1:aL+c7o7Q66HV1gIif+XkNYo9DeorN3l01Vns8mh0mqs=
 github.com/decred/dcrd/dcrutil/v2 v2.0.1/go.mod h1:JdEgF6eh0TTohPeiqDxqDSikTSvAczq0J7tFMyyeD+k=
-github.com/decred/dcrd/wire v1.3.0 h1:X76I2/a8esUmxXmFpJpAvXEi014IA4twgwcOBeIS8lE=
-github.com/decred/dcrd/wire v1.3.0/go.mod h1:fnKGlUY2IBuqnpxx5dYRU5Oiq392OBqAuVjRVSkIoXM=
 github.com/decred/slog v1.0.0 h1:Dl+W8O6/JH6n2xIFN2p3DNjCmjYwvrXsjlSJTQQ4MhE=
 github.com/decred/slog v1.0.0/go.mod h1:zR98rEZHSnbZ4WHZtO0iqmSZjDLKhkXfrPTZQKtAonQ=
 github.com/fsnotify/fsnotify v1.4.7 h1:IXs+QLmnXW2CcXuY+8Mzv/fWEsPGWxqefPtCP5CnV9I=

This contains the changes necessary to move the mining code into the mining package. The motivation behind these changes is to decouple the block template generation from other modules in the codebase to simplify extending it with new features and tests.

sefbkn · 2020-07-19T00:53:05Z

Thanks for the update. Looks like it failed CI due to:
+ go mod tidy
+ git status --porcelain go.mod go.sum
+ UPDATED_MOD_STATUS= M mempool/go.sum
+ [  M mempool/go.sum !=  ]
+ go mod tidy
+ echo Running  modified go.mod and/or go.sum
+ exit 1
Running  modified go.mod and/or go.sum
In other words, go mod tidy needs to be run in the mempool module (and really anything that imports the mining module, but that is only mempool I believe).

Thank you Dave, this did it. Didn't catch your first comment until a bit later, sorry for the PR spam :)

dnldd

Nicely done 👍, tested updates with no issues noticed.

sefbkn force-pushed the move_mining_module branch from 96822d3 to 765b350 Compare October 26, 2019 16:08

sefbkn marked this pull request as ready for review October 26, 2019 16:09

sefbkn force-pushed the move_mining_module branch from 765b350 to bede8da Compare October 26, 2019 17:17

sefbkn changed the title ~~multi: Move mining code into mining module~~ multi: Move mining code into mining package Oct 27, 2019

matheusd reviewed Oct 28, 2019

View reviewed changes

davecgh added this to the 1.6.0 milestone Oct 28, 2019

davecgh requested changes Oct 29, 2019

View reviewed changes

blockmanager.go Outdated Show resolved Hide resolved

log.go Outdated Show resolved Hide resolved

rpcserver.go Outdated Show resolved Hide resolved

sefbkn force-pushed the move_mining_module branch 3 times, most recently from fdcdab8 to 85180b3 Compare February 19, 2020 04:06

dnldd suggested changes Apr 14, 2020

View reviewed changes

davecgh mentioned this pull request Jul 15, 2020

Move rpcserver into a separate internal package. #2263

Closed

18 tasks

sefbkn added 2 commits July 17, 2020 22:35

multi: Decouple blockManager from mining.

52abe16

This removes coupling between the mining code, blockManager, and the main configuration. This change is a step towards moving the mining code into its own package.

multi: Hide CPUMiner WaitGroup.

6c08bef

This change wraps the CPUMiner's WaitGroup reference in preparation for moving the CPUMiner under the mining package.

sefbkn force-pushed the move_mining_module branch from 85180b3 to d0a9466 Compare July 18, 2020 23:20

sefbkn force-pushed the move_mining_module branch from d0a9466 to 4c086ba Compare July 18, 2020 23:57

sefbkn force-pushed the move_mining_module branch from 4c086ba to bfbce16 Compare July 19, 2020 00:43

davecgh mentioned this pull request Jul 19, 2020

mempool: Remove deprecated code. #2273

Merged

davecgh approved these changes Jul 19, 2020

View reviewed changes

dnldd approved these changes Jul 19, 2020

View reviewed changes

davecgh merged commit a1630f5 into decred:master Jul 19, 2020

sefbkn deleted the move_mining_module branch January 26, 2021 05:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

multi: Move mining code into mining package #1965

multi: Move mining code into mining package #1965

sefbkn commented Oct 26, 2019

matheusd left a comment

davecgh left a comment •

edited

Loading

dnldd Apr 14, 2020

sefbkn Jul 19, 2020 •

edited

Loading

sefbkn Jul 19, 2020

davecgh Jul 19, 2020

dnldd commented Apr 15, 2020

davecgh commented Jul 15, 2020 •

edited

Loading

sefbkn commented Jul 17, 2020

davecgh commented Jul 18, 2020

davecgh commented Jul 19, 2020

sefbkn commented Jul 19, 2020

dnldd left a comment

multi: Move mining code into mining package #1965

multi: Move mining code into mining package #1965

Conversation

sefbkn commented Oct 26, 2019

matheusd left a comment

Choose a reason for hiding this comment

davecgh left a comment • edited Loading

Choose a reason for hiding this comment

dnldd Apr 14, 2020

Choose a reason for hiding this comment

sefbkn Jul 19, 2020 • edited Loading

Choose a reason for hiding this comment

sefbkn Jul 19, 2020

Choose a reason for hiding this comment

davecgh Jul 19, 2020

Choose a reason for hiding this comment

dnldd commented Apr 15, 2020

davecgh commented Jul 15, 2020 • edited Loading

sefbkn commented Jul 17, 2020

davecgh commented Jul 18, 2020

davecgh commented Jul 19, 2020

sefbkn commented Jul 19, 2020

dnldd left a comment

Choose a reason for hiding this comment

davecgh left a comment •

edited

Loading

sefbkn Jul 19, 2020 •

edited

Loading

davecgh commented Jul 15, 2020 •

edited

Loading