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

Integration tests cleanup #1297

Merged
merged 13 commits into from
Jan 9, 2025

Conversation

jbesraa
Copy link
Contributor

@jbesraa jbesraa commented Dec 13, 2024

Copy link

codecov bot commented Dec 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 19.14%. Comparing base (7483090) to head (d7e6f3b).
Report is 14 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1297   +/-   ##
=======================================
  Coverage   19.14%   19.14%           
=======================================
  Files         166      166           
  Lines       10987    10987           
=======================================
  Hits         2104     2104           
  Misses       8883     8883           
Flag Coverage Δ
binary_codec_sv2-coverage 0.00% <ø> (ø)
binary_serde_sv2-coverage 3.65% <ø> (ø)
binary_sv2-coverage 5.48% <ø> (ø)
bip32_derivation-coverage 0.00% <ø> (ø)
buffer_sv2-coverage 25.02% <ø> (ø)
codec_sv2-coverage 0.01% <ø> (ø)
common_messages_sv2-coverage 0.13% <ø> (ø)
const_sv2-coverage 0.00% <ø> (ø)
error_handling-coverage 0.00% <ø> (ø)
framing_sv2-coverage 0.29% <ø> (ø)
jd_client-coverage 0.00% <ø> (ø)
jd_server-coverage 7.79% <ø> (ø)
job_declaration_sv2-coverage 0.00% <ø> (ø)
key-utils-coverage 2.39% <ø> (ø)
mining-coverage 2.51% <ø> (?)
mining_device-coverage 0.00% <ø> (ø)
mining_proxy_sv2-coverage 0.70% <ø> (ø)
noise_sv2-coverage 4.35% <ø> (?)
pool_sv2-coverage 2.05% <ø> (ø)
protocols 24.74% <ø> (ø)
roles 6.55% <ø> (ø)
roles_logic_sv2-coverage 8.11% <ø> (ø)
sv2_ffi-coverage 0.00% <ø> (ø)
template_distribution_sv2-coverage 0.00% <ø> (ø)
translator_sv2-coverage 9.60% <ø> (ø)
utils 25.13% <ø> (ø)
v1-coverage 2.47% <ø> (ø)

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.

Copy link
Contributor

github-actions bot commented Dec 13, 2024

🐰 Bencher Report

Branch2024-12-13-int-test-cleanup
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.66
(-1.31%)
60.01
(74.42%)
client_sv2_handle_message_mining📈 view plot
🚷 view threshold
74.66
(-3.51%)
107.55
(69.42%)
client_sv2_mining_message_submit_standard📈 view plot
🚷 view threshold
14.67
(+0.03%)
14.72
(99.64%)
client_sv2_mining_message_submit_standard_serialize📈 view plot
🚷 view threshold
267.46
(+1.22%)
289.08
(92.52%)
client_sv2_mining_message_submit_standard_serialize_deserialize📈 view plot
🚷 view threshold
613.80
(-0.27%)
659.53
(93.07%)
client_sv2_open_channel📈 view plot
🚷 view threshold
164.60
(-0.76%)
179.42
(91.74%)
client_sv2_open_channel_serialize📈 view plot
🚷 view threshold
277.34
(-2.95%)
315.74
(87.84%)
client_sv2_open_channel_serialize_deserialize📈 view plot
🚷 view threshold
381.60
(-0.31%)
402.71
(94.76%)
client_sv2_setup_connection📈 view plot
🚷 view threshold
163.21
(+1.86%)
171.81
(94.99%)
client_sv2_setup_connection_serialize📈 view plot
🚷 view threshold
439.56
(-6.16%)
558.96
(78.64%)
client_sv2_setup_connection_serialize_deserialize📈 view plot
🚷 view threshold
983.12
(-2.25%)
1,214.76
(80.93%)
🐰 View full continuous benchmarking report in Bencher

Copy link
Contributor

github-actions bot commented Dec 13, 2024

🐰 Bencher Report

Branch2024-12-13-int-test-cleanup
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.13
(+0.71%)
2.24
(95.16%)
📈 view plot
🚷 view threshold
473.00
(-0.13%)
490.59
(96.41%)
📈 view plot
🚷 view threshold
734.00
(-0.28%)
759.04
(96.70%)
📈 view plot
🚷 view threshold
6.00
(+18.89%)
11.60
(51.73%)
📈 view plot
🚷 view threshold
39.00
(+0.91%)
41.72
(93.48%)
client_sv2_handle_message_mining📈 view plot
🚷 view threshold
8.27
(+0.51%)
8.40
(98.44%)
📈 view plot
🚷 view threshold
2,137.00📈 view plot
🚷 view threshold
3,158.00
(-0.02%)
3,167.45
(99.70%)
📈 view plot
🚷 view threshold
35.00
(-1.54%)
41.51
(84.31%)
📈 view plot
🚷 view threshold
141.00
(+0.93%)
144.54
(97.55%)
client_sv2_mining_message_submit_standard📈 view plot
🚷 view threshold
6.31
(+0.09%)
6.45
(97.81%)
📈 view plot
🚷 view threshold
1,750.00
(-0.03%)
1,767.59
(99.00%)
📈 view plot
🚷 view threshold
2,552.00
(+0.00%)
2,575.50
(99.09%)
📈 view plot
🚷 view threshold
16.00
(-6.57%)
24.01
(66.63%)
📈 view plot
🚷 view threshold
105.00
(+0.30%)
108.76
(96.54%)
client_sv2_mining_message_submit_standard_serialize📈 view plot
🚷 view threshold
14.78
(+0.36%)
14.94
(98.94%)
📈 view plot
🚷 view threshold
4,694.00
(-0.01%)
4,711.59
(99.63%)
📈 view plot
🚷 view threshold
6,748.00
(-0.12%)
6,786.73
(99.43%)
📈 view plot
🚷 view threshold
52.00
(+13.80%)
63.07
(82.45%)
📈 view plot
🚷 view threshold
222.00
(+0.37%)
226.13
(98.17%)
client_sv2_mining_message_submit_standard_serialize_deserialize📈 view plot
🚷 view threshold
27.79
(+0.69%)
28.06
(99.04%)
📈 view plot
🚷 view threshold
10,645.00
(+0.36%)
10,698.58
(99.50%)
📈 view plot
🚷 view threshold
15,502.00
(+0.40%)
15,593.90
(99.41%)
📈 view plot
🚷 view threshold
92.00
(+9.85%)
99.17
(92.77%)
📈 view plot
🚷 view threshold
338.00
(+0.74%)
343.18
(98.49%)
client_sv2_open_channel📈 view plot
🚷 view threshold
4.48
(+1.78%)
4.58
(97.80%)
📈 view plot
🚷 view threshold
1,461.00
(-0.04%)
1,478.59
(98.81%)
📈 view plot
🚷 view threshold
2,156.00
(-0.22%)
2,184.77
(98.68%)
📈 view plot
🚷 view threshold
10.00
(+22.25%)
13.94
(71.72%)
📈 view plot
🚷 view threshold
65.00
(+3.35%)
67.74
(95.95%)
client_sv2_open_channel_serialize📈 view plot
🚷 view threshold
14.07
(+0.36%)
14.20
(99.08%)
📈 view plot
🚷 view threshold
5,064.00
(-0.01%)
5,081.59
(99.65%)
📈 view plot
🚷 view threshold
7,320.00
(-0.08%)
7,352.80
(99.55%)
📈 view plot
🚷 view threshold
41.00
(+11.99%)
48.37
(84.77%)
📈 view plot
🚷 view threshold
187.00
(+0.53%)
190.86
(97.98%)
client_sv2_open_channel_serialize_deserialize📈 view plot
🚷 view threshold
22.96
(+1.13%)
23.07
(99.53%)
📈 view plot
🚷 view threshold
8,040.00
(+0.10%)
8,057.49
(99.78%)
📈 view plot
🚷 view threshold
11,684.00
(-0.01%)
11,713.44
(99.75%)
📈 view plot
🚷 view threshold
86.00
(+13.47%)
89.97
(95.59%)
📈 view plot
🚷 view threshold
310.00
(+1.93%)
312.83
(99.10%)
client_sv2_setup_connection📈 view plot
🚷 view threshold
4.71
(+0.34%)
4.79
(98.24%)
📈 view plot
🚷 view threshold
1,502.00
(-0.04%)
1,519.59
(98.84%)
📈 view plot
🚷 view threshold
2,277.00
(-0.08%)
2,301.01
(98.96%)
📈 view plot
🚷 view threshold
10.00
(+6.31%)
15.25
(65.58%)
📈 view plot
🚷 view threshold
68.00
(+0.61%)
70.13
(96.97%)
client_sv2_setup_connection_serialize📈 view plot
🚷 view threshold
16.21
(+0.36%)
16.33
(99.31%)
📈 view plot
🚷 view threshold
5,963.00
(-0.01%)
5,980.59
(99.71%)
📈 view plot
🚷 view threshold
8,654.00
(-0.11%)
8,692.69
(99.55%)
📈 view plot
🚷 view threshold
49.00
(+20.57%)
56.26
(87.10%)
📈 view plot
🚷 view threshold
209.00
(+0.36%)
212.13
(98.52%)
client_sv2_setup_connection_serialize_deserialize📈 view plot
🚷 view threshold
35.75
(+0.41%)
35.96
(99.42%)
📈 view plot
🚷 view threshold
14,888.00
(+0.14%)
14,919.26
(99.79%)
📈 view plot
🚷 view threshold
21,868.00
(+0.11%)
21,918.59
(99.77%)
📈 view plot
🚷 view threshold
109.00
(+15.38%)
119.80
(90.99%)
📈 view plot
🚷 view threshold
381.00
(+0.36%)
385.74
(98.77%)
🐰 View full continuous benchmarking report in Bencher

Copy link
Contributor

github-actions bot commented Dec 13, 2024

🐰 Bencher Report

Branch2024-12-13-int-test-cleanup
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,526.80
(-0.38%)
6,928.54
(94.20%)
client-submit-serialize-deserialize📈 view plot
🚷 view threshold
7,384.60
(-0.22%)
7,811.09
(94.54%)
client-submit-serialize-deserialize-handle/client-submit-serialize-deserialize-handle📈 view plot
🚷 view threshold
7,916.40
(-2.09%)
9,414.61
(84.09%)
client-sv1-authorize-serialize-deserialize-handle/client-sv1-authorize-serialize-deserialize-handle📈 view plot
🚷 view threshold
894.27
(+2.90%)
948.07
(94.33%)
client-sv1-authorize-serialize-deserialize/client-sv1-authorize-serialize-deserialize📈 view plot
🚷 view threshold
687.39
(+1.75%)
719.02
(95.60%)
client-sv1-authorize-serialize/client-sv1-authorize-serialize📈 view plot
🚷 view threshold
263.65
(+5.14%)
277.53
(95.00%)
client-sv1-get-authorize/client-sv1-get-authorize📈 view plot
🚷 view threshold
174.14
(+10.09%)
174.85
(99.59%)
client-sv1-get-submit📈 view plot
🚷 view threshold
6,349.50
(+0.07%)
6,780.91
(93.64%)
client-sv1-get-subscribe/client-sv1-get-subscribe📈 view plot
🚷 view threshold
297.47
(+5.01%)
330.62
(89.97%)
client-sv1-subscribe-serialize-deserialize-handle/client-sv1-subscribe-serialize-deserialize-handle📈 view plot
🚷 view threshold
723.58
(-0.55%)
779.44
(92.83%)
client-sv1-subscribe-serialize-deserialize/client-sv1-subscribe-serialize-deserialize📈 view plot
🚷 view threshold
605.70
(+2.38%)
631.23
(95.96%)
client-sv1-subscribe-serialize/client-sv1-subscribe-serialize📈 view plot
🚷 view threshold
208.65
(+0.97%)
226.76
(92.01%)
🐰 View full continuous benchmarking report in Bencher

Copy link
Contributor

github-actions bot commented Dec 13, 2024

🐰 Bencher Report

Branch2024-12-13-int-test-cleanup
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.45
(-0.08%)
8.67
(97.42%)
📈 view plot
🚷 view threshold
3.66
(-1.64%)
3.86
(94.85%)
📈 view plot
🚷 view threshold
5.11
(-2.01%)
5.45
(93.77%)
📈 view plot
🚷 view threshold
10.00
(+17.76%)
16.35
(61.16%)
📈 view plot
🚷 view threshold
94.00
(+2.83%)
96.86
(97.05%)
get_submit📈 view plot
🚷 view threshold
95.31
(-0.05%)
95.61
(99.69%)
📈 view plot
🚷 view threshold
59.26
(-0.23%)
59.71
(99.25%)
📈 view plot
🚷 view threshold
85.08
(-0.27%)
85.82
(99.13%)
📈 view plot
🚷 view threshold
45.00
(+0.59%)
60.28
(74.65%)
📈 view plot
🚷 view threshold
286.00
(+1.86%)
291.91
(97.97%)
get_subscribe📈 view plot
🚷 view threshold
7.92
(-0.94%)
8.23
(96.23%)
📈 view plot
🚷 view threshold
2.76
(-1.92%)
2.94
(93.72%)
📈 view plot
🚷 view threshold
3.83
(-2.25%)
4.14
(92.55%)
📈 view plot
🚷 view threshold
12.00
(-4.06%)
20.70
(57.98%)
📈 view plot
🚷 view threshold
115.00
(+0.40%)
117.94
(97.51%)
serialize_authorize📈 view plot
🚷 view threshold
12.26
(+0.04%)
12.50
(98.07%)
📈 view plot
🚷 view threshold
5.24
(-1.09%)
5.43
(96.56%)
📈 view plot
🚷 view threshold
7.28
(-1.35%)
7.60
(95.73%)
📈 view plot
🚷 view threshold
10.00
(-2.29%)
18.84
(53.07%)
📈 view plot
🚷 view threshold
141.00
(+2.19%)
143.35
(98.36%)
serialize_deserialize_authorize📈 view plot
🚷 view threshold
24.73
(+0.05%)
25.19
(98.17%)
📈 view plot
🚷 view threshold
9.79
(-0.72%)
10.01
(97.75%)
📈 view plot
🚷 view threshold
13.79
(-0.87%)
14.18
(97.25%)
📈 view plot
🚷 view threshold
32.00
(-10.90%)
45.93
(69.67%)
📈 view plot
🚷 view threshold
308.00
(+1.43%)
313.70
(98.18%)
serialize_deserialize_handle_authorize📈 view plot
🚷 view threshold
30.36
(+0.13%)
30.73
(98.82%)
📈 view plot
🚷 view threshold
11.99
(-0.50%)
12.19
(98.39%)
📈 view plot
🚷 view threshold
16.95
(-0.64%)
17.30
(97.99%)
📈 view plot
🚷 view threshold
58.00
(+3.88%)
67.41
(86.04%)
📈 view plot
🚷 view threshold
375.00
(+1.06%)
379.49
(98.82%)
serialize_deserialize_handle_submit📈 view plot
🚷 view threshold
126.55
(+0.08%)
126.78
(99.82%)
📈 view plot
🚷 view threshold
73.12
(-0.18%)
73.53
(99.43%)
📈 view plot
🚷 view threshold
104.76
(-0.22%)
105.51
(99.29%)
📈 view plot
🚷 view threshold
109.00
(+2.19%)
125.85
(86.61%)
📈 view plot
🚷 view threshold
607.00
(+1.53%)
610.67
(99.40%)
serialize_deserialize_handle_subscribe📈 view plot
🚷 view threshold
27.96
(+0.19%)
28.39
(98.52%)
📈 view plot
🚷 view threshold
9.58
(-0.55%)
9.76
(98.13%)
📈 view plot
🚷 view threshold
13.52
(-0.72%)
13.84
(97.67%)
📈 view plot
🚷 view threshold
69.00
(+6.85%)
78.03
(88.43%)
📈 view plot
🚷 view threshold
403.00
(+0.92%)
409.84
(98.33%)
serialize_deserialize_submit📈 view plot
🚷 view threshold
115.31
(+0.05%)
115.71
(99.65%)
📈 view plot
🚷 view threshold
67.89
(-0.24%)
68.42
(99.23%)
📈 view plot
🚷 view threshold
97.36
(-0.30%)
98.29
(99.05%)
📈 view plot
🚷 view threshold
69.00
(+5.31%)
87.60
(78.77%)
📈 view plot
🚷 view threshold
503.00
(+1.93%)
505.33
(99.54%)
serialize_deserialize_subscribe📈 view plot
🚷 view threshold
23.34
(+0.05%)
23.82
(97.98%)
📈 view plot
🚷 view threshold
8.13
(-0.68%)
8.32
(97.75%)
📈 view plot
🚷 view threshold
11.42
(-0.85%)
11.75
(97.24%)
📈 view plot
🚷 view threshold
38.00
(-2.51%)
51.04
(74.45%)
📈 view plot
🚷 view threshold
335.00
(+0.99%)
342.50
(97.81%)
serialize_submit📈 view plot
🚷 view threshold
99.80
(+0.01%)
100.09
(99.71%)
📈 view plot
🚷 view threshold
61.33
(-0.21%)
61.73
(99.34%)
📈 view plot
🚷 view threshold
87.93
(-0.25%)
88.65
(99.19%)
📈 view plot
🚷 view threshold
50.00
(+3.86%)
66.93
(74.70%)
📈 view plot
🚷 view threshold
332.00
(+2.01%)
337.67
(98.32%)
serialize_subscribe📈 view plot
🚷 view threshold
11.44
(+0.35%)
11.60
(98.63%)
📈 view plot
🚷 view threshold
4.11
(-1.21%)
4.28
(95.98%)
📈 view plot
🚷 view threshold
5.69
(-1.56%)
6.00
(94.88%)
📈 view plot
🚷 view threshold
16.00
(+14.80%)
24.21
(66.10%)
📈 view plot
🚷 view threshold
162.00
(+2.17%)
164.68
(98.37%)
🐰 View full continuous benchmarking report in Bencher

@@ -343,13 +344,14 @@ pub async fn start_jdc(
std::time::Duration::from_secs(cert_validity_sec),
);
let ret = jd_client::JobDeclaratorClient::new(jd_client_proxy);
tokio::spawn(async move { ret.start().await });
let ret_clone = ret.clone();
Copy link
Contributor

Choose a reason for hiding this comment

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

This ret can be changed to job_declarator_client

@@ -54,6 +54,7 @@ pub static IS_NEW_TEMPLATE_HANDLED: AtomicBool = AtomicBool::new(true);
/// switching to backup Pools in case of declared custom jobs refused by JDS (which is Pool side).
/// As a solution of last-resort, it is able to switch to Solo Mining until new safe Pools appear
/// in the market.
#[derive(Debug, Clone)]
Copy link
Contributor

Choose a reason for hiding this comment

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

I don’t understand the need to make the configuration structure clonable. Why are we returning configuration objects downstream? While this approach does make all our starter APIs consistent, I don’t see any other benefit. Am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right that none of them is currently used beside the Sniffer. I think in the future some of those roles will have some functionality that we might need while testing, thats the main motivation for including them

@jbesraa jbesraa force-pushed the 2024-12-13-int-test-cleanup branch from 1f80f31 to 0a81c0e Compare December 18, 2024 11:42
@plebhash
Copy link
Collaborator

plebhash commented Dec 24, 2024

@jbesraa please see these commits and if they make sense, cherry-pick them into this PR:


with #1317 in mind, I forked this branch and added the following commits:

  • 38dd0b8 renaming the crate (now snake_case with *_sv2 suffix, following the most common pattern on SRI)
  • 0ec0951 making sniffer module externally public, so that other projects can use this crate as a dependency and import sniffer::Sniffer
  • 9ff8bfd adding a derive for Debug on TemplateProvider, which makes it easy to do some quick debugging

if we merge this PR including these commits we can close #1317


the following commit is a simple cleanup of confusing variable naming:

@jbesraa jbesraa force-pushed the 2024-12-13-int-test-cleanup branch from 0a81c0e to 1a1a43d Compare December 30, 2024 10:47
@jbesraa jbesraa force-pushed the 2024-12-13-int-test-cleanup branch 2 times, most recently from 726a2b6 to 9b3ce59 Compare January 6, 2025 11:47
@jbesraa jbesraa requested a review from plebhash January 6, 2025 11:47
@plebhash
Copy link
Collaborator

plebhash commented Jan 6, 2025

oops, looks like I forgot to fix one import path on deb9747 and now it's breaking CI

should be fixed on plebhash@3328f36

@jbesraa after fixing this I think this is ready for merging

@jbesraa jbesraa force-pushed the 2024-12-13-int-test-cleanup branch from 9b3ce59 to 1814d1d Compare January 6, 2025 13:36
@jbesraa
Copy link
Contributor Author

jbesraa commented Jan 6, 2025

oops, looks like I forgot to fix one import path on deb9747 and now it's breaking CI

should be fixed on plebhash@3328f36

@jbesraa after fixing this I think this is ready for merging

Squashed into deb9747

@plebhash plebhash added this to the 1.3.0 milestone Jan 6, 2025
Copy link
Collaborator

@plebhash plebhash left a comment

Choose a reason for hiding this comment

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

as stated on the PR review call earlier today, I found some issues while running cargo doc which should be addressed before we merge this PR

they came from the fact that some public Rust Doc comments have links to private items, for example:

warning: public documentation for `Sniffer` links to private item `InterceptMessage::response_message`
  --> tests-integration/lib/sniffer.rs:50:7
   |
50 | /// [`InterceptMessage::response_message`].
   |       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ this item is private

I started addressing this, but I quickly noticed some other issues with InterceptMessage

although I previously approved this PR for merging, this rabbit hole made me realize we still have some things to think more deeply about

InterceptMessage::response_message field name

this name is a bit misleading, as this is not really a response, it's just the new message that will replace the old one

so I propose we rename it to replace_message via 3b387ac

optionality of replace_message field

currently the break_on functionality is a bit limited, because it forces the user to replace the message every time they want to kill the Sniffer

imagine a scenario where the user wants to kill Sniffer as soon as some specific message is intercepted, but there's no need to actually modify it with a new one... currently, that is impossible, and the user will be forced to create some dummy new message just to be able to use the break_on flag

so I made some adaptations to make it optional on d5a60c4

BUT I'm not sure this is actually useful and I'm not really asking for this commit to be cherry-picked into this PR, please check the item below

test_sniffer_interrupter is not comprehensive, and motivation for break_on is not clear

the name of this test implies that we are testing that the "interrupt" functionality (break_on: true) works as expected, but we never actually assert that the connection was terminated

we only assert that SetupConnectionSuccess was replaced with SetupConnectionError, so in the very least this test has a bad name

but I believe the problem goes a bit deeper

tbh looking at this test gives me the impression that we're kind of mixing "interrupt" + "intercept" terminology, which IMHO is something we need to be careful moving forward in order to further propagate this confusion

for clarity, here's my understanding of these terms:

  • "intercept": this is a very generic term... it simply means Sniffer took some message and it's about to do something with it (maybe just log it, maybe replace it with a different one)
  • "interrupt": it means that after some interception, the Sniffer is supposed to be killed

during this analysis, I also noticed that if break_on is true, recv_from_down_send_to_up and recv_from_up_send_to_down return a SnifferError::MessageInterrupted

this error is never checked nor used for anything, so what is supposed to happen after this error is returned? does Sniffer terminate its TCP connections? is it dropped from memory? currently, absolutely nothing happens, and we also don't have any test for that (despite the misleading name of test_sniffer_interrupter)

in other words: there's no real "interrupt" functionality, only a superficial appearance of one

which then makes me wonder: what is the point of this "interrupt" functionality in the first place? do we have some real demand for this?

it was introduced on #1228 where the goal was simply to allow for messages to be modified, but I cannot find any reference in our discussions to really justify this extra functionality

in fact, I found this question that I resolved without a proper answer (maybe we discussed it on Discord, or some call?)

so my impression is that this was introduced as a potential "nice-to-have", but it was never properly designed and it feels it's only polluting and adding complexity to the code without any real benefit

so IMHO we should make things simpler and drop this "interrupt" functionality, until we realize that we actually need it (and then properly design and test it)

this is why I believe d5a60c4 is not really useful, but I'm still referencing here to provide the full context of my investigation

however, I still believe 3b387ac is important and I would encourage to be included in this PR before the "interrupt" issue is addressed


after all the investigation above (which took a few hours), I didn't spend any time actually fixing the original Rust Docs issues, as I believe these items should be addressed first

@plebhash
Copy link
Collaborator

plebhash commented Jan 9, 2025

another review point of #1228 that was not properly addressed:

#1228 (comment)

currently, test_sniffer_interrupter (which as stated above, has a misleading name) only has one single sniffer, and we only assert that it holds SetupConnectionError internally

but we do not assert that the downstream actually received this SetupConnectionError , which was the original motivation for my request for 2 sniffers on this test

so we're only partially testing things and this test does not really give us full confidence that this functionality

@jbesraa
Copy link
Contributor Author

jbesraa commented Jan 9, 2025

as stated on the PR review call earlier today, I found some issues while running cargo doc which should be addressed before we merge this PR

they came from the fact that some public Rust Doc comments have links to private items, for example:

warning: public documentation for `Sniffer` links to private item `InterceptMessage::response_message`
  --> tests-integration/lib/sniffer.rs:50:7
   |
50 | /// [`InterceptMessage::response_message`].
   |       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ this item is private

I started addressing this, but I quickly noticed some other issues with InterceptMessage

although I previously approved this PR for merging, this rabbit hole made me realize we still have some things to think more deeply about

InterceptMessage::response_message field name

this name is a bit misleading, as this is not really a response, it's just the new message that will replace the old one

so I propose we rename it to replace_message via 3b387ac

optionality of replace_message field

currently the break_on functionality is a bit limited, because it forces the user to replace the message every time they want to kill the Sniffer

imagine a scenario where the user wants to kill Sniffer as soon as some specific message is intercepted, but there's no need to actually modify it with a new one... currently, that is impossible, and the user will be forced to create some dummy new message just to be able to use the break_on flag

so I made some adaptations to make it optional on d5a60c4

BUT I'm not sure this is actually useful and I'm not really asking for this commit to be cherry-picked into this PR, please check the item below

test_sniffer_interrupter is not comprehensive, and motivation for break_on is not clear

the name of this test implies that we are testing that the "interrupt" functionality (break_on: true) works as expected, but we never actually assert that the connection was terminated

we only assert that SetupConnectionSuccess was replaced with SetupConnectionError, so in the very least this test has a bad name

but I believe the problem goes a bit deeper

tbh looking at this test gives me the impression that we're kind of mixing "interrupt" + "intercept" terminology, which IMHO is something we need to be careful moving forward in order to further propagate this confusion

for clarity, here's my understanding of these terms:

* "intercept": this is a very generic term... it simply means `Sniffer` took some message and it's about to do something with it (maybe just log it, maybe replace it with a different one)

* "interrupt": it means that after some interception, the `Sniffer` is supposed to be killed

during this analysis, I also noticed that if break_on is true, recv_from_down_send_to_up and recv_from_up_send_to_down return a SnifferError::MessageInterrupted

this error is never checked nor used for anything, so what is supposed to happen after this error is returned? does Sniffer terminate its TCP connections? is it dropped from memory? currently, absolutely nothing happens, and we also don't have any test for that (despite the misleading name of test_sniffer_interrupter)

in other words: there's no real "interrupt" functionality, only a superficial appearance of one

which then makes me wonder: what is the point of this "interrupt" functionality in the first place? do we have some real demand for this?

it was introduced on #1228 where the goal was simply to allow for messages to be modified, but I cannot find any reference in our discussions to really justify this extra functionality

in fact, I found this question that I resolved without a proper answer (maybe we discussed it on Discord, or some call?)

so my impression is that this was introduced as a potential "nice-to-have", but it was never properly designed and it feels it's only polluting and adding complexity to the code without any real benefit

so IMHO we should make things simpler and drop this "interrupt" functionality, until we realize that we actually need it (and then properly design and test it)

this is why I believe d5a60c4 is not really useful, but I'm still referencing here to provide the full context of my investigation

however, I still believe 3b387ac is important and I would encourage to be included in this PR before the "interrupt" issue is addressed

after all the investigation above (which took a few hours), I didn't spend any time actually fixing the original Rust Docs issues, as I believe these items should be addressed first

cargo doc bug is here because we tried to add #1317 to this PR although it was not planned initially. I would suggest to remove the commits regarding #1317 and not block this.

Regarding the other concerns, I also recommend to open an issue and discuss things and then open new PR. Your notes are general about the sniffer and integration tests design and I am not sure this pull request should be blocked by that.

My main concern is that this PR is already doing too much, I initially resolved 4-5 issues and you added another 3-4, I don't think it is a good idea to try and more stuff here, it is gonna be too annoying to review/improve/track.

@jbesraa jbesraa force-pushed the 2024-12-13-int-test-cleanup branch 3 times, most recently from 1cefc7c to e55db1d Compare January 9, 2025 12:30
plebhash and others added 6 commits January 9, 2025 14:32
it's important to have a descriptive name for when this is published to crates.io
into the following modules:
- lib: with general purpose functions (e.g.: starters)
- sniffer
- template_provider
this struct is redundant and no other roles follow this pattern
we can do the initialization inside `start_pool`
the `SocketAddr` is not a client, but the socket where we will listen to
also, `listner` is a typo
the original naming was confusing copypasta
..to hold utility functions used internaly only.
@jbesraa jbesraa force-pushed the 2024-12-13-int-test-cleanup branch from e55db1d to d7e6f3b Compare January 9, 2025 12:34
@jbesraa
Copy link
Contributor Author

jbesraa commented Jan 9, 2025

Rebased without adding changes

@plebhash plebhash merged commit a053dc7 into stratum-mining:main Jan 9, 2025
37 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants