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

Remove the unnecessary Create Verify Hash Algorithm. #63

Merged
merged 3 commits into from Oct 23, 2022

Conversation

msporny
Copy link
Member

@msporny msporny commented Oct 16, 2022

This PR removes the unnecessary Create Verify Hash Algorithm, which was hard coded to presume digital signatures only. The hashing algorithm is now defined by cryptosuites, which greatly simplifies how much bouncing around is done between the algorithms in the Data Integrity specification and the cryptosuite specifications.

All of the text being removed here has been replaced with a single sentence in step 3 of section 4.1 Generate Proof: https://pr-preview.s3.amazonaws.com/w3c/vc-data-integrity/pull/63.html#generate-proof


Preview | Diff

@dlongley
Copy link
Contributor

dlongley commented Oct 17, 2022

Hmm, is there a way we can keep this function so it can be reused by cryptosuites? It will be commonly reused and it would be a shame if every cryptosuite had to redefine it or cross link to some other cryptosuite that it would otherwise not depend on.

@msporny
Copy link
Member Author

msporny commented Oct 17, 2022

Hmm, is there a way we can keep this function so it can be reused by cryptosuites? It will be commonly reused and it would be a shame if every cryptosuite had to redefine it or cross link to some other cryptosuite that it would otherwise not depend on.

Every cryptosuite is going to have to define the cryptosuite-specific generate proof algorithm, generate hash algorithm , and verify proof algorithm anyway. Not all of them can share the append the proof hash algorithm, though I do agree that it /might/ be useful to define it in the Data Integrity spec. For other things, like for how BBS prepares its hashData, we are probably not going to allow the same algorithm used to hash the document to be used to hash the proof graph, right?

What parts of the algorithm do you think are going to be common to most cryptosuites?

If I had to guess, it would be the part of the algorithm that transform + hashes the proof graph? I was going to try specifying that in the eddsa-2022 spec and then see if I might be able to generalize that between that, ecdsa-2022 and bbs-2023... though, I guess I could try to guess and write that now in this PR? Is that what you're asking me to do?

Copy link
Member

@TallTed TallTed left a comment

Choose a reason for hiding this comment

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

A couple of words...

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
@msporny msporny requested a review from TallTed October 19, 2022 01:33
Copy link
Contributor

@dlongley dlongley left a comment

Choose a reason for hiding this comment

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

Thanks! We don't need to hold this up based on the concern I raised, we'll figure out the right way to deal with it as we move forward.

@iherman
Copy link
Member

iherman commented Oct 19, 2022

The issue was discussed in a meeting on 2022-10-19

  • no resolutions were taken
View the transcript

2.2. Remove the unnecessary Create Verify Hash Algorithm. (pr vc-data-integrity#63)

See github pull request vc-data-integrity#63.

Manu Sporny: still trying to figure out what the text might be for this.
… we will remove the current wrong language.
… key thing is that the way verify hash is created in BBS will be different to all other crypto suites.
… its hard to predict what will happen with BBS so getting the language right now will be difficult.

Markus Sabadello: might fit into RCH working group.

Manu Sporny: we will remove the wrong text now. Worst case is that every crypto algorithm might have to use same text.

msporny and others added 2 commits October 23, 2022 16:11
@msporny
Copy link
Member Author

msporny commented Oct 23, 2022

@dlongley wrote:

is there a way we can keep this function so it can be reused by cryptosuites

I added an issue marker in 33e83ae to track that we might define this later based on how cryptographic suite specification authoring goes.

@msporny
Copy link
Member Author

msporny commented Oct 23, 2022

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

@msporny msporny merged commit 47774a7 into main Oct 23, 2022
@msporny msporny deleted the msporny-remove-cvh branch October 23, 2022 20:16
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.

None yet

7 participants