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.

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)

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::max().

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.

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.

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.

Updated after discussion here.

utACK c851be4

Looks good. ACK c851be4

Did you consider asserting in GetNewNodeID():

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

utACK c851be4

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 c851be4 (also for 0.14)