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

Propose an implementation of noise_sv2 with optional no_std #1238

Merged
merged 1 commit into from
Jan 10, 2025

Conversation

Georges760
Copy link

@Georges760 Georges760 commented Oct 29, 2024

Following @rrybarczyk comment on #1130.

Some equivalent conversion have been done (does not change any functionality, just using the no_std equivalents) :

  • std::ptr -> core::ptr
  • std::boxed::Box -> alloc::boxed::Box
  • std::vec::Vec -> alloc::vec::Vec
  • std::string::{String, ToString} -> alloc::string::{String, ToString}
  • std::convert::TryInto -> core::convert::TryInto
  • std::fmt::{Debug, Formatter, Result} -> core::fmt::{Debug, Formatter, Result}
  • std::time::Duration -> core::time::Duration

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 from std.

  • 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() and Responder::generate_key_with_rng() take an additional argument: rng implementing rand::Rng + ?Sized
  • SignatureNoiseMessage::sign_with_rng() take an additional argument: rng implementing rand::Rng + rand::CryptoRng
  • Initiator::step_2_with_now() and SignatureNoiseMessage::verify_with_now() take an additional argument: now for the current system time epoch
  • Responder::step_1()_with_now_rng take two additional arguments: rng implementing rand::Rng + rand::CryptoRng and now for the current system time epoch

@Georges760 Georges760 marked this pull request as draft October 29, 2024 10:41
Copy link

codecov bot commented Oct 29, 2024

Codecov Report

Attention: Patch coverage is 63.15789% with 14 lines in your changes missing coverage. Please review.

Project coverage is 19.11%. Comparing base (a053dc7) to head (a474b57).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
protocols/v2/noise-sv2/src/initiator.rs 50.00% 7 Missing ⚠️
protocols/v2/noise-sv2/src/responder.rs 58.82% 7 Missing ⚠️
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     
Flag Coverage Δ
binary_codec_sv2-coverage 0.00% <0.00%> (ø)
binary_serde_sv2-coverage 3.56% <0.00%> (-0.09%) ⬇️
binary_sv2-coverage 5.36% <0.00%> (-0.13%) ⬇️
bip32_derivation-coverage 0.00% <ø> (ø)
buffer_sv2-coverage 25.02% <ø> (ø)
codec_sv2-coverage 0.01% <0.00%> (-0.01%) ⬇️
common_messages_sv2-coverage 0.13% <0.00%> (-0.01%) ⬇️
const_sv2-coverage 0.00% <0.00%> (ø)
error_handling-coverage 0.00% <ø> (ø)
framing_sv2-coverage 0.28% <0.00%> (-0.01%) ⬇️
jd_client-coverage 0.00% <ø> (ø)
jd_server-coverage 7.79% <ø> (ø)
job_declaration_sv2-coverage 0.00% <0.00%> (ø)
key-utils-coverage 2.39% <ø> (ø)
mining-coverage 2.45% <0.00%> (-0.04%) ⬇️
mining_device-coverage 0.00% <ø> (ø)
mining_proxy_sv2-coverage 0.70% <ø> (ø)
noise_sv2-coverage 4.46% <80.00%> (+0.10%) ⬆️
pool_sv2-coverage 2.05% <ø> (ø)
protocols 24.63% <63.15%> (-0.11%) ⬇️
roles 6.55% <ø> (ø)
roles_logic_sv2-coverage 7.95% <0.00%> (-0.17%) ⬇️
sv2_ffi-coverage 0.00% <0.00%> (ø)
template_distribution_sv2-coverage 0.00% <0.00%> (ø)
translator_sv2-coverage 9.60% <ø> (ø)
utils 25.13% <ø> (ø)
v1-coverage 2.42% <0.00%> (-0.06%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Sjors
Copy link
Collaborator

Sjors commented Dec 6, 2024

Is it possible to run tests on both the regular and no_std build?

@Georges760
Copy link
Author

Georges760 commented Dec 6, 2024

Is it possible to run tests on both the regular and no_std build?

#![no_std] for noise_sv2 (this PR) is a proposal currently, because it require to break its API, so I am waiting for @Shourya742 review of the new proposed API (maybe need to be improved/extended/simplified/etc), then I will work on adapting all depending crate to the new API.

At the end there will not be such a 'std' version of noise_sv2, it will be no_std by nature, which means not dependant to std.

And usage (test included) will have to provide a System Time and a Random Number Generator to the crate, they can comme from std (currently the 'enforced' case), or from a dedicated Hardware (often the case in embedded system aka no_std).
In tests, I will be able to provide them from std, to have the current state, or from a mocked embedded provider to simulat real HW.

@Sjors
Copy link
Collaborator

Sjors commented Dec 6, 2024

I see. But I wonder if it's possible & safer to keep two versions, because IIUC no_std misses various safety features.

https://docs.rust-embedded.org/book/intro/no-std.html

@Georges760
Copy link
Author

Georges760 commented Dec 6, 2024

I see. But I wonder if it's possible & safer to keep two versions, because IIUC no_std misses various safety features.

https://docs.rust-embedded.org/book/intro/no-std.html

I see, you refer to stack overflow protection missing in no_std, I didn't know that, thanks to point it to me.

A double API conditioned by the std feature ? yeah why not ! I will push that this weekend.

@Georges760
Copy link
Author

After reflexion, I think that a #![no_std] lib crate still does not enforce the main crate (the one choosing to link against std or not) so stack overflow protection is only lost if and only if the main crate choose to be #![no_std] (real embedded system).

If the main crate is std aware, even if it use some #![no_std] lib as dep, it will profit from the std security features.

handshake example is a good exemple of a 'main' std crate (using std::rand and std::time) using a #![no_std] lib dep (noise_sv2) and providing the Random Number Generator and System Time (of std) through the lib API so the lib does require std itself.

Breaking the API is a problem for std enviroement, and I fully agree it should be broken. So I will push this dual API (std unchanged, no_std new) according to a std feature enabled by default, in order to keep backward compat.

Copy link
Contributor

github-actions bot commented Dec 7, 2024

🐰 Bencher Report

Branchnoise_sv2_no_std
Testbedsv1
Click to view all benchmark results
BenchmarkEstimated CyclesBenchmark Result
1e3 x estimated cycles
(Result Δ%)
Upper Boundary
1e3 x estimated cycles
(Limit %)
InstructionsBenchmark Result
1e3 x instructions
(Result Δ%)
Upper Boundary
1e3 x instructions
(Limit %)
L1 AccessesBenchmark Result
1e3 x accesses
(Result Δ%)
Upper Boundary
1e3 x accesses
(Limit %)
L2 AccessesBenchmark Result
accesses
(Result Δ%)
Upper Boundary
accesses
(Limit %)
RAM AccessesBenchmark Result
accesses
(Result Δ%)
Upper Boundary
accesses
(Limit %)
get_authorize📈 view plot
🚷 view threshold
8.38
(-0.89%)
8.67
(96.62%)
📈 view plot
🚷 view threshold
3.69
(-0.93%)
3.86
(95.53%)
📈 view plot
🚷 view threshold
5.16
(-1.02%)
5.45
(94.72%)
📈 view plot
🚷 view threshold
7.00
(-17.72%)
16.42
(42.64%)
📈 view plot
🚷 view threshold
91.00
(-0.45%)
96.82
(93.99%)
get_submit📈 view plot
🚷 view threshold
95.22
(-0.15%)
95.61
(99.58%)
📈 view plot
🚷 view threshold
59.35
(-0.09%)
59.71
(99.39%)
📈 view plot
🚷 view threshold
85.22
(-0.10%)
85.82
(99.30%)
📈 view plot
🚷 view threshold
45.00
(+0.40%)
60.35
(74.56%)
📈 view plot
🚷 view threshold
279.00
(-0.62%)
291.87
(95.59%)
get_subscribe📈 view plot
🚷 view threshold
7.87
(-1.51%)
8.23
(95.65%)
📈 view plot
🚷 view threshold
2.77
(-1.65%)
2.94
(93.97%)
📈 view plot
🚷 view threshold
3.85
(-1.90%)
4.14
(92.87%)
📈 view plot
🚷 view threshold
14.00
(+11.44%)
20.76
(67.44%)
📈 view plot
🚷 view threshold
113.00
(-1.33%)
117.94
(95.81%)
serialize_authorize📈 view plot
🚷 view threshold
12.10
(-1.28%)
12.51
(96.75%)
📈 view plot
🚷 view threshold
5.27
(-0.59%)
5.43
(97.05%)
📈 view plot
🚷 view threshold
7.33
(-0.64%)
7.60
(96.42%)
📈 view plot
🚷 view threshold
9.00
(-12.33%)
18.89
(47.63%)
📈 view plot
🚷 view threshold
135.00
(-2.14%)
143.35
(94.18%)
serialize_deserialize_authorize📈 view plot
🚷 view threshold
24.59
(-0.52%)
25.19
(97.62%)
📈 view plot
🚷 view threshold
9.84
(-0.19%)
10.01
(98.28%)
📈 view plot
🚷 view threshold
13.88
(-0.19%)
14.18
(97.93%)
📈 view plot
🚷 view threshold
34.00
(-5.39%)
45.91
(74.05%)
📈 view plot
🚷 view threshold
301.00
(-0.86%)
313.67
(95.96%)
serialize_deserialize_handle_authorize📈 view plot
🚷 view threshold
30.10
(-0.74%)
30.73
(97.95%)
📈 view plot
🚷 view threshold
12.02
(-0.28%)
12.19
(98.60%)
📈 view plot
🚷 view threshold
17.00
(-0.32%)
17.30
(98.31%)
📈 view plot
🚷 view threshold
57.00
(+2.04%)
67.45
(84.50%)
📈 view plot
🚷 view threshold
366.00
(-1.35%)
379.48
(96.45%)
serialize_deserialize_handle_submit📈 view plot
🚷 view threshold
126.21
(-0.19%)
126.78
(99.55%)
📈 view plot
🚷 view threshold
73.20
(-0.06%)
73.53
(99.55%)
📈 view plot
🚷 view threshold
104.92
(-0.07%)
105.51
(99.44%)
📈 view plot
🚷 view threshold
108.00
(+1.16%)
125.87
(85.80%)
📈 view plot
🚷 view threshold
593.00
(-0.80%)
610.64
(97.11%)
serialize_deserialize_handle_subscribe📈 view plot
🚷 view threshold
27.71
(-0.70%)
28.38
(97.64%)
📈 view plot
🚷 view threshold
9.58
(-0.47%)
9.76
(98.20%)
📈 view plot
🚷 view threshold
13.53
(-0.58%)
13.84
(97.81%)
📈 view plot
🚷 view threshold
71.00
(+9.86%)
78.15
(90.86%)
📈 view plot
🚷 view threshold
395.00
(-1.06%)
409.80
(96.39%)
serialize_deserialize_submit📈 view plot
🚷 view threshold
115.13
(-0.10%)
115.72
(99.49%)
📈 view plot
🚷 view threshold
68.06
(+0.01%)
68.42
(99.47%)
📈 view plot
🚷 view threshold
97.65
(+0.01%)
98.29
(99.35%)
📈 view plot
🚷 view threshold
65.00
(-0.95%)
87.74
(74.08%)
📈 view plot
🚷 view threshold
490.00
(-0.70%)
505.34
(96.96%)
serialize_deserialize_subscribe📈 view plot
🚷 view threshold
23.13
(-0.83%)
23.82
(97.11%)
📈 view plot
🚷 view threshold
8.14
(-0.51%)
8.32
(97.92%)
📈 view plot
🚷 view threshold
11.46
(-0.56%)
11.75
(97.52%)
📈 view plot
🚷 view threshold
39.00
(-0.10%)
51.12
(76.29%)
📈 view plot
🚷 view threshold
328.00
(-1.10%)
342.48
(95.77%)
serialize_submit📈 view plot
🚷 view threshold
99.60
(-0.19%)
100.09
(99.50%)
📈 view plot
🚷 view threshold
61.41
(-0.07%)
61.73
(99.47%)
📈 view plot
🚷 view threshold
88.09
(-0.08%)
88.65
(99.37%)
📈 view plot
🚷 view threshold
48.00
(-0.45%)
67.05
(71.59%)
📈 view plot
🚷 view threshold
322.00
(-1.05%)
337.63
(95.37%)
serialize_subscribe📈 view plot
🚷 view threshold
11.31
(-0.75%)
11.60
(97.54%)
📈 view plot
🚷 view threshold
4.12
(-1.03%)
4.28
(96.15%)
📈 view plot
🚷 view threshold
5.71
(-1.23%)
6.00
(95.18%)
📈 view plot
🚷 view threshold
15.00
(+7.20%)
24.31
(61.70%)
📈 view plot
🚷 view threshold
158.00
(-0.34%)
164.63
(95.97%)
🐰 View full continuous benchmarking report in Bencher

Copy link
Contributor

github-actions bot commented Dec 7, 2024

🐰 Bencher Report

Branchnoise_sv2_no_std
Testbedsv1
Click to view all benchmark results
BenchmarkLatencyBenchmark Result
nanoseconds (ns)
(Result Δ%)
Upper Boundary
nanoseconds (ns)
(Limit %)
client-submit-serialize📈 view plot
🚷 view threshold
6,381.90
(-2.52%)
6,955.19
(91.76%)
client-submit-serialize-deserialize📈 view plot
🚷 view threshold
7,252.90
(-1.96%)
7,848.35
(92.41%)
client-submit-serialize-deserialize-handle/client-submit-serialize-deserialize-handle📈 view plot
🚷 view threshold
7,848.50
(-2.96%)
9,429.72
(83.23%)
client-sv1-authorize-serialize-deserialize-handle/client-sv1-authorize-serialize-deserialize-handle📈 view plot
🚷 view threshold
871.97
(+0.40%)
943.27
(92.44%)
client-sv1-authorize-serialize-deserialize/client-sv1-authorize-serialize-deserialize📈 view plot
🚷 view threshold
676.44
(+0.03%)
724.68
(93.34%)
client-sv1-authorize-serialize/client-sv1-authorize-serialize📈 view plot
🚷 view threshold
248.98
(-0.47%)
274.15
(90.82%)
client-sv1-get-authorize/client-sv1-get-authorize📈 view plot
🚷 view threshold
155.76
(-1.09%)
165.42
(94.16%)
client-sv1-get-submit📈 view plot
🚷 view threshold
6,192.20
(-2.25%)
6,773.46
(91.42%)
client-sv1-get-subscribe/client-sv1-get-subscribe📈 view plot
🚷 view threshold
277.39
(-2.27%)
338.01
(82.07%)
client-sv1-subscribe-serialize-deserialize-handle/client-sv1-subscribe-serialize-deserialize-handle📈 view plot
🚷 view threshold
733.49
(+0.78%)
780.38
(93.99%)
client-sv1-subscribe-serialize-deserialize/client-sv1-subscribe-serialize-deserialize📈 view plot
🚷 view threshold
599.73
(+1.53%)
628.73
(95.39%)
client-sv1-subscribe-serialize/client-sv1-subscribe-serialize📈 view plot
🚷 view threshold
204.86
(-0.75%)
226.65
(90.39%)
🐰 View full continuous benchmarking report in Bencher

Copy link
Contributor

github-actions bot commented Dec 7, 2024

🐰 Bencher Report

Branchnoise_sv2_no_std
Testbedsv2
Click to view all benchmark results
BenchmarkLatencyBenchmark Result
nanoseconds (ns)
(Result Δ%)
Upper Boundary
nanoseconds (ns)
(Limit %)
client_sv2_handle_message_common📈 view plot
🚷 view threshold
44.08
(-2.58%)
60.01
(73.45%)
client_sv2_handle_message_mining📈 view plot
🚷 view threshold
75.09
(-2.96%)
107.55
(69.82%)
client_sv2_mining_message_submit_standard📈 view plot
🚷 view threshold
14.75
(+0.58%)
14.77
(99.92%)
client_sv2_mining_message_submit_standard_serialize📈 view plot
🚷 view threshold
262.87
(-0.21%)
284.60
(92.37%)
client_sv2_mining_message_submit_standard_serialize_deserialize📈 view plot
🚷 view threshold
601.64
(-2.47%)
681.99
(88.22%)
client_sv2_open_channel📈 view plot
🚷 view threshold
170.62
(+2.65%)
180.15
(94.71%)
client_sv2_open_channel_serialize📈 view plot
🚷 view threshold
279.12
(-2.46%)
314.99
(88.61%)
client_sv2_open_channel_serialize_deserialize📈 view plot
🚷 view threshold
375.86
(-1.62%)
399.70
(94.03%)
client_sv2_setup_connection📈 view plot
🚷 view threshold
157.67
(-1.36%)
171.30
(92.04%)
client_sv2_setup_connection_serialize📈 view plot
🚷 view threshold
461.74
(-1.68%)
556.09
(83.03%)
client_sv2_setup_connection_serialize_deserialize📈 view plot
🚷 view threshold
1,057.60
(+4.48%)
1,230.16
(85.97%)
🐰 View full continuous benchmarking report in Bencher

Copy link
Contributor

github-actions bot commented Dec 7, 2024

🐰 Bencher Report

Branchnoise_sv2_no_std
Testbedsv2
Click to view all benchmark results
BenchmarkEstimated CyclesBenchmark Result
1e3 x estimated cycles
(Result Δ%)
Upper Boundary
1e3 x estimated cycles
(Limit %)
InstructionsBenchmark Result
instructions
(Result Δ%)
Upper Boundary
instructions
(Limit %)
L1 AccessesBenchmark Result
accesses
(Result Δ%)
Upper Boundary
accesses
(Limit %)
L2 AccessesBenchmark Result
accesses
(Result Δ%)
Upper Boundary
accesses
(Limit %)
RAM AccessesBenchmark Result
accesses
(Result Δ%)
Upper Boundary
accesses
(Limit %)
client_sv2_handle_message_common📈 view plot
🚷 view threshold
2.08
(-1.77%)
2.24
(92.83%)
📈 view plot
🚷 view threshold
481.00
(+1.55%)
490.78
(98.01%)
📈 view plot
🚷 view threshold
747.00
(+1.48%)
759.27
(98.38%)
📈 view plot
🚷 view threshold
7.00
(+37.42%)
11.63
(60.20%)
📈 view plot
🚷 view threshold
37.00
(-4.28%)
41.75
(88.63%)
client_sv2_handle_message_mining📈 view plot
🚷 view threshold
8.28
(+0.65%)
8.40
(98.57%)
📈 view plot
🚷 view threshold
2,137.00📈 view plot
🚷 view threshold
3,155.00
(-0.12%)
3,167.41
(99.61%)
📈 view plot
🚷 view threshold
38.00
(+6.76%)
41.56
(91.44%)
📈 view plot
🚷 view threshold
141.00
(+0.92%)
144.56
(97.53%)
client_sv2_mining_message_submit_standard📈 view plot
🚷 view threshold
6.27
(-0.56%)
6.45
(97.18%)
📈 view plot
🚷 view threshold
1,758.00
(+0.42%)
1,767.78
(99.45%)
📈 view plot
🚷 view threshold
2,562.00
(+0.39%)
2,575.71
(99.47%)
📈 view plot
🚷 view threshold
20.00
(+16.52%)
24.08
(83.04%)
📈 view plot
🚷 view threshold
103.00
(-1.62%)
108.79
(94.68%)
client_sv2_mining_message_submit_standard_serialize📈 view plot
🚷 view threshold
14.70
(-0.20%)
14.94
(98.39%)
📈 view plot
🚷 view threshold
4,702.00
(+0.16%)
4,711.78
(99.79%)
📈 view plot
🚷 view threshold
6,761.00
(+0.08%)
6,786.74
(99.62%)
📈 view plot
🚷 view threshold
54.00
(+17.85%)
63.34
(85.26%)
📈 view plot
🚷 view threshold
219.00
(-0.98%)
226.15
(96.84%)
client_sv2_mining_message_submit_standard_serialize_deserialize📈 view plot
🚷 view threshold
27.74
(+0.49%)
28.06
(98.85%)
📈 view plot
🚷 view threshold
10,653.00
(+0.43%)
10,699.77
(99.56%)
📈 view plot
🚷 view threshold
15,515.00
(+0.48%)
15,595.91
(99.48%)
📈 view plot
🚷 view threshold
93.00
(+10.92%)
99.35
(93.61%)
📈 view plot
🚷 view threshold
336.00
(+0.13%)
343.17
(97.91%)
client_sv2_open_channel📈 view plot
🚷 view threshold
4.46
(+1.34%)
4.58
(97.36%)
📈 view plot
🚷 view threshold
1,469.00
(+0.50%)
1,478.78
(99.34%)
📈 view plot
🚷 view threshold
2,168.00
(+0.34%)
2,184.89
(99.23%)
📈 view plot
🚷 view threshold
11.00
(+33.97%)
14.02
(78.48%)
📈 view plot
🚷 view threshold
64.00
(+1.71%)
67.79
(94.40%)
client_sv2_open_channel_serialize📈 view plot
🚷 view threshold
14.00
(-0.17%)
14.20
(98.55%)
📈 view plot
🚷 view threshold
5,072.00
(+0.14%)
5,081.78
(99.81%)
📈 view plot
🚷 view threshold
7,331.00
(+0.07%)
7,352.83
(99.70%)
📈 view plot
🚷 view threshold
45.00
(+22.53%)
48.64
(92.52%)
📈 view plot
🚷 view threshold
184.00
(-1.08%)
190.91
(96.38%)
client_sv2_open_channel_serialize_deserialize📈 view plot
🚷 view threshold
22.80
(+0.39%)
23.07
(98.81%)
📈 view plot
🚷 view threshold
8,048.00
(+0.19%)
8,057.98
(99.88%)
📈 view plot
🚷 view threshold
11,703.00
(+0.15%)
11,714.06
(99.91%)
📈 view plot
🚷 view threshold
84.00
(+10.72%)
90.02
(93.31%)
📈 view plot
🚷 view threshold
305.00
(+0.28%)
312.82
(97.50%)
client_sv2_setup_connection📈 view plot
🚷 view threshold
4.69
(+0.02%)
4.79
(97.94%)
📈 view plot
🚷 view threshold
1,510.00
(+0.49%)
1,519.78
(99.36%)
📈 view plot
🚷 view threshold
2,288.00
(+0.40%)
2,301.21
(99.43%)
📈 view plot
🚷 view threshold
12.00
(+27.15%)
15.31
(78.37%)
📈 view plot
🚷 view threshold
67.00
(-0.89%)
70.14
(95.53%)
client_sv2_setup_connection_serialize📈 view plot
🚷 view threshold
16.17
(+0.11%)
16.33
(99.06%)
📈 view plot
🚷 view threshold
5,971.00
(+0.12%)
5,980.78
(99.84%)
📈 view plot
🚷 view threshold
8,664.00
(+0.00%)
8,692.60
(99.67%)
📈 view plot
🚷 view threshold
53.00
(+29.99%)
56.65
(93.56%)
📈 view plot
🚷 view threshold
207.00
(-0.60%)
212.17
(97.56%)
client_sv2_setup_connection_serialize_deserialize📈 view plot
🚷 view threshold
35.70
(+0.26%)
35.96
(99.28%)
📈 view plot
🚷 view threshold
14,896.00
(+0.19%)
14,920.06
(99.84%)
📈 view plot
🚷 view threshold
21,881.00
(+0.17%)
21,919.64
(99.82%)
📈 view plot
🚷 view threshold
110.00
(+16.21%)
120.17
(91.54%)
📈 view plot
🚷 view threshold
379.00
(-0.17%)
385.73
(98.26%)
🐰 View full continuous benchmarking report in Bencher

@Georges760
Copy link
Author

Thanks to @Sjors good comment, the current API is unchanged and an addition one allow no_std.

This is the way secp256k1 crate is doing it too, see sign_schnorr_with_rng.

@Georges760 Georges760 marked this pull request as ready for review December 7, 2024 11:01
Copy link
Contributor

@Shourya742 Shourya742 left a 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

protocols/v2/noise-sv2/src/handshake.rs Show resolved Hide resolved
protocols/v2/noise-sv2/src/responder.rs Show resolved Hide resolved
protocols/v2/noise-sv2/src/responder.rs Show resolved Hide resolved
protocols/v2/noise-sv2/src/handshake.rs Show resolved Hide resolved
@Georges760
Copy link
Author

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

Copy link
Contributor

@Shourya742 Shourya742 left a 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.

protocols/v2/noise-sv2/src/handshake.rs Show resolved Hide resolved
protocols/v2/noise-sv2/src/initiator.rs Show resolved Hide resolved
protocols/v2/noise-sv2/src/initiator.rs Show resolved Hide resolved
protocols/v2/noise-sv2/src/initiator.rs Show resolved Hide resolved
protocols/v2/noise-sv2/src/responder.rs Show resolved Hide resolved
protocols/v2/noise-sv2/src/responder.rs Show resolved Hide resolved
protocols/v2/noise-sv2/src/responder.rs Show resolved Hide resolved
protocols/v2/noise-sv2/Cargo.toml Outdated Show resolved Hide resolved
@Shourya742
Copy link
Contributor

@Georges760 You’ll need to bump the major version to make CI happy.

@Georges760
Copy link
Author

@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 ?

Copy link
Contributor

@Shourya742 Shourya742 left a 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!

Copy link
Contributor

@Shourya742 Shourya742 left a comment

Choose a reason for hiding this comment

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

ACK

@Georges760 Georges760 force-pushed the noise_sv2_no_std branch 3 times, most recently from 25cbb20 to 3df4528 Compare December 18, 2024 08:27
@Georges760
Copy link
Author

CI is still randomly failing :

  • on last dev call, the re-run made it pass
  • after my last reabse, it PASS
  • after my README last push, it FAIL again (i am unable to re-run the job)

@Georges760
Copy link
Author

AFAIK @Shourya742 already TACK this PR, waiting for @rrybarczyk review.

@Georges760 Georges760 force-pushed the noise_sv2_no_std branch 4 times, most recently from aa777e1 to 26f8be3 Compare January 8, 2025 16:57
Copy link
Collaborator

@rrybarczyk rrybarczyk left a 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.

@plebhash
Copy link
Collaborator

plebhash commented Jan 8, 2025

@Georges760 I think this is almost ready for merging, but commit history looks a bit confusing.

For example, commit title of 771832b is:

utils/key-utils: revert changes, doesn't have to be changed in this PR

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)

@plebhash
Copy link
Collaborator

plebhash commented Jan 8, 2025

CI is still randomly failing :

  • on last dev call, the re-run made it pass
  • after my last reabse, it PASS
  • after my README last push, it FAIL again (i am unable to re-run the job)

we're looking into a way to make it deterministic, apologies for that

but doesn't look like anything serious

@Georges760
Copy link
Author

@Georges760 I think this is almost ready for merging, but commit history looks a bit confusing.

For example, commit title of 771832b is:

utils/key-utils: revert changes, doesn't have to be changed in this PR

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)

I will squash everything tomorrow;)

@Georges760
Copy link
Author

CI is still randomly failing :

  • on last dev call, the re-run made it pass
  • after my last reabse, it PASS
  • after my README last push, it FAIL again (i am unable to re-run the job)

we're looking into a way to make it deterministic, apologies for that

but doesn't look like anything serious

It was pretty ok lately, the last 5 or 6 rebase were all OK, only get Red on this last one of today.

@plebhash plebhash added the ready-to-be-merged triggers auto rebase bot label Jan 9, 2025
Comment on lines +22 to +24
[features]
default = ["std"]
std = ["rand/std", "rand/std_rng", "rand_chacha/std", "secp256k1/rand-std"]
Copy link
Collaborator

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?

Copy link
Author

@Georges760 Georges760 Jan 10, 2025

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.

protocols/v2/noise-sv2/src/handshake.rs Show resolved Hide resolved
@plebhash plebhash merged commit 4ad657a into stratum-mining:main Jan 10, 2025
37 checks passed
@Georges760 Georges760 deleted the noise_sv2_no_std branch January 13, 2025 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-be-merged triggers auto rebase bot
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants