chilldkg: fix enc_secshare deserialization

[siv2r]

While deserializing recovery data, for enc_secshare, we should use Scalar.from_bytes(...) instead of Scalar(int_from_bytes(...)). The latter does not raise a ValueError for overflowed values; it silently applies modulo to produce a valid Scalar element!

Scalar(int_from_bytes(...)) is also used for generating pads (self_pad and ecdh). I think this is acceptable since we’re generating random numbers, not deserializing data.

[jonasnick]

ACK. Thanks @siv2r.

Scalar(int_from_bytes(...)) is also used for generating pads (self_pad and ecdh). I think this is acceptable since we’re generating random numbers,

Acceptable, but probably better to do this in a consistent way unless you really want to wrap around.

[siv2r]

The wrapping around (when creating a scalar from the tagged hash output) occurs in both schnorr_sign and schnorr_verify:

bip-frost-dkg/python/secp256k1proto/bip340.py

Lines 29 to 32 in 6780889

	k0 = (
	int_from_bytes(tagged_hash(tag_prefix + "/nonce", t + P.to_bytes_xonly() + msg))
	% GE.ORDER
	)

bip-frost-dkg/python/secp256k1proto/bip340.py

Lines 66 to 69 in 6780889

	e = (
	int_from_bytes(tagged_hash(tag_prefix + "/challenge", sig[0:32] + pubkey + msg))
	% GE.ORDER
	)

I assumed we generally don’t want to raise a ValueError when generating random scalars, so I left it as-is instead of switching to Scalar.from_hex(...) when generating the pads. Since this ValueError might happen with only negligible probability, we could change it for consistency. Let me know if you’d prefer that, and I’d be happy to update it.

[real-or-random]

I think the proper way is to change secp256k1proto to split APrimeFE.from_int into a APrimeFE.from_int_wrapping, which wraps around, and a APrimeFE.from_int_checked, which raises OverflowError. (APrimeFE is the abstract superclass of Scalar).

Then we can use the appropriate function everywhere. (Not sure if we want to touch bip340.py though, I tried to keep this as close as possible to the reference code in the BIP, so it's okay to work with integers there and keep the explicit %.)

Scalar(int_from_bytes(...)) is also used for generating pads (self_pad and ecdh). I think this is acceptable since we’re generating random numbers,

Acceptable, but probably better to do this in a consistent way unless you really want to wrap around.

The wrapping there was intentional: Wrapping is not only acceptable when the input is uniformly random (or a hash), it yields simpler code because it removes an error path. This was the rationale for why we wrap in BIP340, after a long discussion.

But the ECDH implementation is supposed to replicate whatever libsecp256k1 does, and it errors out in case of overflow...

And then, if we wrap in ECDH, we may as well wrap in self_pad for consistency. Sigh.

Anyway, ACK. Happy to see a PR that implements the above changes but we can also do it later.

[jonasnick]

I think the proper way is to change secp256k1proto to split APrimeFE.from_int into a APrimeFE.from_int_wrapping

Agree