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

Closes #784 #789

Closed
Closed

Conversation

NonsoAmadi10
Copy link
Contributor

What does this PR do?

Refractor update_mempool module in JDS mempool module for simplified async flow

  • Removed the unnecessary use of tokio::task::spawn in update_mempool, leveraging the existing async function context for all operations.
  • Renamed get_raw_mempool_verbose to get_raw_mempool to align with the underlying RPC call.
  • Streamlined error handling by removing .map_err(JdsMempoolError::TokioJoin) since separate async task spawning is no longer used.
  • Simplified control flow with direct return statements for error cases and inline error handling for lock operations.

…async flow

- Removed the unnecessary use of tokio::task::spawn in update_mempool, leveraging the existing async function context for all operations.
- Renamed get_raw_mempool_verbose to get_raw_mempool to align with the underlying RPC call.
- Streamlined error handling by removing .map_err(JdsMempoolError::TokioJoin) since separate async task spawning is no longer used.
- Simplified control flow with direct return statements for error cases and inline error handling for lock operations.

This refactor improves the readability and efficiency of the update_mempool function and aligns the code with best practices for async operations in Rust.
Copy link
Contributor

github-actions bot commented Mar 11, 2024

🐰Bencher

ReportFri, March 22, 2024 at 14:07:05 UTC
ProjectStratum v2 (SRI)
Branchbug-jds-server-#784
Testbedsv2
BenchmarkLatencyLatency Results
nanoseconds (ns) | (Δ%)
Latency Upper Boundary
nanoseconds (ns) | (%)
client_sv2_handle_message_common✅ (view plot)44.533 (-0.09%)45.824 (97.18%)
client_sv2_handle_message_mining✅ (view plot)74.749 (+5.46%)82.526 (90.58%)
client_sv2_mining_message_submit_standard✅ (view plot)14.620 (-0.24%)14.703 (99.44%)
client_sv2_mining_message_submit_standard_serialize✅ (view plot)262.300 (-1.58%)284.460 (92.21%)
client_sv2_mining_message_submit_standard_serialize_deserialize✅ (view plot)578.080 (-0.73%)603.906 (95.72%)
client_sv2_open_channel🚨 (view plot | view alert)172.900 (+4.26%)172.599 (100.17%)
client_sv2_open_channel_serialize✅ (view plot)278.150 (-2.78%)302.038 (92.09%)
client_sv2_open_channel_serialize_deserialize✅ (view plot)380.030 (+0.16%)419.516 (90.59%)
client_sv2_setup_connection🚨 (view plot | view alert)172.260 (+5.52%)172.191 (100.04%)
client_sv2_setup_connection_serialize✅ (view plot)477.240 (+0.42%)505.124 (94.48%)
client_sv2_setup_connection_serialize_deserialize✅ (view plot)1015.100 (+2.35%)1077.384 (94.22%)

Bencher - Continuous Benchmarking
View Public Perf Page
Docs | Repo | Chat | Help

Copy link
Contributor

github-actions bot commented Mar 11, 2024

🐰Bencher

ReportFri, March 22, 2024 at 14:06:58 UTC
ProjectStratum v2 (SRI)
Branchbug-jds-server-#784
Testbedsv2
BenchmarkEstimated CyclesEstimated Cycles Results
estimated cycles | (Δ%)
Estimated Cycles Upper Boundary
estimated cycles | (%)
InstructionsInstructions Results
instructions | (Δ%)
Instructions Upper Boundary
instructions | (%)
L1 AccessesL1 Accesses Results
accesses | (Δ%)
L1 Accesses Upper Boundary
accesses | (%)
L2 AccessesL2 Accesses Results
accesses | (Δ%)
L2 Accesses Upper Boundary
accesses | (%)
RAM AccessesRAM Accesses Results
accesses | (Δ%)
RAM Accesses Upper Boundary
accesses | (%)
client_sv2_handle_message_common✅ (view plot)2119.000 (+3.69%)2145.135 (98.78%)✅ (view plot)473.000 (+1.42%)485.165 (97.49%)✅ (view plot)729.000 (+0.37%)754.206 (96.66%)🚨 (view plot | view alert)12.000 (+73.91%)11.623 (103.24%)✅ (view plot)38.000 (+3.68%)39.284 (96.73%)
client_sv2_handle_message_mining🚨 (view plot | view alert)8265.000 (+1.79%)8223.756 (100.50%)✅ (view plot)2137.000 (+1.24%)2171.639 (98.40%)✅ (view plot)3155.000 (+1.14%)3220.749 (97.96%)✅ (view plot)42.000 (+4.87%)45.648 (92.01%)✅ (view plot)140.000 (+2.08%)140.048 (99.97%)
client_sv2_mining_message_submit_standard✅ (view plot)6334.000 (+0.73%)6461.356 (98.03%)✅ (view plot)1750.000 (+0.11%)1765.653 (99.11%)✅ (view plot)2549.000 (-0.00%)2574.907 (98.99%)✅ (view plot)22.000 (+10.83%)25.568 (86.04%)✅ (view plot)105.000 (+0.96%)108.979 (96.35%)
client_sv2_mining_message_submit_standard_serialize🚨 (view plot | view alert)14985.000 (+2.27%)14899.504 (100.57%)✅ (view plot)4694.000 (+0.04%)4709.653 (99.67%)✅ (view plot)6745.000 (-0.13%)6780.208 (99.48%)✅ (view plot)52.000 (+5.37%)55.712 (93.34%)🚨 (view plot | view alert)228.000 (+4.30%)225.481 (101.12%)
client_sv2_mining_message_submit_standard_serialize_deserialize🚨 (view plot | view alert)27730.000 (+1.55%)27600.330 (100.47%)✅ (view plot)10545.000 (+0.09%)10553.674 (99.92%)✅ (view plot)15335.000 (+0.01%)15359.041 (99.84%)✅ (view plot)85.000 (-1.05%)92.209 (92.18%)🚨 (view plot | view alert)342.000 (+3.70%)338.241 (101.11%)
client_sv2_open_channel✅ (view plot)4565.000 (+2.32%)4608.413 (99.06%)✅ (view plot)1461.000 (+0.19%)1476.719 (98.94%)✅ (view plot)2150.000 (-0.05%)2175.930 (98.81%)✅ (view plot)14.000 (+20.69%)15.444 (90.65%)✅ (view plot)67.000 (+4.12%)68.249 (98.17%)
client_sv2_open_channel_serialize🚨 (view plot | view alert)14380.000 (+1.86%)14332.393 (100.33%)✅ (view plot)5064.000 (+0.06%)5079.719 (99.69%)✅ (view plot)7310.000 (-0.08%)7342.591 (99.56%)✅ (view plot)42.000 (+6.33%)44.405 (94.58%)🚨 (view plot | view alert)196.000 (+3.87%)194.620 (100.71%)
client_sv2_open_channel_serialize_deserialize🚨 (view plot | view alert)22848.000 (+1.67%)22765.580 (100.36%)✅ (view plot)7987.000 (+0.13%)7995.800 (99.89%)✅ (view plot)11608.000 (-0.02%)11638.777 (99.74%)✅ (view plot)78.000 (+7.73%)79.857 (97.67%)🚨 (view plot | view alert)310.000 (+3.33%)307.826 (100.71%)
client_sv2_setup_connection✅ (view plot)4741.000 (+0.53%)4806.045 (98.65%)✅ (view plot)1502.000 (+0.19%)1517.719 (98.96%)✅ (view plot)2276.000 (+0.22%)2299.245 (98.99%)✅ (view plot)10.000 (-16.32%)16.103 (62.10%)✅ (view plot)69.000 (+1.25%)70.491 (97.88%)
client_sv2_setup_connection_serialize✅ (view plot)16376.000 (+1.21%)16385.273 (99.94%)✅ (view plot)5963.000 (+0.05%)5978.719 (99.74%)✅ (view plot)8651.000 (-0.03%)8683.007 (99.63%)✅ (view plot)47.000 (+1.84%)54.348 (86.48%)🚨 (view plot | view alert)214.000 (+2.66%)213.916 (100.04%)
client_sv2_setup_connection_serialize_deserialize🚨 (view plot | view alert)35696.000 (+0.68%)35668.026 (100.08%)✅ (view plot)14814.000 (+0.07%)14822.800 (99.94%)✅ (view plot)21741.000 (-0.03%)21776.631 (99.84%)✅ (view plot)110.000 (+15.06%)110.347 (99.69%)✅ (view plot)383.000 (+1.34%)383.096 (99.98%)

Bencher - Continuous Benchmarking
View Public Perf Page
Docs | Repo | Chat | Help

Copy link
Contributor

github-actions bot commented Mar 11, 2024

🐰Bencher

ReportFri, March 22, 2024 at 14:07:00 UTC
ProjectStratum v2 (SRI)
Branchbug-jds-server-#784
Testbedsv1
BenchmarkEstimated CyclesEstimated Cycles Results
estimated cycles | (Δ%)
Estimated Cycles Upper Boundary
estimated cycles | (%)
InstructionsInstructions Results
instructions | (Δ%)
Instructions Upper Boundary
instructions | (%)
L1 AccessesL1 Accesses Results
accesses | (Δ%)
L1 Accesses Upper Boundary
accesses | (%)
L2 AccessesL2 Accesses Results
accesses | (Δ%)
L2 Accesses Upper Boundary
accesses | (%)
RAM AccessesRAM Accesses Results
accesses | (Δ%)
RAM Accesses Upper Boundary
accesses | (%)
get_authorize✅ (view plot)8474.000 (+0.20%)8515.423 (99.51%)✅ (view plot)3746.000 (-0.10%)3770.368 (99.35%)✅ (view plot)5249.000 (-0.15%)5292.981 (99.17%)✅ (view plot)8.000 (+3.90%)12.330 (64.88%)✅ (view plot)91.000 (+0.72%)92.230 (98.67%)
get_submit✅ (view plot)95575.000 (+0.03%)95619.566 (99.95%)✅ (view plot)59439.000 (-0.02%)59516.791 (99.87%)✅ (view plot)85350.000 (-0.03%)85491.160 (99.83%)✅ (view plot)57.000 (+4.68%)60.782 (93.78%)✅ (view plot)284.000 (+0.44%)285.439 (99.50%)
get_subscribe✅ (view plot)8045.000 (+0.57%)8086.400 (99.49%)✅ (view plot)2841.000 (-0.04%)2847.561 (99.77%)✅ (view plot)3965.000 (-0.14%)3982.091 (99.57%)✅ (view plot)18.000 (+16.88%)20.046 (89.80%)✅ (view plot)114.000 (+0.97%)115.404 (98.78%)
serialize_authorize✅ (view plot)12193.000 (-0.20%)12351.562 (98.72%)✅ (view plot)5317.000 (-0.07%)5341.368 (99.54%)✅ (view plot)7413.000 (-0.08%)7453.205 (99.46%)✅ (view plot)11.000 (-1.35%)15.519 (70.88%)✅ (view plot)135.000 (-0.37%)138.807 (97.26%)
serialize_deserialize_authorize✅ (view plot)24510.000 (+0.04%)24589.686 (99.68%)✅ (view plot)9898.000 (-0.08%)9946.736 (99.51%)✅ (view plot)13955.000 (-0.11%)14044.950 (99.36%)✅ (view plot)39.000 (+3.17%)44.325 (87.99%)✅ (view plot)296.000 (+0.19%)297.812 (99.39%)
serialize_deserialize_handle_authorize✅ (view plot)30199.000 (+0.05%)30272.520 (99.76%)✅ (view plot)12101.000 (-0.03%)12125.368 (99.80%)✅ (view plot)17119.000 (-0.05%)17167.562 (99.72%)✅ (view plot)61.000 (+2.26%)66.871 (91.22%)✅ (view plot)365.000 (+0.14%)367.332 (99.37%)
serialize_deserialize_handle_submit✅ (view plot)126434.000 (+0.01%)126499.899 (99.95%)✅ (view plot)73224.000 (-0.02%)73301.791 (99.89%)✅ (view plot)104939.000 (-0.03%)105083.442 (99.86%)✅ (view plot)127.000 (+8.73%)127.202 (99.84%)✅ (view plot)596.000 (-0.05%)599.097 (99.48%)
serialize_deserialize_handle_subscribe✅ (view plot)27553.000 (+0.22%)27594.767 (99.85%)✅ (view plot)9643.000 (-0.01%)9649.561 (99.93%)✅ (view plot)13633.000 (-0.04%)13653.666 (99.85%)✅ (view plot)68.000 (+3.90%)73.654 (92.32%)✅ (view plot)388.000 (+0.40%)389.605 (99.59%)
serialize_deserialize_submit✅ (view plot)115023.000 (-0.03%)115223.613 (99.83%)✅ (view plot)68001.000 (-0.04%)68156.581 (99.77%)✅ (view plot)97553.000 (-0.05%)97824.094 (99.72%)✅ (view plot)71.000 (+5.50%)72.034 (98.56%)✅ (view plot)489.000 (-0.04%)491.470 (99.50%)
serialize_deserialize_subscribe✅ (view plot)22942.000 (+0.15%)22971.439 (99.87%)✅ (view plot)8195.000 (-0.03%)8208.121 (99.84%)✅ (view plot)11537.000 (-0.05%)11566.346 (99.75%)✅ (view plot)41.000 (+2.50%)47.212 (86.84%)✅ (view plot)320.000 (+0.31%)321.092 (99.66%)
serialize_submit✅ (view plot)99858.000 (-0.01%)100023.086 (99.83%)✅ (view plot)61483.000 (-0.02%)61560.791 (99.87%)✅ (view plot)88198.000 (-0.03%)88333.756 (99.85%)✅ (view plot)57.000 (+3.54%)60.196 (94.69%)✅ (view plot)325.000 (+0.03%)327.591 (99.21%)
serialize_subscribe✅ (view plot)11340.000 (+0.03%)11471.156 (98.86%)✅ (view plot)4188.000 (-0.03%)4194.561 (99.84%)✅ (view plot)5825.000 (-0.07%)5837.690 (99.78%)✅ (view plot)18.000 (+11.80%)20.392 (88.27%)✅ (view plot)155.000 (-0.03%)158.835 (97.59%)

Bencher - Continuous Benchmarking
View Public Perf Page
Docs | Repo | Chat | Help

Copy link
Contributor

github-actions bot commented Mar 11, 2024

🐰Bencher

ReportFri, March 22, 2024 at 14:06:59 UTC
ProjectStratum v2 (SRI)
Branch789/merge
Testbedsv1
BenchmarkLatencyLatency Results
nanoseconds (ns) | (Δ%)
Latency Upper Boundary
nanoseconds (ns) | (%)
client-submit-serialize✅ (view plot)6900.800 (-0.32%)7352.065 (93.86%)
client-submit-serialize-deserialize✅ (view plot)7566.800 (-3.58%)8331.089 (90.83%)
client-submit-serialize-deserialize-handle/client-submit-serialize-deserialize-handle✅ (view plot)8331.700 (-1.26%)8904.434 (93.57%)
client-sv1-authorize-serialize-deserialize-handle/client-sv1-authorize-serialize-deserialize-handle✅ (view plot)905.680 (+0.98%)926.522 (97.75%)
client-sv1-authorize-serialize-deserialize/client-sv1-authorize-serialize-deserialize✅ (view plot)697.370 (+0.28%)709.912 (98.23%)
client-sv1-authorize-serialize/client-sv1-authorize-serialize✅ (view plot)247.450 (+0.00%)253.090 (97.77%)
client-sv1-get-authorize/client-sv1-get-authorize✅ (view plot)156.430 (-0.50%)160.638 (97.38%)
client-sv1-get-submit✅ (view plot)6480.300 (-3.36%)7157.083 (90.54%)
client-sv1-get-subscribe/client-sv1-get-subscribe✅ (view plot)276.920 (-1.49%)293.671 (94.30%)
client-sv1-subscribe-serialize-deserialize-handle/client-sv1-subscribe-serialize-deserialize-handle✅ (view plot)747.100 (+0.11%)763.138 (97.90%)
client-sv1-subscribe-serialize-deserialize/client-sv1-subscribe-serialize-deserialize✅ (view plot)620.190 (+1.02%)630.065 (98.43%)
client-sv1-subscribe-serialize/client-sv1-subscribe-serialize🚨 (view plot | view alert)220.940 (+6.90%)217.671 (101.50%)

Bencher - Continuous Benchmarking
View Public Perf Page
Docs | Repo | Chat | Help

@NonsoAmadi10 NonsoAmadi10 marked this pull request as draft March 11, 2024 16:37
@NonsoAmadi10 NonsoAmadi10 marked this pull request as ready for review March 11, 2024 16:37
@NonsoAmadi10
Copy link
Contributor Author

Hi @pavlenex i think i might need help with passing the MG test. I have been able to fix everything else but this one seems like a constant fail for me. I appreciate your insight in advance

@pavlenex
Copy link
Collaborator

I think @GitGab19 and @lorbax should know better, it seems it's configuration problem bad-pool-config-test.json @GitGab19 does this mean the pool was down when the test was run, I'll re run it.

@NonsoAmadi10
Copy link
Contributor Author

I think it is still failing. I don't know if it is something I am missing on my part

@pavlenex
Copy link
Collaborator

Afair this test is flaky. Let's see what experts have to say. For now I can only post logs

STD ERR:    Compiling pool_sv2 v0.1.0 (/home/runner/work/stratum/stratum/roles/pool)
STD ERR:     Finished dev [optimized + debuginfo] target(s) in 1m 23s
STD ERR:      Running `target/llvm-cov-target/debug/pool_sv2 -c ../test/config/pool-mock-tp-bad-coinbase.toml`
STD OUT: 2024-03-11T17:51:32.642362Z  INFO pool_sv2: Pool INITIALIZING with config: "../test/config/pool-mock-tp-bad-coinbase.toml"
STD OUT: 2024-03-11T17:51:32.642393Z ERROR pool_sv2: Failed to get coinbase output: InvalidOutputScript
STD ERR: info: cargo-llvm-cov currently setting cfg(coverage); you can opt-out it by passing --no-cfg-coverage
STD ERR:     Finished dev [optimized + debuginfo] target(s) in 0.11s
STD ERR:      Running `target/llvm-cov-target/debug/pool_sv2 -h -c ../test/config/pool-mock-tp.toml`
STD OUT: 2024-03-11T17:51:32.983701Z ERROR pool_sv2: Usage: -h/--help, -c/--config <path|default pool-config.toml>
STD ERR: info: cargo-llvm-cov currently setting cfg(coverage); you can opt-out it by passing --no-cfg-coverage
STD ERR:     Finished dev [optimized + debuginfo] target(s) in 0.10s
STD ERR:      Running `target/llvm-cov-target/debug/pool_sv2 -c ../test/config/pool-mock-tp-bad-config.toml`
STD OUT: 2024-03-11T17:51:33.317829Z ERROR pool_sv2: Failed to parse config: missing field `listen_address` at line 1 column 1
STD ERR: info: cargo-llvm-cov currently setting cfg(coverage); you can opt-out it by passing --no-cfg-coverage
STD ERR:     Finished dev [optimized + debuginfo] target(s) in 0.11s
STD ERR:      Running `target/llvm-cov-target/debug/pool_sv2 -c ../test/config/no-config-at-this-path.toml`
STD OUT: 2024-03-11T17:51:33.655205Z ERROR pool_sv2: Failed to read config: No such file or directory (os error 2)
STD ERR: info: cargo-llvm-cov currently setting cfg(coverage); you can opt-out it by passing --no-cfg-coverage
STD ERR:     Finished dev [optimized + debuginfo] target(s) in 0.11s
STD ERR:      Running `target/llvm-cov-target/debug/pool_sv2 -c ../test/config/pool-mock-tp.toml`
STD OUT: 2024-03-11T17:51:33.987445Z  INFO pool_sv2: Pool INITIALIZING with config: "../test/config/pool-mock-tp.toml"
STD OUT: 2024-03-11T17:51:33.988181Z  INFO pool_sv2::lib::template_receiver: Connected to template distribution server at 127.0.0.1:8442
STD OUT: 2024-03-11T17:51:33.988718Z DEBUG network_helpers::noise_connection_tokio: Initializing as downstream for - 127.0.0.1:8442
STD OUT: Initializing as upstream for - 127.0.0.1:38228
STD OUT: Noise handshake complete - 127.0.0.1:38228
STD OUT: 2024-03-11T17:51:33.995967Z DEBUG 

@GitGab19
Copy link
Collaborator

I think @GitGab19 and @lorbax should know better, it seems it's configuration problem bad-pool-config-test.json @GitGab19 does this mean the pool was down when the test was run, I'll re run it.

Everything is up and running on our VPS.
Sometimes MG tests simply fail, I couldn't find a specific reason for that

@GitGab19
Copy link
Collaborator

I would wait for @lorbax addiction to PR #772 btw, since it will likely change something in the part touched by this PR

GitGab19 and others added 17 commits March 22, 2024 14:26
…, get them from jds mempool when a block is found
As suggested in
stratum-mining#772 (comment)
A task that before was blocking now is moved as a separate task on main
As suggested in
stratum-mining#772 (comment)
This commit introduces a verification that all the transactions in a job
are correctly recognized when a submit block message appears.
Error management when some transactions are missing. The jds should not
break. Instead, tries to recover it by triggering the jds mempool to
retrieve the transactions from its bitcoin node
In the message handler the message the triggers the JDS mempool to fill
the transactions is implemented in a bad way. Now it is fixed

Add some documentation
@NonsoAmadi10 NonsoAmadi10 changed the base branch from main to dev March 22, 2024 14:17
@NonsoAmadi10
Copy link
Contributor Author

I would be closing this PR because I don't want to force push and reimplementing this again. Thanks

@NonsoAmadi10 NonsoAmadi10 deleted the bug-jds-server-#784 branch April 2, 2024 17:03
@NonsoAmadi10 NonsoAmadi10 restored the bug-jds-server-#784 branch April 2, 2024 17:25
@NonsoAmadi10 NonsoAmadi10 reopened this Apr 2, 2024
@NonsoAmadi10 NonsoAmadi10 deleted the bug-jds-server-#784 branch April 2, 2024 18:37
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.

None yet

5 participants