rpc: add new btcd extension commands: addminingaddr, delminingaddr, listminingaddrs

[Roasbeef]

This PR adds three new btcd JSON-RPC extension commands: addminingaddr, delminingaddr, and listminingaddrs . These RPC allows the caller to dynamically add, remove and list mining addresses to btcd's list of generation addresses. Before this RPC, doing such a thing would've required a full restart of btcd.

NOTE: This PR depends on btcsuite/btcrpcclient#105.

The addition of this RPC also adds a new degree of flexibility to automated deployments of btcd using orchestration tools like Docker. The original inspiration for this RPC was spurred by some workarounds I had to implement when attempting to package btcd and lnd into a development environment using docker-compose.

A breakdown of the changes is as follows:

• New RPC's: addminingaddr, delminingaddr, and listminingaddrs have been added. In order to ensure thread-safety, a mutex miningAddrMTx has been added to the config struct. This mutex must be held at all times when reading/modifying the miningAddr slice.
• The cpuminer.Config struct has been modified to use a closure function for obtaining new mining addresses. This decouple the current logic of selecting a mining address (randomly select from a list) from the internals of the cpuminer package. In the future if/when alternate selection logic is added, then code within the package shouldn't need to change at all.
• The Config struct has been modified to change the MiningAddr paramter from a []string to a string. When writing integration tests for these new RPC's, I discovered that the address set via the --miningaddr flag was being duplicated. This change fixes the duplication.

An interesting extension of this RPC was briefly discussed on IRC between myself and davec: the call could be extended to possibly accept a BIP0032 extended public key rather than a series of addresses. Within the daemon, when generating new blocks, btcd could instead iterate through the HD keychain grabbing fresh addresses for each block rather than repeating through a statically coded list of addresses. Taking it a set further, further logic would be added to generate multi-sig p2sh addresses from addresses plucked from the keychain.

For simplicity of this patch set, I've only implemented the most basic behavior round dynamically setting generation addresses, however the two extensions I mentioned above my be worth pursuing.

[dajohi]

I'd rather have this be called addminingaddr since 'set' to me implies overwriting the current settings. and then perhaps a delete as well?

[Roasbeef]

Sure, what do you think about also throwing in a companion to the add, and delete calls: list?

[stevenroose]

What happens when a node has multiple mining addresses? Round Robin? Or random?

[Roasbeef]

@stevenroose the current algorithm pseudorandomly selects a mining address from the current set.

[davecgh]

I think having {add,delete}miningaddress and listminingaddresses makes fine sense. I haven't reviewed the code yet, but should be able to get it reviewed later this week.

[Roasbeef]

I've pushed a new version of the PR which adds three new RPC's: addminingaddr, delminingaddr and listminingaddrs. I've also added a new integration tests to exercise the behavior and have also included a commit which fixes (what I consider) to be an existing bug in the configuration parsing.

[davecgh]

Thanks for the updates. One thing I noticed is the second commit converts the MiningAddr config field option from []string to string and I definitely don't think that should be changed. There are already configurations out there that rely on the behavior it provides and changing it would break existing setups.

I know from IRC that you were seeing duplicates, but the reason for that is due to the following change:

-	preCfg := cfg
+	preCfg := &cfg

That was intentionally a copy. Making it a pointer causes it to set the same fields in the same config struct on the second parse which leads to appending entries in any slices twice.

[davecgh]

I forgot to mention in my review that I really like the idea here. It allows miners to avoid downtime when changing things which is quite important for them.

It will also simplify setting up simulation networks since you currently need btcd running first to get wallet to generate an address that you then have to stop btcd to configure with it.

[Roasbeef]

Yep, after this PR portions of the initialization of the rpctest.Harness can also be re-written to utilize the RPC call rather than manually setting the flag on the command line.

[davecgh]

So, I believe I know why you changed the preCfg := cfg copy to preCfg := &cfg originally which is the following vet error:

config.go:377::error: assignment copies lock value to preCfg: main.config contains sync.RWMutex (vet)

Unfortunately, as we've already discussed, it can't be a pointer because of the double parse. Looks like the mutex is going to have to be refactored out of the config struct. That could be a blessing as I suspect it will make the code a little cleaner anyways since the mutex won't have to be littered all throughout the code if it's encapsulated in a concurrent safe type.

[Roasbeef]

I've just pushed a new version of the commit with the addressed review items squashed into place. I've also modified the implementation of the RPC's to utilize a new concurrent-safe struct which houses the mining address. As a result, the implementation is much cleaner as the mutex accesses are no longer strewn across the RPC calls. However, I wasn't quite sure where to put the new struct, so atm it lives in server.go.

[davecgh]

Nice job overall as usual! Also, this needs a rebase to correct the conflicts.

[davecgh]

Now the segwit has been merged, would you please rebase and update this to take care of the final review comments? Thanks @Roasbeef!

[jakesylvestre]

@jcvernaleo (as per #1530)

• Low priority
• Enhancement

Should be closed immediately in favor of #1441