Discard old ProvideMissingTransactionsSuccess messages in JDS #900
Conversation
| Some(declared_job) => { | ||
| let id = declared_job.request_id; | ||
| // check request_id in order to ignore old ProvideMissingTransactionsSuccess (see issue #860) | ||
| if id == message.request_id { |
There was a problem hiding this comment.
I think we're still not fully fixing the issue
regardless of this new conditional on line 164, execution continues after line 200, and JDS still returns DeclareMiningJobSuccess, even if request_id is wrong
There was a problem hiding this comment.
this could be even seen as a potential attack vector:
JDS sent a ProvideMissingTransaction as a verification that declared_job is valid, as a prevention mechanism to avoid invalid Jobs being acknowledged
but JDC could send any garbage on ProvideMissingTransaction.Success and still get a DeclareMiningJobSuccess acknowledgement in return
There was a problem hiding this comment.
Do you think it would be better to send a DeclareMiningJobError (with a new error code like "declared-mining-job-old"?
I implemented this solution after discussion and suggestion by @Fi3 here: #860 (comment)
There was a problem hiding this comment.
Returning an error is wrong. The client is sending a message that in this context is valid. Either you keep track of older declared job and you handle this message using the old job (pointless imo since we are going to use a newer job) or you just ignore the message, you don't have to answer with something.
SendTo::None(None)
There was a problem hiding this comment.
we want to avoid sending DeclareMiningJobSuccess if id != message.request_id
so I think we need to move the logic between lines 200-211 inside the new conditional on line 164 (if id == message.request_id)
and as @Fi3 pointed out, leave the generic case (last line of the function implementation) as Ok(SendTo::None(None))
that way, if JDC is sending garbage on ProvideMissingTransaction.Success (either old or malicious job), it will simply be ignored, and they will never get an acknowledgement in the form of DeclareMiningJobSuccess
There was a problem hiding this comment.
I think he's referring to the loop I mentioned on JDC side (which actually is more related to PR #904).
The one which you are suggesting to manage with a timeout.
I was exploring the idea of returning a DeclareMiningJob.Error in order to manage this case without arbitrary choices like a timeout, but in a cleaner way (imo).
There was a problem hiding this comment.
I missed the suggestion on the timeout, sorry about that. Maybe that's a valid option too.
I'm only strongly opposed to returning a DeclareMiningJob.Success if the job is no longer valid, but I guess we all agree that's not a good solution.
So whatever you guys decide here is fine for me.
There was a problem hiding this comment.
I just managed to exit from the loop in JDC without the necessity of receiving a DeclareMiningJob.Success/Error, and without arbitrary timeouts.
I used a counter to check the number of tasks created into on_new_prev_hash function, added here: 0f28310.
It means that this PR is ready to be merged, since no changes are needed from last review made by @plebhash.
There was a problem hiding this comment.
cool, it needs to be rebased tho, and it seems I cannot do it on Github UI:
There was a problem hiding this comment.
cool, it needs to be rebased tho, and it seems I cannot do it on Github UI:
Just rebased and pushed
|
| Report | Thu, May 16, 2024 at 21:43:19 UTC |
| Project | Stratum v2 (SRI) |
| Branch | 900/merge |
| Testbed | sv1 |
Click to view all benchmark results
| Benchmark | Latency | Latency Results nanoseconds (ns) | (Δ%) | Latency Upper Boundary nanoseconds (ns) | (%) |
|---|---|---|---|
| client-submit-serialize | ✅ (view plot) | 7,027.50 (+1.33%) | 7,279.27 (96.54%) |
| client-submit-serialize-deserialize | ✅ (view plot) | 7,804.20 (-0.43%) | 8,254.25 (94.55%) |
| client-submit-serialize-deserialize-handle/client-submit-serialize-deserialize-handle | ✅ (view plot) | 8,528.40 (+1.20%) | 8,815.69 (96.74%) |
| client-sv1-authorize-serialize-deserialize-handle/client-sv1-authorize-serialize-deserialize-handle | ✅ (view plot) | 929.07 (+3.10%) | 935.38 (99.33%) |
| client-sv1-authorize-serialize-deserialize/client-sv1-authorize-serialize-deserialize | ✅ (view plot) | 703.45 (+0.76%) | 721.18 (97.54%) |
| client-sv1-authorize-serialize/client-sv1-authorize-serialize | ✅ (view plot) | 249.01 (+0.35%) | 254.57 (97.82%) |
| client-sv1-get-authorize/client-sv1-get-authorize | ✅ (view plot) | 157.03 (-0.04%) | 160.82 (97.64%) |
| client-sv1-get-submit | ✅ (view plot) | 6,795.60 (+1.60%) | 7,032.38 (96.63%) |
| client-sv1-get-subscribe/client-sv1-get-subscribe | ✅ (view plot) | 279.23 (+0.13%) | 291.88 (95.67%) |
| client-sv1-subscribe-serialize-deserialize-handle/client-sv1-subscribe-serialize-deserialize-handle | ✅ (view plot) | 762.40 (+1.84%) | 778.88 (97.88%) |
| client-sv1-subscribe-serialize-deserialize/client-sv1-subscribe-serialize-deserialize | ✅ (view plot) | 631.53 (+2.60%) | 638.79 (98.86%) |
| client-sv1-subscribe-serialize/client-sv1-subscribe-serialize | ✅ (view plot) | 205.36 (-0.51%) | 219.63 (93.50%) |
Bencher - Continuous Benchmarking
View Public Perf Page
Docs | Repo | Chat | Help
|
| Report | Thu, May 16, 2024 at 21:43:14 UTC |
| Project | Stratum v2 (SRI) |
| Branch | patch-860 |
| Testbed | sv1 |
Click to view all benchmark results
| Benchmark | Estimated Cycles | Estimated Cycles Results estimated cycles | (Δ%) | Estimated Cycles Upper Boundary estimated cycles | (%) | Instructions | Instructions Results instructions | (Δ%) | Instructions Upper Boundary instructions | (%) | L1 Accesses | L1 Accesses Results accesses | (Δ%) | L1 Accesses Upper Boundary accesses | (%) | L2 Accesses | L2 Accesses Results accesses | (Δ%) | L2 Accesses Upper Boundary accesses | (%) | RAM Accesses | RAM Accesses Results accesses | (Δ%) | RAM Accesses Upper Boundary accesses | (%) |
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| get_authorize | ✅ (view plot) | 8,440.00 (+0.24%) | 8,703.87 (96.97%) | ✅ (view plot) | 3,746.00 (+0.26%) | 3,850.84 (97.28%) | ✅ (view plot) | 5,250.00 (+0.23%) | 5,392.64 (97.35%) | ✅ (view plot) | 8.00 (-1.17%) | 10.42 (76.78%) | ✅ (view plot) | 90.00 (+0.27%) | 93.59 (96.16%) |
| get_submit | ✅ (view plot) | 95,549.00 (+0.01%) | 96,140.59 (99.38%) | ✅ (view plot) | 59,439.00 (-0.03%) | 59,771.49 (99.44%) | ✅ (view plot) | 85,349.00 (-0.04%) | 85,820.85 (99.45%) | ✅ (view plot) | 59.00 (+6.22%) | 63.00 (93.65%) | ✅ (view plot) | 283.00 (+0.25%) | 287.95 (98.28%) |
| get_subscribe | ✅ (view plot) | 8,007.00 (+0.43%) | 8,268.45 (96.84%) | ✅ (view plot) | 2,841.00 (+0.44%) | 2,940.16 (96.63%) | ✅ (view plot) | 3,967.00 (+0.40%) | 4,099.13 (96.78%) | ✅ (view plot) | 17.00 (+4.65%) | 19.57 (86.88%) | ✅ (view plot) | 113.00 (+0.39%) | 116.87 (96.69%) |
| serialize_authorize | ✅ (view plot) | 12,159.00 (-0.16%) | 12,454.56 (97.63%) | ✅ (view plot) | 5,317.00 (+0.18%) | 5,421.84 (98.07%) | ✅ (view plot) | 7,414.00 (+0.18%) | 7,556.61 (98.11%) | ✅ (view plot) | 11.00 (-0.68%) | 13.59 (80.92%) | ✅ (view plot) | 134.00 (-0.67%) | 139.00 (96.40%) |
| serialize_deserialize_authorize | ✅ (view plot) | 24,472.00 (+0.09%) | 24,672.43 (99.19%) | ✅ (view plot) | 9,898.00 (+0.06%) | 10,013.82 (98.84%) | ✅ (view plot) | 13,957.00 (+0.03%) | 14,127.31 (98.79%) | ✅ (view plot) | 38.00 (+2.44%) | 41.76 (90.99%) | ✅ (view plot) | 295.00 (+0.14%) | 297.19 (99.26%) |
| serialize_deserialize_handle_authorize | ✅ (view plot) | 30,199.00 (+0.20%) | 30,327.68 (99.58%) | ✅ (view plot) | 12,101.00 (+0.08%) | 12,205.84 (99.14%) | ✅ (view plot) | 17,119.00 (+0.05%) | 17,272.05 (99.11%) | ✅ (view plot) | 61.00 (+4.43%) | 63.73 (95.72%) | ✅ (view plot) | 365.00 (+0.30%) | 366.69 (99.54%) |
| serialize_deserialize_handle_submit | ✅ (view plot) | 126,472.00 (+0.05%) | 127,036.96 (99.56%) | ✅ (view plot) | 73,224.00 (-0.02%) | 73,614.15 (99.47%) | ✅ (view plot) | 104,937.00 (-0.03%) | 105,497.53 (99.47%) | ✅ (view plot) | 128.00 (+5.79%) | 132.88 (96.33%) | ✅ (view plot) | 597.00 (+0.31%) | 599.46 (99.59%) |
| serialize_deserialize_handle_subscribe | ✅ (view plot) | 27,523.00 (+0.23%) | 27,608.78 (99.69%) | ✅ (view plot) | 9,643.00 (+0.13%) | 9,742.16 (98.98%) | ✅ (view plot) | 13,633.00 (+0.09%) | 13,775.83 (98.96%) | ✅ (view plot) | 69.00 (+5.66%) | 71.00 (97.19%) | ✅ (view plot) | 387.00 (+0.24%) | 388.49 (99.62%) |
| serialize_deserialize_submit | ✅ (view plot) | 115,065.00 (+0.02%) | 115,611.60 (99.53%) | ✅ (view plot) | 68,001.00 (-0.05%) | 68,375.42 (99.45%) | ✅ (view plot) | 97,550.00 (-0.06%) | 98,098.62 (99.44%) | ✅ (view plot) | 73.00 (+5.19%) | 74.85 (97.53%) | ✅ (view plot) | 490.00 (+0.36%) | 492.62 (99.47%) |
| serialize_deserialize_subscribe | ✅ (view plot) | 22,904.00 (+0.17%) | 23,096.28 (99.17%) | ✅ (view plot) | 8,195.00 (+0.14%) | 8,296.20 (98.78%) | ✅ (view plot) | 11,539.00 (+0.12%) | 11,679.64 (98.80%) | ✅ (view plot) | 40.00 (+1.58%) | 43.71 (91.51%) | ✅ (view plot) | 319.00 (+0.21%) | 321.17 (99.32%) |
| serialize_submit | ✅ (view plot) | 99,832.00 (-0.04%) | 100,447.66 (99.39%) | ✅ (view plot) | 61,483.00 (-0.03%) | 61,821.15 (99.45%) | ✅ (view plot) | 88,197.00 (-0.04%) | 88,675.64 (99.46%) | ✅ (view plot) | 59.00 (+5.25%) | 62.19 (94.88%) | ✅ (view plot) | 324.00 (-0.16%) | 329.02 (98.47%) |
| serialize_subscribe | ✅ (view plot) | 11,302.00 (-0.06%) | 11,577.70 (97.62%) | ✅ (view plot) | 4,188.00 (+0.30%) | 4,287.16 (97.69%) | ✅ (view plot) | 5,827.00 (+0.29%) | 5,959.05 (97.78%) | ✅ (view plot) | 17.00 (+3.68%) | 18.90 (89.96%) | ✅ (view plot) | 154.00 (-0.50%) | 158.92 (96.90%) |
Bencher - Continuous Benchmarking
View Public Perf Page
Docs | Repo | Chat | Help
|
| Report | Thu, May 16, 2024 at 21:43:14 UTC |
| Project | Stratum v2 (SRI) |
| Branch | patch-860 |
| Testbed | sv2 |
🚨 2 ALERTS: Threshold Boundary Limits exceeded!
| Benchmark | Measure (units) | View | Value | Lower Boundary | Upper Boundary |
|---|---|---|---|---|---|
| client_sv2_handle_message_common | Latency (nanoseconds (ns)) | 🚨 (view plot | view alert) | 45.35 (+1.71%) | 45.32 (100.06%) | |
| client_sv2_open_channel_serialize_deserialize | Latency (nanoseconds (ns)) | 🚨 (view plot | view alert) | 471.53 (+24.68%) | 415.50 (113.48%) |
Click to view all benchmark results
| Benchmark | Latency | Latency Results nanoseconds (ns) | (Δ%) | Latency Upper Boundary nanoseconds (ns) | (%) |
|---|---|---|---|
| client_sv2_handle_message_common | 🚨 (view plot | view alert) | 45.35 (+1.71%) | 45.32 (100.06%) |
| client_sv2_handle_message_mining | ✅ (view plot) | 73.03 (-0.49%) | 82.58 (88.43%) |
| client_sv2_mining_message_submit_standard | ✅ (view plot) | 14.63 (-0.16%) | 14.69 (99.60%) |
| client_sv2_mining_message_submit_standard_serialize | ✅ (view plot) | 266.71 (+0.73%) | 283.91 (93.94%) |
| client_sv2_mining_message_submit_standard_serialize_deserialize | ✅ (view plot) | 574.96 (-3.03%) | 624.30 (92.10%) |
| client_sv2_open_channel | ✅ (view plot) | 165.41 (-0.08%) | 171.12 (96.66%) |
| client_sv2_open_channel_serialize | ✅ (view plot) | 282.09 (-0.82%) | 295.84 (95.35%) |
| client_sv2_open_channel_serialize_deserialize | 🚨 (view plot | view alert) | 471.53 (+24.68%) | 415.50 (113.48%) |
| client_sv2_setup_connection | ✅ (view plot) | 160.45 (-2.14%) | 174.62 (91.89%) |
| client_sv2_setup_connection_serialize | ✅ (view plot) | 456.61 (-3.55%) | 498.80 (91.54%) |
| client_sv2_setup_connection_serialize_deserialize | ✅ (view plot) | 957.50 (-1.06%) | 1,039.99 (92.07%) |
Bencher - Continuous Benchmarking
View Public Perf Page
Docs | Repo | Chat | Help
|
| Report | Thu, May 16, 2024 at 21:43:13 UTC |
| Project | Stratum v2 (SRI) |
| Branch | patch-860 |
| Testbed | sv2 |
Click to view all benchmark results
| Benchmark | Estimated Cycles | Estimated Cycles Results estimated cycles | (Δ%) | Estimated Cycles Upper Boundary estimated cycles | (%) | Instructions | Instructions Results instructions | (Δ%) | Instructions Upper Boundary instructions | (%) | L1 Accesses | L1 Accesses Results accesses | (Δ%) | L1 Accesses Upper Boundary accesses | (%) | L2 Accesses | L2 Accesses Results accesses | (Δ%) | L2 Accesses Upper Boundary accesses | (%) | RAM Accesses | RAM Accesses Results accesses | (Δ%) | RAM Accesses Upper Boundary accesses | (%) |
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| client_sv2_handle_message_common | ✅ (view plot) | 2,095.00 (+1.91%) | 2,140.72 (97.86%) | ✅ (view plot) | 473.00 (+0.49%) | 486.56 (97.21%) | ✅ (view plot) | 735.00 (+0.46%) | 754.78 (97.38%) | ✅ (view plot) | 6.00 (-18.88%) | 11.87 (50.56%) | ✅ (view plot) | 38.00 (+3.34%) | 38.85 (97.82%) |
| client_sv2_handle_message_mining | ✅ (view plot) | 8,279.00 (+0.89%) | 8,355.87 (99.08%) | ✅ (view plot) | 2,137.00 (+0.47%) | 2,171.53 (98.41%) | ✅ (view plot) | 3,159.00 (+0.49%) | 3,214.95 (98.26%) | ✅ (view plot) | 37.00 (-5.27%) | 43.26 (85.53%) | ✅ (view plot) | 141.00 (+1.40%) | 142.44 (98.99%) |
| client_sv2_mining_message_submit_standard | ✅ (view plot) | 6,352.00 (+1.05%) | 6,398.74 (99.27%) | ✅ (view plot) | 1,750.00 (+0.02%) | 1,763.35 (99.24%) | ✅ (view plot) | 2,552.00 (-0.05%) | 2,575.51 (99.09%) | ✅ (view plot) | 18.00 (+1.17%) | 22.74 (79.15%) | ✅ (view plot) | 106.00 (+1.81%) | 107.17 (98.91%) |
| client_sv2_mining_message_submit_standard_serialize | ✅ (view plot) | 14,889.00 (+0.64%) | 15,068.62 (98.81%) | ✅ (view plot) | 4,694.00 (+0.01%) | 4,707.35 (99.72%) | ✅ (view plot) | 6,754.00 (+0.01%) | 6,775.04 (99.69%) | ✅ (view plot) | 45.00 (-5.54%) | 52.95 (84.99%) | ✅ (view plot) | 226.00 (+1.38%) | 230.85 (97.90%) |
| client_sv2_mining_message_submit_standard_serialize_deserialize | ✅ (view plot) | 27,610.00 (+0.39%) | 27,897.59 (98.97%) | ✅ (view plot) | 10,545.00 (+0.03%) | 10,558.75 (99.87%) | ✅ (view plot) | 15,335.00 (-0.02%) | 15,359.68 (99.84%) | ✅ (view plot) | 89.00 (+5.38%) | 91.03 (97.77%) | ✅ (view plot) | 338.00 (+0.74%) | 346.63 (97.51%) |
| client_sv2_open_channel | ✅ (view plot) | 4,561.00 (+1.42%) | 4,618.69 (98.75%) | ✅ (view plot) | 1,461.00 (+0.05%) | 1,474.62 (99.08%) | ✅ (view plot) | 2,151.00 (-0.07%) | 2,172.92 (98.99%) | ✅ (view plot) | 13.00 (+7.66%) | 15.47 (84.03%) | ✅ (view plot) | 67.00 (+2.66%) | 68.36 (98.01%) |
| client_sv2_open_channel_serialize | ✅ (view plot) | 14,284.00 (+0.34%) | 14,488.89 (98.59%) | ✅ (view plot) | 5,064.00 (+0.02%) | 5,077.62 (99.73%) | ✅ (view plot) | 7,319.00 (+0.02%) | 7,339.35 (99.72%) | ✅ (view plot) | 35.00 (-5.74%) | 41.76 (83.81%) | ✅ (view plot) | 194.00 (+0.86%) | 199.61 (97.19%) |
| client_sv2_open_channel_serialize_deserialize | ✅ (view plot) | 22,768.00 (+0.44%) | 23,085.13 (98.63%) | ✅ (view plot) | 7,987.00 (+0.05%) | 8,001.33 (99.82%) | ✅ (view plot) | 11,613.00 (+0.01%) | 11,636.26 (99.80%) | ✅ (view plot) | 75.00 (+2.32%) | 81.76 (91.73%) | ✅ (view plot) | 308.00 (+0.84%) | 316.55 (97.30%) |
| client_sv2_setup_connection | ✅ (view plot) | 4,737.00 (+0.73%) | 4,769.07 (99.33%) | ✅ (view plot) | 1,502.00 (+0.05%) | 1,515.62 (99.10%) | ✅ (view plot) | 2,277.00 (+0.01%) | 2,299.74 (99.01%) | ✅ (view plot) | 9.00 (-1.85%) | 13.30 (67.68%) | ✅ (view plot) | 69.00 (+1.47%) | 69.69 (99.01%) |
| client_sv2_setup_connection_serialize | ✅ (view plot) | 16,406.00 (+0.83%) | 16,478.97 (99.56%) | ✅ (view plot) | 5,963.00 (+0.01%) | 5,976.62 (99.77%) | ✅ (view plot) | 8,651.00 (-0.05%) | 8,677.60 (99.69%) | ✅ (view plot) | 46.00 (+3.00%) | 49.20 (93.50%) | ✅ (view plot) | 215.00 (+1.79%) | 217.04 (99.06%) |
| client_sv2_setup_connection_serialize_deserialize | ✅ (view plot) | 35,624.00 (+0.22%) | 35,773.09 (99.58%) | ✅ (view plot) | 14,814.00 (+0.03%) | 14,828.33 (99.90%) | ✅ (view plot) | 21,744.00 (-0.02%) | 21,771.84 (99.87%) | ✅ (view plot) | 109.00 (+8.35%) | 115.53 (94.35%) | ✅ (view plot) | 381.00 (+0.30%) | 384.71 (99.04%) |
Bencher - Continuous Benchmarking
View Public Perf Page
Docs | Repo | Chat | Help
|
@GitGab19 were you able to test this solution while reproducing the scenario that triggered the original bug? |
|
@plebhash the problem I was mentioning yesterday in the call is this one: the Basically without a DeclareMiningJobSuccess for future jobs, we never exit the loop, because we never add them into future_jobs through this line:
That's the way the two PRs are linked as I was mentioning yesterday. Suggestions? |
This PR addresses the case in which JDS receives an old ProvideMissingTransactionsSuccess message from JDC. It can happen if a new block is found too quickly, and so a new job is created by JDC, but after that it answer to previous ProvideMissingTransactions to JDS.
Fix #860