Reject weak entropy in nonce_gen when seckey isn't provided

[robot-dreams]

This addresses one of the checklist items in #155

[apoelstra]

I have mixed feelings about this. On the one hand it feels pretty ad-hoc -- there are lots of ways to misuse RNGs and not all of them will result in a bunch of zeros here. On the other hand a lot of them will.

[apoelstra]

I'm leaning toward concept ACK

[robot-dreams]

Thanks for taking a look. And yeah, good point—this was more motivated by preventing counter values than detecting misused RNGs. I'm definitely happy to take suggestions about better checks.

[real-or-random]

(weak) Concept NACK

This is a neat idea (even though it reminds me of "Your password must contain a special character" after I have typed a hex string with 256 bits entropy.) But the reason why I don't like this is that we do this nowhere else. We don't have a similar check for normal keygen for example (and there the consequences could be even worse) And then I feel having such a check in a single function in the musig module is somewhat arbitrary and unexpected. At least we shouldn't do it without having had the discussion in upstream secp256k1.

I think if we really want this, then the exact conditions should be documented in the API docs. This is observable behavior in the end.

[robot-dreams]

@real-or-random Thanks for the thoughtful response! Yeah, even though I'm pretty sure this would catch issues in the wild, I agree there's a good case not to do this.

If there's not strong enthusiasm from anyone else, I can close this and we can revisit in the future when it becomes more obvious what safety checks would be helpful.

[jonasnick]

concept ACK

@real-or-random Just to be clear, you are aware that this check only happens when no seckey is provided? In that case, if you use a session_id with middle bytes 0 you are already clearly violating the API, which requires the session_id to be uniformly random.

the reason why I don't like this is that we do this nowhere else

This sounds like a fallacy. We don't do X for Y, hence we don't do X for Z regardless of its merits. I agree with @robot-dreams that this has a chance of detecting devastating bugs in the wild. And it's basically free. Also, keygen is quite different from this nonce_gen function because the latter requires more thought to be used correctly.

[real-or-random]

This sounds like a fallacy. We don't do X for Y, hence we don't do X for Z regardless of its merits. I agree with @robot-dreams that this has a chance of detecting devastating bugs in the wild. And it's basically free. Also, keygen is quite different from this nonce_gen function because the latter requires more thought to be used correctly.

My reasoning is that we don't do it anywhere else, so noone expects us to do it here. I don't think that part is a fallacy. But you can rightfully ask if we'll really violate any meaningful expectations given what you say here:

@real-or-random Just to be clear, you are aware that this check only happens when no seckey is provided? In that case, if you use a session_id with middle bytes 0 you are already clearly violating the API, which requires the session_id to be uniformly random.

That's a good point. I thought it was only a strong recommendation. Well, I'm happy to be pragmatic here. But I still feel we should document this then like "This function does some basic last-resort checks to ensure session id has been chosen randomly and will return 0 if the checks fails" and "Those checks will fail only with negligible probability when given a uniformly random session_id, so the caller should not retry in case of failure" (would need to be adjusted to the context of the paragraph, "Returns:" etc) or even document the exact check.

Now that I read the API docs again with the case distinction on seckey being present or not: Do you think it will make sense to split this into two functions that take session_id_non_repeating and session_id_uniformly_random? This will make simplify the docs, make the argument naming more explicit, and more importantly, it will make the user look up the docs when he removes the seckey from an existing call, or passes NULL accidentally. The latter sounds like a footgun: The user reads the docs and is super careful to add a seckey. Then in practice some database (=wallet) lookup for the seckey could fail and returns NULL... Or someone else thinks that they could save a roundtrip and removes the seckey because they heard it's not strictly necessary.

[jonasnick]

Do you think it will make sense to split this into two functions [...] ?

I had thought about this earlier and dismissed it because I only expected a small improvement if any because we still need to carefully explain when to use what. But if we'd do this the need for this PR would be reduced significantly; plus you bring up a good point about the seckey being NULL unexpectedly.

session_id_non_repeating and session_id_uniformly_random

We could alternatively keep the current nonce_gen function as is, except that it would enforce that seckey is not NULL and add a nonce_gen_no_seckey function (not sure about that name) that differs from nonce_gen by not taking a seckey and having a secrand32 argument instead of session_id. The naming would hopefully imply that this is supposed to be random and kept secret.

[real-or-random]

But if we'd do this the need for this PR would be reduced significantly

I think it's pretty orthogonal.

If we agree that it's good to split the function (and it seems we do), then let's do this.
If we agree that it's good to reject weak entropy (and we're leaning towards it), then let's do this, too.

Here's another difference to keygen (which is an argument in favor of rejecting weak entropy). Keygen is not entirely ephemeral. Maybe you have an UTXO out there with the pubkey corresponding to a "bad" secret key. But you still want to be able to recompute the pubkey.

But the MuSig session id is entirely ephemeral. If you have bad randomness, throw it away and use new one.

[jonasnick]

I think it's pretty orthogonal.

nonce_gen would require a seckey so we don't reject a weak entropy session_id. We could reject a weak secrand32 in nonce_gen_no_seckey but the potential for mixing up counter and actual randomness is much lower if we split the two functions.

[Rspigler]

We don't have a similar check for normal keygen for example (and there the consequences could be even worse)

Should we?