Skip to content

[doc] improve header file docs#7

Merged
stef merged 1 commit into
stef:masterfrom
enjeck:uodate-header
Jun 24, 2025
Merged

[doc] improve header file docs#7
stef merged 1 commit into
stef:masterfrom
enjeck:uodate-header

Conversation

@enjeck
Copy link
Copy Markdown
Contributor

@enjeck enjeck commented Jun 20, 2025

This PR updates the header file comments. I filled in missing docs, made them more consistent across the files and reworded some things.

A few things to clarify:

  • Was not sure which params are internal and shouldn’t be documented
  • Is n and N different? I assumed they mean same thing (number of participants), and replaced all with n
  • Is "shareholder" same as "peers"? Since I replaced it to be consistent and hopefully less confusing. What about "dealer"?

@sonarqubecloud
Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
C Maintainability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

@stef
Copy link
Copy Markdown
Owner

stef commented Jun 20, 2025

This PR updates the header file comments. I filled in missing docs, made them more consistent across the files and reworded some things.

wow. this is amazing, it'll take some time to have a quick review of this, but you really also filled in the blanks, not only looked at what was already there, and it seems you dug deep and quite understood what is happening. what do you like? hugs? beers? cake? i'm happy to serve any combination of these :)

A few things to clarify:

  • Was not sure which params are internal and shouldn’t be documented

mostly everything in these .h files is public. with very few exceptions if at all. i'd have to check myself if there even is any exceptions.

  • Is n and N different? I assumed they mean same thing (number of participants), and replaced all with n

that depends on the context. mostly it is the same, but in oprf.h which is a kind of bastard child of the original academic paper and the irtf rfc. N was used in the original paper to the unblinded output of the OPRF.

  • Is "shareholder" same as "peers"? Since I replaced it to be consistent and hopefully less confusing. What about "dealer"?

yes, almost. peers is all "parties" that will end up with a result of the dkg/mpc mul. shareholders is parties that hold shares, peers however do not need necessarily hold shares at the beginning of the protocol, but hopefully (unless they are cheaters?) at the end of the protocol. a dealer is a shareholder that deals out sharings of their own secret to the peers, not all shareholders need to be dealers only (2t+1 in the update protocol), and of course shareless peers cannot be dealers, as they have no secret to share...

anyway i am very impressed, and would love to just have a chat with you about your thoughts, i'm sure you have more to say about this than "just" this documentation.

wow. :)

@stef
Copy link
Copy Markdown
Owner

stef commented Jun 20, 2025

in terms of sets these are subsets of the previous:

  • participants (all peers + (s)tp)
  • peers
  • shareholders (peers with shares)
  • dealers (shareholders that actively distribute "sub-shares" to all peers)

@stef
Copy link
Copy Markdown
Owner

stef commented Jun 24, 2025

i managed to review the PR. and did not find anything to comment on, small things like trailing whitespace, typedef and curly brackets on separate lines, minor typos (probably even mine) and the occasional deleted spdx header was what i found. however since i was reviewing it all i also found a bunch of other things, unrelated to this PR, which i also either addressed or marked with todos. all in all a tedious but very much necessary and even more useful chore. thank you very much for contributing and triggering me!

@stef stef merged commit 2a1b577 into stef:master Jun 24, 2025
3 of 4 checks passed
@enjeck
Copy link
Copy Markdown
Contributor Author

enjeck commented Jun 24, 2025

trailing whitespace, typedef and curly brackets on separate lines, minor typos (probably even mine) and the occasional deleted spdx header

@stef I didn't expect you to merge it yet 😅😅. I thought I left this PR in draft mode, so you couldn't do that. I was planning to do some more proofreading

For the typedef and curly brackets on separate lines, a VS code formatter extension automatically did that 🤦‍♀️

@enjeck
Copy link
Copy Markdown
Contributor Author

enjeck commented Jun 24, 2025

trailing whitespace, typedef and curly brackets on separate lines, minor typos (probably even mine) and the occasional deleted spdx header

@stef I didn't expect you to merge it yet 😅😅. I thought I left this PR in draft mode, so you couldn't do that. I was planning to do some more proofreading

For the typedef and curly brackets on separate lines, a VS code formatter extension automatically did that 🤦‍♀️

I'll make another PR to fix these problems.

in terms of sets these are subsets of the previous:

  • participants (all peers + (s)tp)
  • peers
  • shareholders (peers with shares)
  • dealers (shareholders that actively distribute "sub-shares" to all peers)

Okay, thanks for clarifying. I had replaced "shareholders" with peers assuming that peers always have shares. I can revert this.

that depends on the context. mostly it is the same, but in oprf.h which is a kind of bastard child of the original academic paper and the irtf rfc. N was used in the original paper to the unblinded output of the OPRF

I think it's fine to stick with n throughout then. When I initially saw N, it confused me where it came from. Or, whenever mention N, we could specify that it's as defined in the paper. Same for t and T

mostly everything in these .h files is public. with very few exceptions if at all. i'd have to check myself if there even is any exceptions.
Ah, okay. I didn't document some arguments that I assumed wouldn't be useful

@stef
Copy link
Copy Markdown
Owner

stef commented Jun 24, 2025

trailing whitespace, typedef and curly brackets on separate lines, minor typos (probably even mine) and the occasional deleted spdx header

@stef I didn't expect you to merge it yet 😅😅. I thought I left this PR in draft mode, so you couldn't do that. I was planning to do some more proofreading

oh damn, i was not aware. well, no harm done i guess. you still could review stuff, if you want.

For the typedef and curly brackets on separate lines, a VS code formatter extension automatically did that 🤦‍♀️

in the 5 commits preceding the merge i already fixed these.

@stef
Copy link
Copy Markdown
Owner

stef commented Jun 24, 2025

trailing whitespace, typedef and curly brackets on separate lines, minor typos (probably even mine) and the occasional deleted spdx header

@stef I didn't expect you to merge it yet 😅😅. I thought I left this PR in draft mode, so you couldn't do that. I was planning to do some more proofreading
For the typedef and curly brackets on separate lines, a VS code formatter extension automatically did that 🤦‍♀️

I'll make another PR to fix these problems.

in terms of sets these are subsets of the previous:

  • participants (all peers + (s)tp)
  • peers
  • shareholders (peers with shares)
  • dealers (shareholders that actively distribute "sub-shares" to all peers)

Okay, thanks for clarifying. I had replaced "shareholders" with peers assuming that peers always have shares. I can revert this.

nah, it mostly makes sense, this only applies to the toprf-update protocol, where we indeed have peers without share and dealers.

that depends on the context. mostly it is the same, but in oprf.h which is a kind of bastard child of the original academic paper and the irtf rfc. N was used in the original paper to the unblinded output of the OPRF

I think it's fine to stick with n throughout then. When I initially saw N, it confused me where it came from. Or, whenever mention N, we could specify that it's as defined in the paper. Same for t and T

yes. and in oprf.h there is no threshold context at all. so if there is still N mentioned there it means something else.

mostly everything in these .h files is public. with very few exceptions if at all. i'd have to check myself if there even is any exceptions.
Ah, okay. I didn't document some arguments that I assumed wouldn't be useful

actually, just yesterday someone opened this #8 - although afaics none of the functions to be hidden are included in the headers.

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.

2 participants