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

avoid JDS dropping connection on SubmitSolution with missing txs #1025

Merged
merged 2 commits into from
Jul 26, 2024

Conversation

plebhash
Copy link
Collaborator

@plebhash plebhash commented Jul 1, 2024

this PR:

@plebhash plebhash linked an issue Jul 1, 2024 that may be closed by this pull request
@plebhash plebhash force-pushed the 912-jds-missing-txs branch from 1e5340b to 5d8098e Compare July 1, 2024 20:46
Copy link
Contributor

github-actions bot commented Jul 1, 2024

🐰Bencher

ReportFri, July 26, 2024 at 14:13:32 UTC
ProjectStratum v2 (SRI)
Branch912-jds-missing-txs
Testbedsv2
Click to view all benchmark results
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)2,111.00 (+2.42%)2,140.36 (98.63%)✅ (view plot)473.00 (+0.36%)484.87 (97.55%)✅ (view plot)731.00 (-0.14%)751.72 (97.24%)✅ (view plot)10.00 (+28.52%)12.09 (82.68%)✅ (view plot)38.00 (+3.08%)38.74 (98.10%)
client_sv2_handle_message_mining✅ (view plot)8,219.00 (+0.20%)8,323.71 (98.74%)✅ (view plot)2,137.00 (+0.34%)2,168.52 (98.55%)✅ (view plot)3,159.00 (+0.34%)3,210.87 (98.38%)✅ (view plot)39.00 (+0.89%)43.01 (90.67%)✅ (view plot)139.00 (+0.08%)141.56 (98.19%)
client_sv2_mining_message_submit_standard✅ (view plot)6,316.00 (+0.55%)6,381.76 (98.97%)✅ (view plot)1,750.00 (+0.02%)1,761.33 (99.36%)✅ (view plot)2,546.00 (-0.23%)2,571.92 (98.99%)✅ (view plot)26.00 (+31.17%)28.89 (90.01%)✅ (view plot)104.00 (+0.26%)106.44 (97.71%)
client_sv2_mining_message_submit_standard_serialize✅ (view plot)14,657.00 (-0.57%)14,991.48 (97.77%)✅ (view plot)4,694.00 (+0.01%)4,705.33 (99.76%)✅ (view plot)6,752.00 (-0.03%)6,771.77 (99.71%)✅ (view plot)55.00 (+11.34%)57.72 (95.29%)✅ (view plot)218.00 (-1.43%)228.92 (95.23%)
client_sv2_mining_message_submit_standard_serialize_deserialize✅ (view plot)27,469.00 (-0.02%)27,787.43 (98.85%)✅ (view plot)10,585.00 (+0.29%)10,597.69 (99.88%)✅ (view plot)15,399.00 (+0.28%)15,416.84 (99.88%)✅ (view plot)90.00 (+5.47%)92.64 (97.15%)✅ (view plot)332.00 (-0.60%)343.42 (96.68%)
client_sv2_open_channel✅ (view plot)4,451.00 (-0.55%)4,593.01 (96.91%)✅ (view plot)1,461.00 (+0.04%)1,472.61 (99.21%)✅ (view plot)2,156.00 (+0.10%)2,171.41 (99.29%)✅ (view plot)11.00 (-6.08%)14.64 (75.14%)✅ (view plot)64.00 (-1.04%)67.75 (94.47%)
client_sv2_open_channel_serialize✅ (view plot)14,040.00 (-0.87%)14,441.61 (97.22%)✅ (view plot)5,064.00 (+0.01%)5,075.61 (99.77%)✅ (view plot)7,320.00 (+0.02%)7,337.10 (99.77%)✅ (view plot)42.00 (+8.92%)44.55 (94.28%)✅ (view plot)186.00 (-2.14%)198.57 (93.67%)
client_sv2_open_channel_serialize_deserialize✅ (view plot)22,557.00 (-0.26%)22,955.05 (98.27%)✅ (view plot)8,027.00 (+0.39%)8,040.02 (99.84%)✅ (view plot)11,672.00 (+0.36%)11,689.50 (99.85%)✅ (view plot)84.00 (+10.19%)88.60 (94.81%)✅ (view plot)299.00 (-1.32%)313.40 (95.40%)
client_sv2_setup_connection✅ (view plot)4,693.00 (+0.03%)4,755.55 (98.68%)✅ (view plot)1,502.00 (+0.04%)1,513.61 (99.23%)✅ (view plot)2,273.00 (-0.13%)2,295.75 (99.01%)✅ (view plot)15.00 (+40.20%)17.42 (86.12%)✅ (view plot)67.00 (-0.73%)69.57 (96.30%)
client_sv2_setup_connection_serialize✅ (view plot)15,976.00 (-1.27%)16,527.83 (96.66%)✅ (view plot)5,963.00 (+0.01%)5,974.61 (99.81%)✅ (view plot)8,661.00 (+0.05%)8,677.01 (99.82%)✅ (view plot)49.00 (+6.94%)51.33 (95.46%)✅ (view plot)202.00 (-3.08%)218.98 (92.25%)
client_sv2_setup_connection_serialize_deserialize✅ (view plot)35,416.00 (-0.24%)35,716.84 (99.16%)✅ (view plot)14,855.00 (+0.22%)14,868.19 (99.91%)✅ (view plot)21,811.00 (+0.20%)21,828.69 (99.92%)✅ (view plot)110.00 (+7.35%)117.24 (93.82%)✅ (view plot)373.00 (-1.25%)385.36 (96.79%)

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

Copy link
Contributor

github-actions bot commented Jul 1, 2024

🐰Bencher

ReportFri, July 26, 2024 at 14:13:30 UTC
ProjectStratum v2 (SRI)
Branch1025/merge
Testbedsv1
Click to view all benchmark results
BenchmarkLatencyLatency Results
nanoseconds (ns) | (Δ%)
Latency Upper Boundary
nanoseconds (ns) | (%)
client-submit-serialize✅ (view plot)6,524.20 (-3.86%)7,330.04 (89.01%)
client-submit-serialize-deserialize✅ (view plot)7,389.40 (-4.04%)8,284.38 (89.20%)
client-submit-serialize-deserialize-handle/client-submit-serialize-deserialize-handle✅ (view plot)8,016.20 (-3.26%)8,801.89 (91.07%)
client-sv1-authorize-serialize-deserialize-handle/client-sv1-authorize-serialize-deserialize-handle✅ (view plot)913.01 (+1.33%)928.04 (98.38%)
client-sv1-authorize-serialize-deserialize/client-sv1-authorize-serialize-deserialize✅ (view plot)703.89 (+0.52%)724.06 (97.21%)
client-sv1-authorize-serialize/client-sv1-authorize-serialize✅ (view plot)248.10 (+0.05%)255.32 (97.17%)
client-sv1-get-authorize/client-sv1-get-authorize✅ (view plot)156.71 (-0.61%)163.06 (96.10%)
client-sv1-get-submit✅ (view plot)6,258.50 (-4.64%)7,089.45 (88.28%)
client-sv1-get-subscribe/client-sv1-get-subscribe✅ (view plot)278.88 (-0.13%)290.73 (95.92%)
client-sv1-subscribe-serialize-deserialize-handle/client-sv1-subscribe-serialize-deserialize-handle✅ (view plot)776.51 (+2.95%)787.16 (98.65%)
client-sv1-subscribe-serialize-deserialize/client-sv1-subscribe-serialize-deserialize✅ (view plot)607.31 (-1.28%)636.90 (95.35%)
client-sv1-subscribe-serialize/client-sv1-subscribe-serialize✅ (view plot)202.76 (-1.70%)219.75 (92.27%)

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

Copy link
Contributor

github-actions bot commented Jul 1, 2024

🐰Bencher

ReportFri, July 26, 2024 at 14:13:31 UTC
ProjectStratum v2 (SRI)
Branch912-jds-missing-txs
Testbedsv1
Click to view all benchmark results
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)8,538.00 (+0.92%)8,722.56 (97.88%)✅ (view plot)3,746.00 (+0.11%)3,840.74 (97.53%)✅ (view plot)5,248.00 (+0.04%)5,380.47 (97.54%)✅ (view plot)7.00 (-9.12%)9.98 (70.17%)✅ (view plot)93.00 (+2.50%)94.97 (97.92%)
get_submit✅ (view plot)95,509.00 (-0.03%)96,047.79 (99.44%)✅ (view plot)59,439.00 (-0.04%)59,728.80 (99.51%)✅ (view plot)85,359.00 (-0.04%)85,766.53 (99.52%)✅ (view plot)49.00 (-8.48%)62.03 (78.99%)✅ (view plot)283.00 (+0.26%)287.13 (98.56%)
get_subscribe✅ (view plot)7,991.00 (+0.16%)8,230.98 (97.08%)✅ (view plot)2,841.00 (+0.29%)2,927.72 (97.04%)✅ (view plot)3,971.00 (+0.32%)4,084.52 (97.22%)✅ (view plot)13.00 (-15.39%)19.63 (66.24%)✅ (view plot)113.00 (+0.30%)116.39 (97.09%)
serialize_authorize✅ (view plot)12,291.00 (+0.53%)12,490.40 (98.40%)✅ (view plot)5,317.00 (+0.08%)5,411.74 (98.25%)✅ (view plot)7,411.00 (+0.03%)7,543.81 (98.24%)✅ (view plot)10.00 (-5.61%)12.92 (77.37%)✅ (view plot)138.00 (+1.38%)140.62 (98.14%)
serialize_deserialize_authorize✅ (view plot)24,588.00 (+0.37%)24,726.59 (99.44%)✅ (view plot)9,898.00 (-0.01%)10,008.01 (98.90%)✅ (view plot)13,958.00 (-0.05%)14,123.26 (98.83%)✅ (view plot)33.00 (-7.46%)41.08 (80.33%)✅ (view plot)299.00 (+1.06%)300.24 (99.59%)
serialize_deserialize_handle_authorize✅ (view plot)30,327.00 (+0.45%)30,422.95 (99.68%)✅ (view plot)12,101.00 (+0.03%)12,195.74 (99.22%)✅ (view plot)17,117.00 (-0.00%)17,257.10 (99.19%)✅ (view plot)59.00 (+0.16%)64.02 (92.16%)✅ (view plot)369.00 (+1.06%)370.41 (99.62%)
serialize_deserialize_handle_submit✅ (view plot)126,372.00 (-0.02%)126,937.20 (99.55%)✅ (view plot)73,224.00 (-0.03%)73,562.48 (99.54%)✅ (view plot)104,947.00 (-0.03%)105,429.40 (99.54%)✅ (view plot)120.00 (-0.31%)129.65 (92.56%)✅ (view plot)595.00 (+0.01%)598.74 (99.38%)
serialize_deserialize_handle_subscribe✅ (view plot)27,491.00 (+0.10%)27,595.35 (99.62%)✅ (view plot)9,643.00 (+0.09%)9,729.72 (99.11%)✅ (view plot)13,641.00 (+0.10%)13,759.62 (99.14%)✅ (view plot)61.00 (-5.70%)72.73 (83.87%)✅ (view plot)387.00 (+0.23%)388.74 (99.55%)
serialize_deserialize_submit✅ (view plot)114,999.00 (-0.05%)115,557.48 (99.52%)✅ (view plot)68,001.00 (-0.06%)68,344.38 (99.50%)✅ (view plot)97,559.00 (-0.07%)98,065.13 (99.48%)✅ (view plot)65.00 (-4.83%)74.75 (86.95%)✅ (view plot)489.00 (+0.14%)492.09 (99.37%)
serialize_deserialize_subscribe✅ (view plot)22,922.00 (+0.16%)23,091.38 (99.27%)✅ (view plot)8,195.00 (+0.08%)8,284.51 (98.92%)✅ (view plot)11,542.00 (+0.08%)11,665.07 (98.94%)✅ (view plot)36.00 (-6.56%)43.61 (82.56%)✅ (view plot)320.00 (+0.36%)321.65 (99.49%)
serialize_submit✅ (view plot)99,826.00 (-0.05%)100,374.40 (99.45%)✅ (view plot)61,483.00 (-0.04%)61,777.40 (99.52%)✅ (view plot)88,206.00 (-0.04%)88,619.11 (99.53%)✅ (view plot)49.00 (-9.24%)62.32 (78.63%)✅ (view plot)325.00 (+0.10%)328.59 (98.91%)
serialize_subscribe✅ (view plot)11,328.00 (+0.03%)11,565.69 (97.94%)✅ (view plot)4,188.00 (+0.20%)4,274.72 (97.97%)✅ (view plot)5,828.00 (+0.20%)5,943.15 (98.06%)✅ (view plot)15.00 (-6.33%)18.57 (80.76%)✅ (view plot)155.00 (-0.06%)159.05 (97.46%)

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

Copy link
Contributor

github-actions bot commented Jul 1, 2024

🐰Bencher

ReportFri, July 26, 2024 at 14:13:34 UTC
ProjectStratum v2 (SRI)
Branch912-jds-missing-txs
Testbedsv2
Click to view all benchmark results
BenchmarkLatencyLatency Results
nanoseconds (ns) | (Δ%)
Latency Upper Boundary
nanoseconds (ns) | (%)
client_sv2_handle_message_common✅ (view plot)44.52 (-0.21%)45.41 (98.03%)
client_sv2_handle_message_mining✅ (view plot)74.88 (+1.53%)83.35 (89.84%)
client_sv2_mining_message_submit_standard✅ (view plot)14.66 (-0.01%)14.83 (98.87%)
client_sv2_mining_message_submit_standard_serialize✅ (view plot)260.47 (-1.18%)281.82 (92.43%)
client_sv2_mining_message_submit_standard_serialize_deserialize✅ (view plot)598.36 (+0.14%)650.74 (91.95%)
client_sv2_open_channel✅ (view plot)168.85 (+0.88%)180.02 (93.79%)
client_sv2_open_channel_serialize✅ (view plot)271.87 (-3.17%)293.47 (92.64%)
client_sv2_open_channel_serialize_deserialize✅ (view plot)381.12 (+0.77%)417.95 (91.19%)
client_sv2_setup_connection✅ (view plot)156.32 (-4.57%)173.52 (90.09%)
client_sv2_setup_connection_serialize✅ (view plot)449.18 (-4.11%)505.25 (88.90%)
client_sv2_setup_connection_serialize_deserialize✅ (view plot)986.40 (+0.72%)1,053.89 (93.60%)

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

@plebhash plebhash force-pushed the 912-jds-missing-txs branch from 5d8098e to 131bca5 Compare July 9, 2024 14:25
@plebhash plebhash marked this pull request as ready for review July 9, 2024 14:25
@plebhash plebhash force-pushed the 912-jds-missing-txs branch 4 times, most recently from 993430e to d4510a5 Compare July 9, 2024 15:24
@jbesraa
Copy link
Contributor

jbesraa commented Jul 9, 2024

^^

Copy link
Contributor

@jbesraa jbesraa 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. Would be good if someone with more knowledge about MG tests than myself can chime in as well.

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 d4510a5. Just few nits

@plebhash plebhash force-pushed the 912-jds-missing-txs branch 3 times, most recently from 3c9cf45 to 38c87ef Compare July 12, 2024 19:32
@rrybarczyk rrybarczyk self-requested a review July 17, 2024 13:56
@plebhash plebhash force-pushed the 912-jds-missing-txs branch 4 times, most recently from f462ccb to ee17907 Compare July 18, 2024 14:30
@plebhash plebhash force-pushed the 912-jds-missing-txs branch from ee17907 to 5440147 Compare July 23, 2024 16:43
@lorbax
Copy link
Collaborator

lorbax commented Jul 23, 2024

Did you consider to remove a placeholder for the inner error here?
https://github.com/stratum-mining/stratum/blob/dev/roles/jd-server/src/lib/status.rs#L122
If you put something like this

        JdsError::MempoolError(error_inner) => {
            match error_inner {
                EmptyMempool => {...},
                NoClient => {...},
                Rpc(RpcError) => {...},
                PoisonLock(String) => {...},
            }
        }

and return

 send_status(sender, e, error_handling::ErrorBranch::Break).await

or

 send_status(sender, e, error_handling::ErrorBranch::Continue).await

according on when you want the JDS to continue or break

plebhash added a commit to plebhash/stratum that referenced this pull request Jul 24, 2024
following suggestion by @lorbax stratum-mining#1025 (comment)

the implementation diverged a bit from the suggestion, but it was still a good reminder that we should leverage `handle_result!` macro here
@plebhash
Copy link
Collaborator Author

plebhash commented Jul 24, 2024

Did you consider to remove a placeholder for the inner error here? https://github.com/stratum-mining/stratum/blob/dev/roles/jd-server/src/lib/status.rs#L122 If you put something like this

        JdsError::MempoolError(error_inner) => {
            match error_inner {
                EmptyMempool => {...},
                NoClient => {...},
                Rpc(RpcError) => {...},
                PoisonLock(String) => {...},
            }
        }

and return

 send_status(sender, e, error_handling::ErrorBranch::Break).await

or

 send_status(sender, e, error_handling::ErrorBranch::Continue).await

according on when you want the JDS to continue or break

this comment was a good reminder of handle_result!

I ended up with a different solution, which is simpler than what @lorbax suggested above

it can be summarized on this commit: 7b8765a

basically, when the bug described in #912 is triggered, JDS encounters a JdsError::ImpossibleToReconstructBlock, which has the following handle_error implementation:

JdsError::ImpossibleToReconstructBlock(_) => {
send_status(sender, e, error_handling::ErrorBranch::Continue).await
}

so we can simply defer to the default/standard error handling mechanism of JDS, which is the handle_result! macro

plebhash added a commit to plebhash/stratum that referenced this pull request Jul 24, 2024
following suggestion by @lorbax stratum-mining#1025 (comment)

the implementation diverged a bit from the suggestion, but it was still a good reminder that we should leverage `handle_result!` macro here
@plebhash plebhash force-pushed the 912-jds-missing-txs branch from 1d33e46 to 7b8765a Compare July 24, 2024 17:27
plebhash added a commit to plebhash/stratum that referenced this pull request Jul 25, 2024
following suggestion by @lorbax stratum-mining#1025 (comment)

the implementation diverged a bit from the suggestion, but it was still a good reminder that we should leverage `handle_result!` macro here
@plebhash plebhash force-pushed the 912-jds-missing-txs branch from 7b8765a to 493e326 Compare July 25, 2024 03:38
Copy link
Contributor

@jbesraa jbesraa left a comment

Choose a reason for hiding this comment

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

Nice, I learned about handle_result.

I think 3rd commit should be squashed into the first one

@Fi3
Copy link
Collaborator

Fi3 commented Jul 25, 2024

Nice, I learned about handle_result.

I think 3rd commit should be squashed into the first one

I think that in the long term we should get rid of it. I usually prefer handling the task as close as possible from where I start it, I also think that for production code abort the task on dropping is better, since it easier to understand if there is a bug

plebhash added a commit to plebhash/stratum that referenced this pull request Jul 26, 2024
following suggestion by @lorbax stratum-mining#1025 (comment)

the implementation diverged a bit from the suggestion, but it was still a good reminder that we should leverage `handle_result!` macro here
@plebhash plebhash force-pushed the 912-jds-missing-txs branch from 493e326 to f7cd687 Compare July 26, 2024 13:56
plebhash added a commit to plebhash/stratum that referenced this pull request Jul 26, 2024
this is the actual fix for stratum-mining#912

use handle_result! macro

following suggestion by @lorbax stratum-mining#1025 (comment)

the implementation diverged a bit from the suggestion, but it was still a good reminder that we should leverage `handle_result!` macro here
@plebhash plebhash force-pushed the 912-jds-missing-txs branch from f7cd687 to 7a419d3 Compare July 26, 2024 13:59
plebhash added 2 commits July 26, 2024 10:00
this is the actual fix for stratum-mining#912

use handle_result! macro

following suggestion by @lorbax stratum-mining#1025 (comment)

the implementation diverged a bit from the suggestion, but it was still a good reminder that we should leverage `handle_result!` macro here
@plebhash plebhash force-pushed the 912-jds-missing-txs branch from 7a419d3 to a197770 Compare July 26, 2024 14:01
@plebhash plebhash requested a review from jbesraa July 26, 2024 14:02
@pavlenex pavlenex merged commit 1062ec7 into stratum-mining:dev Jul 26, 2024
32 checks passed
@plebhash plebhash deleted the 912-jds-missing-txs branch July 26, 2024 21:53
@plebhash plebhash mentioned this pull request Aug 18, 2024
Fi3 pushed a commit to Fi3/stratum-1 that referenced this pull request Nov 26, 2024
this is the actual fix for stratum-mining#912

use handle_result! macro

following suggestion by @lorbax stratum-mining#1025 (comment)

the implementation diverged a bit from the suggestion, but it was still a good reminder that we should leverage `handle_result!` macro here
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.

JDS can't submit block if some transactions are missing
8 participants