perf: Make the key cracker a bit faster #1622

sudden6 · 2020-05-31T22:30:55Z

more inteligent secret key generation
use OpenMP for multithreading
support prefixes with odd number of hex characters

This change is

nurupo

Reviewed 2 of 2 files at r1.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @sudden6)

other/fun/cracker.c, line 118 at r1 (raw file):

     * afford to rewrite this program.
     */
    if (strlen(argv[1]) > 16) {

Didn't we just calculate strlen(argv[1]) a line above?

other/fun/cracker.c, line 120 at r1 (raw file):

    if (strlen(argv[1]) > 16) {
        printf("Finding a key with more than 16 hex chars as prefix is not supported\n");
        return -1;

Why use -1? 1 is usually used to indicate a failure.

Also, using negative exit code doesn't make sense as they are treated as unsigned. POSIX uses only 8 bits of the exit code and treats it as unsigned number, Windows uses 32-bits and treats it as unsigned.

other/fun/cracker.c, line 123 at r1 (raw file):

    }

    uint8_t hex_prefix[8] = {0};

A constant along the lines of MAX_HEX_PREFIX_LENGTH = 8 could be introduced and used in here and in the strlen() check above as MAX_PREFIX*2.

other/fun/cracker.c, line 125 at r1 (raw file):

    uint8_t hex_prefix[8] = {0};

    size_t prefix_len = hex_string_to_bin(argv[1], prefix_chars_len, hex_prefix);

Since we are using libsodium, we could use sodium_hex2bin() instead.

other/fun/cracker.c, line 151 at r1 (raw file):

    uint32_t longest_match = 0;

    /* Maybe we're lucky */

Why is this code duplication needed? The omp code below would handle it just as well.

other/fun/cracker.c, line 171 at r1 (raw file):

    // Finishes a batch every ~10s on my PC
    uint64_t batch_size = (UINT64_C(1) << 17) * NUM_THREADS();

Instead of hardcoding the value based on someone's CPU, we could make an initial guess on the batch size that takes 10s to process and then check after each batch how long it actually took, correcting our batch_size value to hit the target time.

other/fun/cracker.c, line 176 at r1 (raw file):

    double old_seconds_passed = seconds_passed;

    for (uint64_t tries = 0; tries < UINT64_MAX; tries += batch_size) {

Unless UINT64_MAX is divisible by batch_size, the tries < UINT64_MAX condition would be missed due to tries overflowing with tries += batch_size. The condition should be tries < UINT64_MAX - batch_size.

It would take a very very very long time (years? hundreds of years?) for this to happen, 2^64 is a lot of keys to try, but it can happen as we are not guaranteed to find an 8-byte common prefix in 2^64 tries. 8 bytes is 64 bits, so if they keys were 8 bytes long then it could take at most 2^64 tries to find it, but the key is a lot bigger than 8 bytes and some key tries will result in the same first 8 bytes, so we might exceed 2^64 tries.

Why is this not an infinite while(1) loop anyway, with a unit64_t tries = 0; declared outside the loop?

other/fun/cracker.c, line 179 at r1 (raw file):

        #pragma omp parallel for firstprivate(priv_key_shadow) shared(longest_match, tries, batch_size, hex_prefix, prefix_chars_len) schedule(static) default(none)

        for (uint64_t batch = tries; batch < (tries + batch_size); batch++) {

tries + batch_size might overflow. Shouldn't be an issue if the loop above is fixed though.

other/fun/cracker.c, line 213 at r1 (raw file):

        if (longest_match >= prefix_chars_len) {
            printf("Runtime: %lus, Calculating %e keys/s\n", (unsigned long) seconds_passed, (tries + batch_size) / seconds_passed);

(tries + batch_size) can overflow.

Shouldn't be an issue if the loop is fixed.

other/fun/cracker.c, line 220 at r1 (raw file):

        if (seconds_passed - old_seconds_passed > 5.0) {
            old_seconds_passed = seconds_passed;
            printf("Runtime: %lus, Calculating %e keys/s\n", (unsigned long) seconds_passed, (tries + batch_size) / seconds_passed);

(tries + batch_size) can overflow.

Shouldn't be an issue if the loop is fixed.

other/fun/cracker.c, line 233 at r1 (raw file):

    print_key(priv_key);
    printf("\n");
    return -2;

Don't use negative exit codes, use 2 instead.

sudden6

Thanks for reviewing! Didn't even think someone would bother :D

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @nurupo)

other/fun/cracker.c, line 118 at r1 (raw file):

Previously, nurupo wrote…

Didn't we just calculate strlen(argv[1]) a line above?

Done.

other/fun/cracker.c, line 120 at r1 (raw file):

Previously, nurupo wrote…

Why use -1? 1 is usually used to indicate a failure.

Also, using negative exit code doesn't make sense as they are treated as unsigned. POSIX uses only 8 bits of the exit code and treats it as unsigned number, Windows uses 32-bits and treats it as unsigned.

Fixed. Was thinking of some other project where negative return values indicate failure.

other/fun/cracker.c, line 123 at r1 (raw file):

Previously, nurupo wrote…

A constant along the lines of MAX_HEX_PREFIX_LENGTH = 8 could be introduced and used in here and in the strlen() check above as MAX_PREFIX*2.

Done.

other/fun/cracker.c, line 125 at r1 (raw file):

Previously, nurupo wrote…

Since we are using libsodium, we could use sodium_hex2bin() instead.

sodium_hex2bin() docs say nothing about odd length strings being supported, so I (not so quickly) wrote my own.

other/fun/cracker.c, line 151 at r1 (raw file):

Previously, nurupo wrote…

Why is this code duplication needed? The omp code below would handle it just as well.

Rewrote this completely.

other/fun/cracker.c, line 171 at r1 (raw file):

Previously, nurupo wrote…

Instead of hardcoding the value based on someone's CPU, we could make an initial guess on the batch size that takes 10s to process and then check after each batch how long it actually took, correcting our batch_size value to hit the target time.

Thought about that, but discarded the idea, because it makes batching and more complicated and it's non-critical as long as the runtime is big enough that it's not a significant overhead.

other/fun/cracker.c, line 176 at r1 (raw file):
Thanks for pointing that out, I was trying for correctness and horribly failed.

Why is this not an infinite while(1) loop anyway, with a unit64_t tries = 0; declared outside the loop?

OpenMP only supports certain forms of for loops
I want this to be at least theoretically correct, so no infinite loop

other/fun/cracker.c, line 179 at r1 (raw file):

Previously, nurupo wrote…

tries + batch_size might overflow. Shouldn't be an issue if the loop above is fixed though.

Rewrote this.

other/fun/cracker.c, line 213 at r1 (raw file):

Previously, nurupo wrote…

(tries + batch_size) can overflow.

Shouldn't be an issue if the loop is fixed.

Rewrote this.

other/fun/cracker.c, line 220 at r1 (raw file):

Previously, nurupo wrote…

(tries + batch_size) can overflow.

Shouldn't be an issue if the loop is fixed.

Rewrote this.

other/fun/cracker.c, line 233 at r1 (raw file):

Previously, nurupo wrote…

Don't use negative exit codes, use 2 instead.

Done.

nurupo

Reviewed 1 of 1 files at r2.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @nurupo and @sudden6)

other/fun/cracker.c, line 125 at r1 (raw file):

not so quickly

Heh, it would have been quicker to test if it does or not support off length. Pretty sure it does, otherwise it would have mentioned in the doc. Looking at the code, it seems to advance hex_pos one by one too, processing just one hex character at a time, instead of groups on two.

other/fun/cracker.c, line 171 at r1 (raw file):

Previously, sudden6 wrote…

Thought about that, but discarded the idea, because it makes batching and more complicated and it's non-critical as long as the runtime is big enough that it's not a significant overhead.

It might be too big, e.g. if you run it on a less powerful CPU like RPi's, you might end up be getting status update messages every 60 seconds instead of 5-10 seconds.

other/fun/cracker.c, line 205 at r2 (raw file):

    const uint64_t rem_batch_size = UINT64_MAX % batch_size;

    const uint64_t rem_start = UINT64_MAX - rem_batch_size - 1;

Are you sure it should be -1?

Also, the case with rem_batch_size being zero is weird, as you wouldn't want to run cracker_core() in that case but the current code would.

other/fun/cracker.c, line 213 at r2 (raw file):

    // Reduce time to first stats output
    print_stats(seconds_passed, rem_batch_size + 1);

+1? Why did we process more keys than were remaining?

other/fun/cracker.c, line 222 at r2 (raw file):

    for (uint64_t tries = 0; tries < rem_start; tries += batch_size) {
        cracker_core(tries, tries + batch_size, 0, priv_key_shadow, &longest_match, hex_prefix, prefix_chars_len);

I don't understand what does the 3rd argument do.

sudden6

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @nurupo)

other/fun/cracker.c, line 125 at r1 (raw file):

Previously, nurupo wrote…

not so quickly

Heh, it would have been quicker to test if it does or not support off length. Pretty sure it does, otherwise it would have mentioned in the doc. Looking at the code, it seems to advance hex_pos one by one too, processing just one hex character at a time, instead of groups on two.

After looking at the code for a few minutes, I still can't tell if it's explicitly supported...

other/fun/cracker.c, line 171 at r1 (raw file):

Previously, nurupo wrote…

It might be too big, e.g. if you run it on a less powerful CPU like RPi's, you might end up be getting status update messages every 60 seconds instead of 5-10 seconds.

Would be fine IMO, printing status messages is not the main purpose of this program and results are printed immediately anyway

other/fun/cracker.c, line 205 at r2 (raw file):

Previously, nurupo wrote…

Are you sure it should be -1?

Also, the case with rem_batch_size being zero is weird, as you wouldn't want to run cracker_core() in that case but the current code would.

yes, since the for loop in cracker_core() has the abort condition < UINT64_MAX the iteration variable can never be UINT64_MAX, but we need to test this value too.

My solution to this is, to add an offset parameter, which is added to the loop iteration variable. Outside the mainloop this is set to 1, so now with the condition i < UINT64_MAX and i + offset with offset = 1 we can reach the case UINT64_MAX, without overflowing our iteration variable. To calculate the the iteration with UINT64_MAX we need to run this call to cracker_core at least once and also subtract 1 from the start range, since the offset will compensate for that.

The drawback is, that even if batch_size % 0, we will get two unbalanced loop iterations. That's not a problem though, since OpenMP can handle that and it's only 2 iterations out of very very many that don't run at maximum performance.

other/fun/cracker.c, line 213 at r2 (raw file):

Previously, nurupo wrote…

+1? Why did we process more keys than were remaining?

See above, easiest to think it through is with the case where rem_batch_size = 0.

other/fun/cracker.c, line 222 at r2 (raw file):

Previously, nurupo wrote…

I don't understand what does the 3rd argument do.

Explained above.

nurupo

Reviewable status: complete! 1 of 1 approvals obtained

iphydf · 2021-12-01T20:27:10Z

Do you think it makes sense to make 2 of these, one simple one and one with openmp? cracker.c and cracker_omp.c. That way, the simple cracker stays at 79 lines, which is a nice thing to start hacking on.

sudden6 · 2021-12-01T22:20:58Z

I think OpenMP has very little to do with the increased code size here. It's also easy to ignore, because the program works the same without it, just single threaded.

sudden6 · 2021-12-16T20:36:18Z

@iphydf Added the simple key cracker back as discussed on IRC. I slightly modified the loop condition in L66 to avoid counting the first and last byte of the secret key, since these have a mask applied on them and will generate the same public key multiple times.

JFreegman

Reviewed 3 of 3 files at r3.
Reviewable status: complete! 1 of 1 approvals obtained (waiting on @sudden6)

other/fun/cracker_simple.c, line 23 at r3 (raw file):

#include "../../toxcore/ccompat.h"

static void print_key(uint8_t *client_id)

const

other/fun/cracker_simple.c, line 25 at r3 (raw file):

static void print_key(uint8_t *client_id)
{
    uint32_t j;

declare 'j' in for loop expression

other/fun/cracker_simple.c, line 27 at r3 (raw file):

    uint32_t j;

    for (j = 0; j < 32; j++) {

Magic number

other/fun/cracker_simple.c, line 42 at r3 (raw file):

    long long unsigned int num_tries = 0;

    uint32_t len = strlen(argv[1]) / 2;

const size_t

other/fun/cracker_simple.c, line 43 at r3 (raw file):

    uint32_t len = strlen(argv[1]) / 2;
    unsigned char *key = hex_string_to_bin(argv[1]);

const

other/fun/cracker_simple.c, line 44 at r3 (raw file):

    uint32_t len = strlen(argv[1]) / 2;
    unsigned char *key = hex_string_to_bin(argv[1]);
    uint8_t pub_key[32], priv_key[32], c_key[32];

magic numbers

other/fun/cracker_simple.c, line 46 at r3 (raw file):

    uint8_t pub_key[32], priv_key[32], c_key[32];

    if (len > 32) {

more magic numbers

other/fun/cracker_simple.c, line 56 at r3 (raw file):

    while (1) {
        crypto_scalarmult_curve25519_base(pub_key, priv_key);
        uint32_t i;

declare i in for loop expression

other/fun/cracker_simple.c, line 66 at r3 (raw file):

         * curve25519. Using them would generate duplicate keys.
         */
        for (i = 31; i > 1; --i) {

magic number

sudden6

Reviewable status: complete! 1 of 1 approvals obtained (waiting on @JFreegman)

other/fun/cracker_simple.c, line 23 at r3 (raw file):