net: fix output of peer address in version message #20212

vasild commented at 12:42 PM on October 21, 2020: member

If -logips -debug=net is specified then we print the contents of the version message we send to the peer, including his address. Because the addresses in the version message use pre-BIP155 encoding they cannot represent a Tor v3 address and we would actually send 16 0s instead (a dummy IPv6 address). However we would print the full address in the log message. Before this fix:

2020-10-21T12:24:17Z send version message: version 70016, blocks=653500, us=[::]:0, them=xwjtp3mj427zdp4tljiiivg2l5ijfvmt5lcsfaygtpp6cw254kykvpyd.onion:8333, peer=0

This is confusing because we pretend to send one thing while we actually send another. Adjust the printout to reflect what we are sending. After this fix:

2020-10-21T12:26:54Z send version message: version 70016, blocks=653500, us=[::]:0, them=[::]:0, peer=0

net: fix output of peer address in version message

If `-logips -debug=net` is specified then we print the contents of the
version message we send to the peer, including his address. Because the
addresses in the version message use pre-BIP155 encoding they cannot
represent a Tor v3 address and we would actually send 16 `0`s instead (a
dummy IPv6 address). However we would print the full address in the log
message. Before this fix:

```
2020-10-21T12:24:17Z send version message: version 70016, blocks=653500, us=[::]:0, them=xwjtp3mj427zdp4tljiiivg2l5ijfvmt5lcsfaygtpp6cw254kykvpyd.onion:8333, peer=0
```

This is confusing because we pretend to send one thing while we actually
send another. Adjust the printout to reflect what we are sending. After
this fix:

```
2020-10-21T12:26:54Z send version message: version 70016, blocks=653500, us=[::]:0, them=[::]:0, peer=0
```

af3b0dfc54

fanquake added the label P2P on Oct 21, 2020

promag commented at 11:08 PM on October 21, 2020: member

Concept ACK.

I may be getting this wrong, but IIUC this change also affects PushMessage and because of serialization the end result is the same. Is this desired?

vasild commented at 8:47 AM on October 22, 2020: member

@promag, before this change we would pass CAddress{tor v3} to PushMessage, after this change we will pass CAddress{default constructed}. Both however would serialize as 16 zero bytes.

It could be done also in another way:

// leave PushMessage untouched
// log print:
if addr is v1 compatible
  print addr
else
  print default constructed CAddress

I think that would be a few more lines of code.

promag commented at 9:26 AM on October 22, 2020: member

Or just inlined, like addrYou.IsAddrV1Compatible() ? addrYou : CAddress(CService(), addrYou.nServices)?

Edit, just wanted to raise the difference in the push message call. Again, end result is same for the moment.

vasild force-pushed on Oct 22, 2020

vasild commented at 12:47 PM on October 22, 2020: member

Or just inlined, like ...

Yes (modulo a missing .ToString()).

Adjusted to leave PushMessage() untouched by this change as it looks somewhat safer that way.

promag commented at 1:06 PM on October 22, 2020: member

Code review ACK

in src/net_processing.cpp:508 in 575ef6c4cf outdated

 504 | @@ -505,6 +505,9 @@ static void PushNodeVersion(CNode& pnode, CConnman& connman, int64_t nTime)
 505 |              nonce, strSubVersion, nNodeStartingHeight, ::g_relay_txes && pnode.m_tx_relay != nullptr));
 506 |  
 507 |      if (fLogIPs) {
 508 | +        if (!addrYou.IsAddrV1Compatible()) {

promag commented at 1:07 PM on October 22, 2020:

nit a comment would be nice.

vasild commented at 4:26 PM on October 22, 2020:

Added :)

vasild force-pushed on Oct 22, 2020

DrahtBot commented at 10:48 PM on October 22, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#20217 (net: Remove g_relay_txes by jnewbery)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

promag commented at 11:26 PM on October 22, 2020: member

Code review ACK 5486736cc2edba7e817dc1b13db5268165a22a6b.

MarcoFalke commented at 3:40 PM on October 28, 2020: member

Is this for 0.21?

vasild commented at 4:51 PM on October 28, 2020: member

It is a low impact problem but also low risk patch. The problem did not exist in 0.20 because in 0.20 all addresses are addrv1 compatible.

I would say "yes, for 0.21", but YMMV.

MarcoFalke commented at 3:58 PM on November 2, 2020: member

review ACK 5486736cc2edba7e817dc1b13db5268165a22a6b

The actual address that is no longer logged in this line should still be logged in another line when the connection opens:

    LogPrint(BCLog::NET, "connection from %s accepted\n", addr.ToString());

or

    LogPrint(BCLog::NET, "trying connection %s lastseen=%.1fhrs\n",

MarcoFalke added the label Utils/log/libs on Nov 2, 2020

MarcoFalke added this to the milestone 0.21.0 on Nov 2, 2020

jnewbery commented at 10:35 AM on November 3, 2020: member

From a style perspective, I prefer the original version, where it's immediately obvious that what's being sent out on the wire is the same as what's being logged.

MarcoFalke commented at 11:09 AM on November 3, 2020: member

Agree with @jnewbery . Happy to re-ACK if you decide to re-push the initial version.

vasild force-pushed on Nov 3, 2020

vasild commented at 12:43 PM on November 3, 2020: member

I think it is fine either way. 2 vs 1 wins on the older version. Restored.

MarcoFalke commented at 12:53 PM on November 3, 2020: member

review ACK af3b0dfc5463c42fb9bff39f020fc1728ed44bc7

jnewbery commented at 2:58 PM on November 3, 2020: member

utACK af3b0dfc5463c42fb9bff39f020fc1728ed44bc7

in src/net_processing.cpp:501 in af3b0dfc54

 497 | @@ -498,7 +498,9 @@ static void PushNodeVersion(CNode& pnode, CConnman& connman, int64_t nTime)
 498 |      NodeId nodeid = pnode.GetId();
 499 |      CAddress addr = pnode.addr;
 500 |  
 501 | -    CAddress addrYou = (addr.IsRoutable() && !IsProxy(addr) ? addr : CAddress(CService(), addr.nServices));
 502 | +    CAddress addrYou = addr.IsRoutable() && !IsProxy(addr) && addr.IsAddrV1Compatible() ?

jnewbery commented at 6:33 PM on November 3, 2020:

If you touch this branch again, it might be worth commenting why you're calling IsAddrV1Compatible():

    // version messages can only include addr v1 addresses, since the address
    // fields are a fixed size.
    CAddress addrYou = addr.IsRoutable() && !IsProxy(addr) && addr.IsAddrV1Compatible() ?

MarcoFalke merged this on Nov 4, 2020

MarcoFalke closed this on Nov 4, 2020

vasild deleted the branch on Nov 4, 2020

sidhujag referenced this in commit fa0ff9a721 on Nov 4, 2020

PastaPastaPasta referenced this in commit 95961e06ab on Jun 27, 2021

PastaPastaPasta referenced this in commit 5b62a5c27e on Jun 28, 2021

PastaPastaPasta referenced this in commit 880dbbb94a on Jun 29, 2021

PastaPastaPasta referenced this in commit f18c2331bb on Jul 1, 2021

PastaPastaPasta referenced this in commit 13c0a93383 on Jul 1, 2021

PastaPastaPasta referenced this in commit d363adb494 on Sep 17, 2021

PastaPastaPasta referenced this in commit c92284b3a9 on Sep 19, 2021

PastaPastaPasta referenced this in commit 4c3f917b54 on Sep 21, 2021

PastaPastaPasta referenced this in commit fbf6c930b2 on Sep 24, 2021

kittywhiskers referenced this in commit cefa979fd4 on Oct 12, 2021

Fabcien referenced this in commit a4505a0f69 on Dec 1, 2021

DrahtBot locked this on Feb 15, 2022