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: lazily evaluate for new random_x template #6170

Conversation

hansieodendaal
Copy link
Contributor

@hansieodendaal hansieodendaal commented Feb 26, 2024

Description

  • Changed the merge mining proxy to evaluate RandomX block template changes lazily, similar to the strategy employed for the SHA3 miner. Evaluating this often is very expensive, especially during busy network times with a highly volatile mempool.
  • Removed new template spam from logging
  • Fixed a bug where the command-line network argument igor specified for the merge mining proxy was not recognized in the network-aware 'DomainSeparatedConsensusHasher'
  • Updated the merge mining proxy config.toml to be in line with core

Motivation and Context

  • With recent stress tests on nextnet that lasted multiple hours, RandomX mining virtually ground to a halt during the stress test, whereas SHAR3 mining was able to continue.
  • One merge mining proxy that was monitored for 45 min went up to 4.5 GB in memory usage. During that time the connected base node only advanced one block, while the merge mining proxy requested 428 new block templates from the base node, all of them unique. This could account for ~1 GB in memory usage as block templates are discarded after 20 minutes.

How Has This Been Tested?

  • System-level testing during low network activity (on igor)
  • System-level stress testing:
    • coin split stress test (on igor)
    • one-side stealth transaction stress test (on igor)

What process can a PR reviewer use to test or verify this change?

Code walk-through, system-level testing

Breaking Changes

  • None
  • Requires data directory on base node to be deleted
  • Requires hard fork
  • Other - Please specify

@hansieodendaal hansieodendaal requested a review from a team as a code owner February 26, 2024 13:27
@hansieodendaal hansieodendaal marked this pull request as draft February 26, 2024 13:27
Copy link

github-actions bot commented Feb 26, 2024

Test Results (CI)

1 260 tests   1 259 ✅  16m 41s ⏱️
   39 suites      0 💤
    1 files        1 ❌

For more details on these failures, see this check.

Results for commit 555aacb.

♻️ This comment has been updated with latest results.

@ghpbot-tari-project ghpbot-tari-project added P-acks_required Process - Requires more ACKs or utACKs P-reviews_required Process - Requires a review from a lead maintainer to be merged labels Feb 26, 2024
Copy link

github-actions bot commented Feb 26, 2024

Test Results (Integration tests)

0 tests   0 ✅  0s ⏱️
0 suites  0 💤
2 files    0 ❌
2 errors

For more details on these parsing errors, see this check.

Results for commit 555aacb.

♻️ This comment has been updated with latest results.

@hansieodendaal hansieodendaal force-pushed the ho_lazy_evaluate_randomx_template branch 6 times, most recently from 9206eab to 00b8554 Compare February 29, 2024 12:07
@hansieodendaal hansieodendaal marked this pull request as ready for review February 29, 2024 12:10
- Changed the merge mining proxy to lazily evaluate RandomX block template changes.
  Greedily evaluating this is very expensive, especially during busy network times
  with a highly volatile mempool.
- Fixed a bug where the command-line network argument 'igor' specified for the merge
  mining proxy was not recognized in the network-aware 'DomainSeparatedConsensusHasher'
@hansieodendaal hansieodendaal force-pushed the ho_lazy_evaluate_randomx_template branch from 00b8554 to c6c5079 Compare February 29, 2024 12:19
@ghpbot-tari-project ghpbot-tari-project removed the P-reviews_required Process - Requires a review from a lead maintainer to be merged label Mar 1, 2024
@SWvheerden SWvheerden merged commit d220643 into tari-project:development Mar 1, 2024
9 of 16 checks passed
@hansieodendaal hansieodendaal deleted the ho_lazy_evaluate_randomx_template branch March 1, 2024 09:55
SWvheerden pushed a commit that referenced this pull request Mar 1, 2024
Description
---
Fixed cucumber network environment

Motivation and Context
---
The cucumber environment uses a single runtime environment and the
network should be set prior to use.
Fixes cucumber test failures introduced by #6170.

How Has This Been Tested?
---
Applicable cucumber pass

What process can a PR reviewer use to test or verify this change?
---
Verify that `Scenario: Clear out mempool` passes
**Note:** #6172 also introduced other cucumber errors

<!-- Checklist -->
<!-- 1. Is the title of your PR in the form that would make nice release
notes? The title, excluding the conventional commit
tag, will be included exactly as is in the CHANGELOG, so please think
about it carefully. -->


Breaking Changes
---

- [x] None
- [ ] Requires data directory on base node to be deleted
- [ ] Requires hard fork
- [ ] Other - Please specify

<!-- Does this include a breaking change? If so, include this line as a
footer -->
<!-- BREAKING CHANGE: Description what the user should do, e.g. delete a
database, resync the chain -->
sdbondi added a commit to sdbondi/tari that referenced this pull request Mar 5, 2024
* development:
  feat: make the make_it_rain submission rate a float (tari-project#6180)
  feat: wallet ffi use dns (tari-project#6152)
  chore: add localnet gen block (tari-project#6176)
  fix: hide unmined coinbase (tari-project#6159)
  chore: removes panics from wallet burn task (tari-project#6163)
  test: fix cucumber network environment (tari-project#6175)
  feat: lazily evaluate for new random_x template (tari-project#6170)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-acks_required Process - Requires more ACKs or utACKs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants