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

Refractor update_mempool module in JDS mempool module for simplified async flow #833

Merged
merged 2 commits into from
May 7, 2024

Conversation

NonsoAmadi10
Copy link
Contributor

@NonsoAmadi10 NonsoAmadi10 commented Apr 2, 2024

What does this PR do?

Closes #784

  • Removed the unnecessary use of tokio::task::spawn in update_mempool, leveraging the existing async function context for all operations.
  • Removed the TokioJoin(JoinError) implementation since it is not been used

@NonsoAmadi10
Copy link
Contributor Author

@GitGab19 I have re-reviewed and made some changes here, please I am requesting a review here and also to point out that the MG test is still failing

@GitGab19
Copy link
Collaborator

GitGab19 commented Apr 3, 2024

@GitGab19 I have re-reviewed and made some changes here, please I am requesting a review here and also to point out that the MG test is still failing

I would ask to @lorbax to have a look at this, since he opened issue #784 and he worked on the async management during last release 🙏

@lorbax
Copy link
Collaborator

lorbax commented Apr 3, 2024

I will do it by the next week!

Copy link
Collaborator

@lorbax lorbax left a comment

Choose a reason for hiding this comment

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

This is a good PR
I would be good to have the minor changes that I left as comments

roles/jd-server/src/lib/mempool/mod.rs Outdated Show resolved Hide resolved
roles/jd-server/src/lib/mempool/mod.rs Outdated Show resolved Hide resolved
roles/jd-server/src/lib/mempool/mod.rs Outdated Show resolved Hide resolved
@NonsoAmadi10
Copy link
Contributor Author

@lorbax i have been able to manage the unhappy paths in the the update_mempool function, please I request your feedback on this before I pick up the next issue. Thanks

@plebhash
Copy link
Collaborator

@NonsoAmadi10 can you rebase?

@NonsoAmadi10
Copy link
Contributor Author

Alright I am on it

@NonsoAmadi10
Copy link
Contributor Author

I have rebased. cc: @plebhash @lorbax

@plebhash
Copy link
Collaborator

@lorbax once you do the final review here, are you able to approve?

@pavlenex pavlenex linked an issue Apr 30, 2024 that may be closed by this pull request
@NonsoAmadi10 NonsoAmadi10 changed the title Closes #784 Refractor update_mempool module in JDS mempool module for simplified async flow Apr 30, 2024
@pavlenex pavlenex mentioned this pull request May 1, 2024
@Fi3 Fi3 deleted the branch stratum-mining:dev May 3, 2024 09:37
@Fi3 Fi3 closed this May 3, 2024
@Fi3 Fi3 reopened this May 3, 2024
@Fi3
Copy link
Collaborator

Fi3 commented May 3, 2024

This PR needs to be rebased on dev

@NonsoAmadi10 NonsoAmadi10 force-pushed the bug-jds-server-refactor branch 3 times, most recently from 10dce88 to 877f265 Compare May 3, 2024 15:40
Copy link
Collaborator

@GitGab19 GitGab19 left a comment

Choose a reason for hiding this comment

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

LGTM.
@lorbax could you double check when you get a chance pls?

@lorbax
Copy link
Collaborator

lorbax commented May 6, 2024

@NonsoAmadi10 you must also remove the TokioJoin variant of JdsMempoolError

@Fi3
Copy link
Collaborator

Fi3 commented May 6, 2024

The right pattern to follow when we implement a variant that is only a container for an error that come from a library is to implement From for the error like here:

https://github.com/stratum-mining/stratum/blob/main/roles/jd-server/src/lib/error.rs#L55

Copy link
Collaborator

@lorbax lorbax left a comment

Choose a reason for hiding this comment

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

Run local TP, pool, jds, jdc, tproxy and cpuminer without issues.
The code looks good to me.

@lorbax lorbax merged commit b46355b into stratum-mining:dev May 7, 2024
9 checks passed
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.

small bugs in the JDS mempool
5 participants