[patch + MG test] tproxy receiving old share #910

plebhash · 2024-05-16T00:55:35Z

Changelog:

added the actual fix to the #791 bug
adds MG test

Todo:

it seems the bugfix is breaking message-generator/test/translation-proxy/translation-proxy.json

github-actions · 2024-05-16T01:00:07Z

Bencher

Report	Wed, June 19, 2024 at 12:51:49 UTC
Project	Stratum v2 (SRI)
Branch	mg-test-tproxy-old-share
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,538.00 (+2.12%)	8,825.90 (96.74%)	✅ (view plot)	3,746.00 (+1.03%)	3,890.90 (96.28%)	✅ (view plot)	5,248.00 (+0.86%)	5,441.63 (96.44%)	✅ (view plot)	7.00 (-5.34%)	10.58 (66.18%)	✅ (view plot)	93.00 (+4.29%)	95.67 (97.21%)
get_submit	✅ (view plot)	95,509.00 (+0.11%)	96,255.42 (99.22%)	✅ (view plot)	59,439.00 (+0.02%)	59,753.54 (99.47%)	✅ (view plot)	85,359.00 (+0.01%)	85,787.17 (99.50%)	✅ (view plot)	49.00 (-8.66%)	62.77 (78.07%)	✅ (view plot)	283.00 (+1.21%)	292.70 (96.69%)
get_subscribe	✅ (view plot)	7,991.00 (+1.27%)	8,382.64 (95.33%)	✅ (view plot)	2,841.00 (+1.64%)	2,986.04 (95.14%)	✅ (view plot)	3,971.00 (+1.59%)	4,158.02 (95.50%)	✅ (view plot)	13.00 (-16.62%)	19.84 (65.51%)	✅ (view plot)	113.00 (+1.31%)	118.32 (95.51%)
serialize_authorize	✅ (view plot)	12,291.00 (+1.32%)	12,597.48 (97.57%)	✅ (view plot)	5,317.00 (+0.72%)	5,461.90 (97.35%)	✅ (view plot)	7,411.00 (+0.62%)	7,605.24 (97.45%)	✅ (view plot)	10.00 (-5.35%)	13.37 (74.80%)	✅ (view plot)	138.00 (+2.49%)	141.50 (97.53%)
serialize_deserialize_authorize	✅ (view plot)	24,588.00 (+0.67%)	24,764.09 (99.29%)	✅ (view plot)	9,898.00 (+0.28%)	10,046.62 (98.52%)	✅ (view plot)	13,958.00 (+0.21%)	14,166.26 (98.53%)	✅ (view plot)	33.00 (-10.33%)	41.98 (78.61%)	✅ (view plot)	299.00 (+1.49%)	299.04 (99.99%)
serialize_deserialize_handle_authorize	✅ (view plot)	30,327.00 (+0.84%)	30,528.61 (99.34%)	✅ (view plot)	12,101.00 (+0.32%)	12,245.90 (98.82%)	✅ (view plot)	17,117.00 (+0.25%)	17,316.67 (98.85%)	✅ (view plot)	59.00 (+0.54%)	63.90 (92.34%)	✅ (view plot)	369.00 (+1.66%)	370.19 (99.68%)
serialize_deserialize_handle_submit	✅ (view plot)	126,372.00 (+0.12%)	127,236.64 (99.32%)	✅ (view plot)	73,224.00 (+0.06%)	73,619.13 (99.46%)	✅ (view plot)	104,947.00 (+0.05%)	105,497.14 (99.48%)	✅ (view plot)	120.00 (+1.74%)	133.99 (89.56%)	✅ (view plot)	595.00 (+0.45%)	604.50 (98.43%)
serialize_deserialize_handle_subscribe	✅ (view plot)	27,491.00 (+0.41%)	27,782.80 (98.95%)	✅ (view plot)	9,643.00 (+0.48%)	9,788.04 (98.52%)	✅ (view plot)	13,641.00 (+0.47%)	13,831.65 (98.62%)	✅ (view plot)	61.00 (-6.95%)	73.02 (83.54%)	✅ (view plot)	387.00 (+0.53%)	390.36 (99.14%)
serialize_deserialize_submit	✅ (view plot)	114,999.00 (+0.03%)	115,675.63 (99.42%)	✅ (view plot)	68,001.00 (-0.03%)	68,352.72 (99.49%)	✅ (view plot)	97,559.00 (-0.05%)	98,067.85 (99.48%)	✅ (view plot)	65.00 (-5.62%)	74.74 (86.96%)	✅ (view plot)	489.00 (+0.61%)	496.60 (98.47%)
serialize_deserialize_subscribe	✅ (view plot)	22,922.00 (+0.48%)	23,194.56 (98.82%)	✅ (view plot)	8,195.00 (+0.53%)	8,340.01 (98.26%)	✅ (view plot)	11,542.00 (+0.50%)	11,735.35 (98.35%)	✅ (view plot)	36.00 (-8.49%)	43.87 (82.06%)	✅ (view plot)	320.00 (+0.62%)	322.35 (99.27%)
serialize_submit	✅ (view plot)	99,826.00 (+0.08%)	100,567.20 (99.26%)	✅ (view plot)	61,483.00 (+0.02%)	61,801.45 (99.48%)	✅ (view plot)	88,206.00 (+0.01%)	88,640.06 (99.51%)	✅ (view plot)	49.00 (-10.80%)	62.11 (78.90%)	✅ (view plot)	325.00 (+0.84%)	334.40 (97.19%)
serialize_subscribe	✅ (view plot)	11,328.00 (+0.74%)	11,707.51 (96.76%)	✅ (view plot)	4,188.00 (+1.11%)	4,333.04 (96.65%)	✅ (view plot)	5,828.00 (+1.07%)	6,017.90 (96.84%)	✅ (view plot)	15.00 (-7.24%)	18.87 (79.49%)	✅ (view plot)	155.00 (+0.50%)	160.59 (96.52%)

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

github-actions · 2024-05-16T01:00:08Z

Bencher

Report	Wed, June 19, 2024 at 12:51:50 UTC
Project	Stratum v2 (SRI)
Branch	mg-test-tproxy-old-share
Testbed	sv2

🚨 10 ALERTS: Threshold Boundary Limits exceeded!

Benchmark	Measure (units)	View	Value	Upper Boundary
client_sv2_mining_message_submit_standard	L2 Accesses (accesses)	🚨 (view plot \| view alert)	26.00 (+44.34%)	24.72 (105.19%)
client_sv2_mining_message_submit_standard_serialize	L2 Accesses (accesses)	🚨 (view plot \| view alert)	56.00 (+17.78%)	54.15 (103.41%)
client_sv2_mining_message_submit_standard_serialize_deserialize	Instructions (instructions)	🚨 (view plot \| view alert)	10,585.00 (+0.37%)	10,574.01 (100.10%)
client_sv2_mining_message_submit_standard_serialize_deserialize	L1 Accesses (accesses)	🚨 (view plot \| view alert)	15,399.00 (+0.36%)	15,382.69 (100.11%)
client_sv2_open_channel_serialize_deserialize	Instructions (instructions)	🚨 (view plot \| view alert)	8,027.00 (+0.49%)	8,016.32 (100.13%)
client_sv2_open_channel_serialize_deserialize	L1 Accesses (accesses)	🚨 (view plot \| view alert)	11,672.00 (+0.46%)	11,656.76 (100.13%)
client_sv2_open_channel_serialize_deserialize	L2 Accesses (accesses)	🚨 (view plot \| view alert)	84.00 (+12.60%)	83.97 (100.03%)
client_sv2_setup_connection	L2 Accesses (accesses)	🚨 (view plot \| view alert)	15.00 (+59.80%)	14.33 (104.66%)
client_sv2_setup_connection_serialize_deserialize	Instructions (instructions)	🚨 (view plot \| view alert)	14,855.00 (+0.27%)	14,843.88 (100.07%)
client_sv2_setup_connection_serialize_deserialize	L1 Accesses (accesses)	🚨 (view plot \| view alert)	21,811.00 (+0.26%)	21,794.12 (100.08%)

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,077.00 (+0.65%)	2,154.92 (96.38%)	✅ (view plot)	473.00 (+0.35%)	484.74 (97.58%)	✅ (view plot)	732.00 (-0.05%)	751.97 (97.34%)	✅ (view plot)	10.00 (+36.36%)	11.14 (89.77%)	✅ (view plot)	37.00 (+0.04%)	39.31 (94.11%)
client_sv2_handle_message_mining	✅ (view plot)	8,253.00 (+0.57%)	8,329.97 (99.08%)	✅ (view plot)	2,137.00 (+0.33%)	2,168.26 (98.56%)	✅ (view plot)	3,158.00 (+0.28%)	3,211.16 (98.34%)	✅ (view plot)	39.00 (+2.34%)	42.92 (90.87%)	✅ (view plot)	140.00 (+0.69%)	141.80 (98.73%)
client_sv2_mining_message_submit_standard	✅ (view plot)	6,316.00 (+0.57%)	6,385.85 (98.91%)	✅ (view plot)	1,750.00 (+0.02%)	1,761.18 (99.37%)	✅ (view plot)	2,546.00 (-0.29%)	2,572.78 (98.96%)	🚨 (view plot \| view alert)	26.00 (+44.34%)	24.72 (105.19%)	✅ (view plot)	104.00 (+0.09%)	106.77 (97.41%)
client_sv2_mining_message_submit_standard_serialize	✅ (view plot)	14,695.00 (-0.57%)	15,012.49 (97.89%)	✅ (view plot)	4,694.00 (+0.01%)	4,705.18 (99.76%)	✅ (view plot)	6,750.00 (-0.06%)	6,772.18 (99.67%)	🚨 (view plot \| view alert)	56.00 (+17.78%)	54.15 (103.41%)	✅ (view plot)	219.00 (-1.58%)	229.49 (95.43%)
client_sv2_mining_message_submit_standard_serialize_deserialize	✅ (view plot)	27,499.00 (+0.04%)	27,804.11 (98.90%)	🚨 (view plot \| view alert)	10,585.00 (+0.37%)	10,574.01 (100.10%)	🚨 (view plot \| view alert)	15,399.00 (+0.36%)	15,382.69 (100.11%)	✅ (view plot)	89.00 (+5.43%)	90.35 (98.51%)	✅ (view plot)	333.00 (-0.58%)	344.04 (96.79%)
client_sv2_open_channel	✅ (view plot)	4,417.00 (-1.87%)	4,629.33 (95.41%)	✅ (view plot)	1,461.00 (+0.04%)	1,472.46 (99.22%)	✅ (view plot)	2,157.00 (+0.20%)	2,169.92 (99.40%)	✅ (view plot)	11.00 (-9.74%)	15.22 (72.30%)	✅ (view plot)	63.00 (-3.61%)	68.75 (91.64%)
client_sv2_open_channel_serialize	✅ (view plot)	14,006.00 (-1.47%)	14,457.88 (96.87%)	✅ (view plot)	5,064.00 (+0.01%)	5,075.46 (99.77%)	✅ (view plot)	7,321.00 (+0.04%)	7,336.13 (99.79%)	✅ (view plot)	42.00 (+11.74%)	42.53 (98.74%)	✅ (view plot)	185.00 (-3.49%)	198.94 (92.99%)
client_sv2_open_channel_serialize_deserialize	✅ (view plot)	22,557.00 (-0.42%)	22,990.75 (98.11%)	🚨 (view plot \| view alert)	8,027.00 (+0.49%)	8,016.32 (100.13%)	🚨 (view plot \| view alert)	11,672.00 (+0.46%)	11,656.76 (100.13%)	🚨 (view plot \| view alert)	84.00 (+12.60%)	83.97 (100.03%)	✅ (view plot)	299.00 (-1.84%)	314.24 (95.15%)
client_sv2_setup_connection	✅ (view plot)	4,659.00 (-0.92%)	4,780.76 (97.45%)	✅ (view plot)	1,502.00 (+0.04%)	1,513.46 (99.24%)	✅ (view plot)	2,274.00 (-0.12%)	2,296.25 (99.03%)	🚨 (view plot \| view alert)	15.00 (+59.80%)	14.33 (104.66%)	✅ (view plot)	66.00 (-2.88%)	70.26 (93.94%)
client_sv2_setup_connection_serialize	✅ (view plot)	15,942.00 (-2.02%)	16,553.92 (96.30%)	✅ (view plot)	5,963.00 (+0.01%)	5,974.46 (99.81%)	✅ (view plot)	8,662.00 (+0.08%)	8,674.39 (99.86%)	✅ (view plot)	49.00 (+8.47%)	49.62 (98.76%)	✅ (view plot)	201.00 (-4.79%)	219.51 (91.57%)
client_sv2_setup_connection_serialize_deserialize	✅ (view plot)	35,416.00 (-0.38%)	35,771.55 (99.01%)	🚨 (view plot \| view alert)	14,855.00 (+0.27%)	14,843.88 (100.07%)	🚨 (view plot \| view alert)	21,811.00 (+0.26%)	21,794.12 (100.08%)	✅ (view plot)	110.00 (+8.30%)	114.53 (96.04%)	✅ (view plot)	373.00 (-1.76%)	385.91 (96.65%)

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

github-actions · 2024-05-16T01:00:11Z

Bencher

Report	Wed, June 19, 2024 at 12:51:53 UTC
Project	Stratum v2 (SRI)
Branch	910/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)	6,536.00 (-4.48%)	7,321.49 (89.27%)
client-submit-serialize-deserialize	✅ (view plot)	7,387.60 (-4.78%)	8,294.82 (89.06%)
client-submit-serialize-deserialize-handle/client-submit-serialize-deserialize-handle	✅ (view plot)	7,968.40 (-4.49%)	8,803.99 (90.51%)
client-sv1-authorize-serialize-deserialize-handle/client-sv1-authorize-serialize-deserialize-handle	✅ (view plot)	910.65 (+1.01%)	939.34 (96.95%)
client-sv1-authorize-serialize-deserialize/client-sv1-authorize-serialize-deserialize	✅ (view plot)	695.09 (-0.71%)	724.40 (95.95%)
client-sv1-authorize-serialize/client-sv1-authorize-serialize	✅ (view plot)	248.65 (-0.46%)	264.23 (94.10%)
client-sv1-get-authorize/client-sv1-get-authorize	✅ (view plot)	160.62 (+1.76%)	164.38 (97.71%)
client-sv1-get-submit	✅ (view plot)	6,319.70 (-4.64%)	7,092.84 (89.10%)
client-sv1-get-subscribe/client-sv1-get-subscribe	✅ (view plot)	285.29 (+3.72%)	295.00 (96.71%)
client-sv1-subscribe-serialize-deserialize-handle/client-sv1-subscribe-serialize-deserialize-handle	✅ (view plot)	752.70 (-0.05%)	786.69 (95.68%)
client-sv1-subscribe-serialize-deserialize/client-sv1-subscribe-serialize-deserialize	✅ (view plot)	620.66 (+0.82%)	638.79 (97.16%)
client-sv1-subscribe-serialize/client-sv1-subscribe-serialize	✅ (view plot)	204.64 (-1.01%)	218.10 (93.83%)

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

github-actions · 2024-05-16T01:00:11Z

Bencher

Report	Wed, June 19, 2024 at 12:51:49 UTC
Project	Stratum v2 (SRI)
Branch	mg-test-tproxy-old-share
Testbed	sv2

Click to view all benchmark results

Benchmark	Latency	Latency Results nanoseconds (ns) \| (Δ%)	Latency Upper Boundary nanoseconds (ns) \| (%)
client_sv2_handle_message_common	✅ (view plot)	44.93 (+0.77%)	45.22 (99.35%)
client_sv2_handle_message_mining	✅ (view plot)	74.56 (+0.95%)	82.09 (90.82%)
client_sv2_mining_message_submit_standard	✅ (view plot)	14.65 (-0.05%)	14.70 (99.62%)
client_sv2_mining_message_submit_standard_serialize	✅ (view plot)	267.28 (+1.00%)	283.03 (94.44%)
client_sv2_mining_message_submit_standard_serialize_deserialize	✅ (view plot)	588.39 (-0.92%)	625.21 (94.11%)
client_sv2_open_channel	✅ (view plot)	163.63 (-1.55%)	173.77 (94.16%)
client_sv2_open_channel_serialize	✅ (view plot)	278.67 (-1.64%)	293.82 (94.85%)
client_sv2_open_channel_serialize_deserialize	✅ (view plot)	379.93 (+0.59%)	416.42 (91.24%)
client_sv2_setup_connection	✅ (view plot)	163.55 (-0.28%)	173.99 (94.00%)
client_sv2_setup_connection_serialize	✅ (view plot)	452.51 (-4.56%)	503.76 (89.83%)
client_sv2_setup_connection_serialize_deserialize	✅ (view plot)	956.55 (-1.23%)	1,034.76 (92.44%)

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

lorbax · 2024-06-01T17:36:17Z

look at how are the other tests. There is a directory with the name of the test, that contains the json tests and a script (that in your case will be translation-proxy-old-share.sh) which precompiles the binaries used by the test and the test itself. This is done in order to launch the test from the root directory of the project. Recall also to add the

      - name: Run translation-proxy
        run: sh ./test/message-generator/test/translation-proxy/translation-proxy-old-share.sh

here
https://github.com/plebhash/stratum/blob/mg-test-tproxy-old-share/.github/workflows/coverage.yaml#L123

I would personally specify in the test (or wherever it is clear to see) that the test passes because there is a bug, and have to be changed whenever the bug is fixed.

lorbax

It must be added the machinery needed to run this new test among the others during the workflow of github actions.
Other two minor fixes should be addressed.

Apart from this, looks good to me.

test/message-generator/test/translation-proxy-old-share.json

plebhash · 2024-06-02T17:58:20Z

@lorbax I made the changes from your feedback, plus the actual bugfix (described on the PR header)

protocols/v2/roles-logic-sv2/src/channel_logic/channel_factory.rs

plebhash · 2024-06-17T14:12:42Z

@lorbax can you approve the PR if you feel it's ok?

plebhash · 2024-06-18T22:20:59Z

it seems the bugfix is breaking message-generator/test/translation-proxy/translation-proxy.json

reverting to draft, since this needs to be fixed before the PR is merged

lorbax · 2024-06-19T07:23:55Z

it seems the bugfix is breaking message-generator/test/translation-proxy/translation-proxy.json

reverting to draft, since this needs to be fixed before the PR is merged

actually I think that your test is correct. Every time we launched translation_proxy.json MG test was kind of a false positive. This is due basically to the fact that we are using the sv1-mining-device test utils. This mining device is partially developed, and since the TProxy accepted the shares with invalid ID, presumably this mining device is sending only invalid shares.
I see the output of the error

which tells us that expected a SubmitSharesExtended and instead received an UpdateChannel. Clearly, since the TProxy is not receiving any valid share from the miner, SubmitSharesExtended will never be sent. On the other hand, it can send UpdateChannel to the upstream, and this behaviour is perfectly fine.
A list of things that we can do:

(the worst) change TM test action to await the UpdateChannel message, regardless of the invalid shares produced by the miner to the proxy. This is a rapid fix, but doing so we are actually giving up some interoperability. It may be viable for the purpose of this PR, given that we open an issue to fix the miner and solve it with a bit of prio.
include the cpu miner binary in the repo and use that instead. It is a solution that works, but in general I am against the practice of including binaries in a repo.
fix the sv1-mining-device within this PR, but I actually don't know how much time is needed (I should look at the mining-device code)

Give me your impressions about it, I can help with it.

plebhash · 2024-06-19T10:58:44Z

it seems the bugfix is breaking message-generator/test/translation-proxy/translation-proxy.json
reverting to draft, since this needs to be fixed before the PR is merged

actually I think that your test is correct. Every time we launched translation_proxy.json MG test was kind of a false positive. This is due basically to the fact that we are using the sv1-mining-device test utils. This mining device is partially developed, and since the TProxy accepted the shares with invalid ID, presumably this mining device is sending only invalid shares. I see the output of the error which tells us that expected a SubmitSharesExtended and instead received an UpdateChannel. Clearly, since the TProxy is not receiving any valid share from the miner, SubmitSharesExtended will never be sent. On the other hand, it can send UpdateChannel to the upstream, and this behaviour is perfectly fine. A list of things that we can do:

(the worst) change TM test action to await the UpdateChannel message, regardless of the invalid shares produced by the miner to the proxy. This is a rapid fix, but doing so we are actually giving up some interoperability. It may be viable for the purpose of this PR, given that we open an issue to fix the miner and solve it with a bit of prio.

include the cpu miner binary in the repo and use that instead. It is a solution that works, but in general I am against the practice of including binaries in a repo.

fix the sv1-mining-device within this PR, but I actually don't know how much time is needed (I should look at the mining-device code)

Give me your impressions about it, I can help with it.

that is a very comprehensive outline, thanks for the info!

I think the best approach is to fix sv1-mining-device. I found a bug:

stratum/roles/test-utils/sv1-mining-device/src/job.rs

Lines 25 to 31 in 7841021

    
           impl Job { 
        
               pub fn from_notify(notify_msg: server_to_client::Notify<'_>, extranonce: Vec<u8>) -> Self { 
        
                   // TODO: Hard coded for demo. Should be properly translated from received Notify message 
        
                   // Right now, Notify.job_id is a string, but the Job.job_id is a u32 here. 
        
                   let job_id = 1u32;

I will fix it on this same PR.

cc @rrybarczyk just so you're aware as seems you have some git history on this area of the codebase

plebhash · 2024-06-19T11:25:33Z

@lorbax I fixed the bug described on my previous message via 61c5726

however, translation-proxy.sh kept breaking at WRONG MESSAGE TYPE expected: 27 received: 22

That seems to come from the fact that shares are being rejected by tProxy, as we can verify on these logs:

STD OUT: 2024-06-19T11:55:52.361642Z DEBUG translator_sv2::lib::downstream_sv1::downstream: Down: Handling mining.submit: Submit { user_name: "user", job_id: "1", extra_nonce2: Extranonce(Owned([0, 0, 0, 0, 0, 0, 0, 0])), time: HexU32Be(1718798212), nonce: HexU32Be(3089), version_bits: None, id: 0 }
STD OUT: 2024-06-19T11:55:52.361647Z DEBUG translator_sv2::lib::downstream_sv1::downstream: To DOWN: OkResponse(Response { id: 0, error: None, result: Bool(false) })
STD OUT: 2024-06-19T11:55:52.361665Z DEBUG translator_sv2::lib::downstream_sv1::downstream: Sending to Mining Device: 127.0.0.1:44818 - "{\"id\":0,\"error\":null,\"result\":false}\n"

a false result on the context of mining.submit means the share was rejected, and the patch introduced by this PR is changing the logic on share rejection.

I tweaked test/config/tproxy-config-no-jd-sv1-cpu-md.toml to set a smaller min_individual_miner_hashrate via aadd202

The goal was to avoid tProxy rejecting shares. The test seems stable now.

lorbax · 2024-06-20T13:33:46Z

@lorbax I fixed the bug described on my previous message via 61c5726

however, translation-proxy.sh kept breaking at WRONG MESSAGE TYPE expected: 27 received: 22

That seems to come from the fact that shares are being rejected by tProxy, as we can verify on these logs:
STD OUT: 2024-06-19T11:55:52.361642Z DEBUG translator_sv2::lib::downstream_sv1::downstream: Down: Handling mining.submit: Submit { user_name: "user", job_id: "1", extra_nonce2: Extranonce(Owned([0, 0, 0, 0, 0, 0, 0, 0])), time: HexU32Be(1718798212), nonce: HexU32Be(3089), version_bits: None, id: 0 }
STD OUT: 2024-06-19T11:55:52.361647Z DEBUG translator_sv2::lib::downstream_sv1::downstream: To DOWN: OkResponse(Response { id: 0, error: None, result: Bool(false) })
STD OUT: 2024-06-19T11:55:52.361665Z DEBUG translator_sv2::lib::downstream_sv1::downstream: Sending to Mining Device: 127.0.0.1:44818 - "{\"id\":0,\"error\":null,\"result\":false}\n"
a false result on the context of mining.submit means the share was rejected, and the patch introduced by this PR is changing the logic on share rejection.

I tweaked test/config/tproxy-config-no-jd-sv1-cpu-md.toml to set a smaller min_individual_miner_hashrate via aadd202

The goal was to avoid tProxy rejecting shares. The test seems stable now.

I think that what you've done is to make the t-proxy make a lot more shares that it is supposed to do and consequentially provide a share to the upstream before UpdateChannel is sent. Makes sense.

jbesraa · 2024-06-20T13:44:34Z

@lorbax @plebhash Could we please make the commit history nicer by not pushing "fmt" or other similar commits into dev/main branches?

jbesraa · 2024-06-20T13:48:45Z

I would revert this PR TBH, clear the git history and also get two approvals. First approval was not reconfirmed after additional code changes.

plebhash · 2024-06-20T14:02:59Z

@lorbax @plebhash Could we please make the commit history nicer by not pushing "fmt" or other similar commits into dev/main branches?

sure, I think this is a good suggestion

I would revert this PR TBH, clear the git history and also get two approvals. First approval was not reconfirmed after additional code changes.

that wouldn't be simple, because we would need to force-push to dev. I don't have rights for that.

we can always do a PR that reverts each commit individually, but I think that would miss the point

I think the most important action would be to establish this as a development policy and make sure all core devs are aligned, and that would help messing up commit history going forward

feel free to send PRs suggesting new conventions, perhaps this could fit RELEASE.md

lorbax · 2024-06-20T14:03:51Z

I would revert this PR TBH, clear the git history and also get two approvals. First approval was not reconfirmed after additional code changes.

Your points are good. We can discuss it, I was agree with the author in merging it, but we should have found another reviewer!

plebhash mentioned this pull request May 16, 2024

invalid-job-id errors on tProxy don't result in rejection of shares #791

Closed

plebhash changed the title ~~MG test tproxy old share~~ MG test tproxy receiving old share May 16, 2024

pavlenex added this to the 1.0.2 milestone May 20, 2024

plebhash force-pushed the mg-test-tproxy-old-share branch from bb9c643 to bdd804e Compare May 21, 2024 18:28

pavlenex assigned plebhash May 28, 2024

lorbax requested changes Jun 1, 2024

View reviewed changes

test/message-generator/test/translation-proxy-old-share.json Outdated Show resolved Hide resolved

plebhash force-pushed the mg-test-tproxy-old-share branch from 78749a0 to c638d52 Compare June 2, 2024 16:51

plebhash marked this pull request as draft June 2, 2024 16:51

plebhash marked this pull request as ready for review June 2, 2024 17:53

plebhash force-pushed the mg-test-tproxy-old-share branch from a29e603 to 3b4484f Compare June 2, 2024 18:30

plebhash requested a review from lorbax June 2, 2024 18:39

xyephy reviewed Jun 2, 2024

View reviewed changes

protocols/v2/roles-logic-sv2/src/channel_logic/channel_factory.rs Outdated Show resolved Hide resolved

plebhash force-pushed the mg-test-tproxy-old-share branch from 919d29d to 1bb0997 Compare June 2, 2024 19:45

xyephy approved these changes Jun 3, 2024

View reviewed changes

plebhash changed the title ~~MG test tproxy receiving old share~~ [patch + MG test] tproxy receiving old share Jun 5, 2024

plebhash added 8 commits June 17, 2024 20:01

add test for tProxy receiving a share with old job_id

66905c1

fix rejection of old sv1 shares

430d20b

follow new mg template + fix job_id on sv1_messages.mining.submit

6838d2a

fix test doc

af85517

add translation-proxy-old-share to Github Actions

7898209

fmt

950d4a7

avoid sending old share upstream

f0bae0b

revert clippy changes

d5515c5

fmt

d649b0a

lorbax force-pushed the mg-test-tproxy-old-share branch from 2f5a2a1 to d649b0a Compare June 17, 2024 18:01

lorbax approved these changes Jun 17, 2024

View reviewed changes

plebhash marked this pull request as draft June 18, 2024 22:21

fix sv1-mining-device handle_notify manipulation of job_id

61c5726

plebhash added 5 commits June 19, 2024 09:23

update last_job_id if is_a && && last_notify.is_some()

e72da86

reduce hashrate for tproxy-config-no-jd-sv1-cpu-md.toml

aadd202

fmt

6b6938e

clippy

5183938

fmt

1ce6283

plebhash marked this pull request as ready for review June 19, 2024 15:12

lorbax merged commit 61b3959 into stratum-mining:dev Jun 20, 2024
13 checks passed

plebhash deleted the mg-test-tproxy-old-share branch June 20, 2024 13:43

plebhash mentioned this pull request Jun 24, 2024

fix translation-proxy-old-share.sh permission #1000

Merged

plebhash mentioned this pull request Aug 18, 2024

Release 1.0.2 #1112

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[patch + MG test] tproxy receiving old share #910

[patch + MG test] tproxy receiving old share #910

plebhash commented May 16, 2024 •

edited

Loading

github-actions bot commented May 16, 2024 •

edited

Loading

github-actions bot commented May 16, 2024 •

edited

Loading

github-actions bot commented May 16, 2024 •

edited

Loading

github-actions bot commented May 16, 2024 •

edited

Loading

lorbax commented Jun 1, 2024

lorbax left a comment •

edited

Loading

plebhash commented Jun 2, 2024 •

edited

Loading

plebhash commented Jun 17, 2024

plebhash commented Jun 18, 2024

lorbax commented Jun 19, 2024

plebhash commented Jun 19, 2024 •

edited

Loading

plebhash commented Jun 19, 2024 •

edited

Loading

lorbax commented Jun 20, 2024

jbesraa commented Jun 20, 2024

jbesraa commented Jun 20, 2024 •

edited

Loading

plebhash commented Jun 20, 2024 •

edited

Loading

lorbax commented Jun 20, 2024

[patch + MG test] tproxy receiving old share #910

[patch + MG test] tproxy receiving old share #910

Conversation

plebhash commented May 16, 2024 • edited Loading

github-actions bot commented May 16, 2024 • edited Loading

Bencher

github-actions bot commented May 16, 2024 • edited Loading

Bencher

github-actions bot commented May 16, 2024 • edited Loading

Bencher

github-actions bot commented May 16, 2024 • edited Loading

Bencher

lorbax commented Jun 1, 2024

lorbax left a comment • edited Loading

Choose a reason for hiding this comment

plebhash commented Jun 2, 2024 • edited Loading

plebhash commented Jun 17, 2024

plebhash commented Jun 18, 2024

lorbax commented Jun 19, 2024

plebhash commented Jun 19, 2024 • edited Loading

plebhash commented Jun 19, 2024 • edited Loading

lorbax commented Jun 20, 2024

jbesraa commented Jun 20, 2024

jbesraa commented Jun 20, 2024 • edited Loading

plebhash commented Jun 20, 2024 • edited Loading

lorbax commented Jun 20, 2024

plebhash commented May 16, 2024 •

edited

Loading

github-actions bot commented May 16, 2024 •

edited

Loading

github-actions bot commented May 16, 2024 •

edited

Loading

github-actions bot commented May 16, 2024 •

edited

Loading

github-actions bot commented May 16, 2024 •

edited

Loading

lorbax left a comment •

edited

Loading

plebhash commented Jun 2, 2024 •

edited

Loading

plebhash commented Jun 19, 2024 •

edited

Loading

plebhash commented Jun 19, 2024 •

edited

Loading

jbesraa commented Jun 20, 2024 •

edited

Loading

plebhash commented Jun 20, 2024 •

edited

Loading