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

Generate k256 test vectors on secp #9

Merged
merged 2 commits into from
Sep 4, 2024
Merged

Conversation

DanGould
Copy link
Contributor

No description provided.

@DanGould DanGould force-pushed the bitcoin-add-missing-k256-tests branch 4 times, most recently from 907592d to 486836c Compare August 28, 2024 20:36
RFC 9180 makes use of aes-gcm, so we want to generate test vectors
supporting that aead, but we don't need them in production in this
crate.
@DanGould DanGould force-pushed the bitcoin-add-missing-k256-tests branch from 0186344 to fc63116 Compare August 29, 2024 15:11
@DanGould DanGould marked this pull request as ready for review August 29, 2024 21:31
@DanGould
Copy link
Contributor Author

@spacebear21 I requested your review knowing that you're not familiar with this codebase. My main concern is whether or not you think this has major organizational problems. I'm confident the vectors it generates work in 3 separate implementations, 2 based on this rust-hpke codebase and one based on the hpke-rs codebase referenced in the MR history.

Does my (unimplemented) thinking to separate into a kat module with common functions and separate kat/test.rs to check vectors from kat/gen.rs that could be a callable binary to produce them, documented in the README make sense? Anything else that leaps out at you as unmanageable with an understanding that you're only doing a light review since you're not familiar with the codebase?

@spacebear21
Copy link

At a high level this seems reasonable to me. Nice work on getting those vectors ready.

I don't see any organizational issues with splitting up the kat module. I take it the goal is to eventually get secp256k1 support into the upstream rust-hpke, at which point we won't need to maintain a bespoke bitcoin-hpke codebase? Unless is there some other motivation for keeping this fork?

@DanGould
Copy link
Contributor Author

DanGould commented Sep 3, 2024

I'd like to not have to maintain a fork, but I think most payjoin targets will already have chacha20poly1305, sha256, and secp256k1 available through bitcoin dependencies and our fork can depend on those same dependencies to reduce the size of the dep tree.

Perhaps it can all be upstream with feature gates to rust-hpke, but I'm not sure. It seems like that crate's minimum requirement for a main branch merge was to have an RFC specifying the DHKEM

@DanGould DanGould force-pushed the bitcoin-add-missing-k256-tests branch from fc63116 to ea07c06 Compare September 4, 2024 16:51
@DanGould
Copy link
Contributor Author

DanGould commented Sep 4, 2024

The only change post @spacebear21's comment was #[ignore]ing the test generation since the file is committed and only manual intervention is needed to generate vectors.

@DanGould DanGould merged commit bc31b3d into main Sep 4, 2024
24 checks passed
@DanGould DanGould deleted the bitcoin-add-missing-k256-tests branch September 4, 2024 18:27
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