Skip to content

feat(mergedmining): support dummy mining on coordinator #966

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

Merged
merged 1 commit into from
Aug 14, 2024

Conversation

jansegre
Copy link
Member

@jansegre jansegre commented Mar 4, 2024

Motivation

On the mainnet miners mostly use pools with custom integrations, but on the testnet the whole stack is in our control. For testing the newly activated (on the testnet) extended merkle_path len, we benefit from having the merged mining coordinator be able to do it seamlessly with some injected dummy merkle path.

Acceptance Criteria

  • Add --x-dummy-merged-mining and --x-dummy-merkle-len to run_merged_mining subcommand, for running a coordinator with a dummy chain (instead of BTC), and with a dummy merkle path with the specified length;
  • Adapt the merged mining coordinator to support mining without a BTC node and with dummy data instead.

Checklist

  • If you are requesting a merge into master, confirm this code is production-ready and can be included in future releases as soon as it gets merged

Copy link

codecov bot commented Mar 4, 2024

Codecov Report

Attention: Patch coverage is 52.94118% with 16 lines in your changes missing coverage. Please review.

Project coverage is 84.99%. Comparing base (e61463e) to head (f1b24cd).

Files Patch % Lines
hathor/merged_mining/coordinator.py 54.54% 11 Missing and 4 partials ⚠️
hathor/client.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #966      +/-   ##
==========================================
- Coverage   85.08%   84.99%   -0.10%     
==========================================
  Files         314      314              
  Lines       23919    23939      +20     
  Branches     3614     3621       +7     
==========================================
- Hits        20351    20346       -5     
- Misses       2863     2880      +17     
- Partials      705      713       +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

glevco
glevco previously approved these changes Apr 2, 2024
Copy link
Contributor

@glevco glevco left a comment

Choose a reason for hiding this comment

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

I'm just wondering if this couldn't be simpler, for example, couldn't we just have a way to create a merge mined block and resolve it with the CPU miner? I mean, instead of having to start the coordinator, create a job, etc

@jansegre jansegre force-pushed the feat/mm-coordinator-dummy branch from 9a3a1e4 to e48581d Compare April 11, 2024 16:40
@jansegre jansegre marked this pull request as ready for review April 11, 2024 16:46
@jansegre jansegre requested a review from msbrogli as a code owner April 11, 2024 16:46
@jansegre jansegre force-pushed the feat/mm-coordinator-dummy branch from e48581d to 07d2a63 Compare April 11, 2024 16:47
@jansegre
Copy link
Member Author

I'm just wondering if this couldn't be simpler, for example, couldn't we just have a way to create a merge mined block and resolve it with the CPU miner? I mean, instead of having to start the coordinator, create a job, etc

It definitely can be made simpler mining on CPU, however it's hard to make it in real-time to mine an actual block what will be accepted and used by the rest of the network. Which there wouldn't be anything wrong with a block that is voided for being old, but it'd be better if we have a more realistic test where blocks are accepted. Also doing it in the coordinator makes stratum clients mine blocks with the extended merkle path, which is good to confirm it works with no special changes.

@jansegre jansegre requested a review from msbrogli May 3, 2024 14:29
@jansegre jansegre force-pushed the feat/mm-coordinator-dummy branch from 07d2a63 to 1a0c9db Compare May 3, 2024 14:29
@jansegre jansegre requested a review from glevco May 3, 2024 14:29
glevco
glevco previously approved these changes May 7, 2024
@jansegre jansegre force-pushed the feat/mm-coordinator-dummy branch from 1a0c9db to 1f7facd Compare June 10, 2024 13:09
@jansegre jansegre force-pushed the feat/mm-coordinator-dummy branch from 1f7facd to f1b24cd Compare July 26, 2024 16:36
msbrogli
msbrogli previously approved these changes Aug 9, 2024
@jansegre jansegre dismissed stale reviews from msbrogli and glevco via 47473dd August 14, 2024 15:45
@jansegre jansegre force-pushed the feat/mm-coordinator-dummy branch from f1b24cd to 47473dd Compare August 14, 2024 15:45
Copy link

🐰Bencher

ReportWed, August 14, 2024 at 15:47:04 UTC
Projecthathor-core
Branchfeat/mm-coordinator-dummy
Testbedubuntu-22.04
Click to view all benchmark results
BenchmarkLatencyLatency Results
nanoseconds (ns) | (Δ%)
Latency Lower Boundary
nanoseconds (ns) | (%)
Latency Upper Boundary
nanoseconds (ns) | (%)
sync-v2 (up to 20000 blocks)✅ (view plot)104,176,275,883.32 (-12.98%)95,769,881,797.87 (91.93%)143,654,822,696.81 (72.52%)

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

@jansegre jansegre merged commit 5972abc into master Aug 14, 2024
12 checks passed
@jansegre jansegre deleted the feat/mm-coordinator-dummy branch August 14, 2024 16:33
@jansegre jansegre mentioned this pull request Oct 4, 2024
2 tasks
@jansegre jansegre mentioned this pull request Dec 11, 2024
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants