Enhance SetupConnection Handling and Implement Job declaration Flag Checks #1035

Shourya742 · 2024-07-05T11:42:15Z

This PR closes #853:

Completed MG Test.
Add flag check during SetupConnection call.

github-actions · 2024-07-05T11:46:23Z

Bencher

Report	Mon, July 29, 2024 at 21:25:09 UTC
Project	Stratum v2 (SRI)
Branch	feature/asyncFlagSet
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,111.00 (+2.51%)	2,138.26 (98.72%)	✅ (view plot)	473.00 (+0.20%)	489.12 (96.71%)	✅ (view plot)	731.00 (-0.29%)	757.72 (96.47%)	✅ (view plot)	10.00 (+28.11%)	12.58 (79.48%)	✅ (view plot)	38.00 (+3.32%)	38.60 (98.45%)
client_sv2_handle_message_mining	✅ (view plot)	8,219.00 (+0.20%)	8,324.56 (98.73%)	✅ (view plot)	2,137.00 (+0.34%)	2,168.66 (98.54%)	✅ (view plot)	3,159.00 (+0.34%)	3,211.04 (98.38%)	✅ (view plot)	39.00 (+0.79%)	43.11 (90.46%)	✅ (view plot)	139.00 (+0.08%)	141.58 (98.18%)
client_sv2_mining_message_submit_standard	✅ (view plot)	6,316.00 (+0.61%)	6,379.47 (99.01%)	✅ (view plot)	1,750.00 (-0.02%)	1,765.70 (99.11%)	✅ (view plot)	2,546.00 (-0.28%)	2,576.86 (98.80%)	✅ (view plot)	26.00 (+31.74%)	28.78 (90.36%)	✅ (view plot)	104.00 (+0.39%)	106.49 (97.66%)
client_sv2_mining_message_submit_standard_serialize	✅ (view plot)	14,657.00 (-0.55%)	14,997.37 (97.73%)	✅ (view plot)	4,694.00 (-0.01%)	4,709.70 (99.67%)	✅ (view plot)	6,752.00 (-0.05%)	6,777.78 (99.62%)	✅ (view plot)	55.00 (+11.49%)	57.70 (95.32%)	✅ (view plot)	218.00 (-1.38%)	229.15 (95.13%)
client_sv2_mining_message_submit_standard_serialize_deserialize	✅ (view plot)	27,469.00 (-0.00%)	27,787.71 (98.85%)	✅ (view plot)	10,585.00 (+0.29%)	10,601.08 (99.85%)	✅ (view plot)	15,399.00 (+0.27%)	15,422.25 (99.85%)	✅ (view plot)	90.00 (+5.45%)	92.93 (96.84%)	✅ (view plot)	332.00 (-0.57%)	343.53 (96.64%)
client_sv2_open_channel	✅ (view plot)	4,451.00 (-0.50%)	4,597.88 (96.81%)	✅ (view plot)	1,461.00 (-0.01%)	1,476.92 (98.92%)	✅ (view plot)	2,156.00 (+0.05%)	2,178.55 (98.97%)	✅ (view plot)	11.00 (-6.49%)	14.93 (73.67%)	✅ (view plot)	64.00 (-0.86%)	67.95 (94.18%)
client_sv2_open_channel_serialize	✅ (view plot)	14,040.00 (-0.87%)	14,447.66 (97.18%)	✅ (view plot)	5,064.00 (-0.00%)	5,079.92 (99.69%)	✅ (view plot)	7,320.00 (+0.01%)	7,343.80 (99.68%)	✅ (view plot)	42.00 (+8.74%)	45.15 (93.02%)	✅ (view plot)	186.00 (-2.11%)	198.83 (93.55%)
client_sv2_open_channel_serialize_deserialize	✅ (view plot)	22,557.00 (-0.25%)	22,958.48 (98.25%)	✅ (view plot)	8,027.00 (+0.39%)	8,043.38 (99.80%)	✅ (view plot)	11,672.00 (+0.35%)	11,695.09 (99.80%)	✅ (view plot)	84.00 (+10.30%)	88.58 (94.83%)	✅ (view plot)	299.00 (-1.29%)	313.63 (95.34%)
client_sv2_setup_connection	✅ (view plot)	4,693.00 (+0.06%)	4,758.93 (98.61%)	✅ (view plot)	1,502.00 (-0.01%)	1,517.92 (98.95%)	✅ (view plot)	2,273.00 (-0.18%)	2,301.36 (98.77%)	✅ (view plot)	15.00 (+41.18%)	17.39 (86.25%)	✅ (view plot)	67.00 (-0.64%)	69.71 (96.11%)
client_sv2_setup_connection_serialize	✅ (view plot)	15,976.00 (-1.27%)	16,535.51 (96.62%)	✅ (view plot)	5,963.00 (-0.00%)	5,978.92 (99.73%)	✅ (view plot)	8,661.00 (+0.03%)	8,683.92 (99.74%)	✅ (view plot)	49.00 (+6.91%)	51.62 (94.93%)	✅ (view plot)	202.00 (-3.07%)	219.29 (92.12%)
client_sv2_setup_connection_serialize_deserialize	✅ (view plot)	35,416.00 (-0.23%)	35,723.55 (99.14%)	✅ (view plot)	14,855.00 (+0.21%)	14,871.52 (99.89%)	✅ (view plot)	21,811.00 (+0.20%)	21,834.27 (99.89%)	✅ (view plot)	110.00 (+7.45%)	117.21 (93.85%)	✅ (view plot)	373.00 (-1.23%)	385.70 (96.71%)

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

github-actions · 2024-07-05T11:46:23Z

Bencher

Report	Mon, July 29, 2024 at 21:25:13 UTC
Project	Stratum v2 (SRI)
Branch	feature/asyncFlagSet
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.66 (+0.20%)	45.19 (98.83%)
client_sv2_handle_message_mining	✅ (view plot)	73.33 (+0.41%)	79.63 (92.09%)
client_sv2_mining_message_submit_standard	✅ (view plot)	14.65 (+0.03%)	14.74 (99.44%)
client_sv2_mining_message_submit_standard_serialize	✅ (view plot)	247.70 (-6.06%)	283.56 (87.35%)
client_sv2_mining_message_submit_standard_serialize_deserialize	✅ (view plot)	586.15 (-1.42%)	625.88 (93.65%)
client_sv2_open_channel	✅ (view plot)	163.46 (-1.58%)	173.09 (94.44%)
client_sv2_open_channel_serialize	✅ (view plot)	271.77 (-3.58%)	297.37 (91.39%)
client_sv2_open_channel_serialize_deserialize	✅ (view plot)	381.67 (+1.13%)	417.08 (91.51%)
client_sv2_setup_connection	✅ (view plot)	159.20 (-2.61%)	173.71 (91.65%)
client_sv2_setup_connection_serialize	✅ (view plot)	452.22 (-3.55%)	506.68 (89.25%)
client_sv2_setup_connection_serialize_deserialize	✅ (view plot)	958.18 (-1.85%)	1,048.76 (91.36%)

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

github-actions · 2024-07-05T11:46:24Z

Bencher

Report	Mon, July 29, 2024 at 21:25:08 UTC
Project	Stratum v2 (SRI)
Branch	1035/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,401.20 (-5.55%)	7,349.90 (87.09%)
client-submit-serialize-deserialize	✅ (view plot)	7,481.20 (-2.84%)	8,297.36 (90.16%)
client-submit-serialize-deserialize-handle/client-submit-serialize-deserialize-handle	✅ (view plot)	8,023.90 (-3.01%)	8,835.62 (90.81%)
client-sv1-authorize-serialize-deserialize-handle/client-sv1-authorize-serialize-deserialize-handle	✅ (view plot)	922.29 (+2.48%)	928.30 (99.35%)
client-sv1-authorize-serialize-deserialize/client-sv1-authorize-serialize-deserialize	✅ (view plot)	692.81 (-0.78%)	718.33 (96.45%)
client-sv1-authorize-serialize/client-sv1-authorize-serialize	✅ (view plot)	245.49 (-1.08%)	255.72 (96.00%)
client-sv1-get-authorize/client-sv1-get-authorize	✅ (view plot)	156.16 (-0.58%)	161.81 (96.51%)
client-sv1-get-submit	✅ (view plot)	6,244.20 (-4.77%)	7,112.03 (87.80%)
client-sv1-get-subscribe/client-sv1-get-subscribe	✅ (view plot)	281.99 (+1.03%)	291.08 (96.88%)
client-sv1-subscribe-serialize-deserialize-handle/client-sv1-subscribe-serialize-deserialize-handle	✅ (view plot)	763.10 (+1.12%)	791.51 (96.41%)
client-sv1-subscribe-serialize-deserialize/client-sv1-subscribe-serialize-deserialize	✅ (view plot)	610.13 (-0.62%)	637.21 (95.75%)
client-sv1-subscribe-serialize/client-sv1-subscribe-serialize	✅ (view plot)	204.28 (-1.04%)	222.74 (91.71%)

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

github-actions · 2024-07-05T11:46:24Z

Bencher

Report	Mon, July 29, 2024 at 21:25:10 UTC
Project	Stratum v2 (SRI)
Branch	feature/asyncFlagSet
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 (+0.98%)	8,714.85 (97.97%)	✅ (view plot)	3,746.00 (+0.09%)	3,842.75 (97.48%)	✅ (view plot)	5,248.00 (+0.02%)	5,383.58 (97.48%)	✅ (view plot)	7.00 (-8.75%)	10.26 (68.20%)	✅ (view plot)	93.00 (+2.69%)	94.52 (98.39%)
get_submit	✅ (view plot)	95,509.00 (-0.03%)	96,049.49 (99.44%)	✅ (view plot)	59,439.00 (-0.04%)	59,731.59 (99.51%)	✅ (view plot)	85,359.00 (-0.04%)	85,770.59 (99.52%)	✅ (view plot)	49.00 (-8.52%)	62.21 (78.77%)	✅ (view plot)	283.00 (+0.32%)	286.98 (98.61%)
get_subscribe	✅ (view plot)	7,991.00 (+0.21%)	8,228.90 (97.11%)	✅ (view plot)	2,841.00 (+0.27%)	2,929.79 (96.97%)	✅ (view plot)	3,971.00 (+0.29%)	4,087.92 (97.14%)	✅ (view plot)	13.00 (-15.34%)	19.85 (65.50%)	✅ (view plot)	113.00 (+0.43%)	116.28 (97.18%)
serialize_authorize	✅ (view plot)	12,291.00 (+0.57%)	12,483.95 (98.45%)	✅ (view plot)	5,317.00 (+0.06%)	5,413.75 (98.21%)	✅ (view plot)	7,411.00 (+0.02%)	7,546.93 (98.20%)	✅ (view plot)	10.00 (-5.32%)	13.20 (75.77%)	✅ (view plot)	138.00 (+1.50%)	140.26 (98.39%)
serialize_deserialize_authorize	✅ (view plot)	24,588.00 (+0.39%)	24,716.61 (99.48%)	✅ (view plot)	9,898.00 (-0.02%)	10,009.94 (98.88%)	✅ (view plot)	13,958.00 (-0.06%)	14,126.22 (98.81%)	✅ (view plot)	33.00 (-7.45%)	41.27 (79.95%)	✅ (view plot)	299.00 (+1.12%)	299.63 (99.79%)
serialize_deserialize_handle_authorize	✅ (view plot)	30,327.00 (+0.46%)	30,411.61 (99.72%)	✅ (view plot)	12,101.00 (+0.03%)	12,197.75 (99.21%)	✅ (view plot)	17,117.00 (-0.01%)	17,260.05 (99.17%)	✅ (view plot)	59.00 (+0.16%)	64.15 (91.98%)	✅ (view plot)	369.00 (+1.10%)	369.87 (99.77%)
serialize_deserialize_handle_submit	✅ (view plot)	126,372.00 (-0.03%)	126,941.76 (99.55%)	✅ (view plot)	73,224.00 (-0.03%)	73,564.97 (99.54%)	✅ (view plot)	104,947.00 (-0.03%)	105,432.95 (99.54%)	✅ (view plot)	120.00 (-0.32%)	129.72 (92.51%)	✅ (view plot)	595.00 (+0.01%)	598.79 (99.37%)
serialize_deserialize_handle_subscribe	✅ (view plot)	27,491.00 (+0.11%)	27,592.63 (99.63%)	✅ (view plot)	9,643.00 (+0.08%)	9,731.79 (99.09%)	✅ (view plot)	13,641.00 (+0.09%)	13,762.83 (99.11%)	✅ (view plot)	61.00 (-5.82%)	72.85 (83.73%)	✅ (view plot)	387.00 (+0.27%)	388.51 (99.61%)
serialize_deserialize_submit	✅ (view plot)	114,999.00 (-0.05%)	115,561.61 (99.51%)	✅ (view plot)	68,001.00 (-0.06%)	68,346.91 (99.49%)	✅ (view plot)	97,559.00 (-0.07%)	98,068.84 (99.48%)	✅ (view plot)	65.00 (-4.89%)	74.79 (86.91%)	✅ (view plot)	489.00 (+0.14%)	492.11 (99.37%)
serialize_deserialize_subscribe	✅ (view plot)	22,922.00 (+0.18%)	23,086.51 (99.29%)	✅ (view plot)	8,195.00 (+0.07%)	8,286.56 (98.90%)	✅ (view plot)	11,542.00 (+0.07%)	11,668.37 (98.92%)	✅ (view plot)	36.00 (-6.54%)	43.81 (82.18%)	✅ (view plot)	320.00 (+0.41%)	321.34 (99.58%)
serialize_submit	✅ (view plot)	99,826.00 (-0.05%)	100,376.77 (99.45%)	✅ (view plot)	61,483.00 (-0.04%)	61,780.21 (99.52%)	✅ (view plot)	88,206.00 (-0.04%)	88,623.23 (99.53%)	✅ (view plot)	49.00 (-9.28%)	62.50 (78.40%)	✅ (view plot)	325.00 (+0.14%)	328.50 (98.94%)
serialize_subscribe	✅ (view plot)	11,328.00 (+0.06%)	11,564.32 (97.96%)	✅ (view plot)	4,188.00 (+0.18%)	4,276.79 (97.92%)	✅ (view plot)	5,828.00 (+0.18%)	5,946.52 (98.01%)	✅ (view plot)	15.00 (-6.17%)	18.85 (79.57%)	✅ (view plot)	155.00 (+0.03%)	159.02 (97.47%)

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

jbesraa · 2024-07-09T09:43:49Z

@Shourya742 could you please tide up the commit history?

Shourya742 · 2024-07-09T09:58:07Z

@Shourya742 could you please tide up the commit history?

Ah, I'm not sure if we should do that. This PR is based on the remaining tasks left from one of the stale PRs (#918). I'm hesitant to undermine the previous contributor's commit.

Shourya742 · 2024-07-10T07:21:05Z

@jbesraa made the commits modular and relevant to the context. Please review and suggest any changes.

jbesraa

Looks good. Just added two nits.
Could you also please take out the Cargo.lock file changes?

roles/jd-server/src/lib/job_declarator/mod.rs

test/message-generator/mock/job-declarator-mock-invalid-flag.json

test/message-generator/test/job-declarator-flag-test/job-declarator-flag-test.json

test/message-generator/mock/job-declarator-mock-invalid-flag.json

test/message-generator/messages/common_messages.json

protocols/v2/subprotocols/common-messages/src/setup_connection.rs

roles/jd-server/src/lib/job_declarator/mod.rs

plebhash

this PR has a solid foundation and demonstrates some great work by @Shourya742

but we still need to improve some things before we are able to merge it

plebhash · 2024-07-18T20:33:50Z

@lorbax provided some valuable feedback where basically @Shourya742 and I were missing the fact that some deeper changes to JDS were actually necessary

if we carefully read #853, a logical conclusion is that JDS needs a new config parameter, where the user chooses if JDS should support async jobs or not

that was achieved on this commit: ebc8a72

also, we need MG tests that will cover this entire truth table:

async jobs allowed	async jobs requested	`SetupConnection.Success`
T	T	T
T	F	T
F	T	F
F	F	T

that was achieved on this commit: e6ab468

I suggest @Shourya742 reviews these two commits, and if he agrees, cherry-pick them into this PR

then, @lorbax can verify if now we finally have a sane solution to #853

jbesraa

This looks very close. Added few comments.
Could you please rearrange the commits as well?

protocols/v2/subprotocols/common-messages/src/setup_connection.rs

Shourya742 · 2024-07-23T15:07:51Z

This looks very close. Added few comments. Could you please rearrange the commits as well?

Yes, I'll squash a few of my commits and add descriptions according to the context

jbesraa · 2024-07-23T15:43:57Z

This looks very close. Added few comments. Could you please rearrange the commits as well?

Yes, I'll squash a few of my commits and add descriptions according to the context

Thanks, this might be helpful for when writing commits https://cbea.ms/git-commit/

Shourya742 · 2024-07-23T16:31:26Z

@jbesraa I’ve added the suggested changes. Please review and let me know if any further modifications are needed.

pavlenex · 2024-07-25T10:49:08Z

@Shourya742 can you fix this https://github.com/stratum-mining/stratum/actions/runs/10062714214/job/27906911880 ? Everything else is passing.

pavlenex · 2024-07-25T13:31:57Z

This one is waiting for @lorbax's review according to the last dev call and then it's good to go.

lorbax · 2024-07-26T09:25:30Z

I think that now the logic of the flag checking is fine.
Nevertheless, perhaps the author should open an issue regarding the fact that the standard behavior is to have the JDS work with that flag unset, and async_mining_allowed should offered optionally. Not having this may expose the pool to this this attack (pointed out in this comment).

Fi3 · 2024-07-27T07:17:48Z

I think that now the logic of the flag checking is fine. Nevertheless, perhaps the author should open an issue regarding the fact that the standard behavior is to have the JDS work with that flag unset, and async_mining_allowed should offered optionally. Not having this may expose the pool to this this attack (pointed out in this comment).

if the flag is set this is not an attack but the desired behavior (by flag definition). For example if you save everything and you calculate the miner reward in a later moment or somewhere else and not each time that you receive a share (what I expect most pool will do) setting this flag is ok.

Also I do not thing that the issue that you refer should be considered an issue see my comment: #1053 (comment)

lorbax · 2024-07-28T07:03:43Z

I think that now the logic of the flag checking is fine. Nevertheless, perhaps the author should open an issue regarding the fact that the standard behavior is to have the JDS work with that flag unset, and async_mining_allowed should offered optionally. Not having this may expose the pool to this this attack (pointed out in this comment).

if the flag is set this is not an attack but the desired behavior (by flag definition). For example if you save everything and you calculate the miner reward in a later moment or somewhere else and not each time that you receive a share (what I expect most pool will do) setting this flag is ok.

Also I do not thing that the issue that you refer should be considered an issue see my comment: #1053 (comment)

I agree that the issue does not exists, I misread it.
Nevertheless, JDS works only with that flag set, while that flag set indicates a feature.

plebhash · 2024-07-28T21:46:26Z

Nevertheless, JDS works only with that flag set, while that flag set indicates a feature.

hopefully this PR fixes this

plebhash · 2024-07-29T21:20:29Z

rebasing so we can merge

before issue stratum-mining#853 was reported, JDS would simply support async jobs by default and completely ignore this flag. but checking this flag implies that JDS could either support async jobs or not, and that is what this commit does. a new `asyn_mining_allowed` parameter is introduced to the TOML config files, and that is used when: - checking for the flags of `SetupConnection` messages - responding to `AllocateMiningJobToken` messages

as already described in the previous commit, we introduced a new JDS config parameter (`async_mining_allowed`) so now, we need 2 separate tests: - a JDS that supports async jobs - a JDS that does not support async jobs and for each test, we need a mock that: - sends a `SetupConnection` with flag 0 and asserts the expected outcome - sends a `SetupConnection` with flag 1 and asserts the expected outcome if JDS does not support async jobs and receives a `SetupConnection` with flag 1, the expected outcome is `SetupConnection.Error` in all other cases, the expected outcome is a `SetupConnection.Success` with the same flag as the original `SetupConnection`

Shourya742 mentioned this pull request Jul 5, 2024

Issue with flags checking and related roles behaviour #853

Closed

Shourya742 marked this pull request as ready for review July 6, 2024 08:26

Shourya742 force-pushed the feature/asyncFlagSet branch from 57f8ffc to 4ff0ca3 Compare July 6, 2024 10:11

GitGab19 linked an issue Jul 9, 2024 that may be closed by this pull request

Issue with flags checking and related roles behaviour #853

Closed

Shourya742 force-pushed the feature/asyncFlagSet branch from 4ff0ca3 to 8d10d6d Compare July 10, 2024 07:19

lorbax force-pushed the feature/asyncFlagSet branch from 8d10d6d to 178991b Compare July 10, 2024 16:33

jbesraa reviewed Jul 11, 2024

View reviewed changes

roles/jd-server/src/lib/job_declarator/mod.rs Outdated Show resolved Hide resolved

test/message-generator/mock/job-declarator-mock-invalid-flag.json Outdated Show resolved Hide resolved

plebhash self-requested a review July 11, 2024 22:56