Move special CAddress-without-nTime logic to net_processing #20541

sipa · 2020-12-01T19:04:37Z

Historically, the VERSION message contains an "addrMe" and an "addrYou". As these are sent before version negotiation is complete, the protocol version is INIT_PROTO_VERSION (209), and in that protocol, CAddress is serialized without nTime.

This is in fact the only situation left where a CAddress is (de)serialized without nTime. As it's such a simple structure (CService for ip/port + uint64_t for nServices), just inline that structure in the few places where it occurs, and remove the logic for dealing with missing nTime from CAddress.

DrahtBot · 2020-12-02T05:16:00Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#22778 (net processing: Reduce resource usage for inbound block-relay-only connections by jnewbery)
#22777 (net processing: don't request tx relay on feeler connections by jnewbery)
#21160 (net/net processing: Move tx inventory into net_processing 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.

src/net_processing.cpp

src/protocol.h

maflcko · 2020-12-02T10:41:31Z

Concept ACK.

Does this conflict with any backports we might want to do for 0.21?

sipa · 2020-12-02T23:23:54Z

@MarcoFalke I don't think so. It doesn't conflict with #20516 (either can be backported without affecting the other). Anything else you're thinking of?

vasild

ACK 158c0ec1

It is somewhat worrying that all tests passed with the swapped services and address, but when I think about it - it is 8 bytes service flags + 16 bytes address + 2 bytes port. I guess we can't do any sanity check on those as any 26 random bytes could be valid.

maflcko · 2020-12-03T07:28:25Z

It is somewhat worrying that all tests passed with the swapped services and address

Indeed. I think we should add unit test vectors for all message types with an example message and the expected deserialization? Assert that both are identical. Though, message format (deserialization) is handled in the same function that does the processing, so I am not sure how to test this in general without rewriting net_processing.

sipa · 2020-12-03T19:08:57Z

It is somewhat worrying that all tests passed with the swapped services and address

It is, but that's at least partially explained by the fact that we barely use these values. One is ignored entirely, the other is used for statistics about our local IP. The nService fields are also ignored entirely.

I think we should add unit test vectors for all message types with an example message and the expected deserialization?

That makes sense.

naumenkogs · 2020-12-04T01:48:49Z

Concept ACK

lontivero · 2020-12-04T02:55:53Z

ACK 158c0ec

sipa · 2021-07-03T00:13:41Z

Rebased.

jnewbery

Concept ACK.

src/net_processing.cpp

jnewbery · 2021-07-06T08:32:51Z

src/net_processing.cpp

I'm not sure that references to version 31402 are very useful (beyond being a historical footnote). The version message contains {services, ipv6 addr, port} elements - the fact that that also used to be how address records used to be serialized in addr messages is not helpful in understanding the current protocol.

I think they're useful for the time being, as I'd expect more descriptions and documentations of the protocol will keep referring to these fields as CAddresses.

Seems reasonable

jnewbery · 2021-07-06T08:41:16Z

When we receive a version message, we ignore the following fields:

(4) receiver's services
(7) sender's services (duplicate with field 2 - services)
(8/9) sender's ipv6 addr/port

Perhaps in the next p2p version bump, we should explicitly document those fields as unused, since there's no purpose for them.

vasild

ACK 775954ac5ffd2b0d6f89a5b35e4120f0d170c884

src/net_processing.cpp

Historically, the VERSION message contains an "addrMe" and an "addrYou". As these are sent before version negotiation is complete, the protocol version is INIT_PROTO_VERSION (209), and in that protocol, CAddress is serialized without nTime. This is in fact the only situation left where a CAddress is (de)serialized without nTime. As it's such a simple structure (CService for ip/port + uint64_t for nServices), just inline that structure in the few places where it occurs, and remove the logic for dealing with missing nTime from CAddress.

The us=... message in the debug log when sending a version message is always [::]:0, because we no longer send our own address there. Therefore, this information is entirely redundant. Remove it.

Zero-1729

crACK 75290ae

jonatack

ACK 75290ae

Tested on signet

$ ./src/bitcoind -signet -debug=net -logips
$ grep version ~/.bitcoin/signet/debug.log
...
2021-08-22T15:01:44Z received: version (103 bytes) peer=6
2021-08-22T15:01:44Z sending version (109 bytes) peer=6
2021-08-22T15:01:44Z send version message: version 70016, blocks=52221, them=[::]:0, txrelay=1, peer=6
2021-08-22T15:01:44Z receive version message: /Satoshi:22.99.0/: version 70016, blocks=52221, us=[::]:0, txrelay=1, peer=6, peeraddr=127.0.0.1:40326
2021-08-22T15:02:06Z sending version (109 bytes) peer=7
2021-08-22T15:02:06Z send version message: version 70016, blocks=52221, them=[::]:0, txrelay=1, peer=7
2021-08-22T15:02:07Z received: version (103 bytes) peer=7
2021-08-22T15:02:07Z receive version message: /Satoshi:22.99.0/: version 70016, blocks=52221, us=[::]:0, txrelay=1, peer=7, peeraddr=q3z526imdoo2pdujkfp5vslshni2niewlbxfokejfkddkezxutagezyd.onion:38333
2021-08-22T15:02:07Z New outbound peer connected: version: 70016, blocks=52221, peer=7, peeraddr=q3z526imdoo2pdujkfp5vslshni2niewlbxfokejfkddkezxutagezyd.onion:38333 (outbound-full-relay)
2021-08-22T15:02:11Z sending version (109 bytes) peer=8
2021-08-22T15:02:11Z send version message: version 70016, blocks=52221, them=[::]:0, txrelay=1, peer=8
2021-08-22T15:02:12Z received: version (103 bytes) peer=8
2021-08-22T15:02:12Z receive version message: /Satoshi:21.99.0/: version 70016, blocks=52221, us=[::]:0, txrelay=1, peer=8, peeraddr=uvj2uaxjztqtvmd5d27bgmfyfn6v3c2px2am2utbniuea55eblhid7qd.onion:38333
2021-08-22T15:02:12Z New outbound peer connected: version: 70016, blocks=52221, peer=8, peeraddr=uvj2uaxjztqtvmd5d27bgmfyfn6v3c2px2am2utbniuea55eblhid7qd.onion:38333 (outbound-full-relay)
2021-08-22T15:02:12Z sending version (109 bytes) peer=9
2021-08-22T15:02:12Z send version message: version 70016, blocks=52221, them=95.217.184.148:38333, txrelay=1, peer=9
2021-08-22T15:02:13Z received: version (102 bytes) peer=9
2021-08-22T15:02:13Z receive version message: /Satoshi:0.21.1/: version 70016, blocks=52221, us=184.75.221.35:38484, txrelay=1, peer=9, peeraddr=95.217.184.148:38333
2021-08-22T15:02:13Z New outbound peer connected: version: 70016, blocks=52221, peer=9, peeraddr=95.217.184.148:38333 (outbound-full-relay)
2021-08-22T15:02:20Z sending version (109 bytes) peer=10
2021-08-22T15:02:20Z send version message: version 70016, blocks=52221, them=[::]:0, txrelay=1, peer=10
2021-08-22T15:02:21Z received: version (102 bytes) peer=10
2021-08-22T15:02:21Z receive version message: /Satoshi:0.21.1/: version 70016, blocks=52221, us=[::]:0, txrelay=1, peer=10, peeraddr=s7fcvn5rblem7tiquhhr7acjdhu7wsawcph7ck44uxyd6sismumemcyd.onion:38333
2021-08-22T15:02:21Z New outbound peer connected: version: 70016, blocks=52221, peer=10, peeraddr=s7fcvn5rblem7tiquhhr7acjdhu7wsawcph7ck44uxyd6sismumemcyd.onion:38333 (outbound-full-relay)

jonatack · 2021-08-22T15:04:40Z

src/net_processing.cpp

-                           CAddress(CService(), addr.nServices);
-    CAddress addrMe = CAddress(CService(), nLocalNodeServices);
+    CService addr_you = addr.IsRoutable() && !IsProxy(addr) && addr.IsAddrV1Compatible() ? addr : CService();
+    uint64_t your_services{addr.nServices};


nit, can be const

- uint64_t my_services{pnode.GetLocalServices()}; - uint64_t nonce = pnode.GetLocalNonce(); + const uint64_t my_services{pnode.GetLocalServices()}; + const uint64_t nonce{pnode.GetLocalNonce()}; const int nNodeStartingHeight{m_best_height}; - NodeId nodeid = pnode.GetId(); + const NodeId nodeid{pnode.GetId()}; CAddress addr = pnode.addr; - CService addr_you = addr.IsRoutable() && !IsProxy(addr) && addr.IsAddrV1Compatible() ? addr : CService(); - uint64_t your_services{addr.nServices}; + const CService addr_you{addr.IsRoutable() && !IsProxy(addr) && addr.IsAddrV1Compatible() ? addr : CService()}; + const uint64_t your_services{addr.nServices};

Will include this if I have to retouch for other reasons.

vasild

ACK 75290ae

maflcko · 2021-08-23T16:20:09Z

crACK

…_proces…

jnewbery · 2021-08-25T15:47:38Z

Code review ACK 75290ae

The comment here:

bitcoin/src/protocol.h

Line 439 in 75290ae

    
           //! Always included in serialization, except in the network format on INIT_PROTO_VERSION.

Can probably be removed at this point.

maflcko · 2021-08-25T15:49:16Z

#22780

sipa added P2P Refactoring labels Dec 1, 2020

sipa force-pushed the 202012_addr_without_time_is_no_addr branch from 5ce0928 to ebe8db0 Compare December 1, 2020 19:30

This was referenced Dec 2, 2020

Well-defined CAddress disk serialization, and addrv2 anchors.dat #20516

Merged

net: CAddress deser: use stream's version, not what's coming from disk #20509

Closed

net: Remove g_relay_txes #20217

Merged

vasild reviewed Dec 2, 2020

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

src/net_processing.cpp Outdated Show resolved Hide resolved

src/net_processing.cpp Outdated Show resolved Hide resolved

src/net_processing.cpp Outdated Show resolved Hide resolved

src/protocol.h Outdated Show resolved Hide resolved

sipa force-pushed the 202012_addr_without_time_is_no_addr branch from ebe8db0 to 158c0ec Compare December 2, 2020 23:21

vasild approved these changes Dec 3, 2020

View reviewed changes

bitcoin deleted a comment Dec 3, 2020

DrahtBot added the Needs rebase label Dec 10, 2020

bitcoin deleted a comment Mar 8, 2021

sipa force-pushed the 202012_addr_without_time_is_no_addr branch from 158c0ec to 775954a Compare July 3, 2021 00:13

DrahtBot removed the Needs rebase label Jul 3, 2021

jnewbery reviewed Jul 6, 2021

View reviewed changes

vasild approved these changes Jul 9, 2021

View reviewed changes

DrahtBot mentioned this pull request Aug 13, 2021

net/net processing: Move tx inventory into net_processing #21160

Merged

sipa force-pushed the 202012_addr_without_time_is_no_addr branch from 775954a to 79aa69c Compare August 20, 2021 19:17

maflcko reviewed Aug 21, 2021

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

src/net_processing.cpp Outdated Show resolved Hide resolved

src/net_processing.cpp Outdated Show resolved Hide resolved

sipa added 2 commits August 21, 2021 19:16

Drop us=... message in net debug for sending version message

75290ae

The us=... message in the debug log when sending a version message is always [::]:0, because we no longer send our own address there. Therefore, this information is entirely redundant. Remove it.

sipa force-pushed the 202012_addr_without_time_is_no_addr branch from 79aa69c to 75290ae Compare August 21, 2021 23:17

Zero-1729 reviewed Aug 22, 2021

View reviewed changes

jonatack reviewed Aug 22, 2021

View reviewed changes

This was referenced Aug 23, 2021

net processing: Reduce resource usage for inbound block-relay-only connections #22778

Merged

net processing: don't request tx relay on feeler connections #22777

Merged

vasild approved these changes Aug 23, 2021

View reviewed changes

maflcko merged commit dbcb574 into bitcoin:master Aug 23, 2021

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 23, 2021

Merge bitcoin#20541: Move special CAddress-without-nTime logic to net…

4d6494a

…_proces…

bitcoin locked as resolved and limited conversation to collaborators Aug 25, 2022

Move special CAddress-without-nTime logic to net_processing #20541

Move special CAddress-without-nTime logic to net_processing #20541

Uh oh!

Conversation

sipa commented Dec 1, 2020

Uh oh!

DrahtBot commented Dec 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maflcko commented Dec 2, 2020

Uh oh!

sipa commented Dec 2, 2020

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Dec 3, 2020

Uh oh!

sipa commented Dec 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

naumenkogs commented Dec 4, 2020

Uh oh!

lontivero commented Dec 4, 2020

Uh oh!

sipa commented Jul 3, 2021

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jnewbery Jul 6, 2021

Choose a reason for hiding this comment

Uh oh!

sipa Aug 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery Aug 25, 2021

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Jul 6, 2021

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Zero-1729 left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonatack Aug 22, 2021

Choose a reason for hiding this comment

Uh oh!

sipa Aug 23, 2021

Choose a reason for hiding this comment

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Aug 23, 2021

Uh oh!

jnewbery commented Aug 25, 2021

Uh oh!

maflcko commented Aug 25, 2021

Uh oh!

Reviewers

DrahtBot commented Dec 2, 2020 •

edited

Loading

sipa commented Dec 3, 2020 •

edited

Loading

sipa Aug 20, 2021 •

edited

Loading

jonatack left a comment •

edited

Loading