We currently assign a destination peer for relaying particular addresses of nodes every 24 hours, and then rotate. This is done for rate-limiting (ultimately for privacy leak reduction I think?).
Before this change, 24 hours was defined as uint. I replaced it with std::chrono, which is mockable and type-safe.
Also added couple tests for this behavior.
1374@@ -1373,7 +1375,11 @@ static void RelayAddress(const CAddress& addr, bool fReachable, const CConnman&
1375 // Use deterministic randomness to send to the same nodes for 24 hours
1376 // at a time so the m_addr_knowns of the chosen nodes prevent repeats
1377 uint64_t hashAddr = addr.GetHash();
1378- const CSipHasher hasher = connman.GetDeterministicRandomizer(RANDOMIZER_ID_ADDRESS_RELAY).Write(hashAddr << 32).Write((GetTime() + hashAddr) / (24 * 60 * 60));
1379+
1380+ std::chrono::seconds rotate_interval = std::chrono::duration_cast<std::chrono::seconds>(ROTATE_ADDR_RELAY_DEST_INTERVAL);
You are right. I spent 30 minutes fighting with this code, and couldn’t come up with something better.
But after you messaged I tried once more and it worked. Will force push in a second.
1374@@ -1373,7 +1375,11 @@ static void RelayAddress(const CAddress& addr, bool fReachable, const CConnman&
1375 // Use deterministic randomness to send to the same nodes for 24 hours
1376 // at a time so the m_addr_knowns of the chosen nodes prevent repeats
1377 uint64_t hashAddr = addr.GetHash();
1378- const CSipHasher hasher = connman.GetDeterministicRandomizer(RANDOMIZER_ID_ADDRESS_RELAY).Write(hashAddr << 32).Write((GetTime() + hashAddr) / (24 * 60 * 60));
1379+
1380+ std::chrono::seconds rotate_interval = std::chrono::duration_cast<std::chrono::seconds>(ROTATE_ADDR_RELAY_DEST_INTERVAL);
1381+ std::chrono::seconds current_time = GetTime<std::chrono::seconds>();
1382+ uint64_t time_seed = current_time.count() / rotate_interval.count();
0 const uint64_t time_seed {GetTime<std::chrono::seconds>().count() / std::chrono::seconds{ROTATE_ADDR_RELAY_DEST_INTERVAL}.count()};
Everything in one line with compile-time checks enabled would look like this, btw
@naumenkogs It was very inscrutable code, but the construction with double-hashing the address hash was by design.
The idea is that the address both influences the hash directly, but also determines when the “reset time of the day” is (by including hashaddr in (time + hashaddr)/DAY, not every address will change salt simultaneously throughout the day).
The shifting is pointless, we should get rid of it. It seems to be a silly evolution of this 2010 Satoshi code: 5cbf75324d1509a1262b65c5073314a4da3f6d77 (where it made sense because everything was XORed together, and the address used the high bits, while the time used the low ones).
1374@@ -1373,7 +1375,10 @@ static void RelayAddress(const CAddress& addr, bool fReachable, const CConnman&
1375 // Use deterministic randomness to send to the same nodes for 24 hours
1376 // at a time so the m_addr_knowns of the chosen nodes prevent repeats
1377 uint64_t hashAddr = addr.GetHash();
1378- const CSipHasher hasher = connman.GetDeterministicRandomizer(RANDOMIZER_ID_ADDRESS_RELAY).Write(hashAddr << 32).Write((GetTime() + hashAddr) / (24 * 60 * 60));
1379+
1380+ std::chrono::seconds current_time = GetTime<std::chrono::seconds>();
1381+ uint64_t time_seed = current_time / ROTATE_ADDR_RELAY_DEST_INTERVAL;
1382+ const CSipHasher hasher = connman.GetDeterministicRandomizer(RANDOMIZER_ID_ADDRESS_RELAY).Write(hashAddr).Write(time_seed);
0 const CSipHasher hasher = connman.GetDeterministicRandomizer(RANDOMIZER_ID_ADDRESS_RELAY).Write(hashAddr).Write((current_time + hashAddr) / ROTATE_ADDR_RELAY_DEST_INTERVAL);
Yeah, I’m not 100% sure that this measure is effective against anything, but it won’t hurt to have a less symmetry. This mehaviour may leak extra information if someone tries to analyze how addresses are flowing, but since we don’t have any real evidence, we probably should stick to the existing reasoning.
Had to slightly adjust the code you suggested, because adding uint to chrono type doesn’t work right away.
1374@@ -1373,7 +1375,11 @@ static void RelayAddress(const CAddress& addr, bool fReachable, const CConnman&
1375 // Use deterministic randomness to send to the same nodes for 24 hours
1376 // at a time so the m_addr_knowns of the chosen nodes prevent repeats
1377 uint64_t hashAddr = addr.GetHash();
1378- const CSipHasher hasher = connman.GetDeterministicRandomizer(RANDOMIZER_ID_ADDRESS_RELAY).Write(hashAddr << 32).Write((GetTime() + hashAddr) / (24 * 60 * 60));
1379+
1380+ std::chrono::seconds current_time = GetTime<std::chrono::seconds>();
1381+ // Adding address hash makes exact rotation time different per address, while preserving periodicity.
1382+ uint64_t time_addr = (current_time + std::chrono::hours(hashAddr)) / ROTATE_ADDR_RELAY_DEST_INTERVAL;
hashAddr was of type seconds, now it is type hours. What was the reason to change it?
1374@@ -1373,7 +1375,11 @@ static void RelayAddress(const CAddress& addr, bool fReachable, const CConnman&
1375 // Use deterministic randomness to send to the same nodes for 24 hours
1376 // at a time so the m_addr_knowns of the chosen nodes prevent repeats
1377 uint64_t hashAddr = addr.GetHash();
1378- const CSipHasher hasher = connman.GetDeterministicRandomizer(RANDOMIZER_ID_ADDRESS_RELAY).Write(hashAddr << 32).Write((GetTime() + hashAddr) / (24 * 60 * 60));
1379+
1380+ std::chrono::seconds current_time = GetTime<std::chrono::seconds>();
1381+ // Adding address hash makes exact rotation time different per address, while preserving periodicity.
1382+ uint64_t time_addr = (current_time + std::chrono::seconds(hashAddr)) / ROTATE_ADDR_RELAY_DEST_INTERVAL;
0 uint64_t time_addr = (current_time + std::chrono::seconds{static_cast<int64_t>(hashAddr)}) / ROTATE_ADDR_RELAY_DEST_INTERVAL;
Maybe try this to avoid https://travis-ci.org/github/bitcoin/bitcoin/jobs/675780723#L4756
1374@@ -1373,7 +1375,11 @@ static void RelayAddress(const CAddress& addr, bool fReachable, const CConnman&
1375 // Use deterministic randomness to send to the same nodes for 24 hours
1376 // at a time so the m_addr_knowns of the chosen nodes prevent repeats
1377 uint64_t hashAddr = addr.GetHash();
1378- const CSipHasher hasher = connman.GetDeterministicRandomizer(RANDOMIZER_ID_ADDRESS_RELAY).Write(hashAddr << 32).Write((GetTime() + hashAddr) / (24 * 60 * 60));
1379+
1380+ std::chrono::seconds current_time = GetTime<std::chrono::seconds>();
1381+ // Adding address hash makes exact rotation time different per address, while preserving periodicity.
1382+ uint64_t time_addr = (current_time + std::chrono::seconds{static_cast<int64_t>(hashAddr)}) / ROTATE_ADDR_RELAY_DEST_INTERVAL;
0 uint64_t time_addr = static_cast<uint64_t>((current_time + std::chrono::seconds{static_cast<int64_t>(hashAddr)}) / ROTATE_ADDR_RELAY_DEST_INTERVAL);
0 node0 stderr net_processing.cpp:1381:26: runtime error: implicit conversion from type 'typename common_type<long, long>::type' (aka 'long') of value -28739051454386 (64-bit, signed) to type 'uint64_t' (aka 'unsigned long') changed the value to 18446715334658097230 (64-bit, unsigned)
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
No conflicts as of last run.
1434@@ -1433,7 +1435,11 @@ static void RelayAddress(const CAddress& addr, bool fReachable, const CConnman&
1435 // Use deterministic randomness to send to the same nodes for 24 hours
1436 // at a time so the m_addr_knowns of the chosen nodes prevent repeats
1437 uint64_t hashAddr = addr.GetHash();
1438- const CSipHasher hasher = connman.GetDeterministicRandomizer(RANDOMIZER_ID_ADDRESS_RELAY).Write(hashAddr << 32).Write((GetTime() + hashAddr) / (24 * 60 * 60));
1439+
1440+ std::chrono::seconds current_time = GetTime<std::chrono::seconds>();
1441+ // Adding address hash makes exact rotation time different per address, while preserving periodicity.
1442+ uint64_t time_addr = static_cast<uint64_t>((current_time + std::chrono::seconds{static_cast<int64_t>(hashAddr)}) / ROTATE_ADDR_RELAY_DEST_INTERVAL);
Annoyingly, I think this is UB, as hashAddr may exceed 2^63, and casting a larger number to a signed 64-bit integer would be outside its range (and overflowing signed integers is undefined behavior).
Suggestion: uint64_t time_addr = (current_time + std::chrono::seconds{hashAddr % ROTATE_ADDR_RELAY_DEST_INTERVAL.count()}) / ROTATE_ADDR_RELAY_DEST_INTERVAL;, and make ROTATE_ADDR_RELAY_DEST_INTERVAL to be of type std::chrono::seconds.
hashAddr from uint64_t to int64_t, which is implementation-defined behavior if it overflows (I was wrong claiming earlier that it was UB; arithmetic that overflows signed types is UB, but just the conversion to a signed type is at worst implementation-defined).
std::chrono::seconds from an int64_t, but according to https://en.cppreference.com/w/cpp/chrono/duration, std::chrono::seconds is only guaranteed to have 35 bits – is this problematic?
Uh, right. I think that could be a problem. This will need something like
0uint64_t time_addr = (uint64_t{current_time / std::chrono::seconds{1}} + hashAddr) / (ROTATE_ADDR_RELAY_DEST_INTERVAL / std::chrono::seconds{1});
(I prefer using x / std::chrono::seconds{1} over x.count() to get the number of seconds of x, as the former expression remains meaningful if the type of x changes to something other than std::chrono::seconds).
My understanding was that std::chrono::seconds{int64_t{}} wouldn’t compile on platforms where seconds isn’t backed by at least 64 bits. I don’t expect any such platform to exist.
@ sipa there is also a count_seconds() helper.
91+ msg.addrs[0].time = mocked_time + 10 * 60
92+ with self.nodes[0].assert_debug_log([
93+ 'received: addr (31 bytes) peer=0',
94+ ]):
95+ addr_source.send_and_ping(msg)
96+ mocked_time += 2 * 60 * 60
ONE_HOUR, TWO_HOURS, and ONE_DAY.
85+ addr_receivers = [addr_receiver]
86+ for i in range(20):
87+ addr_receiver = self.nodes[0].add_p2p_connection(AddrReceiver())
88+ addr_receivers.append(addr_receiver)
89+
90+ for i in range(11): # 11 times jump for 2 hours, so less than 24 hours -> at most one rotation
0 for _ in range(11): # 11 times jump for 2 hours, so less than 24 hours -> at most one rotation
104+ mocked_time = int(time.time()) + 200000
105+ self.nodes[0].setmocktime(mocked_time)
106+ for node in addr_receivers:
107+ node.received_addr = False
108+
109+ for i in range(10): # 11 times jump for a day, so less than 24 hours -> at most one rotation
0 for _ in range(10): # 11 times jump for a day, so less than 24 hours -> at most one rotation
Should this be 10 or 11?
112+ self.nodes[0].setmocktime(mocked_time)
113+ with self.nodes[0].assert_debug_log([
114+ 'received: addr (31 bytes) peer=0',
115+ ]):
116+ addr_source.send_and_ping(msg)
117+ mocked_time += 60 * 60 # should be enough to cover 30-min Poisson in most cases.
0 mocked_time += 60 * 60 # should be enough to cover 30-min Poisson in most cases.
117+ mocked_time += 60 * 60 # should be enough to cover 30-min Poisson in most cases.
118+ self.nodes[0].setmocktime(mocked_time)
119+ for receiver in addr_receivers:
120+ receiver.sync_with_ping()
121+ nodes_received_addr = [node for node in addr_receivers if node.received_addr]
122+ assert(len(nodes_received_addr) > 4)
0 assert len(nodes_received_addr) > 4
or even better (adding it to the imported methods at the top)
0 assert_greater_than(len(nodes_received_addr), 4)
96+ mocked_time += 2 * 60 * 60
97+ self.nodes[0].setmocktime(mocked_time)
98+ for receiver in addr_receivers:
99+ receiver.sync_with_ping()
100+ nodes_received_addr = [node for node in addr_receivers if node.received_addr]
101+ assert(len(nodes_received_addr) <= 4)
0 assert len(nodes_received_addr) <= 4
81+ mocked_time = int(time.time()) + 100000
82+ self.nodes[0].setmocktime(mocked_time)
83+ msg.addrs = gen_addrs(1, mocked_time)
84+ addr_receiver.received_addr = False
85+ addr_receivers = [addr_receiver]
86+ for i in range(20):
0 for _ in range(20):
76+ mocked_time += 30 * 60
77+ self.nodes[0].setmocktime(mocked_time)
78 addr_receiver.sync_with_ping()
79
80+ self.log.info('Check that within 24 hours an addr relay destination is rotated at most once')
81+ mocked_time = int(time.time()) + 100000
I don’t grok what/why regarding the 100000 value. It would be nice to comment and/or hoist it to a variable.
Same for 200000 at line 100.
(Tested) Concept ACK.
Some questions and style/PEP8 comments below, feel free to ignore.
ACK 72d7e24
Thanks for clarifying the tests! I spent some time messing around with them, refactored, and sped them up from 26 to 7 seconds on my machine. Feel free to use/steal/ignore: https://github.com/jonatack/bitcoin/commits/refactor-p2p_addr_relay-tests
25+from test_framework.util import assert_equal, assert_greater_than, assert_greater_than_or_equal
26
27+ONE_MINUTE = 60
28+TEN_MINUTES = 10 * ONE_MINUTE
29+HALF_HOUR = 3 * TEN_MINUTES
30+ONE_HOUR = 2 * HALF_HOUR
The HALF_HOUR constant doesn’t seem to be used anywhere below, i.e. it can be removed.
0ONE_HOUR = 60 * ONE_MINUTE
(or 6 * TEN_MINUTES)
20@@ -21,8 +21,16 @@
21 P2P_SERVICES,
22 )
23 from test_framework.test_framework import BitcoinTestFramework
24-from test_framework.util import assert_equal, assert_greater_than
25+from test_framework.util import assert_equal, assert_greater_than, assert_greater_than_or_equal
nit:
0from test_framework.util import (
1 assert_equal,
2 assert_greater_than,
3 assert_greater_than_or_equal,
4)
Concept ACK
The first commits replaces GetTime() and the timing constants related to the randomization of ADDR messages with the mockable variants, i.e. it enables mocking but there shouldn’t be any change in behaviour if mocking is not used. Will take a deeper look on the test soon, just left some nits (feel free to ignore).
1701@@ -1700,7 +1702,11 @@ void PeerManagerImpl::RelayAddress(NodeId originator,
1702 // Use deterministic randomness to send to the same nodes for 24 hours
1703 // at a time so the m_addr_knowns of the chosen nodes prevent repeats
1704 uint64_t hashAddr = addr.GetHash();
1705- const CSipHasher hasher = m_connman.GetDeterministicRandomizer(RANDOMIZER_ID_ADDRESS_RELAY).Write(hashAddr << 32).Write((GetTime() + hashAddr) / (24 * 60 * 60));
1706+
1707+ std::chrono::seconds current_time = GetTime<std::chrono::seconds>();
1708+ // Adding address hash makes exact rotation time different per address, while preserving periodicity.
1709+ uint64_t time_addr = static_cast<uint64_t>((current_time + std::chrono::seconds{static_cast<int64_t>(hashAddr)}) / ROTATE_ADDR_RELAY_DEST_INTERVAL);
static_cast<uint64_t>(std::chrono::seconds)? I can’t figure out from reading https://en.cppreference.com/w/cpp/chrono/duration whether this is supposed to be defined, but maybe I’m missing something.
1701@@ -1700,7 +1702,11 @@ void PeerManagerImpl::RelayAddress(NodeId originator,
1702 // Use deterministic randomness to send to the same nodes for 24 hours
1703 // at a time so the m_addr_knowns of the chosen nodes prevent repeats
1704 uint64_t hashAddr = addr.GetHash();
1705- const CSipHasher hasher = m_connman.GetDeterministicRandomizer(RANDOMIZER_ID_ADDRESS_RELAY).Write(hashAddr << 32).Write((GetTime() + hashAddr) / (24 * 60 * 60));
1706+
1707+ std::chrono::seconds current_time = GetTime<std::chrono::seconds>();
1708+ // Adding address hash makes exact rotation time different per address, while preserving periodicity.
1709+ uint64_t time_addr = static_cast<uint64_t>((current_time + std::chrono::seconds{static_cast<int64_t>(hashAddr)}) / ROTATE_ADDR_RELAY_DEST_INTERVAL);
1710+ const CSipHasher hasher = m_connman.GetDeterministicRandomizer(RANDOMIZER_ID_ADDRESS_RELAY).Write(hashAddr << 32).Write(time_addr);
<< 32 here, I think.
GetTime is already mockable, so this only changes the type. Though, if this is too much hassle to get right and too hard to review (due to static_cast and {}-cast hell), then maybe leave the code as is and only add the test?
123@@ -124,7 +124,9 @@ static const unsigned int NODE_NETWORK_LIMITED_MIN_BLOCKS = 288;
124 /** Average delay between local address broadcasts */
125 static constexpr auto AVG_LOCAL_ADDRESS_BROADCAST_INTERVAL = 24h;
126 /** Average delay between peer address broadcasts */
127-static constexpr auto AVG_ADDRESS_BROADCAST_INTERVAL = 30s;
128+static constexpr std::chrono::seconds AVG_ADDRESS_BROADCAST_INTERVAL{30};
static constexpr auto NAME = <numer><suffix>; as is used here is a bit more readable I think. I’d suggest using the same for the line below.
1747@@ -1746,8 +1748,12 @@ void PeerManagerImpl::RelayAddress(NodeId originator,
1748 // Relay to a limited number of other nodes
1749 // Use deterministic randomness to send to the same nodes for 24 hours
1750 // at a time so the m_addr_knowns of the chosen nodes prevent repeats
1751- const uint64_t hashAddr{addr.GetHash()};
1752- const CSipHasher hasher{m_connman.GetDeterministicRandomizer(RANDOMIZER_ID_ADDRESS_RELAY).Write(hashAddr).Write((GetTime() + hashAddr) / (24 * 60 * 60))};
1753+ uint64_t hashAddr = addr.GetHash();
Check that within 24h addr of a given node is forwarded
to the same peer(s), and then the destination is
rotated every 24h.
Co-authored-by: Jon Atack <jon@atack.com>
