[rebase of #845] rpc: add new btcd extension commands: addminingaddr, delminingaddr, listminingaddrs #1441

yashbhutwala · 2019-06-26T01:29:25Z

This is a rebase of #845 on master, closes #1411 😃

This commit adds three new btcd extension commands: addminingaddr, delminingaddr, and listminingaddrs. These new commands allows users to dynamically add, remove and list the current set of mining addresses available to btcd.

This commit implements a new btcd extension RPC calls: addminingaddr, delminingaddr, and listminingaddrs. These RPC calls allows a caller to add, remove and list mining addresses dynamically without restarting a running btcd node. Within btcd’s set of active mining addresses, all addresses must be unique. Incoming RPC calls that attempt to add duplicate generation addresses are ignored. Similarly, any attempts to delete a non-existent mining address will result in an error. In order to implement this new RPC call in a thread-safe manner, a new mutex has been added to synchronize access the `miningAddrs` attribute within the `config` struct.

This commit modifies the existing `cpuminer.Config` struct to replace with the `MiningAddrs` attribute and manual generation address selection with a new function closure. When called, this function closure returns a mining address to be used within the next generated block. This modification adds a layer of abstraction over the storage and selection logic for obtaining a new mining address.

…gaddrs

This commit fixes a slight bug in the parsing of the supplied mining addresses within the configuration. Before this commit, the code was checking the length of `cfg.MiningAddrs` (the populated config value) instead of `cfg.miningAddrs` the value in which the valid mining addresses are stored within.

dannypaz · 2019-07-21T03:38:08Z

Would love to have this functionality to test apps w/ btcd (neutrino). @Roasbeef

dannypaz · 2019-07-21T21:31:39Z

FYI was seeing a null pointer reference when running this code in test. s.server returns null. The fix is to add a second arg s server to newRpcServer to set the server property for those methods.

jakesylvestre · 2020-03-04T13:32:25Z

@jcvernaleo (as per #1530)

Low priority
Enhancement

dannypaz · 2020-03-05T20:13:05Z

What needs to be done for this to be merged?

jcvernaleo · 2020-03-05T21:56:56Z

@dannypaz if you could rebase and squash it that would definitely help on my end. I know you rebased this a long time ago but since I'm trying to finally get some of these old PRs in there is going to be some churn. This one is pretty well reviewed already so it is high on my list.

jakesylvestre · 2020-03-05T21:58:23Z

Will take another quick pass at reviewing in the mean time once rebased

Rjected

I'm really pushing for the use of an interface here, as there is state coupled with the MiningAddr method, and a struct had to be defined in order to implement MiningAddr.

Rjected · 2020-05-25T23:45:44Z

mining/cpuminer/cpuminer.go

+	// MiningAddr is a function which when called yields an address to
+	// to use as the payment address for generated blocks.
+	MiningAddr func() btcutil.Address


Given your implementation of this function requires you to define a struct miningAddrStore and it exists for the sole purpose of implementing this function, I think it would be a lot cleaner to put an interface here. Then, we don't have to add anything to the server struct in server.go.

Rjected · 2020-05-25T23:56:39Z

btcd.go

@@ -331,3 +333,7 @@ func main() {
 		os.Exit(1)
 	}
 }
+
+func init() {
+	prand.Seed(time.Now().UnixNano())


Why create an init() in the main btcd.go just for the sake of seeding rand? Maybe this should go somewhere else, as math/rand is used elsewhere in the codebase - see peer/peer.go.

Rjected · 2020-05-25T23:59:10Z

docs/json_rpc_api.md

@@ -672,7 +675,6 @@ The following is an overview of the RPC methods which are implemented by btcd, b
 |Description|Returns the version of the JSON-RPC API built into this release of btcd.|
 |Returns|`{ (json object)`<br />&nbsp;&nbsp;`"btcdjsonrpcapi": {`<br />&nbsp;&nbsp;&nbsp;&nbsp;`"versionstring": "x.y.z",  (string) the version of the JSON-RPC API`<br />&nbsp;&nbsp;&nbsp;&nbsp;`"major": x,  (numeric) the major version of the JSON-RPC API`<br />&nbsp;&nbsp;&nbsp;&nbsp;`"minor": y,  (numeric) the minor version of the JSON-RPC API`<br />&nbsp;&nbsp;&nbsp;&nbsp;`"patch": z,  (numeric) the patch version of the JSON-RPC API`<br />&nbsp;&nbsp;&nbsp;&nbsp;`"prerelease": "",  (string) prerelease info for the JSON-RPC API`<br />&nbsp;&nbsp;&nbsp;&nbsp;`"buildmetadata": ""  (string) metadata about the server build`<br />&nbsp;&nbsp;`}`<br />`}`|
 |Example Return|`{`<br />&nbsp;&nbsp;`"btcdjsonrpcapi": {`<br />&nbsp;&nbsp;&nbsp;&nbsp;`"versionstring": "1.0.0",`<br />&nbsp;&nbsp;&nbsp;&nbsp;`"major": 1,  `<br />&nbsp;&nbsp;&nbsp;&nbsp;`"minor": 0,`<br />&nbsp;&nbsp;&nbsp;&nbsp;`"patch": 0,`<br />&nbsp;&nbsp;&nbsp;&nbsp;`"prerelease": "",`<br />&nbsp;&nbsp;&nbsp;&nbsp;`"buildmetadata": ""`<br />&nbsp;&nbsp;`}`<br />`}`|
-[Return to Overview](#MethodOverview)<br />


Should un-delete this

Rjected · 2020-05-25T23:59:50Z

docs/json_rpc_api.md

@@ -685,6 +687,43 @@ The following is an overview of the RPC methods which are implemented by btcd, b
 |Description|Returns block headers starting with the first known block hash from the request.|
 |Returns|`[ (json array of strings)`<br />&nbsp;&nbsp;`"blockheader",`<br />&nbsp;&nbsp;`...`<br />`]`|
 |Example Return|`[`<br />&nbsp;&nbsp;`"0000002099417930b2ae09feda10e38b58c0f6bb44b4d60fa33f0e000000000000000000d53...",`<br />&nbsp;&nbsp;`"000000203ba25a173bfd24d09e0c76002a910b685ca297bd09a17b020000000000000000702..."`<br />`]`|
+


Need to add another "Return to overview" here for getheaders.

Rjected · 2020-05-26T00:01:39Z

rpcserver.go

 	"addnode":               handleAddNode,
 	"createrawtransaction":  handleCreateRawTransaction,
 	"debuglevel":            handleDebugLevel,
 	"decoderawtransaction":  handleDecodeRawTransaction,
 	"decodescript":          handleDecodeScript,
-	"estimatefee":           handleEstimateFee,


Why is estimatefee removed here?

Rjected · 2020-05-26T00:09:46Z

rpcserver.go

+	if err != nil {
+		return nil, &btcjson.RPCError{
+			Code:    btcjson.ErrRPCInvalidAddressOrKey,
+			Message: "Invalid address or key: " + err.Error(),


Should use a fmt.Sprintf here

Rjected · 2020-05-26T00:14:28Z

rpcserver.go

+			Message: fmt.Sprintf("Mining address %v is on the "+
+				"wrong network: ", activeNetParams.Params),


Wouldn't this just substitute the %v with activeNetParams.Params? Maybe the line should be fmt.Sprintf("Mining address %s is on the wrong network: %s", addr.String(), activeNetParams.Params.Name) or something similar.

Rjected · 2020-05-26T00:15:24Z

rpcserver.go

+	if err := s.server.miningAddrs.RemoveAddr(addr); err != nil {
+		return nil, &btcjson.RPCError{
+			Code:    btcjson.ErrRPCInvalidAddressOrKey,
+			Message: fmt.Sprintf("Mining address %v not found", addr),


Would be useful to use the String() method on address here rather than just %v'ing it

Rjected · 2020-05-26T00:17:07Z

rpcserver.go

+			Message: "Invalid address or key: " + err.Error(),
+		}
+	}
+
+	// Ensure the the passed address belongs to the same network that btcd
+	// is currently on.
+	if !addr.IsForNet(activeNetParams.Params) {
+		return nil, &btcjson.RPCError{
+			Code: btcjson.ErrRPCInvalidAddressOrKey,
+			Message: fmt.Sprintf("Mining address %v is on the "+
+				"wrong network: ", activeNetParams.Params),


Same comments as above, use fmt.Sprintf for line 926 and modify the Message to be more informative in line 935 and 936

Rjected · 2020-05-26T00:27:30Z

rpcserverhelp.go

@@ -29,6 +29,10 @@ var helpDescsEnUS = map[string]string{
 	"debuglevel--result0":    "The string 'Done.'",
 	"debuglevel--result1":    "The list of subsystems",

+	// AddMiningAddrCmd help.
+	"addminingaddr--synopsis": "Adds an additional available mining address to the set of mining addresses for the daemon",


Maybe remove "for the daemon"

jakesylvestre · 2020-08-31T19:39:25Z

@yashbhutwala we're going to have to rebase against master to fix these conflicts and get these tests to stop running on travis

Roasbeef and others added 7 commits June 25, 2019 21:00

btcjson: add btcd extension commands for mining addr control

7aeca79

This commit adds three new btcd extension commands: addminingaddr, delminingaddr, and listminingaddrs. These new commands allows users to dynamically add, remove and list the current set of mining addresses available to btcd.

integration: add tests for addminingaddr, delminingaddr and listminin…

3130c12

…gaddrs

docs/json_rpc_api: document the new mining addr control RPCs

41c3df5

patch based on @davecgh's last review comments on btcsuite#845

8d2cbad

dannypaz mentioned this pull request Jul 21, 2019

added server to rpcserver dannypaz/btcd#2

Merged

jakesylvestre mentioned this pull request Mar 4, 2020

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

Open

jcvernaleo added enhancement low priority labels Mar 4, 2020

Rjected suggested changes May 26, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[rebase of #845] rpc: add new btcd extension commands: addminingaddr, delminingaddr, listminingaddrs #1441

[rebase of #845] rpc: add new btcd extension commands: addminingaddr, delminingaddr, listminingaddrs #1441

yashbhutwala commented Jun 26, 2019

dannypaz commented Jul 21, 2019

dannypaz commented Jul 21, 2019

jakesylvestre commented Mar 4, 2020

dannypaz commented Mar 5, 2020

jcvernaleo commented Mar 5, 2020

jakesylvestre commented Mar 5, 2020

Rjected left a comment

Rjected May 25, 2020

Rjected May 25, 2020

Rjected May 25, 2020

Rjected May 25, 2020

Rjected May 26, 2020

Rjected May 26, 2020

Rjected May 26, 2020

Rjected May 26, 2020

Rjected May 26, 2020

Rjected May 26, 2020

jakesylvestre commented Aug 31, 2020

		Message: fmt.Sprintf("Mining address %v is on the "+
		"wrong network: ", activeNetParams.Params),

[rebase of #845] rpc: add new btcd extension commands: addminingaddr, delminingaddr, listminingaddrs #1441

Are you sure you want to change the base?

[rebase of #845] rpc: add new btcd extension commands: addminingaddr, delminingaddr, listminingaddrs #1441

Conversation

yashbhutwala commented Jun 26, 2019

dannypaz commented Jul 21, 2019

dannypaz commented Jul 21, 2019

jakesylvestre commented Mar 4, 2020

dannypaz commented Mar 5, 2020

jcvernaleo commented Mar 5, 2020

jakesylvestre commented Mar 5, 2020

Rjected left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakesylvestre commented Aug 31, 2020