-
Notifications
You must be signed in to change notification settings - Fork 327
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
CIP-0100 | Add test vector file #782
CIP-0100 | Add test vector file #782
Conversation
So, according to the spec, I believe my original values to be correct, but incomplete. There are two different hashes to compute:
The example provided in the document and via the example files is the former, and my document is missing examples for the latter. The chain of reasoning here is:
So I think the correct thing to do here is to either re-evaluate this decision, or make this distinction clearer and add the test vectors / example hash for the canonicalization of the full document, along side the hash of the |
In particular, this section describes the process used:
The tool you used is indeed the one I used as well, and I believe it follows the canonicalization spec. |
Thank you for the explanation @Quantumplation
I will do this within this PR. |
I don't fully understand how the |
It is the canonicalized version that is used for signing, see the comment above and in particular the highlighted section from the spec. As far as I can tell, it is correctly generated. |
I thought this generates |
Ah ok, I guess I did include both examples, my bad. I'm trying to figure out where Regardless, this PR makes a bit more sense. Let me try to reproduce independently to confirm. |
Damn, yea, it looks like it was inaccurate as of my commit here: 93999e0#diff-57ee612f4824a274191cfea80c0ae926f7e128c42a2b6d8ba5187065d63f28b9 Not sure what happened, because I remember being very meticulous about generating it exactly because it would be used as a reference 🤔 |
I have added a test vector file with explanation on how to recreate as well as missing intermediate file via a3c46f8. When I have time I think I will redo the example with a proper |
If you'd like, I can do that this weekend with my actual keys, since I'm the one listed in the authors :) |
That would be great, thank you |
Possibly there is a bigger issue of circular dependencies when trying to create signatures. I've opened a different issue #783 |
@Ryun1 @Quantumplation @kderme I've classified this as an |
Just a quick note here for @Ryun1 and @Quantumplation but I noticed that the canonical file is not using "permalinks" that reference a specific pull request, which means that the canonicalization may fail and break hashing if the context of a field ever changes between generation, publication, and later validation... |
@Crypto2099 the URLs in the context should never change. The URLs are more unique identifiers than anything, and aren't used by computers to pull in any data. They're only meant to uniquely identify and disambiguate between fields. They're URI's for a humans sake, as a convenience, so they can go read about what those fields mean if they want. Even if Github disappeared, and the spec was hosted elsewhere, the URLs should stay the same so that computer consumers continue to know the fields means the same thing. Thus, it's important to have a stable URL that points at the "latest" version of the spec, in case it's updated with any clarifications / corrections, but also for the spec to not change. Any substantial changes to the meaning of these fields should be a new CIP, which would result in new URLs, to disambiguate. |
@Ryun1 I think you should remove For example, it changes all the node identifiers when you do it as you've described in the test vectors. Here's what I get when I canonicalize via the one described in the CIP:
Now, i'm open to changing it to the scheme you described, but it is a change to the spec, so that's probably worth a wider discussion. |
In fact, looking into this more, it looks like the canonicalization algorithm is particularly bad for this purpose, in that it depends on the actual content stored in the fields, not just the fields themselves. This is frustrating, as most canonicalization algorithms are content agnostic (see, for example, the CBOR canonicalization, which just specifies map keys are in alphabetical order, use of definite vs indefinite containers, etc.) So we will need to revise the signing process regardless. I would suggest something like the following then:
This way is equivalent to what it's described on this branch, but to me is a bit confusingly worded. i.e. it talks about "adding in" the body, and makes reference to |
ah this is unfortunate
I will amend |
I have refactored, and fixed wording as pointed out in #782 comment. Changes
I think this is everything that is needed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I can tell (and trusting @Ryun1 the rest of the way) the last round of feedback has been incorporated: though @Quantumplation I would personally prefer to wait on merging this until you can also confirm.
We will put this on the CIP editors agenda for Tuesday 18th. cc @rphair |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything looks in order, good to go
Add test vector file and re-make provided example