Addnode optimization and addnode access via RPC

[TheBlueMatt]

Useful Changes:

• Moves the DNS lookup of -addnode nodes into the repeated loop, allowing -addnode to follow DNS changes.
• Try more than the first address for a DNS -addnode.

Two new RPC commands:

• addnode . Adds the same as if -addnode were used. Note that it can take up to two minutes before the addnode thread runs again and connects to the new node.
• getaddednodeinfo [node]. Gets the list of added nodes including whether they are connected and to which node they are connected. Or gets the list of connections to [node] given [node] is specified in the same way as with -addnode/addnode.
  eg:

getaddednodeinfo dnsseed.bluematt.me
[
{
"addednode" : "dnsseed.bluematt.me",
"connectedto" : [
"95.154.99.150:8333",
"80.221.217.69:8333"
]
}
]

or

getaddednodeinfo
[
{
"addednode" : "10.232.4.10",
"connectedto" : [
"10.232.4.10:8333"
]
},
{
"addednode" : "dnsseed.bluematt.me",
"connectedto" : [
"95.154.99.150:8333",
"80.221.217.69:8333"
]
}
]

[gavinandresen]

addnode RPC: cool.

getaddednodeinfo : An Object with duplicated keys ("connectedto") is going to cause problems in some languages. And I don't understand the multiple connectedto's-- if I addnode a node, I might get more than one connection to it?

[TheBlueMatt]

Changed to array.

Sure, there are a number of scenarios where you may get more than one connection to a given -addnode (when the -addnode is dns and maps to multiple addresses). -addnode itself wont make more than one connection, but you may get an incoming connection, or happen to connect to another ip that is mapped to from your added node.

[gmaxwell]

Hm. I wonder if it should work instead like this:

getaddednodeinfo -> "addednode": [ "dnsseed.bluematt.me", "foobarbaz.onion" ]
^ what you actually have addnoded

getaddednodeinfo dnsseed.bluematt.me -> "address" : [ "95.154.99.150:8333", "..."]
^ how they are currently being resolved by bitcoind.

And leave the connected status (in/out/not connected) up to getpeerinfo?

Otherwise I think the connected status needs to tristate, in/out/not.

[sipa]

I wonder whether addnode is the right thing to expose - it is intended to be used for stable trusted nodes you know and is something more of a config setting. For an RPC, it may be more useful to have a one-shot command "connect now to X, no matter what".

Eventually, I think the entire network config should be exposed via RPC, but I'd do that via some "setnetconfig [config]" call, rather than specific RPCs for every detail of the config.

[TheBlueMatt]

@gmaxwell In terms of listing the full list of addresses the added node resolves to...yes, that would be better. In terms of making the user make two/three calls to get at the info on whether or not a given added node is connected, I entirely disagree here. I'll add a in/out/not connected flag.

@sipa The goal is to allow a long-running node to add trusted nodes without having to restart. In terms of having a one-shot connection, this was discussed back when addnode was changed, since it used to be a one-off, I was then of the opinion that addnode shouldn't be scrapped to allow for that and keepnode was clearer in this case... In any case, I agree that there should be an option for a one-off connection creator RPC, Ill throw that in when I touch this up (connecttonode?).

[gmaxwell]

@TheBlueMatt Yea, I actually wrote the tristate first and then thought some people might disagree with the duplication.

I'm pretty sure we should have commands to edit addnode on running nodes. I'm tired of restarting my p2pool bitcoind's just to dork around with the addnode settings. Perhaps the oneshot could just be a addnode foo oneshot instead of another RPC?

[TheBlueMatt]

Tweaked as a result of suggestions.
Now you have:
addnode <add|remove|onetry> and getaddednodeinfo [node]
getaddednodeinfo may look something like this:

[
{
"addednode" : "10.232.4.10",
"connected" : true,
"addresses" : [
{
"address" : "10.232.4.10:8333",
"connected" : "outbound"
}
]
},
{
"addednode" : "dnsseed.bluematt.me",
"connected" : true,
"addresses" : [
{
"address" : "[2001:470:9ff2:2:a001:3cff:fea5:a49]:8333",
"connected" : "outbound"
},
{
"address" : "174.106.80.125:8333",
"connected" : "false"
},
{
"address" : "88.201.220.137:8333",
"connected" : "false"
},
...
]
}
]

[jgarzik]

addnode ACK

[luke-jr]

This needs rebasing.

[TheBlueMatt]

Test Plan (can someone run through this to make the powers-that-be happy) :
Replace with an IP of a bitcoin node which you can kill/restart at will.
Replace with a dns name which has a very low TTL which you can change to point to a different IP during testing without waiting too long.
Note that many actions will only take effect after a delay of up to two minutes,
you can change those delays for testing purposes by modifying the Sleep calls in ThreadOpenAddedConnections.

Start a node with no outbound connections (-connect=0.0.0.0). RPC "addnode add".
Check that a new connection to is made and "getaddednodeinfo true" gives the IP Address as connected/outbound.
Kill the node at and verify that it is now reported as unconnected in getaddednodeinfo/getpeerinfo.
Restart the node at and verify a new connection is made.
RPC "addnode remove" and check that the node is no longer in getaddednodeinfo and still connected (in getpeerinfo).
Kill the node at and restart and verify that no new connection to it is made.

RPC "addnode add" where DNS Name is just a simple single IP.
Verify a new connection is made and "getaddednodeinfo true" shows the DNS Name in addition to the IP and the connection is outbound.
Change the IP Address points to and verify that there are now two connections open - one to the old IP Address, one to the new.
getaddednodeinfo should only show the new, getpeerinfo should show both.
Kill both nodes and restart, verify that only the new IP Address gets a new connection.
Change to point to two IP Addresses, verify that only one of the two nodes gets a new connection, kill that node, verify that the other node gets a new connection.

[gavinandresen]

The powers that be will kick in a BTC or two bounty for somebody to execute the test plan (and save their debug.logs somewhere, so we really know they ran the test).

Recruiting a tester from the #bitcoin IRC channel or from https://bitcointalk.org/index.php?board=12.0 seems to work.

[BitcoinPullTester]

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/f2bd6c28e6bddd75d56d580c28f45d2a8be065ab for binaries and test log.

[apoelstra]

Ran the test as BlueMatt suggested. Everything worked correctly. A full report and relevant debug.log files are at

http://download.wpsoftware.net/code/bitcoin-1549/

[gavinandresen]

Thanks @apoelstra , 1 BTC "thanks for testing" tip sent.

ACK on code changes, pulling.