Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FastRandomContext improvements and switch to ChaCha20 #9792

Merged
merged 5 commits into from Apr 24, 2017

Conversation

Projects
None yet
6 participants
@sipa
Copy link
Member

sipa commented Feb 18, 2017

This switches FastRandomContext to use a ChaCha20-based random number generator. It also makes the class richer by adding support for getting single bits of entropy.

Benchmarks (also added) show that rand32 became around 5.25x slower on my machine (from 1.5ns to 8ns), but the new randbool is 15% faster than the old one (1.3ns).

@gmaxwell

This comment has been minimized.

Copy link
Member

gmaxwell commented Feb 18, 2017

src/addrman.cpp: nKBucket = (nKBucket + insecure_rand.rand32()) % ADDRMAN_TRIED_BUCKET_COUNT;
src/addrman.cpp: nKBucketPos = (nKBucketPos + insecure_rand.rand32()) % ADDRMAN_BUCKET_SIZE;
src/addrman.cpp: nUBucket = (nUBucket + insecure_rand.rand32()) % ADDRMAN_NEW_BUCKET_COUNT;
src/addrman.cpp: nUBucketPos = (nUBucketPos + insecure_rand.rand32()) % ADDRMAN_BUCKET_SIZE;
src/bench/checkqueue.cpp: p.resize(insecure_rand.rand32() % (PREVECTOR_SIZE*2));
src/net.h: vAddrToSend[insecure_rand.rand32() % vAddrToSend.size()] = _addr;

Usage wants a randrange a lot more than a rand32

src/test/crypto_tests.cpp Outdated
@@ -439,4 +440,29 @@ BOOST_AUTO_TEST_CASE(aes_cbc_testvectors) {
"b2eb05e2c39be9fcda6c19078c6a9d1b3f461796d6b0d6b2e0c2a72b4d80e644");
}

BOOST_AUTO_TEST_CASE(chacha20_testvector)

This comment has been minimized.

@jonasschnelli

jonasschnelli Feb 19, 2017

Member

Maybe add all test vectors (only 5) from the IEFT draft specs: https://github.com/jonasschnelli/chacha20poly1305/blob/master/tests.c#L35

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Feb 19, 2017

Concept ACK

@sipa sipa force-pushed the sipa:chacha branch 2 times, most recently Feb 19, 2017

@sipa

This comment has been minimized.

Copy link
Member Author

sipa commented Feb 20, 2017

Added randrange and the test vectors @jonasschnelli suggested.

@@ -183,11 +183,8 @@ class prevector_tester {
}

~prevector_tester() {
BOOST_CHECK_MESSAGE(passed, "insecure_rand_Rz: "

This comment has been minimized.

@TheBlueMatt

TheBlueMatt Feb 22, 2017

Contributor

Looks like this disabled a bunch of tests?

This comment has been minimized.

@sipa

sipa Feb 25, 2017

Author Member

Oops, nice catch. Fixed.

configure.ac Outdated
@@ -533,6 +533,9 @@ AC_CHECK_DECLS([bswap_16, bswap_32, bswap_64],,,
#include <byteswap.h>
#endif])

AC_MSG_CHECKING(for __builtin_clzl)

This comment has been minimized.

@TheBlueMatt

TheBlueMatt Feb 22, 2017

Contributor

Hmm...is it definitely the case that all the compilers we support have this? Can we not have some fallback for those that do not?

This comment has been minimized.

@sipa

sipa Feb 25, 2017

Author Member

I don't know of any compilers that don't. I'd be happy to write a fallback if there is one that isn't.

This comment has been minimized.

@laanwj

laanwj Feb 25, 2017

Member

I think MSVC doesn't.

src/random.h Outdated
@@ -35,6 +35,7 @@ void GetStrongRandBytes(unsigned char* buf, int num);
*/
class FastRandomContext {
private:
bool requires_seed;

This comment has been minimized.

@TheBlueMatt

TheBlueMatt Feb 22, 2017

Contributor

...except you never check requires_seed to do the actual seeding?

This comment has been minimized.

@sipa

sipa Feb 25, 2017

Author Member

Fixed, and added a test to catch that.

src/random.h Outdated
unsigned long randrange(unsigned long range)
{
--range;
int bits = 8 * sizeof(long) - __builtin_clzl(range);

This comment has been minimized.

@gmaxwell

gmaxwell Feb 23, 2017

Member

CLZ is undefined for 0, a range of 1 is dumb but might be mechanically generated in some case. At a minimum there should be a comment that range must be greater than 1.

This comment has been minimized.

@sipa

sipa Feb 25, 2017

Author Member

Fixed.

@sipa sipa force-pushed the sipa:chacha branch 2 times, most recently Feb 25, 2017

@sipa

This comment has been minimized.

Copy link
Member Author

sipa commented Feb 25, 2017

Added a wrapper for __builtin_clzl, added unit tests, and fixed a few edge cases.

@sipa sipa force-pushed the sipa:chacha branch 3 times, most recently Feb 26, 2017

src/random.h Outdated
@@ -89,6 +90,16 @@ class FastRandomContext {
}
}

uint64_t randrange(uint64_t range)

This comment has been minimized.

@gmaxwell

gmaxwell Feb 28, 2017

Member

This needs a comment that points out that range returned will be [0..range) and that range must not be zero.

This comment has been minimized.

@sipa

sipa Mar 29, 2017

Author Member

Fixed.

@sipa sipa force-pushed the sipa:chacha branch to 4fd2d2f Mar 29, 2017

@sipa

This comment has been minimized.

Copy link
Member Author

sipa commented Mar 29, 2017

Rebased.

@laanwj laanwj added this to Blockers in High-priority for review Apr 13, 2017

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor

TheBlueMatt commented Apr 14, 2017

Looks good to me. I didnt re-verify the chacha code is correct, and dont know that the makefile changes are sane.

@gmaxwell
Copy link
Member

gmaxwell left a comment

utACK

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Apr 24, 2017

utACK 4fd2d2f

@laanwj laanwj merged commit 4fd2d2f into bitcoin:master Apr 24, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Apr 24, 2017

Merge #9792: FastRandomContext improvements and switch to ChaCha20
4fd2d2f Add a FastRandomContext::randrange and use it (Pieter Wuille)
1632922 Switch FastRandomContext to ChaCha20 (Pieter Wuille)
e04326f Add ChaCha20 (Pieter Wuille)
663fbae FastRandom benchmark (Pieter Wuille)
c21cbe6 Introduce FastRandomContext::randbool() (Pieter Wuille)

Tree-SHA512: 7fff61e3f6d6dc6ac846ca643d877b377db609646dd401a0e8f50b052c6b9bcd2f5fc34de6bbf28f04afd1724f6279ee163ead5f37d724fb782a00239f35db1d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.