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

Address siv2r's comments #67

Merged
merged 6 commits into from
Dec 16, 2024

Conversation

jonasnick
Copy link
Collaborator

No description provided.

Copy link
Collaborator

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

ACK mod nits

Did you test on Python 3.9?

README.md Outdated

[^mr-kem]: This implements a multi-recipient multi-key key encapsulation mechanism (MR-MK-KEM) secure under the static Diffie-Hellman assumption [[Theorem 2, PPS14](https://doi.org/10.1145/2590296.2590329)].

When `j = i` (i.e., when a participant encrypts a VSS share for themselves), the computationally expensive ECDH key exchange is unnecessary.
Instead, the participant repurposes the secret decryption key as a symmetric key, such that`pad_ii` is computed as the tagged hash of the decryption key, public encryption nonce, and context.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Instead, the participant repurposes the secret decryption key as a symmetric key, such that`pad_ii` is computed as the tagged hash of the decryption key, public encryption nonce, and context.
Instead, the participant repurposes the secret decryption key as a symmetric key, such that `pad_ii` is computed as the tagged SHA256 of the decryption key, public encryption nonce, and context.

(We write tagged SHA256 elsewhere.)

Do you think it makes sense to add a reference/footnote to BIP340 at the first occurrence of "tagged SHA256"?

If yes, then perhaps "Tagged hash" is even better, as this is the term used in BIP340. In either case, I think it's a good idea to use the same term throughout the doc.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, makes sense, done.

@@ -25,9 +25,9 @@ def schnorr_sign(
P = d0 * G
assert not P.infinity
d = d0 if P.has_even_y() else GE.ORDER - d0
t = xor_bytes(bytes_from_int(d), tagged_hash("BIP0340/aux", aux_rand))
t = xor_bytes(bytes_from_int(d), tagged_hash(f"{tag_prefix}/aux", aux_rand))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you considered simple concatenation using + instead of an f-string? Seems more natural to me, but perhaps it's just me.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switched to + (I had asked an LLM which told me that the format string version is the recommended and the most idiomatic variant in python 3).

@real-or-random real-or-random linked an issue Dec 14, 2024 that may be closed by this pull request
@siv2r
Copy link
Contributor

siv2r commented Dec 15, 2024

ACK

Did you test on Python 3.9?

I tested this code (./all.sh after modifying it to explicitly use a specific Python version) with Python 3.9.21, and it ran successfully. It also executed without issues on Python 3.8.10.

This commit makes the reference code work with python 3.9, the oldest version
that is not end-of-life.
Tag prefix is applied to all tagged hashes, in particular those related to nonce
generation. Without this commit, the API is extremely dangerous because signing
twice with the same secret key but different challenge_tag would result in nonce
reuse.
@jonasnick
Copy link
Collaborator Author

Did you test on Python 3.9?

Yes. That's the one thing that's easy in NixOS ;)

nix shell nixpkgs#python39
./all.sh

@real-or-random real-or-random merged commit eaac382 into BlockstreamResearch:master Dec 16, 2024
1 check passed
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

Successfully merging this pull request may close these issues.

Comments on README.md and the Python specification
3 participants