Comments on Python Code

[siv2r]

Most of these comments relate to improving error handling clarity in the reference implementation.

• In hostpubkey_gen, currently a ValueError is raised when hostseckey is out of range. It would be clearer to wrap the pubkey_gen_plain call in a try-except block, catching the ValueError and raising a HostSeckeyError instead.
  • The participant_step1 function already follows a similar pattern.
• In chilldkg.participant_step1, we should explicitly check if len(random) != 32 and raise a clear ValueError. Currently, this condition causes a crash because encpedpop.participant_step1 asserts it implicitly.
• In chilldkg.participant_step2:
  • We should add checks for [1] "Host secret key does not match any host public key" and [2] hostseckey length (32 bytes), similar to those present in participant_step1.
    • This seems to have been overlooked, as the documentation indicates that step2 raises HostSeckeyError, but it doesn't currently do so.
  • The docstring also fails to mention the FaultyCoordinatorError.
• The checks in simplepedpop.participant_step1 currently raise ValueError or IndexError. It might be more consistent to replace these with assert statements, similar to encpedpop.participant_step1.
• certeq_verify raises a ValueError for an invalid certificate length, which callers (participant_finalize, coordinator_finalize, and recover) propagate directly. It would be clearer if these callers caught this ValueError and raised their own specific exceptions instead.
  • Also, the docstring of chilldkg.participant_finalize mentions raising FaultyCoordinatorError, but currently, this error is never actually raised. Implementing the above suggestion would resolve this discrepancy.
• The docstring of chilldkg.participant_finalize currently misses mentioning the cmsg2 argument.
  • This issue is already fixed in Serialize Messages as Bytes #88.
• encpedpop.coordinator_step1 currently raises a FaultyParticipantOrCoordinatorError. Should it instead raise FaultyParticipantError?
  • Also, the docstring of chilldkg.coordinator_step1 does not currently mention this error.
• The __all__ list in chilldkg.py is missing FaultyParticipantError and InvalidSignatureInCertificateError.
• In encpedpop.participant_step2, the current code snippet:
  

  bip-frost-dkg/python/chilldkg_ref/encpedpop.py

  Lines 227 to 228 in 1e34161

  	pads = decaps_multi(deckey, enckeys[idx], pubnonces, enc_context, idx)
  	secshare = enc_secshare - Scalar.sum(*pads)

  
  can be simplified to:

  secshare = decrypt_sum(deckey, enckeys[idx], pubnonces, enc_context, idx, enc_secshare)

[jonasnick]

Thanks a lot @siv2r. Great comments! I opened PR #93 to address most of them.

We should add checks for [1] "Host secret key does not match any host public key"

In participant_step2 this would require a group multiplication to obtain the
hostpubkey corresponding to the given hostseckey. So I'd avoid doing that.

The checks in simplepedpop.participant_step1 currently raise ValueError or
IndexError. It might be more consistent to replace these with assert statements,
similar to encpedpop.participant_step1.

I changed the assertions in encpedpop.participant_step1 to ValueErrors. This seems to be more idiomatic python when validating input arguments.

certeq_verify raises a ValueError for an invalid certificate length, which callers (participant_finalize, coordinator_finalize, and recover) propagate directly. I

I believe the ValueError cannot be raised in recover because deserialization will fail even before that, causing a RecoveryDataError to be raised. ValueError is probably fine for coordinator_finalize, because it means that the length of the pmsgs list is wrong. I added API documentation for the list length. As for participant_finalize, I think a cert of wrong size should be caught when deserializing the coordinators message. So we will probably have a special Error for that anyway. Added that to the todos below.

chilldkg.participant_finalize mentions raising FaultyCoordinatorError, but currently, this error is never actually raised

It will be raised when deserialization fails. So I kept it for now.

In encpedpop.participant_step2, the current code snippet: [...] can be simplified to:

I don't think so because we need to define pads.

Remaining TODOs

• Make sure that the ValueError for a cert of invalid length cannot be raised in participant_finalize by raising some deserializaiton error when receiving a coordinator message of wrong size.
• Make sure that chilldkg.participant_finalize actually raises FaultyCoordinatorError when deserialization is added.

[real-or-random]

Error handling is pretty complex, and this issue shows this. It would be nice to have tests that pass invalid arguments. But that's a lot of work...