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

naumenkogs · 2020-04-15T01:13:29Z

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.

src/net_processing.cpp

naumenkogs · 2020-04-15T01:29:06Z

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

src/net_processing.cpp

sipa · 2020-04-15T02:05:30Z

@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: 5cbf753 (where it made sense because everything was XORed together, and the address used the high bits, while the time used the low ones).

src/net_processing.cpp

naumenkogs · 2020-04-15T14:05:34Z

Updated the code with suggestion by @sipa, and simplified tests.
They are a bit loosened now, but more robust to failures caused by randomness.

src/net_processing.cpp

DrahtBot · 2020-04-23T21:06:44Z

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

Conflicts

No conflicts as of last run.

jonatack

(Tested) Concept ACK.

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

jonatack · 2020-06-28T15:45:29Z

src/net_processing.cpp

+
+    std::chrono::seconds current_time = GetTime<std::chrono::seconds>();
+    // Adding address hash makes exact rotation time different per address, while preserving periodicity.
+    uint64_t time_addr = static_cast<uint64_t>((current_time + std::chrono::seconds{static_cast<int64_t>(hashAddr)}) / ROTATE_ADDR_RELAY_DEST_INTERVAL);


nit: Can this be simpler? It's more code and (even with the removed bit shifting) a bit more complex to understand.

We iterated over this line a couple times with @MarcoFalke, so I'm out of ideas here.
Open to suggestions!

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.

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

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?

Uh, right. I think that could be a problem. This will need something like

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

Github is weird, I found this un-resolved issue totally accidentally...

Okay the latest version should be safe?

test/functional/p2p_addr_relay.py

naumenkogs · 2020-07-01T10:10:23Z

@jonatack addressed all the things.

maflcko · 2022-01-04T10:29:54Z

It would be good to reply to the comments. Either addressing them or denying/closing them.

maflcko · 2022-01-04T11:10:34Z

Or alternatively submit the test in a separate pull request, since this is refactoring, so the test should already pass on master.

src/net_processing.cpp

maflcko · 2022-01-05T08:33:08Z

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?

src/net_processing.cpp

…RelayAddress fa9f455 refactor: Remove pointless and confusing shift in RelayAddress (MarcoFalke) Pull request description: The second argument written to the siphash is already quantized to 24 hours, so it seems confusing to quantize the first argument to 32 bits (out of 64 bits). > The shifting is pointless, we should get rid of it. It seems to be a silly evolution of this 2010 Satoshi code: 5cbf753 (where it made sense because everything was XORed together, and the address used the high bits, while the time used the low ones). (Copied from bitcoin/bitcoin#18642 (comment)) (The original code was `uint256 hashRand = hashSalt ^ (((int64)addr.ip)<<32) ^ ((GetTime()+addr.ip)/(24*60*60));`) This also allows to remove a integer sanitizer suppression for the whole file. ACKs for top commit: laanwj: Code review ACK fa9f455 sipa: utACK fa9f455 promag: Code review ACK fa9f455. Tree-SHA512: f5fd107464ccd839d6749aed6914b4935e39ab42906546b3f3810a7339fc4633fef931a1783a287572af5ec64525626fa91d147d8ff52eb076740465bf5cf839

…ress fa9f455 refactor: Remove pointless and confusing shift in RelayAddress (MarcoFalke) Pull request description: The second argument written to the siphash is already quantized to 24 hours, so it seems confusing to quantize the first argument to 32 bits (out of 64 bits). > The shifting is pointless, we should get rid of it. It seems to be a silly evolution of this 2010 Satoshi code: 5cbf753 (where it made sense because everything was XORed together, and the address used the high bits, while the time used the low ones). (Copied from bitcoin#18642 (comment)) (The original code was `uint256 hashRand = hashSalt ^ (((int64)addr.ip)<<32) ^ ((GetTime()+addr.ip)/(24*60*60));`) This also allows to remove a integer sanitizer suppression for the whole file. ACKs for top commit: laanwj: Code review ACK fa9f455 sipa: utACK fa9f455 promag: Code review ACK fa9f455. Tree-SHA512: f5fd107464ccd839d6749aed6914b4935e39ab42906546b3f3810a7339fc4633fef931a1783a287572af5ec64525626fa91d147d8ff52eb076740465bf5cf839

src/net_processing.cpp

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>

jonatack

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

jonatack

ACK 2ff8f4d

…on of a…

fanquake added the P2P label Apr 15, 2020

sipa reviewed Apr 15, 2020

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

maflcko reviewed Apr 15, 2020

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

naumenkogs force-pushed the 2020_04_mock_addr_rotation_time branch from 3c8319f to 8690d40 Compare April 15, 2020 01:30

sipa reviewed Apr 15, 2020

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

naumenkogs force-pushed the 2020_04_mock_addr_rotation_time branch from 8690d40 to ea6a2fd Compare April 15, 2020 14:05

naumenkogs force-pushed the 2020_04_mock_addr_rotation_time branch from ea6a2fd to cca35ff Compare April 15, 2020 14:12

maflcko reviewed Apr 15, 2020

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

naumenkogs force-pushed the 2020_04_mock_addr_rotation_time branch 2 times, most recently from 40c7baf to ae5eaf5 Compare April 16, 2020 14:39

maflcko reviewed Apr 17, 2020

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

naumenkogs force-pushed the 2020_04_mock_addr_rotation_time branch from ae5eaf5 to 72c4db3 Compare April 17, 2020 00:18

maflcko reviewed Apr 21, 2020

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

naumenkogs force-pushed the 2020_04_mock_addr_rotation_time branch from 72c4db3 to 8f0355e Compare April 21, 2020 20:48

DrahtBot mentioned this pull request Apr 23, 2020

Refactor: Move consts to their correct translation units #17383

Merged

DrahtBot added the Needs rebase label Apr 25, 2020

naumenkogs force-pushed the 2020_04_mock_addr_rotation_time branch from 8f0355e to d1ede75 Compare May 18, 2020 16:35

DrahtBot removed the Needs rebase label May 18, 2020

DrahtBot mentioned this pull request Jun 19, 2020

net, test: invalid p2p messages and test framework improvements #19272

Merged

DrahtBot added the Needs rebase label Jun 24, 2020

naumenkogs force-pushed the 2020_04_mock_addr_rotation_time branch from d1ede75 to ab4107a Compare June 25, 2020 09:10

DrahtBot removed the Needs rebase label Jun 25, 2020

naumenkogs force-pushed the 2020_04_mock_addr_rotation_time branch from ab4107a to 177df94 Compare June 25, 2020 11:28

jonatack reviewed Jun 28, 2020

View reviewed changes

naumenkogs force-pushed the 2020_04_mock_addr_rotation_time branch from 177df94 to 72d7e24 Compare July 1, 2020 09:14

maflcko mentioned this pull request Jan 4, 2022

Remove pointless and confusing shift in RelayAddress #23970

Merged

naumenkogs force-pushed the 2020_04_mock_addr_rotation_time branch from 7ba6b19 to b3c71ea Compare January 4, 2022 14:09

sdaftuar reviewed Jan 4, 2022

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

sipa reviewed Jan 4, 2022

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

naumenkogs force-pushed the 2020_04_mock_addr_rotation_time branch from b3c71ea to fca9977 Compare January 5, 2022 10:15

maflcko reviewed Jan 5, 2022

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

DrahtBot added the Needs rebase label Jan 6, 2022

naumenkogs force-pushed the 2020_04_mock_addr_rotation_time branch 2 times, most recently from c4a4eba to f6177ff Compare January 10, 2022 12:35

DrahtBot removed the Needs rebase label Jan 10, 2022

maflcko reviewed Jan 25, 2022

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

naumenkogs force-pushed the 2020_04_mock_addr_rotation_time branch from f6177ff to f2de1b2 Compare January 26, 2022 16:36

DrahtBot added the Needs rebase label Mar 2, 2022

naumenkogs and others added 2 commits March 13, 2022 16:40

Use std::chrono for salting when randomizing ADDR destination

77ccb7f

Add tests for addr destination rotation

2ff8f4d

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>

jonatack reviewed Mar 13, 2022

View reviewed changes

naumenkogs force-pushed the 2020_04_mock_addr_rotation_time branch 3 times, most recently from f2de1b2 to 2ff8f4d Compare March 14, 2022 06:21

DrahtBot removed the Needs rebase label Mar 14, 2022

jonatack reviewed Mar 14, 2022

View reviewed changes

fanquake requested a review from maflcko April 26, 2022 14:36

maflcko merged commit f0a834e into bitcoin:master Apr 27, 2022

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 29, 2022

Merge bitcoin#18642: Use std::chrono for the time to rotate destinati…

fef23d9

…on of a…

bitcoin locked and limited conversation to collaborators Apr 27, 2023

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

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

Uh oh!

Conversation

naumenkogs commented Apr 15, 2020

Uh oh!

Uh oh!

naumenkogs commented Apr 15, 2020

Uh oh!

Uh oh!

sipa commented Apr 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

naumenkogs commented Apr 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DrahtBot commented Apr 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sipa Jan 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

naumenkogs commented Jul 1, 2020

Uh oh!

maflcko commented Jan 4, 2022

Uh oh!

maflcko commented Jan 4, 2022

Uh oh!

Uh oh!

Uh oh!

maflcko commented Jan 5, 2022

Uh oh!

Uh oh!

Uh oh!

jonatack left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sipa commented Apr 15, 2020 •

edited

Loading

naumenkogs commented Apr 15, 2020 •

edited

Loading

DrahtBot commented Apr 23, 2020 •

edited

Loading

sipa Jan 4, 2022 •

edited

Loading

jonatack left a comment •

edited

Loading