[GUI|MINE] Mining Tab by WetOne · Pull Request #969 · Veil-Project/veil

WetOne · 2021-10-02T01:22:22Z

Problem
Mining requires use of QT

Root Cause
No Mining interface in the QT implementation

Solution
Implement a Mining Tab to allow for mining to be activated and controlled through the GUI.. The mining tab will control mining activity initiated through the CLI.

CaveSpectre11 · 2021-10-03T14:55:10Z

src/qt/veil/miningwidget.h

@@ -0,0 +1,53 @@
+// Copyright (c) 2019 The Veil developers


NIT: Be sure to give appropriate copyright when adding files (e.g. 2021) or update to include 2021 if it's not done yet (there's a script we can run but might as well do it when we've edited a file)

CaveSpectre11 · 2021-10-03T14:58:38Z

src/qt/veil/miningwidget.cpp

+void MiningWidget::setWalletModel(WalletModel *model){
+    this->walletModel = model;
+
+    connect(model, SIGNAL(updateMiningFields()), this, SLOT(updateMiningFields()));
+}
+
+MiningWidget::~MiningWidget()
+{
+    delete ui;
+}
+
+void MiningWidget::onUpdateAlgorithm() {


NIT. I know it's a copied and edited file, but be sure to try and clean up a new file to a single style.

void module::method(params){

void module::method(params) {

void module::method(params) {

CaveSpectre11 · 2021-10-03T15:12:27Z

src/qt/veil/miningwidget.cpp

+        if (nThreads > (nCores - 1)) {
+            openToastDialog("Too many threads", mainWindow->getGUI());
+            return;
+        }
+
+        if ((nAlgo == MINE_SHA256D) && (nThreads > (nCores - 1)))
+            openToastDialog(QString::fromStdString("SHA256D limited to " + (nCores-1)), mainWindow->getGUI());


Two comments here. First: I'm contemplating it, but for cli based mining, they -can- override this check; so I'm leaning towards a "do it anyway" type button in case they really want to do it.

Second: The second "if" statement won't be hit. If (nThreads > (nCores - 1)) then it will pop "Too many threads" and won't ever get down to the check about the SHA256D limitation. The only way it can get to the MIN_SHA256D check is if the second condition isn't met. Quite simply, that first nThreads check shouldn't be there, since it is intended to restrict SHA256D (Per PR #879)

We can implement an override in a later iteration. This PR can lay down a baseline. I'm inclined to leave the first check and remove the 2nd check (first check applies to ProgPow as well). It is almost irrelevant as the GUI currently restricts the user. I'm inclined to leave it for security check. Thoughts?

I'm fine with the first as a safety check (until the option to override is added), given that the GUI should prevent the casual user from hitting this.

With a better understanding of the history of the checks I'm OK with removing the first one. I'd like to do something where if the user exceeds the recommended number of threads (which would be Cores -1) a popup appears allowing the user to override. I'm still inclined to have that be a separate PR.

I have the formatting changes ready but have not pushed them. @CaveSpectre11, let me know if you want the thread limits changed in this PR or a different one. After that I will either push the changes I have now or add the other changes and push it in one go.

Zannick · 2021-10-08T15:22:38Z

src/qt/veil/miningwidget.cpp

+            openToastDialog(QString::fromStdString("SHA256D limited to " + (nCores-1)), mainWindow->getGUI());
+
+        if ((nAlgo == MINE_RANDOMX) && (nThreads < 4)) {
+            openToastDialog(QString::fromStdString("RandomX must be at least 4 threads"), mainWindow->getGUI());


Is there some way to indicate that this message doesn't prevent mining but changes the threads and starts anyway? Actually, scratch that. I would prefer the GUI enforce that during selection instead of on enabling mining--it wouldn't have to be a pop-up there. And I see below you already have the range being set for randomx.

(I guess that means that this is a safety check just in case the user manages to get a lower number anyway? Then it's fine.)

You pretty much hit it on the head. The GUI is the first line of defense, performing a range check prior to enabling. This check is for redundancy (in case someone finds a way to get a lower number).

Zannick · 2021-10-08T15:31:24Z

src/qt/bitcoingui.cpp

+    miningAction->setStatusTip(tr("Mining"));
+    miningAction->setToolTip(miningAction->statusTip());
+    miningAction->setCheckable(true);
+    miningAction->setShortcut(QKeySequence(Qt::ALT + Qt::Key_6));


If this is above the settings tab, it should have the lower shortcut.

There are a couple errors on the key sequence. Good catch. I'm going to clean it up. It will be the following:
Overview -> 1
Send -> 2
Receive-> 3
Mine -> 4
Address -> 5
Settings -> 6

This is the order of the icons top to bottom.

WetOne · 2021-10-10T03:02:20Z

The latest push addresses

Copywrite
formatting
the check for # of threads (both in code and adjusts range when algorithm is changed
the activates the Use Max Threads button.

CaveSpectre11

ACK 7703eb2

Zannick

utACK 7703eb2

Zannick · 2021-10-16T15:16:10Z

src/qt/veil/miningwidget.cpp

+        bool setAlgoResult = SetMiningAlgorithm(algoStr); 
+
+        if (setAlgoResult) {
+            openToastDialog(QString::fromStdString("Aglorithm Switch Success!"), mainWindow->getGUI());


Since this is on a message presented to the user, and not a rare message; I'm going to upgrade this above just a 'nit'.

Zannick · 2021-10-16T15:28:34Z

src/qt/veil/miningwidget.cpp

+
+    setMineActiveTxt(mineOn);
+
+    ui->lblHashRate->setText(QString("Mining at %1 H/s").arg(QString::number(GetHashSpeed())));


In a future PR, we might want to change this to GetRecentHashSpeed() (#967)

Zannick

utACK 781c6a6

CaveSpectre11 requested review from Zannick and codeofalltrades October 3, 2021 14:50

CaveSpectre11 assigned CaveSpectre11 and WetOne and unassigned CaveSpectre11 Oct 3, 2021

CaveSpectre11 requested a review from seanPhill October 3, 2021 14:51

CaveSpectre11 added Component: GUI Primarily related to the display of the user interface Component: Miner Both PoW and PoS block creation Tag: Waiting For Code Review Waiting for code review from a core developer labels Oct 3, 2021

CaveSpectre11 self-requested a review October 3, 2021 14:52

CaveSpectre11 reviewed Oct 3, 2021

View reviewed changes

Zannick reviewed Oct 8, 2021

View reviewed changes

WetOne force-pushed the miningAlgoTab branch from 673a0f5 to 7703eb2 Compare October 10, 2021 02:57

CaveSpectre11 approved these changes Oct 16, 2021

View reviewed changes

Zannick approved these changes Oct 16, 2021

View reviewed changes

[GUI|MINE] Mining Tab

781c6a6

WetOne force-pushed the miningAlgoTab branch from 7703eb2 to 781c6a6 Compare October 16, 2021 19:37

Zannick approved these changes Oct 16, 2021

View reviewed changes

CaveSpectre11 added Code Review: Passed and removed Tag: Waiting For Code Review Waiting for code review from a core developer labels Oct 17, 2021

CaveSpectre11 merged commit c66163b into Veil-Project:master Oct 17, 2021


		setMineActiveTxt(mineOn);

		ui->lblHashRate->setText(QString("Mining at %1 H/s").arg(QString::number(GetHashSpeed())));

Conversation

WetOne commented Oct 2, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Zannick Oct 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WetOne commented Oct 10, 2021

Uh oh!

CaveSpectre11 left a comment

Choose a reason for hiding this comment

Uh oh!

Zannick left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Zannick left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Zannick Oct 8, 2021 •

edited

Loading