-
Notifications
You must be signed in to change notification settings - Fork 37
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
Adding support for HistoryProofV2 #433
Conversation
@dillonrg -- I saw that Kevin has renamed in PR428 the MostRecent to MostRecentInsecure in HistoryParams. Since I am reusing MostRecent for history V2 as well that created a merge conflict. I have thought there are two options 1/ Rename it back to MostRecent. This is the easiest and since in this PR explicitly mark V1 of HistoryProof as deprecate, it feels like risk of misuse is small. 2/ Leave HistoryParams untouched and add a new struct HistoryParamsV2 (similarly, HistoryVerificationParamsV2). This makes the message clearer but the unit tests macros that I have added in this PR will need to be modified to pass in those structs as parameters. Let me know what you think and I can make the changes! cc @kevinlewi for FYI. |
cc @eozturk1 @afterdusk for your thoughts on the choices above. |
ebca97f
to
afa6d12
Compare
@haochenuw Thanks for working on this! I think, rather than having a key_history() and key_history_v2() interface, it might be better to instead just have one key_history() function, but add an additional parameter that allows the caller to essentially specify the version. Then, the return of the function can be an enum, something like:
which can be parsed by key_history_verify() as well. That way, we also avoid having to throw everything into macros for the tests that test both versions. Let me know what you think about the suggested change... I am hoping it will lead to a smaller change overall and simpler code. Also, no need to push each commit to the PR, you can just force-push to erase the history and have a single commit change for the PR. |
6efeb0c
to
de7797f
Compare
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.
Fantastic! Some nits in comments but overall LGTM!
de7797f
to
5fde96a
Compare
All comments addressed. Closing. |
The main change in this PR is the introduction of HistoryProofV2 which improves security properties for limited history proofs. It is based upon #422, with changes to maintain backward compatibility and support unit tests for both versions of history proof.