Use std::chrono for the time to rotate destination of addr messages + tests #18642

pull naumenkogs wants to merge 2 commits into bitcoin:master from naumenkogs:2020_04_mock_addr_rotation_time changing 2 files +71 −2
  1. naumenkogs commented at 1:13 am on April 15, 2020: member

    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.

  2. fanquake added the label P2P on Apr 15, 2020
  3. in src/net_processing.cpp:1379 in 3c8319f57b outdated
    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);
    


    sipa commented at 1:18 am on April 15, 2020:
    Is this duration_cast needed? I think conversion should be automatic.

    naumenkogs commented at 1:25 am on April 15, 2020:

    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.

  4. naumenkogs commented at 1:29 am on April 15, 2020: member
    @sipa btw, take a look at how hashAddr was fed to the hasher twice (with a shift, yeah). I removed it, although I don’t know why it was there in the first place… Feeding it once like I do should be sufficient.
  5. in src/net_processing.cpp:1381 in 3c8319f57b outdated
    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();
    


    MarcoFalke commented at 1:29 am on April 15, 2020:
    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 commented at 1:30 am on April 15, 2020:
    can we keep it to 2 lines? I think what you suggest is hard to read.
  6. naumenkogs force-pushed on Apr 15, 2020
  7. sipa commented at 2:05 am on April 15, 2020: member

    @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).

  8. in src/net_processing.cpp:1381 in 8690d401a7 outdated
    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);
    


    sipa commented at 2:06 am on April 15, 2020:
    0    const CSipHasher hasher = connman.GetDeterministicRandomizer(RANDOMIZER_ID_ADDRESS_RELAY).Write(hashAddr).Write((current_time + hashAddr) / ROTATE_ADDR_RELAY_DEST_INTERVAL);
    

    naumenkogs commented at 1:54 pm on April 15, 2020:

    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.

  9. naumenkogs force-pushed on Apr 15, 2020
  10. naumenkogs commented at 2:05 pm on April 15, 2020: member
    Updated the code with suggestion by @sipa, and simplified tests. They are a bit loosened now, but more robust to failures caused by randomness.
  11. naumenkogs force-pushed on Apr 15, 2020
  12. in src/net_processing.cpp:1381 in cca35ff247 outdated
    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;
    


    MarcoFalke commented at 2:23 pm on April 15, 2020:
    Previously hashAddr was of type seconds, now it is type hours. What was the reason to change it?

    naumenkogs commented at 3:20 pm on April 15, 2020:
    There is no real difference, as long as it is an order of hours or more. Otherwise, it won’t add enough randomness.

    MarcoFalke commented at 3:40 pm on April 15, 2020:
    You will get rid of the “reset time of the day” feature. Generally I don’t like refactoring commits to also change behavior.

    naumenkogs commented at 3:52 pm on April 15, 2020:
    Okay, made it seconds again so that we don’t change the behavior. Although I think we won’t loose any properties if hours are used.
  13. naumenkogs force-pushed on Apr 15, 2020
  14. naumenkogs force-pushed on Apr 16, 2020
  15. in src/net_processing.cpp:1381 in ae5eaf5262 outdated
    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;
    


    MarcoFalke commented at 0:10 am on April 17, 2020:
    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

  16. naumenkogs force-pushed on Apr 17, 2020
  17. in src/net_processing.cpp:1381 in 72c4db3521 outdated
    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;
    


    MarcoFalke commented at 2:49 pm on April 21, 2020:
    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)
    
  18. naumenkogs force-pushed on Apr 21, 2020
  19. DrahtBot commented at 9:06 pm on April 23, 2020: member

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

    Conflicts

    No conflicts as of last run.

  20. DrahtBot added the label Needs rebase on Apr 25, 2020
  21. naumenkogs force-pushed on May 18, 2020
  22. DrahtBot removed the label Needs rebase on May 18, 2020
  23. DrahtBot added the label Needs rebase on Jun 24, 2020
  24. naumenkogs force-pushed on Jun 25, 2020
  25. DrahtBot removed the label Needs rebase on Jun 25, 2020
  26. naumenkogs force-pushed on Jun 25, 2020
  27. in src/net_processing.cpp:1708 in 177df94a6c outdated
    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);
    


    jonatack commented at 3:45 pm on June 28, 2020:
    nit: Can this be simpler? It’s more code and (even with the removed bit shifting) a bit more complex to understand.

    naumenkogs commented at 12:54 pm on June 30, 2020:
    We iterated over this line a couple times with @MarcoFalke, so I’m out of ideas here. Open to suggestions!

    sipa commented at 7:35 pm on July 7, 2020:

    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.


    sipa commented at 6:41 pm on January 4, 2022:
    This isn’t resolved actually; it’s still casting 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).

    sdaftuar commented at 7:06 pm on January 4, 2022:
    We’re constructing a 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?

    sipa commented at 7:44 pm on January 4, 2022:

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


    MarcoFalke commented at 8:30 am on January 5, 2022:

    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.


    naumenkogs commented at 9:45 am on January 5, 2022:
    Github is weird, I found this un-resolved issue totally accidentally…

    naumenkogs commented at 10:15 am on January 5, 2022:
    Okay the latest version should be safe?
  28. in test/functional/p2p_addr_relay.py:92 in 177df94a6c outdated
    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
    


    jonatack commented at 3:48 pm on June 28, 2020:
    In 6 places (here, above at line 72 and line 87, below at lines 106+107 and 113), it could be good to move the multiplications out of the loops and up to constants at the top like ONE_HOUR, TWO_HOURS, and ONE_DAY.
  29. in test/functional/p2p_addr_relay.py:86 in 177df94a6c outdated
    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
    


    jonatack commented at 3:50 pm on June 28, 2020:
    0        for _ in range(11):  # 11 times jump for 2 hours, so less than 24 hours -> at most one rotation
    
  30. in test/functional/p2p_addr_relay.py:105 in 177df94a6c outdated
    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
    


    jonatack commented at 3:50 pm on June 28, 2020:
    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?

  31. in test/functional/p2p_addr_relay.py:113 in 177df94a6c outdated
    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.
    


    jonatack commented at 3:53 pm on June 28, 2020:
    0                mocked_time += 60 * 60  # should be enough to cover 30-min Poisson in most cases.
    
  32. in test/functional/p2p_addr_relay.py:118 in 177df94a6c outdated
    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)
    


    jonatack commented at 3:54 pm on June 28, 2020:
    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)
    
  33. in test/functional/p2p_addr_relay.py:97 in 177df94a6c outdated
     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)
    


    jonatack commented at 3:55 pm on June 28, 2020:
    0        assert len(nodes_received_addr) <= 4
    

    jonatack commented at 4:10 pm on June 28, 2020:
    Why the value of 4 here and line 118? Perhaps add a comment.
  34. in test/functional/p2p_addr_relay.py:82 in 177df94a6c outdated
    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):
    


    jonatack commented at 3:55 pm on June 28, 2020:
    0        for _ in range(20):
    
  35. in test/functional/p2p_addr_relay.py:77 in 177df94a6c outdated
    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
    


    jonatack commented at 4:03 pm on June 28, 2020:

    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.

  36. jonatack commented at 4:12 pm on June 28, 2020: member

    (Tested) Concept ACK.

    Some questions and style/PEP8 comments below, feel free to ignore.

  37. naumenkogs force-pushed on Jul 1, 2020
  38. naumenkogs commented at 10:10 am on July 1, 2020: member
    @jonatack addressed all the things.
  39. jonatack commented at 1:46 pm on July 6, 2020: member

    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

  40. sipa commented at 7:17 pm on September 30, 2020: member
    utACK 72d7e2400759dacfc8860341d5bb90459e363a81. I only casually reviewed the tests.
  41. fanquake requested review from sdaftuar on Oct 1, 2020
  42. fanquake requested review from amitiuttarwar on Oct 1, 2020
  43. MarcoFalke commented at 8:04 am on October 1, 2020: member
    ACK 72d7e2400759dacfc8860341d5bb90459e363a81
  44. DrahtBot added the label Needs rebase on Dec 14, 2020
  45. naumenkogs force-pushed on Sep 15, 2021
  46. fanquake commented at 2:08 am on September 16, 2021: member
    Aside from this needing rebase for conflicts, can you rebase on master so that new CIs run. That should remove appveyor as well.
  47. MarcoFalke commented at 7:36 am on September 22, 2021: member
    @naumenkogs Is this up for grabs?
  48. naumenkogs force-pushed on Sep 27, 2021
  49. MarcoFalke removed the label Needs rebase on Sep 27, 2021
  50. naumenkogs force-pushed on Sep 27, 2021
  51. naumenkogs commented at 1:14 pm on September 27, 2021: member
    Okay, it was hard to rebase @jonatack test contributed, but I just did it. Ready for review.
  52. in test/functional/p2p_addr_relay.py:29 in 7ba6b190b6 outdated
    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
    


    theStack commented at 8:50 pm on December 19, 2021:

    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)

  53. in test/functional/p2p_addr_relay.py:24 in 7ba6b190b6 outdated
    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
    


    theStack commented at 8:53 pm on December 19, 2021:

    nit:

    0from test_framework.util import (
    1    assert_equal,
    2    assert_greater_than,
    3    assert_greater_than_or_equal,
    4)
    
  54. theStack commented at 9:05 pm on December 19, 2021: member

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

  55. MarcoFalke commented at 9:49 am on January 4, 2022: member
    @naumenkogs Is this up for grabs?
  56. naumenkogs commented at 10:26 am on January 4, 2022: member
    @MarcoFalke I mean, it’s up-to-date (apart from those little nits) and reviewable. I probably don’t have energy to nudge people for reviewing this, so if that makes it “up for grabs”, then yes.
  57. MarcoFalke commented at 10:29 am on January 4, 2022: member
    It would be good to reply to the comments. Either addressing them or denying/closing them.
  58. MarcoFalke commented at 11:10 am on January 4, 2022: member
    Or alternatively submit the test in a separate pull request, since this is refactoring, so the test should already pass on master.
  59. naumenkogs force-pushed on Jan 4, 2022
  60. in src/net_processing.cpp:1708 in dd83d9f050 outdated
    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);
    


    sdaftuar commented at 6:29 pm on January 4, 2022:
    Can you explain why it is safe / defined behavior to perform a 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.

    sdaftuar commented at 6:38 pm on January 4, 2022:
    Sorry for the noise, @sipa explained this to me (the result of the division is being cast, which is not a duration but a number).
  61. in src/net_processing.cpp:1709 in b3c71eafbe outdated
    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);
    


    sipa commented at 7:45 pm on January 4, 2022:
    No need for the << 32 here, I think.

    MarcoFalke commented at 8:33 am on January 5, 2022:
    I’ve pulled that out into #23970, since it also resolves a sanitizer issue.

    naumenkogs commented at 9:43 am on January 5, 2022:
    @MarcoFalke does it mean i can leave it as it is here?

    MarcoFalke commented at 9:48 am on January 5, 2022:
    You can either leave it as is or take it, but if you take it, make sure to also remove the suppression like in #23970.
  62. MarcoFalke commented at 8:33 am on January 5, 2022: member
    Just to note 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?
  63. naumenkogs force-pushed on Jan 5, 2022
  64. in src/net_processing.cpp:127 in fca9977a15 outdated
    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};
    


    MarcoFalke commented at 10:17 am on January 5, 2022:
    why is this changed? This also conflicts with #23832

    naumenkogs commented at 10:21 am on January 5, 2022:
    I think it was like that from the very beginning? But sure, I can drop this.

    sipa commented at 8:20 pm on January 5, 2022:
    Using the 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.
  65. fanquake referenced this in commit 17fdbefd3f on Jan 5, 2022
  66. sidhujag referenced this in commit 8bb46dbb3c on Jan 6, 2022
  67. DrahtBot added the label Needs rebase on Jan 6, 2022
  68. naumenkogs force-pushed on Jan 10, 2022
  69. naumenkogs force-pushed on Jan 10, 2022
  70. DrahtBot removed the label Needs rebase on Jan 10, 2022
  71. in src/net_processing.cpp:1751 in f6177ff039 outdated
    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();
    


    MarcoFalke commented at 9:49 am on January 25, 2022:
    why is this changed?
  72. naumenkogs force-pushed on Jan 26, 2022
  73. DrahtBot added the label Needs rebase on Mar 2, 2022
  74. Use std::chrono for salting when randomizing ADDR destination 77ccb7fce1
  75. Add tests for addr destination rotation
    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>
    2ff8f4dd81
  76. jonatack commented at 3:58 pm on March 13, 2022: member
    @naumenkogs if helpful, here is a rebase of this pull: https://github.com/jonatack/bitcoin/commits/pr18642-rebase-to-master-at-e04720e. If you don’t have time to update this in a week or so, I’ll open a PR for you with the same commits.
  77. naumenkogs force-pushed on Mar 14, 2022
  78. naumenkogs force-pushed on Mar 14, 2022
  79. naumenkogs force-pushed on Mar 14, 2022
  80. DrahtBot removed the label Needs rebase on Mar 14, 2022
  81. jonatack commented at 4:47 pm on March 14, 2022: member
    ACK 2ff8f4dd81dc484fe38ddd9db63cc8fd30192245
  82. fanquake requested review from MarcoFalke on Apr 26, 2022
  83. MarcoFalke merged this on Apr 27, 2022
  84. MarcoFalke closed this on Apr 27, 2022

  85. sidhujag referenced this in commit fef23d989a on Apr 29, 2022
  86. DrahtBot locked this on Apr 27, 2023

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-08 16:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me