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

Unnecessary call to secp256k1_sha256_initialize #1179

Open
Coding-Enthusiast opened this issue Dec 16, 2022 · 5 comments
Open

Unnecessary call to secp256k1_sha256_initialize #1179

Coding-Enthusiast opened this issue Dec 16, 2022 · 5 comments

Comments

@Coding-Enthusiast
Copy link
Contributor

When computing tagged-hashes for Schnorr sigs the 3 methods (challenge, aux, nonce) first call secp256k1_sha256_initialize that sets the hashstate (ie. s[0] to s[7] and bytes) to their default SHA256 values then they each immediately change all those values to the precomputed "midstate" values. The first call to secp256k1_sha256_initialize seems wasteful.

static void secp256k1_nonce_function_bip340_sha256_tagged(secp256k1_sha256 *sha) {
secp256k1_sha256_initialize(sha);
sha->s[0] = 0x46615b35ul;
sha->s[1] = 0xf4bfbff7ul;
sha->s[2] = 0x9f8dc671ul;
sha->s[3] = 0x83627ab3ul;
sha->s[4] = 0x60217180ul;
sha->s[5] = 0x57358661ul;
sha->s[6] = 0x21a29e54ul;
sha->s[7] = 0x68b07b4cul;
sha->bytes = 64;
}

static void secp256k1_sha256_initialize(secp256k1_sha256 *hash) {
hash->s[0] = 0x6a09e667ul;
hash->s[1] = 0xbb67ae85ul;
hash->s[2] = 0x3c6ef372ul;
hash->s[3] = 0xa54ff53aul;
hash->s[4] = 0x510e527ful;
hash->s[5] = 0x9b05688cul;
hash->s[6] = 0x1f83d9abul;
hash->s[7] = 0x5be0cd19ul;
hash->bytes = 0;
}

Cross post: bitcoin/bitcoin#26712

@real-or-random
Copy link
Contributor

I think the call is correct at this level of abstraction. The caller in the schnorrsig module is not supposed to know what secp256k1_sha256_initialize() does, e.g., it could initialize further struct members in the future. Moreover, it's not a performance problem because any sane compiler will hopefully drop the call.

However, that means that one could say that the code in the schnorrsig module shouldn't set the struct members directly. This was also discussed in #731 but I ended up not doing it. I think my reasoning was that a midstate is really just a triple of the state of "internal" arrays, buffer of unwritten bytes and bytes written so far. This matches the struct exactly. So one way to look at this is that the struct is "public" to other modules, and those modules can simply write it to.

If we really want to fix this code smell, here are three ways:

  1. We could still introduce a setter function but the API would be somewhat annoying due to the 8 integers plus the array (which means we need a pointer). https://github.com/bitcoin-core/secp256k1/pull/731/files#diff-c2d5f1f7616875ab71cd41b053cfb428696988ff89642b931a0963d50f34f7e8R557-R558.
  2. Alternatively, we could restrict the API to midstates with no unwritten bytes (i.e., where the byte counter is divisible by 64). This is all the callers currently need and then we could at least drop the pointer argument. This would mean we introduce a function secp256k1_sha256_initialize_midstate(uint64_t bytes, uint32_t s1, ..., uint32_t s2). (Or alternatively make the 8 arguments an array if 8 arguments feel too many. But I think 8 args is still fine and they provide more type-safety than an array.)
  3. We could make the hash module offer specific initialization functions, e.g., secp256k1_sha256_initialize_bip340_nonce. This works but it feels a little bit wrong to have the bip340-specific code in the hash module.

I prefer solution 2. @Coding-Enthusiast, would be by any chance be interested in working on this?

@Coding-Enthusiast
Copy link
Contributor Author

My C knowledge is readonly!

How about letting the hash module handle the tagged hash computation in general. Something like what bitcoin core project does in the interpreter.cpp file using HashWriter which I believe is reusable and also doesn't need hard-coded midstate values.

@real-or-random
Copy link
Contributor

and also doesn't need hard-coded midstate values.

We want hard-coded midstates for performance.

@Coding-Enthusiast
Copy link
Contributor Author

I see. My understanding is that HashWriter precomputes the midstates and is stored in memory to be reused so the performance should be the same.

@sipa
Copy link
Contributor

sipa commented Dec 19, 2022

@Coding-Enthusiast That's not easily doable in C, as it has no runtime-computed constants. We'd need to store it in the context or so, and compute it on initialization. I think that's overkill for what we're doing here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants