search

dogecoin / dogecoin

very currency
MIT License
14.4k stars 2.8k forks source link

backport: Add perf counter data to GetStrongRandBytes state in scheduler #3495

Closed xanimo closed 1 month ago

xanimo commented 1 month ago

This is an intermediate step in regards to updating the code base prior to integration of #3229. Note it should most likely be added prior to #3487.

EDIT: These commits add an internal method to add new random data to our internal random number generator state (AddDataToRng) and adds performance counter data to that state (RandAddSeedSleep) within the CScheduler::serviceQueue. As discussed below, these commits are eventually removed.

patricklodder commented 1 month ago

Could you please fix the description of what this does?

The first commit does something extra: it adds a rand seeding mechanism that includes a sleep, and that entire mechanism was removed again with d61f2bb0 later on.

Which part of adding ASmap needs this change?

(edit: I read the title 5 times more and it actually in a way does cover it 😂)

xanimo commented 1 month ago

Yeah I'll work on it. It's pretty much a direct copy of https://github.com/bitcoin/bitcoin/pull/10372 but in any case I believe this to be necessary en route to both removing OpenSSL here and to cleanly allow cherry-picking of https://github.com/bitcoin/bitcoin/pull/15224 after https://github.com/bitcoin/bitcoin/pull/14955.

That last one was a bit more complex to pick because they updated their naming conventions for locks and mutexes in https://github.com/sipa/bitcoin/commit/9c4dc597ddc66acfd58a945a5ab11f833731abba and https://github.com/sipa/bitcoin/commit/190bf62be1214b072513c7fd7e01cc191723967c.

patricklodder commented 1 month ago

I believe this to be necessary en route to both removing OpenSSL [..] and to cleanly allow cherry-picking

This code was also removed before the removal of OpenSSL, see: https://github.com/bitcoin/bitcoin/pull/17265#issuecomment-546678965 for the discussion. I'm not a fan of doing a pick for things we then overwrite (twice!) in the same "project". It adds massive review pressure for code that never should make it into a release in the first place. Also, if code like this just disappears from Bitcoin Core, I worry that there's been a (non-public) finding on it.

I guess that what this really means is that I was wrong in my supposition that fully removing OpenSSL can be done cleanly. So it's good we find this now.