-
Notifications
You must be signed in to change notification settings - Fork 140
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
Propose an implementation of noise_sv2
with optional no_std
#1238
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1238 +/- ##
==========================================
- Coverage 19.14% 19.11% -0.04%
==========================================
Files 166 166
Lines 10987 11047 +60
==========================================
+ Hits 2104 2112 +8
- Misses 8883 8935 +52
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
2375b22
to
f9319d7
Compare
Is it possible to run tests on both the regular and |
At the end there will not be such a 'std' version of And usage (test included) will have to provide a System Time and a Random Number Generator to the crate, they can comme from |
I see. But I wonder if it's possible & safer to keep two versions, because IIUC |
I see, you refer to A double API conditioned by the std feature ? yeah why not ! I will push that this weekend. |
After reflexion, I think that a If the main crate is handshake example is a good exemple of a 'main' std crate (using Breaking the API is a problem for |
f9319d7
to
da6e1c1
Compare
Bencher Report
Click to view all benchmark results
|
Bencher Report
Click to view all benchmark results
|
Bencher Report
Click to view all benchmark results
|
Thanks to @Sjors good comment, the current API is unchanged and an addition one allow This is the way |
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.
utack 9f2a908. Will test the PR once with the whole setup up and running. Rest looks ok on first glance with minor nits
PR good to merge (for me) nothing more to add, you can test it |
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.
tack feeb225. Just a few minor nits left, and we’re good to go. The handshake messages look as expected.
@Georges760 You’ll need to bump the major version to make CI happy. |
do you want me to do it in this PR ? also should I rebase on main ? |
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.
These doc comments are exactly similar std counterparts. Could you add a quick note on why we need this with the random number generator? instead of this. Also, we usually skip 'Arguments' and 'Return type' sections in our docs, so if you can update those too, that’d be great!
70a3a27
to
5f1817c
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.
ACK
5f1817c
to
d6c8c8b
Compare
25cbb20
to
3df4528
Compare
CI is still randomly failing :
|
7176f7d
to
dda9233
Compare
dda9233
to
e137377
Compare
AFAIK @Shourya742 already TACK this PR, waiting for @rrybarczyk review. |
aa777e1
to
26f8be3
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.
Looks good to me. Thanks @Georges760.
@Georges760 I think this is almost ready for merging, but commit history looks a bit confusing. For example, commit title of 771832b is:
I'm not sure how to interpret this... is this commit intended to be on this PR or not? also some commits could be squashed (or dropped, in case they're not meant to be on this PR) |
we're looking into a way to make it deterministic, apologies for that but doesn't look like anything serious |
I will squash everything tomorrow;) |
It was pretty ok lately, the last 5 or 6 rebase were all OK, only get Red on this last one of today. |
26f8be3
to
03c1927
Compare
…PI with `*_with_rng` and `*_with_now`
03c1927
to
a474b57
Compare
[features] | ||
default = ["std"] | ||
std = ["rand/std", "rand/std_rng", "rand_chacha/std", "secp256k1/rand-std"] |
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.
Why are we keeping std
for this crate, but we haven't done the same for the others in the previous PRs?
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.
to not break current API relying on std
.
I basically added an intermediary API for no_std
(so it is only a semver MINOR upgrade) and keep the std
dependant API under the std
feature.
For all other crates, there were no std
dependencies than couldn't be replaced by their core/alloc
equivalent, so they bacame trully no_std
. For noise_sv2
two std
deps (system time and random number generator) cannot be done by core/alloc
so to not break the current API, an other have to be added where we provided them and not assume using the one from std
.
Following @rrybarczyk comment on #1130.
Some equivalent conversion have been done (does not change any functionality, just using the
no_std
equivalents) :To have a
no-std
version, the--no-default-features
must be used.Current public API (
std
dependant) is unchanged.Additional public API for
no_std
compliance is available using the*_with_rng
and*_with_now
suffix, and the corresponding arguments. This delegate the choice of the Ramdom Number Generator and the current System Time to the caller, instead of assuming using the ones fromstd
.Initiator::new_with_rng()
,Initiator::from_raw_k_with_rng()
,Initiator::without_pk_with_rng()
,Responder::new_with_rng()
,Responder::from_authority_kp_with_rng()
andResponder::generate_key_with_rng()
take an additional argument:rng
implementing rand::Rng + ?SizedSignatureNoiseMessage::sign_with_rng()
take an additional argument:rng
implementing rand::Rng + rand::CryptoRngInitiator::step_2_with_now()
andSignatureNoiseMessage::verify_with_now()
take an additional argument:now
for the current system time epochResponder::step_1()_with_now_rng
take two additional arguments:rng
implementing rand::Rng + rand::CryptoRng andnow
for the current system time epoch