Update Procedures related to Optional Features #182

Wind4Greg · 2024-07-16T17:59:08Z

This PR addresses issue #180. In particular it takes into account the updated APIs/procedures in the drafts:

Sections 3.4.5 Base Proof Serialization (bbs-2023), 3.4.6 Add Derived Proof (bbs-2023), 3.4.7 Verify Derived Proof (bbs-2023) and some of their sub-procedures have been updated to use the the new APIs from these drafts. Importantly the two different flavors of pseudonyms now have a unified proof verification procedure which has resulted in simplified processing.

Preview | Diff

dlongley

Approving, but see comments. Thanks!

dlongley · 2024-07-16T18:31:04Z

index.html

@@ -878,7 +878,7 @@ <h4>createDisclosureData</h4>
                </li>
                <li>
 If |featureOption| equals `"anonymous_holder_binding"`,
-set `bbsProof` to the value computed by the `ProofGen` procedure from
+set `bbsProof` to the value computed by the `BlindProofGen` procedure from


There's an inconsistency here between referring to the function name from the appropriate BBS spec or referring to the title of the procedure section in that spec. Here we use the function name (BlindProofGen) and below we use the section title Calculate Pseudonym and Hidden PID Proof Generation with Pseudonym. We should pick one of these and me consistent.

@dlongley yes, when the draft IETF specs didn't have a function name (pseudonym signing) I used section names. Then I dropped the parenthesis after function names, i.e., BlindProofGen() is actually a function that is defined. Ideas/preferences for consistency?

I think I'd prefer the function name (and the parenthesis can be omitted) because it's shorter and more specific for implementers. I can live with full titles as well if people want that.

Bumping on this -- it can be handled in another PR if necessary, but we should get consistency at some point.

Consistency is good. Clarity is better.

Is BlindProofGen a function or a procedure?
Is Calculate Pseudonym a procedure or a function?

I believe I've seen both procedures and functions (elsewhere) named with Pascal case. I think I've only seen multi-word names on procedures, not functions.

I think including the () helps clarify that the thing being discussed is a function, as I don't think I've ever seen a () suffix on a procedure name.

All of which to say — I would prefer to keep the () and use Pascal- or camel-case to name functions, and to use multi-word names for procedures, and to always be explicit about function calls vs procedure runs.

@Wind4Greg have @TallTed's issues been addressed? I'd like to merge this PR, but not until we've addressed @TallTed's concerns.

Hi @msporny not quite sure what to do about @TallTed's concerns some of the inconsistent naming comes from the IETF drafts. I'm not sure we differentiate between functions and procedures as terms. If there is something concrete that needs to be changed happy to update.

We need to be consistent with what we call these things. As someone not familiar with some of this new text, I'd question what the difference is between a "function" and a "procedure". I'd be worried that I am missing something important when seeing those words if they're used interchangeably. IOW, we should pick one and be consistent in its usage. I suggest we use the word "algorithm" throughout all of the specifications as I believe that's what we do throughout the other specifications.

I try to avoid the use of the word "function" because some developers believe it to be referring to a software library call.

I avoid the word "procedure" because readers believe it's a loosely defined set of tasks where the implementation details are unimportant.

I use the word algorithm because it doesn't have the same connotations as "function" nor "procedure" -- it is more abstract than a function but less abstract than a procedure.

Can we update ALL of the references to refer to "algorithms" and NOT use the () notation because that implies a concrete function (which the IETF specifications are not).

dlongley · 2024-07-16T18:32:05Z

index.html

@@ -953,7 +958,7 @@ <h4>createDisclosureData</h4>
            </li>
            <li>
 Return an object with properties matching |bbsProof|, "verifierLabelMap" for |labelMap|,
-|mandatoryIndexes|, |selectiveIndexes|, |revealDocument|, and |pseudonym|, if
+|mandatoryIndexes|, |selectiveIndexes|, |revealDocument|, |pseudonym|, and |L| if


We should use a better semantic name than L even if we want to say that it maps to L in a low-level BBS spec. The letter L itself can be confused with other symbols -- but at least this is less likely with an upper-case L.

Hi @dlongley, L is the number of issuer signed BBS messages. Or in W3C VC-DI-BBS speak the number of non-mandatory n-quads. When working with Blind BBS and Pseudonym BBS proofs (W3C derived proofs) this is a new parameter that is needed during verification since we separated out the "group generators" for the blinded (holder) messages from the issuer messages to simplify indexing and to unify pseudonym proof verification. I'll take any suggestions for naming!

dlongley · 2024-07-16T18:33:41Z

index.html

 |bbsProof|, |compressedLabelMap|, |mandatoryIndexes|, |selectiveIndexes|,
-|presentationHeader|, and |pseudonym|.
+|presentationHeader| and |L|.


I see this changed from pseudonym to L ... I'm not checking this closely right now, but will do so during implementation.

@dlongley both anonymous holder binding and pseudonyms features need the L parameter now. Pseudonyms still need pseudonyms ;-).

TallTed

Just a few nits

index.html

Wind4Greg · 2024-07-17T16:20:12Z

@TallTed (and @dlongley ) I updated to replace the L variable name with lengthBBSMessages and cleaned up its usage in the procedures related to verification. Note that the Anonymous Holder Binding feature needs the lengthBBSMessages (L) parameter, while both pseudonym features need the pseudonym and lengthBBSMessages (L) parameters. Hopefully I've gotten the conjunctions right for this.

TallTed

Some fixes to the changes, and some new changes. Grammar, punctuation, etc.

index.html

Wind4Greg · 2024-07-18T18:16:17Z

@TallTed and @dlongley I discovered an off by one error while updating on the optional feature test vectors. I put in a fix here. So asking for a re-review to be formal about things. Cheers @Wind4Greg

index.html

Wind4Greg · 2024-08-07T16:04:10Z

Hi @dlongley and @TallTed, I added the updates test vectors to the optional features to this PR since it hasn't been merged yet and it would have been confusing to attach them elsewhere. Hence to keep our procedures sound I'm asking for a re-review. No text has been changed only the JSON test vector files that are imported via ReSpec.

dlongley

Approving the additions with the same caveat that I haven't fully updated DB's implementation yet to run against them; I will do so in the near future and hopefully confirm interoperability. I bumped another comment around consistency in how we reference core BBS primitives that we will want to address at some point.

dlongley · 2024-08-07T17:16:35Z

index.html

@@ -878,7 +878,7 @@ <h4>createDisclosureData</h4>
                </li>
                <li>
 If |featureOption| equals `"anonymous_holder_binding"`,
-set `bbsProof` to the value computed by the `ProofGen` procedure from
+set `bbsProof` to the value computed by the `BlindProofGen` procedure from


Bumping on this -- it can be handled in another PR if necessary, but we should get consistency at some point.

msporny · 2024-08-17T13:43:43Z

index.html

@@ -878,7 +878,7 @@ <h4>createDisclosureData</h4>
                </li>
                <li>
 If |featureOption| equals `"anonymous_holder_binding"`,
-set `bbsProof` to the value computed by the `ProofGen` procedure from
+set `bbsProof` to the value computed by the `BlindProofGen` procedure from


We need to be consistent with what we call these things. As someone not familiar with some of this new text, I'd question what the difference is between a "function" and a "procedure". I'd be worried that I am missing something important when seeing those words if they're used interchangeably. IOW, we should pick one and be consistent in its usage. I suggest we use the word "algorithm" throughout all of the specifications as I believe that's what we do throughout the other specifications.

I try to avoid the use of the word "function" because some developers believe it to be referring to a software library call.

I avoid the word "procedure" because readers believe it's a loosely defined set of tasks where the implementation details are unimportant.

I use the word algorithm because it doesn't have the same connotations as "function" nor "procedure" -- it is more abstract than a function but less abstract than a procedure.

Can we update ALL of the references to refer to "algorithms" and NOT use the () notation because that implies a concrete function (which the IETF specifications are not).

…on improvements. Co-authored-by: Ted Thibodeau Jr <[email protected]>

…age in verify derived proof procedures.

…on improvements. Co-authored-by: Ted Thibodeau Jr <[email protected]>

Co-authored-by: Ted Thibodeau Jr <[email protected]>

msporny · 2024-08-28T14:57:37Z

Normative, multiple reviews, changes requested and made, no objections, merging.

Wind4Greg requested review from TallTed, dlongley and msporny July 16, 2024 17:59

dlongley approved these changes Jul 16, 2024

View reviewed changes

TallTed suggested changes Jul 16, 2024

View reviewed changes

index.html Outdated Show resolved Hide resolved

index.html Outdated Show resolved Hide resolved

index.html Outdated Show resolved Hide resolved

index.html Outdated Show resolved Hide resolved

index.html Outdated Show resolved Hide resolved

Wind4Greg requested a review from TallTed July 17, 2024 16:20

TallTed suggested changes Jul 17, 2024

View reviewed changes

Wind4Greg requested a review from TallTed July 18, 2024 16:41

TallTed approved these changes Jul 18, 2024

View reviewed changes

Wind4Greg requested review from dlongley and TallTed July 18, 2024 18:16

TallTed reviewed Jul 18, 2024

View reviewed changes

index.html Outdated Show resolved Hide resolved

dlongley approved these changes Jul 18, 2024

View reviewed changes

Wind4Greg requested review from TallTed and dlongley August 7, 2024 16:04

dlongley approved these changes Aug 7, 2024

View reviewed changes

msporny approved these changes Aug 16, 2024

View reviewed changes

msporny requested changes Aug 17, 2024

View reviewed changes

Wind4Greg and others added 9 commits August 28, 2024 10:55

Update signing procedures in section "Base Proof Serialization".

cab2b66

Update proof generation procedures for optional feature API updates.

b5c8bc0

Update Optional Feature description and summary tables.

cade589

Update proof verify related procedures.

792549b

Update Procedures related to Optional Features: wording and punctuati…

bb3e984

…on improvements. Co-authored-by: Ted Thibodeau Jr <[email protected]>

Replace L variable name with longer lengthBBSMessages name. Fix up us…

3a13067

…age in verify derived proof procedures.

Update Procedures related to Optional Features: wording and punctuati…

46c9b0c

…on improvements. Co-authored-by: Ted Thibodeau Jr <[email protected]>

Technical change to fix an off by one error.

26d2c19

Update Procedures related to Optional Features: wording improvements.

e748287

Co-authored-by: Ted Thibodeau Jr <[email protected]>

Wind4Greg added 2 commits August 28, 2024 10:55

Update test vectors based on IETF Blind BBS and Pseudonym draft updates.

01a6d3f

Formatted JSON files.

b6e2835

msporny force-pushed the update-feature-procedures branch from 482fd48 to b6e2835 Compare August 28, 2024 14:55

msporny added normative CR1 labels Aug 28, 2024

msporny merged commit 79e1b0e into w3c:main Aug 28, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Procedures related to Optional Features #182

Update Procedures related to Optional Features #182

Wind4Greg commented Jul 16, 2024 •

edited by pr-preview bot

Loading

dlongley left a comment

dlongley Jul 16, 2024

Wind4Greg Jul 16, 2024

dlongley Jul 16, 2024 •

edited

Loading

dlongley Aug 7, 2024

TallTed Aug 7, 2024

msporny Aug 16, 2024

Wind4Greg Aug 16, 2024

msporny Aug 17, 2024

dlongley Jul 16, 2024

Wind4Greg Jul 16, 2024

dlongley Jul 16, 2024

Wind4Greg Jul 16, 2024

TallTed left a comment

Wind4Greg commented Jul 17, 2024

TallTed left a comment

Wind4Greg commented Jul 18, 2024

Wind4Greg commented Aug 7, 2024

dlongley left a comment

dlongley Aug 7, 2024

msporny Aug 17, 2024

msporny commented Aug 28, 2024

Update Procedures related to Optional Features #182

Update Procedures related to Optional Features #182

Conversation

Wind4Greg commented Jul 16, 2024 • edited by pr-preview bot Loading

dlongley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlongley Jul 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TallTed left a comment

Choose a reason for hiding this comment

Wind4Greg commented Jul 17, 2024

TallTed left a comment

Choose a reason for hiding this comment

Wind4Greg commented Jul 18, 2024

Wind4Greg commented Aug 7, 2024

dlongley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msporny commented Aug 28, 2024

Wind4Greg commented Jul 16, 2024 •

edited by pr-preview bot

Loading

dlongley Jul 16, 2024 •

edited

Loading