Seed RNG with precision timestamps on receipt of net messages.

TheBlueMatt commented at 4:51 pm on November 23, 2019: member

See title. Exposes a generic dead-simple “SeedEvent” interface, but currently just used for net messages.

fanquake added the label P2P on Nov 23, 2019

fanquake added the label Utils/log/libs on Nov 23, 2019

in src/random.h:98 in e7db8012be outdated

93+ * Gathers entropy from the low bits of the time at which events occur. Should
94+ * be called with a few bytes describing the event at the time an event occurs.
95+ *
96+ * Thread-safe.
97+ */
98+void SeedEvent(const unsigned char* event_type_buf, size_t buf_len);

sipa commented at 5:00 pm on November 23, 2019:

Do we expect the argument to always be a string? The interface could be made a bit simpler and safer if that’s the case (you could also avoid the size_t prefix, and instead just write the c_str() including null byte, which self-describes the length).

TheBlueMatt commented at 5:02 pm on November 23, 2019:

I presume not, both for upcoming p2p encryption where the message type may just be a char, and I currently have some radio stuff hooked up that dumps RSSI information into this.

laanwj commented at 5:23 pm on November 23, 2019:

I kind of prefer this general (ptr,size) interface to using C strings, whose use we’re generally trying to avoid in bitcoin code. std::string_view would be a safer interface, if only C++11 had it.

sipa commented at 6:24 pm on November 23, 2019:

Here is a more C++ish interface, that serializes (through the serialization framework) all arguments passed to SeedEvent (without any allocations): https://github.com/sipa/bitcoin/tree/bluematt_2019-11-rng-netmsg

TheBlueMatt commented at 6:28 pm on November 23, 2019:

Oof, that’s….quite a bigger diff. Are you worried about the current interface introducing bugs? I don’t think it’s particularly likely (nor are we likely to add calls to this everywhere), and it matches the rest of the random. interface.

sipa commented at 6:35 pm on November 23, 2019:

If you switch to SHA256 (which you did) it’s much smaller.

It also enables efficiently adding extra information without first copying everything into a buffer (say, you could call SeedEvent(msg.command, peerid); just as easily)

I agree it’s perhaps overkill - up to you.

in src/random.cpp:491 in e7db8012be outdated

487@@ -485,6 +488,14 @@ static void SeedPeriodic(CSHA512& hasher, RNGState& rng)
488     // High-precision timestamp
489     SeedTimestamp(hasher);
490 
491+    {

sipa commented at 5:06 pm on November 23, 2019:

If you’d abstract this code block out, and then call it from say both SeedPeriodic and SeedSlow, the additional entropy would always be available to strong RNG output, even if a minute has not yet passed. You could make the hashing step conditional on events_hasher.Size() != 0.

TheBlueMatt commented at 6:03 pm on November 23, 2019:

Hmm, you mean in SeedSlow Finalize the events_hasher, then write the buffer back into the events_hasher and have SeedPeriodic wipe it? Cause SeedSlow doesn’t store the data back into the RNGState.

sipa commented at 6:26 pm on November 23, 2019:

All the Seed* functions just add to a CSHA512; ultimately, they’re all called from ProcRand, which mixes it with the existing RNGState, and produces output.

SeedPeriodic goes a step further, in trying to make sure that the existing RNGState is also fed through the strengthener, but that isn’t really required.

So just copying this code block to SeedSlow would work.

TheBlueMatt commented at 6:28 pm on November 23, 2019:

Right, so I did this.

TheBlueMatt force-pushed on Nov 23, 2019

TheBlueMatt commented at 6:29 pm on November 23, 2019: member

It was pointed out that this would make processing of small net messages particularly slower. Instead of only mixing in extra randomness for large net messages I switched it to SHA256 to use our existing ASM optimizations here. Also changed the fn name to be closer to the rest of random.h.

in src/random.cpp:582 in 95f5708067 outdated

576@@ -553,6 +577,16 @@ void GetRandBytes(unsigned char* buf, int num) noexcept { ProcRand(buf, num, RNG
577 void GetStrongRandBytes(unsigned char* buf, int num) noexcept { ProcRand(buf, num, RNGLevel::SLOW); }
578 void RandAddPeriodic() { ProcRand(nullptr, 0, RNGLevel::PERIODIC); }
579 
580+void RandAddEvent(const unsigned char* event_type_buf, size_t buf_len) {
581+    LOCK(events_mutex);
582+    events_hasher.Write((unsigned char *)&buf_len, sizeof(size_t));

sipa commented at 6:37 pm on November 23, 2019:

You could also limit this to 4 bytes (or even 2…), if size is an issue.

TheBlueMatt commented at 6:43 pm on November 23, 2019:

Hmm, I’m not too worried about it? Its mostly up to the caller and right now net will only ever pass in 12+4+8 bytes per call, which means it will never be more than one extra hash per two messages, which seems reasonable.

in src/random.cpp:586 in 95f5708067 outdated

581+    LOCK(events_mutex);
582+    events_hasher.Write((unsigned char *)&buf_len, sizeof(size_t));
583+    events_hasher.Write(event_type_buf, buf_len);
584+    // Get the low four bytes of the performance counter. This translates to roughly the
585+    // subsecond part.
586+    int32_t perfcounter = (GetPerformanceCounter() & 0xffffffff);

sipa commented at 6:37 pm on November 23, 2019:

Nit: uint32_t is probably more appropriate

sipa commented at 6:44 pm on November 23, 2019: member

A comment on including the timestamp: GetPerformanceCounter on ARM is not extremely cheap (it involves a system call), perhaps you’d want a way for the caller to provide their own timestamp (or timestamp-like variable), as I assume generally they already have one?

TheBlueMatt commented at 6:48 pm on November 23, 2019: member

Hmm, net does have one, but its also mostly intended for things exactly like net, where you are handling an “event” that is the result of a syscall already…Unless its particularly more expensive than poll() + read(), I’m not too worried :). Its not like we’re gonna call it somewhere where performance in nanoseconds is absolutely critical, though may be calling it in places where DoS is a risk. So…don’t think its worth breaking the abstraction for it.

sipa commented at 6:52 pm on November 23, 2019: member

I would indeed assume that it’s not more expensive than poll+read, good point.

sipa commented at 7:18 pm on November 23, 2019: member

Here is another idea: use the serialization-based interface, but instead feeding it directly into events_hasher, feed it into a SipHasher instead. Then feed a few bits (8, 16, 32?) of the SipHash of all passed data to events_hasher. That should be both simpler (no need to expose internals as much), and faster (much less SHA256’ing, don’t hold lock as long).

TheBlueMatt force-pushed on Nov 23, 2019

TheBlueMatt commented at 7:21 pm on November 23, 2019: member

Even simpler idea, just use the damn message hash instead of the command :). Its nice and small (4 bytes), plus has more entropy in it anyway. Since our (real) goal is to get the time when we receive a message, that’s more than sufficient, but we also steal our peers’ entropy in their ping nonces :).

TheBlueMatt force-pushed on Nov 23, 2019

sipa commented at 8:27 pm on November 23, 2019: member

Nice… if the caller has too much data they can always siphash or wahtever it thenselves. You could simplify the interface by just allowing a uint32_t to be passed or so.

TheBlueMatt force-pushed on Nov 23, 2019

TheBlueMatt commented at 8:44 pm on November 23, 2019: member

Done. Its now an explicit uint32_t.

TheBlueMatt force-pushed on Nov 23, 2019

in src/net.cpp:701 in ca83443569 outdated

696@@ -693,6 +697,10 @@ CNetMessage V1TransportDeserializer::GetMessage(const CMessageHeader::MessageSta
697     msg.m_message_size = hdr.nMessageSize;
698     msg.m_raw_message_size = hdr.nMessageSize + CMessageHeader::HEADER_SIZE;
699 
700+    // We just received a message off the wire, harvest entropy from the time (and the message checksum)
701+    uint32_t msg_hash = (hash.begin()[0] << 8*3) | (hash.begin()[1] << 8*2) | (hash.begin()[2] << 8*1) | hash.begin()[3];

sipa commented at 8:53 pm on November 23, 2019:

ReadLE32(hash.begin()) ?

Seed RNG with precision timestamps on receipt of net messages. 02d8c56a18

in src/random.h:94 in ca83443569 outdated

88@@ -89,6 +89,14 @@ void GetStrongRandBytes(unsigned char* buf, int num) noexcept;
89  */
90 void RandAddPeriodic();
91 
92+/**
93+ * Gathers entropy from the low bits of the time at which events occur. Should
94+ * be called with a few bytes describing the event at the time an event occurs.

sipa commented at 9:02 pm on November 23, 2019:

This comment seems outdated now.

TheBlueMatt commented at 9:06 pm on November 23, 2019:

Heh, I figured I’d leave it, since it doesn’t actually matter what the 4 bytes are, they just have to be aligned and typed as a u32.

TheBlueMatt force-pushed on Nov 23, 2019

in src/random.cpp:465 in 02d8c56a18

460+
461+    unsigned char events_hash[32];
462+    events_hasher.Finalize(events_hash);
463+    hasher.Write(events_hash, 32);
464+
465+    // Re-initialize the hasher with the finalized state to use later.

sipa commented at 9:50 pm on November 23, 2019:

Ha, neat.

sipa commented at 9:51 pm on November 23, 2019: member

utACK https://github.com/bitcoin/bitcoin/commit/02d8c56a18b9a2960888d6ec1209955105bae847

DrahtBot commented at 10:22 pm on November 23, 2019: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#17167 (Allow whitelisting outgoing connections by luke-jr)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

sipa commented at 10:27 pm on November 23, 2019: member

This effectively adds one SHA256 compression per 8 messages/connections. That seems acceptably low, compared to the work already done for message checksumming.

meshcollider commented at 7:03 am on November 24, 2019: contributor

utACK 02d8c56a18b9a2960888d6ec1209955105bae847

TheBlueMatt commented at 5:30 pm on November 24, 2019: member

The AppVeyor build fail occurs before build even starts, so is a false positive.

practicalswift commented at 0:02 am on November 25, 2019: contributor

Concept ACK: nice idea and straightforward implementation

practicalswift commented at 0:11 am on November 25, 2019: contributor

Clang Thread Safety Analysis annotations seem to be missing? :)

naumenkogs commented at 8:09 pm on December 3, 2019: member

Concept ACK

laanwj commented at 12:12 pm on December 4, 2019: member

ACK 02d8c56a18b9a2960888d6ec1209955105bae847

laanwj referenced this in commit 41919631d5 on Dec 4, 2019

laanwj merged this on Dec 4, 2019

laanwj closed this on Dec 4, 2019

practicalswift commented at 1:22 pm on December 4, 2019: contributor

@laanwj I was a bit surprised to see this merged without thread safety annotations, or more specifically before the thread safety questions were addressed. Was there a reason for that? :)

laanwj commented at 1:39 pm on December 4, 2019: member

It has three ACKs and passes travis.

If you want to add any annotations, go ahead, but we’re not going to block merges on always new requirements.

sidhujag referenced this in commit 93e4da26d6 on Dec 4, 2019

sipsorcery commented at 8:37 pm on December 4, 2019: member

This change is causing problems on Windows. No appveyor tests have passed since it was merged and I’ve also verified locally that bitcoind.exe, bitcoin-qt.exe, bench_bitcoin.exe and test_bitcoin.exe are failing. Failing in this case means exiting almost immediately with no error message.

Running the programs in the Visual Studio debugger gets an Access Violation exception reported on the LOCK(events_mutex) call. I’m always cautious with Access Violation exceptions as often the cause could have been an earlier call and the reported site is a side effect.

As a quick and dirty test I moved the declaration of static Mutex events_mutex to be a local static variable in the two methods using it. The programs were all able to run normally after that. I realise that’s defeating the purpose of the mutex so isn’t a fix.

sipa commented at 8:39 pm on December 4, 2019: member

@sipsorcery I should have known that would happen; I suspect it is invoking the RNG before global initialization is complete, and thus potentially before events_mutex is initialized. Working on a fix.

EDIT: see #17670.

MarcoFalke referenced this in commit 5d0b7f9e3d on Dec 5, 2019

MarcoFalke referenced this in commit cf43f3f0a8 on Dec 5, 2019

sidhujag referenced this in commit eac33a1534 on Dec 6, 2019

jasonbcox referenced this in commit f9c9a3ff0c on Nov 5, 2020

sidhujag referenced this in commit 860e92192b on Nov 10, 2020

sidhujag referenced this in commit 8acc626212 on Nov 10, 2020

random-zebra referenced this in commit 93f43f0f81 on Apr 14, 2021

DrahtBot locked this on Dec 16, 2021

Seed RNG with precision timestamps on receipt of net messages. #17573

Conflicts