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

feat(mine): Add an internal Zcash miner to Zebra #8136

Merged
merged 56 commits into from
Jan 11, 2024
Merged

feat(mine): Add an internal Zcash miner to Zebra #8136

merged 56 commits into from
Jan 11, 2024

Conversation

teor2345
Copy link
Collaborator

@teor2345 teor2345 commented Jan 8, 2024

Motivation

It would be easier to test Zebra if we could mine blocks without external mining software.

This PR depends on changes to equihash:
zcash/librustzcash#1088
zcash/librustzcash#1083

This is an experimental "free sprint" project.

PR Author Checklist

Check before marking the PR as ready for review:

  • Will the PR name make sense to users?
  • Does the PR have a priority label?
  • Have you added or updated tests?
  • Is the documentation up to date?
    • List of features in zebrad/src/lib.rs
    • List of tasks in zebrad/src/commands/start.rs
For significant changes:
  • Is there a summary in the CHANGELOG?
  • Can these changes be split into multiple PRs?

If a checkbox isn't relevant to the PR, mark it as done.

Specifications

This code obeys the consensus rules by generating valid blocks, but it uses the existing verifiers. So there are no consensus-critical changes here.

Complex Code or Requirements

This code launches concurrent async tasks and OS threads. We should check that panics and shutdowns are handled correctly. In particular, miner threads should restart mining with each new block template.

We should check that any common or expected errors are handled by retrying rather than panicking.

We should check that the threads are actually doing different work, they should have nonces with large gaps between them.

Solution

Rust Features:

  • add an internal-miner feature and dependencies

Solver:

  • depend on the new solver code in equihash
  • add an equihash::Solution::solve() method with difficulty checks

Miner:

  • add a miner task to the start command
  • wait until a valid template is available
  • spawn to a blocking async executor thread
  • spawn to a low-priority OS thread
  • handle shutdowns by cancelling the miner

Parallel Mining:

  • use a different nonce for each thread
  • spawn multiple miner threads based on a new config, use available_parallelism() by default
  • select! {} on solutions or new block templates using long_poll_id
  • cancel and restart the solver if there's a new template
  • rate-limit template changes

Related Changes:

  • Fix a bug in the get_block_template RPC change detection
  • make RPC instances cloneable and LongPollId copyable
  • use the same generic constraints for the RPC struct and impls
  • move some code around so it's easier to re-use
  • add utility and convenience methods to existing types

Testing

I manually verified that:

  • the mining threads are running, and there are approximately the configured number of threads
  • they are using 100% CPU
  • they are low priority (blue in htop)
  • they produce valid blocks (according to Zebra's verifier)
  • the miner runs overnight successfully
  • the blocks are mined off the tip
  • the blocks are accepted by other nodes
  • the block nonces match the pattern used by Zebra, and it's not just the first thread successfully mining

Review

This PR is ready for review.

I'd like to focus on bugs and overall code structure in this review.

Reviewer Checklist

Check before approving the PR:

  • Does the PR scope match the ticket?
  • Are there enough tests to make sure it works? Do the tests cover the PR motivation?
  • Are all the PR blockers dealt with?
    PR blockers can be dealt with in new tickets or PRs.

And check the PR Author checklist is complete.

Follow Up Work

  • Get the equihash changes merged and released, and update that dependency
  • Check we have at least one peer before starting mining (the existing sync to tip check partly covers this)
  • Optional: Add a cached state test that mines a block on testnet?

@teor2345 teor2345 added P-Optional ✨ C-feature Category: New features A-concurrency Area: Async code, needs extra work to make it work properly. no-review-reminders Turn off review reminders labels Jan 8, 2024
@teor2345 teor2345 self-assigned this Jan 8, 2024
@github-actions github-actions bot added the C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG label Jan 8, 2024
@teor2345
Copy link
Collaborator Author

teor2345 commented Jan 8, 2024

This seems to work on testnet, but it's currently causing orphaned chain forks due to some missing code:

  • cancel and restart the solver if there's a new template
Screenshot 2024-01-08 at 13 42 42

@teor2345 teor2345 marked this pull request as ready for review January 10, 2024 22:40
@teor2345 teor2345 requested review from a team as code owners January 10, 2024 22:40
@teor2345 teor2345 requested review from upbqdn and removed request for a team January 10, 2024 22:40
@teor2345
Copy link
Collaborator Author

This should be feature-complete. There aren't any new tests, but the existing tests have good coverage, and I tested it manually.

I am just writing the docs now.

@teor2345
Copy link
Collaborator Author

Now the docs should be complete as well.

zebrad/src/components/miner.rs Show resolved Hide resolved
zebrad/src/components/miner.rs Show resolved Hide resolved
zebrad/src/components/miner.rs Show resolved Hide resolved
@teor2345 teor2345 added A-dependencies Area: Dependency file updates A-rpc Area: Remote Procedure Call interfaces and removed C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG no-review-reminders Turn off review reminders labels Jan 11, 2024
Copy link
Contributor

@arya2 arya2 left a comment

Choose a reason for hiding this comment

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

This looks excellent, thank you.

@mergify mergify bot merged commit 2ac6921 into main Jan 11, 2024
181 checks passed
@mergify mergify bot deleted the internal-mine branch January 11, 2024 14:41
@oxarbitrage oxarbitrage restored the internal-mine branch April 30, 2024 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-concurrency Area: Async code, needs extra work to make it work properly. A-dependencies Area: Dependency file updates A-rpc Area: Remote Procedure Call interfaces C-feature Category: New features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants