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

Keep HMAC case consistent #547

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@OrfeasLitos
Copy link
Contributor

OrfeasLitos commented Jan 15, 2019

Change HMAC to lowercase in packet description, as it later appears in
lowercase. The uppercase version is used in the hops_data, so this
change helps avoid confusion.

@sstone
Copy link
Collaborator

sstone left a comment

It would then be consistent to also use hmac instead of HMAC in the description of hops_data.

@OrfeasLitos

This comment has been minimized.

Copy link
Contributor Author

OrfeasLitos commented Jan 18, 2019

I couldn't find the exact spot, can you tell me the line?

@sstone

This comment has been minimized.

Copy link
Collaborator

sstone commented Jan 19, 2019

In the description of hops_data lines 171/175 we still have HMAC, it would be more consistent to use hmac here as well.

@OrfeasLitos

This comment has been minimized.

Copy link
Contributor Author

OrfeasLitos commented Jan 19, 2019

I was suggesting to use hmac for the single last HMAC at the end of an onion_packet and HMAC for each of the many HMACs that appear within hops_data. Do you suggest to change all HMAC occurrences to hmac instead?

Observe that the HMAC of lines 171 and 175 is of the kind that is within hops_data, hence according to the proposed convention it should remain uppercase.

@sstone

This comment has been minimized.

Copy link
Collaborator

sstone commented Jan 20, 2019

Yes I'm suggesting it's consistent to use lowercase here as well (we use lowercase for field names everywhere else).

@OrfeasLitos

This comment has been minimized.

Copy link
Contributor Author

OrfeasLitos commented Jan 20, 2019

In that case, would you consider some other way for differentiating the two kinds of HMAC, e.g. hop_hmac and packet_hmac? This is consistent with lowercase field names and helps differentiate the two kinds of HMAC, thus reducing confusion. Sorry for my insistence, but the point of this PR is to help the reader tell the two kinds of HMAC apart easily.

@sstone

This comment has been minimized.

Copy link
Collaborator

sstone commented Jan 20, 2019

Sorry for my insistence, but the point of this PR is to help the reader tell the two kinds of HMAC apart easily.

Yes but I don't think it's needed, though fixing naming inconsistencies may be useful. This part of the spec is fairly technical and probably harder to understand than other BOLTs but I don't think that people trying to implement it would get confused by the packet/hop data HMAC fields having the same name.

In other words, for me using lowercase for the hmac fields -> ACK, having hmac for one field and HMAC for the other -> NACK

@OrfeasLitos OrfeasLitos force-pushed the OrfeasLitos:fix-hmac-case branch from 5065476 to 01b09a2 Jan 20, 2019

@OrfeasLitos

This comment has been minimized.

Copy link
Contributor Author

OrfeasLitos commented Jan 20, 2019

Ok, I just changed all fields to lowercase hmac.

@TheBlueMatt

This comment has been minimized.

Copy link
Collaborator

TheBlueMatt commented Jan 21, 2019

IRC discussion indicated most folks prefer upper-case for cryptographic primitives, especially when they're acronyms.

Keep HMAC case consistent
Change `hmac` to `HMAC` fields in packet description to keep consistent
with the convention of uppercase cryptographic acronym field names

@OrfeasLitos OrfeasLitos force-pushed the OrfeasLitos:fix-hmac-case branch from 01b09a2 to 05dd5f6 Jan 22, 2019

@OrfeasLitos

This comment has been minimized.

Copy link
Contributor Author

OrfeasLitos commented Feb 4, 2019

Fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment