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

fix indentation of code blocks, keeping them at 69 columns #417

Merged
merged 4 commits into from
Apr 15, 2023

Conversation

conradoplg
Copy link
Collaborator

RFC must have at most 72 columns. Since code blocks are indented with 3 spaces, this means that they must have at most 69 columns.

The text parts (inputs, outputs) were indented manually. The Python parts were indented using autopep8. Some parts may look weird but that's how they are usually indented in Python. Let me know if anything is too strange, it's likely possible to change it by e.g. adding more parenthesis.

To see that everything have been fixed, just run make and see that there are no warnings are output.

Note that it was also giving a warning Warning: Expected a valid submissionType (stream) setting, one of IETF, IAB, IRTF, independent, but found None. Will use 'IETF'. Thus I explicitly set it to IETF.

Closes #406

draft-irtf-cfrg-frost.md Outdated Show resolved Hide resolved
draft-irtf-cfrg-frost.md Outdated Show resolved Hide resolved
draft-irtf-cfrg-frost.md Outdated Show resolved Hide resolved
draft-irtf-cfrg-frost.md Outdated Show resolved Hide resolved
draft-irtf-cfrg-frost.md Outdated Show resolved Hide resolved
@dconnolly dconnolly requested a review from chris-wood March 17, 2023 00:52
@chris-wood
Copy link
Collaborator

@conradoplg I don't feel strongly about this being valid Python, but if that's the motivation, can we make the reference implementation match this exactly?

@conradoplg
Copy link
Collaborator Author

@conradoplg I don't feel strongly about this being valid Python, but if that's the motivation, can we make the reference implementation match this exactly?

I can, but that wouldn't accomplish much, because they already differ, e.g. spec:

def compute_binding_factors(commitment_list, msg):
  msg_hash = H4(msg)
  encoded_commitment_hash = H5(
      encode_group_commitment_list(commitment_list))
  rho_input_prefix = msg_hash || encoded_commitment_hash

  binding_factor_list = []
  for (identifier, hiding_nonce_commitment,
       binding_nonce_commitment) in commitment_list:
    rho_input = rho_input_prefix || G.SerializeScalar(identifier)
    binding_factor = H1(rho_input)
    binding_factor_list.append((identifier, binding_factor))
  return binding_factor_list

While the reformatted reference implementation would look like this:

def compute_binding_factors(G, H, commitment_list, msg):
  msg_hash = H.H4(msg)
  encoded_commitment_hash = H.H5(
      encode_group_commitment_list(
          G, commitment_list))
  rho_input_prefix = msg_hash + encoded_commitment_hash

  binding_factors = {}
  rho_inputs = {}
  for _, (i, D, E) in enumerate(commitment_list):
    rho_input = rho_input_prefix + G.serialize_scalar(i)
    binding_factor = H.H1(rho_input)
    rho_inputs[i] = rho_input
    binding_factors[i] = binding_factor
  return binding_factors, rho_inputs

Also it seems better to keep using 4-space identation in the reference code for readability.

@chris-wood
Copy link
Collaborator

chris-wood commented Mar 17, 2023

Yeah, they differ now, but I thought it might be nice if they were identical. That way there's no question about inconsistencies.

@conradoplg
Copy link
Collaborator Author

Yeah, they differ now, but I thought it might be nice if they were identical. That way there's no question about inconsistencies.

What I meant is that they are different in content, not only in formatting, e.g. H.H5 vs H5, one uses dict and the other uses list, one receives G and H and the other not, etc. I'm not sure we can make then fully equal. We can try making them as similar as possible, but that seems it would work better in a separate PR

@chris-wood
Copy link
Collaborator

Ah, yes, right! I'm going to mull on this for a bit, if that's OK? There's no rush to merge it.

@chris-wood
Copy link
Collaborator

OK, after thinking about this, I'm totally comfortable trying to address parity in a separate PR, keeping this one focused on the original width issue. I can own the followup issue on parity.

Co-authored-by: Christopher Wood <[email protected]>
@dconnolly dconnolly merged commit 1fc8233 into master Apr 15, 2023
@dconnolly dconnolly deleted the fix-indentation branch April 15, 2023 08:24
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.

Fix line length
3 participants