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

Streamline create_jobs_task #478

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

eandersson
Copy link
Collaborator

@eandersson eandersson commented Nov 14, 2024

I simplified the code by merging process_mining_job and generate_additional_work into a single function, as their separation was unnecessary - they performed the same task except for how extranonce_2 was incremented in generate_additional_work. I moved the extranonce_2 logic into the while loop for better clarity and flow, and removed the unnecessary yielding mechanism

In addition this also changes how extranonce_2 is rolled between notifies.

New version

0 -> 1 -> 2 -> Notify -> 0 -> 1 -> 2 -> Notify -> 0 -> 1 -> 2 etc

Old version

0 -> 1 -> 2 -> Notify -> 0 -> 3 -> 4 -> Notify -> 0 -> 5 -> 6 etc

@eandersson eandersson changed the title Simplify create_jobs_task Streamline create_jobs_task Nov 15, 2024
@WantClue WantClue requested a review from skot November 16, 2024 21:53
@WantClue WantClue added help wanted Extra attention is needed accepted This issue will be worked on labels Nov 16, 2024
@eandersson eandersson requested a review from mutatrum November 20, 2024 08:44
@mutatrum
Copy link
Contributor

mutatrum commented Nov 20, 2024

Looking at it now. There's a difference in where extranonce_2 is reset back to 0, in the old code it was inside the while (GLOBAL_STATE->stratum_queue.count < 1 && GLOBAL_STATE->abandon_work == 0) loop, in the new code it's outside this loop.

I logged the extranonce_2, and with the old code there's a weird pattern which I don't think is what was supposed to happen:

I (40738) create_jobs_task: extranonce_2: 3500000000000000
I (41238) create_jobs_task: extranonce_2: 3600000000000000
I (41738) create_jobs_task: extranonce_2: 3700000000000000
I (42238) create_jobs_task: extranonce_2: 3800000000000000
I (42308) stratum_task: rx: "mining.notify"
I (42438) create_jobs_task: extranonce_2: 0000000000000000
I (43238) create_jobs_task: extranonce_2: 3900000000000000
I (43738) create_jobs_task: extranonce_2: 3a00000000000000
I (44238) create_jobs_task: extranonce_2: 3b00000000000000

With the new code, it actually resets after a mining notify:

I (30067) create_jobs_task: extranonce_2: 2100000000000000
I (30567) create_jobs_task: extranonce_2: 2200000000000000
I (31067) create_jobs_task: extranonce_2: 2300000000000000
I (31567) create_jobs_task: extranonce_2: 2400000000000000
I (31737) stratum_task: rx: "mining.notify"
I (32067) create_jobs_task: extranonce_2: 0000000000000000
I (32567) create_jobs_task: extranonce_2: 0100000000000000
I (33067) create_jobs_task: extranonce_2: 0200000000000000
I (33567) create_jobs_task: extranonce_2: 0300000000000000

Is resetting the extranonce_2 a better approach for covering the space, or is that dictated by other fields anyways? Maybe even simpler to have it just continuously increase, and never reset.

Other than this, it looks good. Not sure why there was such a big chunk of code duplicated. Nice cleanup.

@eandersson
Copy link
Collaborator Author

eandersson commented Nov 20, 2024

Looking at it now. There's a difference in where extranonce_2 is reset back to 0, in the old code it was inside the while (GLOBAL_STATE->stratum_queue.count < 1 && GLOBAL_STATE->abandon_work == 0) loop, in the new code it's outside this loop.

I logged the extranonce_2, and with the old code there's a weird pattern which I don't think is what was supposed to happen:

I (40738) create_jobs_task: extranonce_2: 3500000000000000
I (41238) create_jobs_task: extranonce_2: 3600000000000000
I (41738) create_jobs_task: extranonce_2: 3700000000000000
I (42238) create_jobs_task: extranonce_2: 3800000000000000
I (42308) stratum_task: rx: "mining.notify"
I (42438) create_jobs_task: extranonce_2: 0000000000000000
I (43238) create_jobs_task: extranonce_2: 3900000000000000
I (43738) create_jobs_task: extranonce_2: 3a00000000000000
I (44238) create_jobs_task: extranonce_2: 3b00000000000000

With the new code, it actually resets after a mining notify:

I (30067) create_jobs_task: extranonce_2: 2100000000000000
I (30567) create_jobs_task: extranonce_2: 2200000000000000
I (31067) create_jobs_task: extranonce_2: 2300000000000000
I (31567) create_jobs_task: extranonce_2: 2400000000000000
I (31737) stratum_task: rx: "mining.notify"
I (32067) create_jobs_task: extranonce_2: 0000000000000000
I (32567) create_jobs_task: extranonce_2: 0100000000000000
I (33067) create_jobs_task: extranonce_2: 0200000000000000
I (33567) create_jobs_task: extranonce_2: 0300000000000000

Is resetting the extranonce_2 a better approach for covering the space, or is that dictated by other fields anyways? Maybe even simpler to have it just continuously increase, and never reset.

Other than this, it looks good. Not sure why there was such a big chunk of code duplicated. Nice cleanup.

Good callout. I'll just move it back to static for now.

main/tasks/create_jobs_task.c Outdated Show resolved Hide resolved
main/tasks/create_jobs_task.c Outdated Show resolved Hide resolved
main/tasks/create_jobs_task.c Show resolved Hide resolved
@mutatrum
Copy link
Contributor

LGTM, can just use a little more cleanup.

@eandersson eandersson requested a review from mutatrum November 28, 2024 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted This issue will be worked on help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants