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

bip-324: fix FSChaCha20 type error #1478

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jamesob
Copy link
Member

@jamesob jamesob commented Jul 14, 2023

The FSChaCha20 class, as written in the BIP's pseudocode, doesn't have an encrypt() method (even though the equivalent class in bip-0324/reference.py does), so use crypt() to be clear that we're not using FSChaCha20Poly1305 for peer.send_L.

The FSChaCha20 class, as written in the BIP's pseudocode, doesn't have
an encrypt() method (even though the equivalent class in the
`bip-0324/reference.py` does), so use crypt() to be clear that we're
not using FSChaCha20Poly1305 for `peer.send_L`.
@jamesob
Copy link
Member Author

jamesob commented Jul 14, 2023

(Probably worth tagging @real-or-random @jonasschnelli @sipa)

@jamesob jamesob force-pushed the jamesob-23-07-bip324-err branch 4 times, most recently from 3390236 to f1cd630 Compare July 21, 2023 18:41
For some reason, the fragments `b''` and `__init__` don't display
properly in Github's mediawiki format, so special-case those.
@jamesob
Copy link
Member Author

jamesob commented Jul 21, 2023

While in the neighborhood, I've added Python syntax highlighting where possible. Apparently Python <source> fragments containing __init__() don't render properly, probably due to a collision with mediawiki's italics syntax.

Copy link
Contributor

@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.

Thanks! It seems people actually read this. :)

Sad that GitHub rendering is that broken...

assert len(contents) <= 2**24 - 1
header = (ignore << IGNORE_BIT_POS).to_bytes(HEADER_LEN, 'little')
plaintext = header + contents
aead_ciphertext = peer.send_P.encrypt(aad, plaintext)
enc_contents_len = peer.send_L.encrypt(len(contents).to_bytes(LENGTH_FIELD_LEN, 'little'))
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm., I think I'd prefer to add encrypt and decrypt methods to the class.

CHACHA20POLY1305_EXPANSION = 16

def v2_receive_packet(peer, aad=b'', skip_decoy=True):
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Did you change these intentionally? Now we have both ' and " in the source, which is also not very consistent.

@real-or-random
Copy link
Contributor

cc @dhruv

@sipa
Copy link
Member

sipa commented Jan 5, 2024

@jamesob Rebase?

@murchandamus
Copy link
Contributor

Are you still working on this, @jamesob?

@murchandamus murchandamus added the PR Author action required Needs updates, has unaddressed review comments, or is otherwise waiting for PR author label May 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR Author action required Needs updates, has unaddressed review comments, or is otherwise waiting for PR author Proposed BIP modification
Projects
None yet
5 participants