net: gracefully handle NodeId wrapping #10176

1. 
   theuni commented at 4:52 PM on April 10, 2017: member

   As discussed in #10143.

   • Explicitly define NodeId's size as an int64_t. This keeps the range uniform so that it may be exposed to external APIs. Signed ints are allowed in order to represent special values.

   
   Edit: as per the discussion below, only the int64_t change is needed.
   

2. MarcoFalke added the label P2P on Apr 10, 2017
3. theuni force-pushed on Apr 10, 2017
4. 
   laanwj commented at 7:45 AM on April 11, 2017: member

   I'm a bit ambivalent about going to 64 bit ints for the node id. I think it is a good temporary workaround for the wrap-around problem, but as you do want to handle wrapping, it seems a double measure. At the least please add a regression test for the wrapping code, as with 64 bits is so unlikely to ever trigger that the code will rot.

5. 
   gmaxwell commented at 8:38 AM on April 11, 2017: contributor

   We could take a tip from the linux kernel counter handling and cause it to immediately wrap shortly after start by starting shortly below the wrap point, but esp with 64-bit IDs, the really long IDs would be kind of obnoxious. (also this wouldn't test the skipping code)

6. in src/net.cpp:2196 in 96628e102e outdated

   2193 | +    NodeId newId;
   2194 | +    {
   2195 | +        LOCK(cs_setTakenNodeIds);
   2196 | +        do {
   2197 | +            if (nLastNodeId == std::numeric_limits<NodeId>::max()) {
   2198 | +                nLastNodeId = 0;
   

   ---

   ---

   

   gmaxwell commented at 8:42 AM on April 11, 2017:

   Not -1? (or =0 and having the ++ in an else?) Why should it fail to reconsider reusing 0?

   ---

   

   gmaxwell commented at 8:45 AM on April 11, 2017:

   The theoretical infinite loop is perhaps unfortunate, perhaps there should be an assert if size of the set is => max. (the reason for doing that would be more obvious if you imagine someone deciding to reduce the id to a s16 since its more than enough for current connections. :) )... or an exception.

   ---

   

   theuni commented at 2:33 PM on April 11, 2017:

   Thanks, I'll address both of those. Edit: No need to address these if we're not going to worry about wrapping.

7. 
   jnewbery commented at 2:12 PM on April 11, 2017: member

   utACK 17c93bf4f6cdf259c735becffd2e319ecc222dd4 , although I'd prefer NodeID to by a uint32_t

   EDIT: I'd prefer either using a uint32_t with this wrapping logic or a uint64_t without the new logic. Perhaps just add an assert in GetNewNodeId that we haven't reached std::numeric_limits<NodeId>::max().

8. 
   TheBlueMatt commented at 2:18 PM on April 11, 2017: member

   I'd kinda prefer we /just/ switch it to a int64_t and drop all the wrap logic. With a 64-bit ID you'd need billions of connections per second for years.

9. 
   sipa commented at 2:31 PM on April 11, 2017: member

   Agree with @TheBlueMatt.

   With a 64-bit ID you'd need billions of connections per second for years.

   585 years of 1 billion connections per second. I think that's sufficient.

10. 
    theuni commented at 2:41 PM on April 11, 2017: member

    Hmm.

    This originally switched to int32_t and addressed the wrapping as a non-theoretical problem. @TheBlueMatt pointed out that it may be possible that a user could mistakenly assume that the post-wrap nodeid "0" today is the same nodeid "0" that was connected 3 months ago, and take an action on the wrong node. Bumping to an int64_t solves that, so it seemed like a reasonable change.

    Thinking about this again after that change, yea, I suppose there's now no need to worry about wrapping at all.

11. net: define NodeId as an int64_t

    This should make occurances of NodeId wrapping essentially impossible for
    real-world usage.

    c851be4b25
12. theuni force-pushed on Apr 12, 2017
13. 
    theuni commented at 6:20 PM on April 12, 2017: member

    Updated after discussion here.

14. 
    TheBlueMatt commented at 6:52 PM on April 12, 2017: member

    utACK c851be4b25905977ca471c42435dc590fd2ff2f5

15. 
    jnewbery commented at 7:48 PM on April 12, 2017: member

    Looks good. ACK c851be4b25905977ca471c42435dc590fd2ff2f5

    Did you consider asserting in GetNewNodeID():

        assert(nLastNodeId.load() < std::numeric_limits<int64_t>::max());
    

16. 
    jtimon commented at 8:18 PM on April 12, 2017: contributor

    utACK c851be4

17. 
    laanwj commented at 2:34 PM on April 13, 2017: member

    585 years of 1 billion connections per second. I think that's sufficient.

    Yes, restarting your node every century at least is a good precaution in any case.

    Some JSON implementations will start to lose precision at 52 bits (due to using doubles for numbers). But not worried about that here.

    utACK https://github.com/bitcoin/bitcoin/commit/c851be4b25905977ca471c42435dc590fd2ff2f5 (also for 0.14)

18. laanwj added the label Needs backport on Apr 13, 2017
19. laanwj added this to the milestone 0.14.1 on Apr 13, 2017
20. laanwj merged this on Apr 13, 2017
21. laanwj closed this on Apr 13, 2017

    ---

22. laanwj referenced this in commit cf8a8b1028 on Apr 13, 2017
23. laanwj referenced this in commit 30fa231011 on Apr 14, 2017
24. laanwj removed the label Needs backport on Apr 14, 2017
25. codablock referenced this in commit fb6992de5d on Jan 26, 2018
26. andvgal referenced this in commit cc0945d38b on Jan 6, 2019
27. CryptoCentric referenced this in commit cb032fabc5 on Feb 27, 2019
28. MarcoFalke locked this on Sep 8, 2021

Contributors

theunilaanwjgmaxwelljnewberyTheBlueMattsipajtimon

Milestone
0.14.1