Rework addnode behaviour

sipa commented at 1:43 pm on May 28, 2016: member

This is an alternative to #8097 that:

Maintains consistency between ThreadOpenAddedNodeConnections and getaddednodeinfo, without any DNS resolving from RPC.
Deals with the use case of added names whose IP mapping changes over time (switching over to the new IP only when the connection to the old one closes, though).
Gets rid of the fDns argument to getaddednodeinfo (turning it into a dummy)
Simplifies the code significantly by introducing a shared GetAddedNodeInfo in net.cpp.
The existing addnode logic that tries all resolved addresses for a name is pushed down into ConnectSocketByName (picking at random).
The existing addnode logic to prevent multiple connections to the same IP is pushed down into ConnectNode (by storing colliding names into the CNode’s addrName field).

Untested.

sipa force-pushed on May 28, 2016

laanwj commented at 11:26 am on May 30, 2016: member

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.

sipa commented at 12:39 pm on May 30, 2016: member

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…

laanwj commented at 1:15 pm on May 31, 2016: member

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?

theuni commented at 3:39 pm on May 31, 2016: member

Concept ACK. Thanks, @sipa!

Will review in detail.

theuni commented at 0:21 am on June 1, 2016: member

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 https://github.com/bitcoin/bitcoin/pull/8085/commits/fc2561f7803e9b354c152bd4bd92678bd941fcb0 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.

sipa commented at 1:44 pm on June 1, 2016: member

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?

theuni commented at 4:24 pm on June 1, 2016: member

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.

theuni commented at 1:04 am on June 4, 2016: member

@sipa This needs https://github.com/theuni/bitcoin/commit/c22dc8eb0a4a598c159559e7a3140bc6af653c56 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.

in src/net.cpp: in ea5fa8038b outdated

406+            // Also store the name we used to connect in that CNode, so that future FindNode() calls to that
407+            // name catch this early.
408+            CNode* pnode = FindNode((CService)addrConnect);
409+            if (pnode)
410+            {
411+                pnode->AddRef();

theuni commented at 1:42 am on June 4, 2016:

What Release()s this?

sipa commented at 10:42 pm on June 6, 2016:

I just copied the behaviour of the “// Look for existing connection” logic above. I assume that ConnectNode always returns something with an incremented refcount that the caller is supposed to deal with.

theuni commented at 4:15 pm on June 8, 2016:

Ok. I was already suspicious of the current refcounting logic, but copying the current behavior makes sense. I’ll look into that separately.

sipa force-pushed on Jun 8, 2016

sipa commented at 12:23 pm on June 8, 2016: member

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

laanwj commented at 6:32 am on June 9, 2016: member

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

Rework addnode behaviour

* Use CNode::addeName to track whether a connection to a name is already open
  * A new connection to a previously-connected by-name addednode is only opened when
    the previous one closes (even if the name starts resolving to something else)
  * At most one connection is opened per addednode (even if the name resolves to multiple)
* Unify the code between ThreadOpenAddedNodeConnections and getaddednodeinfo
  * Information about open connections is always returned, and the dns argument becomes a dummy
  * An IP address and inbound/outbound is only reported for the (at most 1) open connection

1111b80df8

Prevent duplicate connections where one is by name and another by ip f9f5cfc506

Randomize name lookup result in ConnectSocketByName 1a5a4e6488

sipa force-pushed on Jun 13, 2016

sipa commented at 9:59 pm on June 13, 2016: member

Rebased.

laanwj commented at 10:28 am on June 14, 2016: member

Nit: let’s make the dummy argument optional.

laanwj commented at 11:32 am on June 14, 2016: member

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):

 013:31:26 getaddednodeinfo false
 1
 213:31:26
 3[
 4  {
 5    "addednode": "testnet-seed.bitcoin.petertodd.org",
 6    "connected": false,
 7    "addresses": [
 8    ]
 9  }
10]

laanwj commented at 11:46 am on June 14, 2016: member

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

0  {
1    "addednode": "testnettest.com",
2    "connected": true,
3    "addresses": [
4      {
5        "address": "52.72.156.74:18333",
6        "connected": "outbound"
7      }
8    ]
9  }

TheBlueMatt commented at 7:38 pm on June 14, 2016: member

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.

gmaxwell commented at 10:17 am on June 15, 2016: contributor

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

laanwj merged this on Jun 16, 2016

laanwj closed this on Jun 16, 2016

laanwj referenced this in commit 3f89a534ac on Jun 16, 2016

MarcoFalke commented at 3:03 pm on June 17, 2016: member

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

 02016-06-13 23:42:03 receive version message: /bitcoinj:0.14.1/: version 70001, blocks=0, us=127.0.0.1:8333, peer=12469
 12016-06-13 23:42:03 AdvertiseLocal: advertising address [2a01:4f8:172:10da::2]:8333
 22016-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
 32016-06-13 23:42:05 AdvertiseLocal: advertising address 138.201.55.219:8333
 42016-06-13 23:42:06 POTENTIAL DEADLOCK DETECTED
 52016-06-13 23:42:06 Previous lock order was:
 62016-06-13 23:42:06  pnode->cs_vRecvMsg  net.cpp:1731 (TRY)
 72016-06-13 23:42:06  cs_main  main.cpp:4441
 82016-06-13 23:42:06  (1) pfrom->cs_filter  main.cpp:4497
 92016-06-13 23:42:06  (2) cs_vSend  net.cpp:2437
102016-06-13 23:42:06 Current lock order is:
112016-06-13 23:42:06  (2) pnode->cs_vSend  net.cpp:1750 (TRY)
122016-06-13 23:42:06  cs_main  main.cpp:5693 (TRY)
132016-06-13 23:42:06  pto->cs_inventory  main.cpp:5896
142016-06-13 23:42:06  (1) pto->cs_filter  main.cpp:5919
152016-06-17 13:52:54

random-zebra referenced this in commit 0b84a5025d on May 26, 2020

DrahtBot locked this on Sep 8, 2021

Rework addnode behaviour #8113