This is a more invasive change than #8097, but concept ACK.

Untested.

Looks like we don't have any RPC tests for getaddednodeinfo, nor addnode besides onetry mode.

Basic testing for getaddednodeinfo/addnode should certainly be added, but the specific case being changed here would be very hard to test without beind able to mock out name resolving...

Yes, agreed. Maybe that's a future possibility after the net refactor. IMO the most important privacy invariant for DNS changes is that we don't ever do DNS queries when -dns=0 is set. Would be nice to have an auto test of some kind for that.

In any case that's a tangent. I think #8097 was nice in that it was only a local change to an (apparently rarely used) RPC call. This involves some regression risk for code outside that as well. On the other hand this is a more thorough solution, and at least one person (@TheBlueMatt) cares about the functionality it seems better to do that.

@theuni can you take a look?

Concept ACK. Thanks, @sipa!

Will review in detail.

This looks great. A few notes:

• GetAddedNodeInfo() doesn't check for pnode->fDisconnect. I'm not sure if it needs to bother (probably not).
• ConnectSocketByName() only tries 1 of the resolved results, which seems to conflict with the PR description. Unless I'm reading incorrectly, adding "foo.com" which resolves to 10 ips will mean 2 min between tries for its ips, with random collisions mixed in.
• It looks like this will mean up to a 2min lag from addnode rpc to attempted first connection. We can introduce a condvar after fc2561f in lieu of the hard-coded wait.

I'll work on some real testing. Yes, I have planned (for later, not initially) for functionality to spoof resolved results for easier dns testing as part of the net refactor. For now, I'll just spoof system-wide.

GetAddedNodeInfo() doesn't check for pnode->fDisconnect. I'm not sure if it needs to bother (probably not).

I don't think that matters. If fDisconnect is set, it tends to be processed quickly.

ConnectSocketByName() only tries 1 of the resolved results, which seems to conflict with the PR description. Unless I'm reading incorrectly, adding "foo.com" which resolves to 10 ips will mean 2 min between tries for its ips, with random collisions mixed in.

Only the random order changed. The old code would consecutively try all IPs from all addnodes, but with an inner loop over the -addnode arguments (with 0.5s delay), and an outer loop (with 120s delay) over the multiple lookup results.

It looks like this will mean up to a 2min lag from addnode rpc to attempted first connection. We can introduce a condvar after fc2561f in lieu of the hard-coded wait.

Sounds like a good idea, but that's independent of this PR, right?

Yikes, yet another misreading of this code on my part. I misunderstood the connection logic post-resolve. So nevermind the ConnectSocketByName comment.

Yes, the condvar can come later.

@sipa This needs theuni@c22dc8e in order for 'getaddednodeinfo' to properly report the connected ip.

Tested (via RPC, at least) ACK with the fix above. The rest looks sane. Though I suspect @TheBlueMatt will object to the behavioral change here. IMO it's more sane in that it lines up with my expectations of how this would work, but it breaks the "addnode a dns entry and it will quickly detect a change and create a new connection" persistence model.

I'd like to point out that this logic is also much easier to re-implement in the refactored code, as the internal connection handler doesn't need to keep up with application state. Instead, it will just retry a dns connection any time the current one disconnects.

Rebased, and incorporated @theuni's fix to the RPC output.

@TheBlueMatt can you give your OK on whether this still supports your use case?

Rebased.

Nit: let's make the dummy argument optional.

I don't have any luck using it to connect to hostnames. E.g. when trying to add a testnet seed (which I know for fact is returing valid addresses most of the time):

13:31:26 getaddednodeinfo false

13:31:26
[
  {
    "addednode": "testnet-seed.bitcoin.petertodd.org",
    "connected": false,
    "addresses": [
    ]
  }
]

Got it to work now, after setting up a fake hostname instead of using a seed, and prodding it a bit:

  {
    "addednode": "testnettest.com",
    "connected": true,
    "addresses": [
      {
        "address": "52.72.156.74:18333",
        "connected": "outbound"
      }
    ]
  }

I'm really not a huge fan of not following DNS changes until after the previous connection dies, but it does not effect me, personally. In any case, all of the addnode logic needs to be better documented somewhere.

utACK. I think the new behavior is more useful and explicable.

Not sure if related but this was reported on IRC by @jonasschnelli:

2016-06-13 23:42:03 receive version message: /bitcoinj:0.14.1/: version 70001, blocks=0, us=127.0.0.1:8333, peer=12469
2016-06-13 23:42:03 AdvertiseLocal: advertising address [2a01:4f8:172:10da::2]:8333
2016-06-13 23:42:05 receive version message: /bitcoinj:0.14.2/Bitcoin Wallet:4.55/: version 70001, blocks=416193, us=127.0.0.1:8333, peer=12470
2016-06-13 23:42:05 AdvertiseLocal: advertising address 138.201.55.219:8333
2016-06-13 23:42:06 POTENTIAL DEADLOCK DETECTED
2016-06-13 23:42:06 Previous lock order was:
2016-06-13 23:42:06  pnode->cs_vRecvMsg  net.cpp:1731 (TRY)
2016-06-13 23:42:06  cs_main  main.cpp:4441
2016-06-13 23:42:06  (1) pfrom->cs_filter  main.cpp:4497
2016-06-13 23:42:06  (2) cs_vSend  net.cpp:2437
2016-06-13 23:42:06 Current lock order is:
2016-06-13 23:42:06  (2) pnode->cs_vSend  net.cpp:1750 (TRY)
2016-06-13 23:42:06  cs_main  main.cpp:5693 (TRY)
2016-06-13 23:42:06  pto->cs_inventory  main.cpp:5896
2016-06-13 23:42:06  (1) pto->cs_filter  main.cpp:5919
2016-06-17 13:52:54