[GUI|MINE] Mining Tab #969
Conversation
CaveSpectre11
added
Component: GUI
src/qt/veil/miningwidget.h
Outdated
| @@ -0,0 +1,53 @@ | |||
| // Copyright (c) 2019 The Veil developers | |||
There was a problem hiding this comment.
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)
src/qt/veil/miningwidget.cpp
Outdated
| void MiningWidget::setWalletModel(WalletModel *model){ | ||
| this->walletModel = model; | ||
|
|
||
| connect(model, SIGNAL(updateMiningFields()), this, SLOT(updateMiningFields())); | ||
| } | ||
|
|
||
| MiningWidget::~MiningWidget() | ||
| { | ||
| delete ui; | ||
| } | ||
|
|
||
| void MiningWidget::onUpdateAlgorithm() { |
There was a problem hiding this comment.
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)
{
src/qt/veil/miningwidget.cpp
Outdated
| 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()); |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| 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()); |
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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).
src/qt/bitcoingui.cpp
Outdated
| miningAction->setStatusTip(tr("Mining")); | ||
| miningAction->setToolTip(miningAction->statusTip()); | ||
| miningAction->setCheckable(true); | ||
| miningAction->setShortcut(QKeySequence(Qt::ALT + Qt::Key_6)); |
There was a problem hiding this comment.
If this is above the settings tab, it should have the lower shortcut.
There was a problem hiding this comment.
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.
|
The latest push addresses
|
src/qt/veil/miningwidget.cpp
Outdated
| bool setAlgoResult = SetMiningAlgorithm(algoStr); | ||
|
|
||
| if (setAlgoResult) { | ||
| openToastDialog(QString::fromStdString("Aglorithm Switch Success!"), mainWindow->getGUI()); |
There was a problem hiding this comment.
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'.
|
|
||
| setMineActiveTxt(mineOn); | ||
|
|
||
| ui->lblHashRate->setText(QString("Mining at %1 H/s").arg(QString::number(GetHashSpeed()))); |
There was a problem hiding this comment.
In a future PR, we might want to change this to GetRecentHashSpeed() (#967)
CaveSpectre11
added
Code Review: Passed
and removed
Tag: Waiting For Code Review
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.