p2p: always set nTime for self-advertisements #25314

mzumsande commented at 7:55 PM on June 8, 2022: contributor

This logic was recently changed in https://github.com/bitcoin/bitcoin/commit/0cfc0cd32239d3c08d2121e028b297022450b320 to overwrite addrLocal with the address they gave us when self-advertising to an inbound peer. But if we don't also change nTime again from the default TIME_INIT, our peer will not relay our advertised address any further.

mzumsande commented at 7:56 PM on June 8, 2022: contributor

ping @vasild

fanquake added the label P2P on Jun 8, 2022

mzumsande force-pushed on Jun 8, 2022

naumenkogs commented at 11:10 AM on June 9, 2022: member

utACK 65749ee3933898b54a77443e647ed5c1d12e0f99 good catch. could be a good functional test?

mzumsande commented at 2:09 PM on June 9, 2022: contributor

good catch. could be a good functional test?

I think that would be hard, because the local addresses in the functional tests are unroutable, so nodes don't self-advertise there. There is even the workaround of the -addrmantest parameter used in p2p_node_network_limited.py for this, but that just returns a fixed addr so the code changed here isn't executed. However, the unit test net_tests/get_local_addr_for_peer_port covers this (and would fail without the update in this PR). Before, it would just check that the expected and actual CAddress have a default-initialized nTime.

in src/net.cpp:254 in 65749ee393 outdated

 250 | @@ -251,6 +251,7 @@ std::optional<CAddress> GetLocalAddrForPeer(CNode *pnode)
 251 |              // For inbound connections, assume both the address and the port
 252 |              // as seen from the peer.
 253 |              addrLocal = CAddress{pnode->GetAddrLocal(), addrLocal.nServices};
 254 | +            addrLocal.nTime = GetAdjustedTime();

vasild commented at 3:32 PM on June 9, 2022:

Here we want to preserve the nTime as it was before the addrLocal = assignment. So it is better to use addrLocal.nTime instead of GetAdjustedTime() because using the latter seeps knowledge that GetLocalAddress() has set nTime to GetAdjustedTime() and so if that is changed to set it to e.g. GetFooTime(), this line would have to be changed too.

Luckily CAddress has a constructor that takes nTime which is handy:

addrLocal = CAddress{pnode->GetAddrLocal(), addrLocal.nServices, addrLocal.nTime};

mzumsande commented at 6:09 PM on June 9, 2022:

Good suggestion, will do with the next push.

in src/test/net_tests.cpp:684 in 65749ee393 outdated

 677 | @@ -678,7 +678,10 @@ BOOST_AUTO_TEST_CASE(get_local_addr_for_peer_port)
 678 |      // Our address:port as seen from the peer, completely different from the above.
 679 |      in_addr peer_us_addr;
 680 |      peer_us_addr.s_addr = htonl(0x02030405);
 681 | -    const CAddress peer_us{CService{peer_us_addr, 20002}, NODE_NETWORK};
 682 | +    CAddress peer_us{CService{peer_us_addr, 20002}, NODE_NETWORK};
 683 | +    int64_t current_time{GetAdjustedTime()};
 684 | +    SetMockTime(current_time);
 685 | +    peer_us.nTime = current_time;

vasild commented at 3:58 PM on June 9, 2022:

These changes would not be needed if the test below is fixed to just compare the address and port - that is/was the intention, but in the last check:

BOOST_CHECK(*chosen_local_addr == peer_us);

it ends up comparing to CAddress objects which also compares time and services. That is not intended. Better change it to:

BOOST_CHECK(static_cast<CService>(*chosen_local_addr) == peer_us);

or even more obvious:

BOOST_CHECK(static_cast<CService>(*chosen_local_addr) == static_cast<CService>(peer_us));

mzumsande commented at 6:09 PM on June 9, 2022:

Wouldn't it be better to have the test compare the entire advertised CAddress, especially with the difficulties of writing functional test for self-advertisements? I know it is not the main intention for this test, but I think it is nice to have this tested on the side, especially with the footgunny CAddress(CService, ServiceFlags) ctor that is used so frequently, probably to avoid int64_t / uint32_t compiler warnings when inserting a int64_t time into the full ctor with nTime. So maybe it would be better to instead also raise the earlier check to the Level of CAddress?

vasild commented at 6:42 PM on June 9, 2022:

Alright, it is good to test more, even though this test is targeted at testing the ports stuff. IMO it is better for one test to test one thing, so maybe create a new test that just calls GetLocalAddrForPeer() without the ports complications from the get_local_addr_for_peer_port test. Anyway, up to you. If the existent test is to be modified, then I guess something like this should suffice:

     m_node.args->ForceSetArg("-bind", strprintf("3.4.5.6:%u", bind_port));
 
+    const uint32_t current_time = static_cast<uint32_t>(GetAdjustedTime());
+
+    SetMockTime(current_time);
+
     // Our address:port as seen from the peer, completely different from the above.
     in_addr peer_us_addr;
     peer_us_addr.s_addr = htonl(0x02030405);
-    const CAddress peer_us{CService{peer_us_addr, 20002}, NODE_NETWORK};
+    const CAddress peer_us{CService{peer_us_addr, 20002}, NODE_NETWORK, current_time};
 
     // Create a peer with a routable IPv4 address (outbound).

mzumsande commented at 3:02 PM on June 10, 2022:

I took your suggestion to modify, and also changed the check for expected to the level of CAddress. I think it's natural for a unit test of a function that returns an object to check the entire object is as expected, and not just parts of it, especially if it's just two more fields and a (thorough) separate test that would have caught this issue would have required to replicate some of the port stuff in order to also get to the branch where nTime was left default-initialized.

vasild commented at 4:01 PM on June 9, 2022: contributor

Concept ACK

The proficiency of the engineers is just astonishing in this community (@mzumsande)! How the hell did you catch this?

mzumsande commented at 6:15 PM on June 9, 2022: contributor

The proficiency of the engineers is just astonishing in this community (@mzumsande)! How the hell did you catch this?

Thank you! Pretty much by chance - I was reviewing #25303 and while looking at this commit msg I was wondering if it was possible to somehow have addresses with default-initialised nTime in addrman or addr relay - that's how I stumbled upon this.

p2p: always set nTime for self-advertisements

If we self-advertised to an inbound peer with the address they gave us,
nTime was left default-initialized, so that our peer wouldn't relay it
any further along.

99b9e5f3a9

mzumsande force-pushed on Jun 10, 2022

mzumsande commented at 3:03 PM on June 10, 2022: contributor

65749ee -> 99b9e5f Addressed feedback by @vasild (thanks!)

vasild approved

vasild commented at 7:29 AM on June 13, 2022: contributor

ACK 99b9e5f3a9fa29bbc1e45fc958470fbcc207ef23

Thank you!

naumenkogs commented at 9:50 AM on June 13, 2022: member

ACK 99b9e5f3a9fa29bbc1e45fc958470fbcc207ef23

laanwj added this to the milestone 24.0 on Jun 13, 2022

fanquake requested review from ajtowns on Jun 13, 2022

DrahtBot commented at 3:52 PM on June 16, 2022: contributor

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

Conflicts

No conflicts as of last run.

laanwj commented at 9:54 PM on June 21, 2022: member

Code review ACK 99b9e5f3a9fa29bbc1e45fc958470fbcc207ef23 Great catch!

laanwj merged this on Jun 21, 2022

laanwj closed this on Jun 21, 2022

mzumsande commented at 11:24 PM on June 21, 2022: contributor

Maybe consider backporting to 23.x if this meets the bar.

mzumsande deleted the branch on Jun 21, 2022

fanquake referenced this in commit 4ebf6e35dc on Jun 22, 2022

fanquake commented at 10:41 AM on June 22, 2022: member

Maybe consider backporting to 23.x if this meets the bar.

I've added this for backport in #25316

sidhujag referenced this in commit b63260e221 on Jun 22, 2022

MarcoFalke referenced this in commit a33ec8a693 on Jul 8, 2022

bitcoin locked this on Jul 21, 2023