ec_seckey_tweak_add - documentation vs. behavior mismatch #1013

I tests.c line 5092 you state that tweaking with zero returns 1 (success) and leaves the seckey unchanged - this is what really happens

/* Tweak of zero leaves the value unchanged. */
    memset(ctmp2, 0, 32);
    CHECK(secp256k1_ec_seckey_tweak_add(ctx, ctmp, ctmp2) == 1);

In documentation however for ec_seckey_tweak_add in secp256k1.h you state that:

 *  In:    tweak32: pointer to a 32-byte tweak. If the tweak is invalid according to
 *                  secp256k1_ec_seckey_verify, this function returns 0. For
 *                  uniformly random 32-byte arrays the chance of being invalid
 *                  is negligible (around 1 in 2^128).
 */
SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_ec_seckey_tweak_add(

which does not make any sense as 32 long byte string of zeros is invalid according to ec_seckey_verify, yet in this function it returns one - success - and key is not tweaked.

Confusing...

Hmm ok, it checks the tweak for overflow but not checks if it is zero.

Fixing this would arguably be a breaking change? (I won't be surprised if higher level protocols use this assumption)

If you pass the same tweak to secp256k1_ec_pubkey_tweak_add it should return 0

I can see the same behavior even for secp256k1_ec_pubkey_tweak_add it returns 1 (success) and public_key is not tweaked... also test.c agrees with this

 /* Tweak of zero leaves the value unchanged. */
    memset(ctmp2, 0, 32);
    CHECK(secp256k1_ec_seckey_tweak_add(ctx, ctmp, ctmp2) == 1);
    CHECK(secp256k1_memcmp_var(orderc, ctmp, 31) == 0 && ctmp[31] == 0x40);
    memcpy(&pubkey2, &pubkey, sizeof(pubkey));
    # Below is the important line
    CHECK(secp256k1_ec_pubkey_tweak_add(ctx, &pubkey, ctmp2) == 1);

only illegal callback functions triggers with !secp256k1_fe_is_zero(&ge->x)'

what should be the correct behavior ? for both {seckey,pubkey}_tweak_add ?

We tried to clarify this in https://github.com/bitcoin-core/secp256k1/pull/701/commits/5894e1f1df74b23435cfd1e9a8b25f22ac631ebe but apparently it made it worse.

what should be the correct behavior ? for both {seckey,pubkey}_tweak_add ?

I don't know. The tweaks are mostly intended to be used with the output of a hash function, so 0 will appear only with negligible probability. In that case, it does not matter what the behavior is for 0.

Tweaking with zero is not an issue per se. I think we should allow it. It enables the user to get the full arithmetic. (And disallowing it now would be an API change...) The only caveat is that we don't catch the case that the user is passing zero-initialized values (that should have been written to). But I don't think the footgun potential is big here. This is not typically not used with the output of a PRG for example.

I can see the same behavior even for secp256k1_ec_pubkey_tweak_add it returns 1 (success) and public_key is not tweaked... also test.c agrees with this

Oh I see now that if (secp256k1_gej_is_infinity(&pt)) checks the output, not the tweak (I apologize, I was on my phone).

I don't know. The tweaks are mostly intended to be used with the output of a hash function, so 0 will appear only with negligible probability. In that case, it does not matter what the behavior is for 0.

You could argue the same for tweaks that are bigger than the group size, but these also have the problem of 2 different tweaks producing the same result, but then again signing has the same "problem".

I don't know. The tweaks are mostly intended to be used with the output of a hash function, so 0 will appear only with negligible probability. In that case, it does not matter what the behavior is for 0.

and

Tweaking with zero is not an issue per se.

Then I would say that comments should be changed to omit If the tweak is invalid according to secp256k1_ec_seckey_verify, this function returns 0 or explicitly state that 0 is exception to the rule.

I've also found that ec_{seckey,pubkey}_tweak_mul correctly raise according to the comments. you can check it out here https://github.com/scgbckbone/python-secp256k1/blob/f6831d3e1b2e4fc73e9141f9fd6604aa386c21b7/tests/test_secp256k1.py#L280-L348

Yeah, I think we should change the docs here.

I've also found that ec_{seckey,pubkey}_tweak_mul correctly raise according to the comments. you can check it out here scgbckbone/python-secp256k1@f6831d3/tests/test_secp256k1.py#L280-L348

Hm, yeah. I wasn't really aware the add and mul tweak functions behave differently here. It's not elegant but it makes sense: You really don't want to multiply with 0...