p2pool: Restart monerod only when needed and with proper args

[devhyper]

Resolves #3911 and may resolve #3935

[SChernykh]

Depends on https://github.com/monero-project/monero-gui/pull/3913

I would say it replaces #3913, so that one can be closed?

[selsta]

One question. If someone has their monerod managed through something like systemd, does systemd even allow it that a different program restarts monerod with different parameters?

[devhyper]

One question. If someone has their monerod managed through something like systemd, does systemd even allow it that a different program restarts monerod with different parameters?

@selsta The current behavior is that monerod restarts every time p2pool mining is started. This PR will just reduce the amount of restarts. I think in the case of systemd, monerod will be restarted, but not under systemd anymore, it will be its own separate process. I can open a separate PR to detect if monerod is under systemd.

[devhyper]

Should be ready to go now, haven't tested yet though.

[devhyper]

@selsta Just tested it, it works.

[devhyper]

@selsta

[selsta]

@devhyper will try to get it merged

[plowsof]

I can't compile this: (planning to confirm systemd monerod also)

/home//dev/monero-gui/src/daemon/DaemonManager.cpp: In member function ‘bool DaemonManager::checkUnderSystemd()’:
/home//dev/monero-gui/src/daemon/DaemonManager.cpp:358:45: warning: ‘static int QProcess::execute(const QString&)’ is deprecated: Use QProcess::execute(const QString &program, const QStringList &arguments) instead [-Wdeprecated-declarations]
  358 |         int underSystemd = QProcess::execute("/bin/sh -c \"ps -eo pid,cgroup | grep " + pid + " | grep -q .service$\"");
In file included from /usr/include/x86_64-linux-gnu/qt5/QtCore/QProcess:1,
                 from /home//dev/monero-gui/src/daemon/DaemonManager.h:37,
                 from /home//dev/monero-gui/src/daemon/DaemonManager.cpp:29:
/usr/include/x86_64-linux-gnu/qt5/QtCore/qprocess.h:265:16: note: declared here
  265 |     static int execute(const QString &command);
/home//dev/monero-gui/src/daemon/DaemonManager.cpp: In member function ‘QString DaemonManager::getArgs()’:
/home//dev/monero-gui/src/daemon/DaemonManager.cpp:367:17: error: no matching function for call to ‘DaemonManager::running(NetworkType::Type)’
  367 |     if (!running(NetworkType::MAINNET)) {
/home//dev/monero-gui/src/daemon/DaemonManager.cpp:239:6: note: candidate: ‘bool DaemonManager::running(NetworkType::Type, const QString&) const’
  239 | bool DaemonManager::running(NetworkType::Type nettype, const QString &dataDir) const
/home//dev/monero-gui/src/daemon/DaemonManager.cpp:239:6: note:   candidate expects 2 arguments, 1 provided
make[3]: *** [src/CMakeFiles/monero-wallet-gui.dir/build.make:391: src/CMakeFiles/monero-wallet-gui.dir/daemon/DaemonManager.cpp.o] Error 1
make[3]: Leaving directory '/home//dev/monero-gui/build'
make[2]: *** [CMakeFiles/Makefile2:4361: src/CMakeFiles/monero-wallet-gui.dir/all] Error 2
make[2]: Leaving directory '/home//dev/monero-gui/build'
make[1]: *** [Makefile:136: all] Error 2
make[1]: Leaving directory '/home//dev/monero-gui/build'
make: *** [Makefile:32: default] Error 2

I will also test linux but for now, moving onto windows:

Win 10 using docker-windows-static (i need a 2nd opinion on these)

Test: monerod args --zmq-pub=tcp://127.0.0.1:18083 --disable-dns-checkpoints
Expected: monerod will not restart / mining begins
Result: pass

Video

no_restart.webm

Test: monerod args --prune-blockchain --zmq-pub=tcp://127.0.0.1:18083
Expected: monerod will restart because --disable-dns-checkpoints not present
Result: monerod restarts but custom arg --prune-blockchain not present

Video

prune_zmq_fine.webm

Test: no args
Expected: monerod restarts with --zmq-pub and --disable-dns-checkpoints set
Result: endless loop / not working

Video

no_args_loop.webm

Test: monerod with only arg --prune-blockchain
Expected: monerod to restart with prune/zmq/disable-dns
Result: endless loops / not working

Video

prune_only_loop.webm

Test: monerod with only arg --zmq-pub=tcp://127.0.0.1:18083
Expected: monerod to restart with zmq+disable-dns
Result: pass

Video

fine_with_tcp_only.webm

[devhyper]

@plowsof Deprecation and compile errors fixed, most of the cases should be fixed as well. I'll test some more when I can.

[plowsof]

Thanks! linux compiles now. some issues: Linux: (self compiled) daemon args in gui: --prune-blockchain --zmq-pub=tcp://127.0.0.1:18083 --disable-dns-checkpoints error seen after attempting to start mining:

Failed to parse arguments: unrecognised option '--detach,--prune-blockchain,--zmq-pub=tcp://127.0.0.1:18083,--disable-dns-checkpoints,--check-updates,disabled,--non-interactive,--max-concurrency,4'

daemon args in gui: just --prune-blockchain error seen:

Failed to parse arguments: option '--detach' cannot be specified more than once

Win 10 docker-static: i could not get any tests to pass - im seeing an infinite loop on all. when you have time to give a 2nd opinion that would be much appreciated (as im not 100% confident in my windows setup)

[devhyper]

@plowsof This should fix it, I'm currently not able to test right now, will be able to test in a few days.

[plowsof]

Thanks! Only seeing the Failed to parse arguments: option '--detach' cannot be specified more than once error now on linux. I can review also in a few days no problem.

DaemonManager::start takes in all the daemon args, and then blindly adds --detatch if using linux. so if customDaemonArgs is "everything" (defaults included) passed to ::start then (--detach and other defaults will be added)

--zmq-pub=tcp://127.0.0.1:18083 and --zmq-pub tcp://127.0.0.1:18083 are both valid args

[plowsof]

@devhyper i have made the adjustments so all test cases pass. I will test some time tomorrow to confirm, then make a pull request to your branch with the commit. We can get final touches done / merged asap after

[plowsof]

Win 10 using docker-windows-static https://github.com/monero-project/monero-gui/commit/5a4554f2aaff8cf6d15c5d320e9f31b076c1bc0f systemd detection: fixed in the merged commit*

To test the js parser:

play with it in e.g. jfiddle / w3schools editor

<!DOCTYPE html>
<html>
<body>

<p id="demo"></p>

<script>
var customDaemonArgs = "--log-level 0 --check-updates disabled --non-interactive --max-concurrency 4"
//var customDaemonArgs = "--check-updates disabled --non-interactive --max-concurrency 4 --some-random variables"
//var customDaemonArgs = "--check-updates disabled --non-interactive --max-concurrency 4"
//var customDaemonArgs = "--detach --data-dir --bootstrap-daemon-address --prune-blockchain --no-sync --check-updates --non-interactive --max-concurrency --no-zmq --zmq-pub=tcp://127.0.0.1:18083"
//these args will be deleted because DaemonManager::start will re-add them later.
//--no-zmq must be deleted. removing '--zmq-pub=tcp...' lets us blindly add '--zmq-pub tcp...' later without risking duplication.
var defaultArgs = ["--detach","--data-dir","--bootstrap-daemon-address","--prune-blockchain","--no-sync","--check-updates","--non-interactive","--max-concurrency","--no-zmq","--zmq-pub=tcp://127.0.0.1:18083"]
var customDaemonArgsArray = customDaemonArgs.split(' ');
var flag = "";
var allArgs = [];
var p2poolArgs = ["--zmq-pub tcp://127.0.0.1:18083","--disable-dns-checkpoints"];
var daemonArgs = "";
//create an array (allArgs) of ['--arg value','--arg2','--arg3']
for (let i = 0; i < customDaemonArgsArray.length; i++) {
    if(!customDaemonArgsArray[i].startsWith("--")) {
        flag += " " + customDaemonArgsArray[i]
    } else {
        if(flag){
            allArgs.push(flag)
        }
        flag = customDaemonArgsArray[i]
    }
}
allArgs.push(flag)
//pop from allArgs if value is inside the deleteme array (defaultArgs)
for (let i = (allArgs.length - 1); i >= 0; i--) {
    for (let n = (defaultArgs.length - 1); n >= 0; n--) {
        if(allArgs[i].includes(defaultArgs[n])) {
            allArgs.splice(i,1)
            defaultArgs.splice(n,1)
            if(i==0) {
                break
            }
            i-=1
        }
    }
}
//append required p2pool flags
for (let i = 0; i < p2poolArgs.length; i++) {
    if(!allArgs.includes(p2poolArgs[i])) {
        allArgs.push(p2poolArgs[i])
        continue
    }
}
document.getElementById("demo").innerHTML = "Input:<br>" + customDaemonArgs + "<br><br>Monerod flags: <br>" + allArgs.join(" ");
</script>

</body>
</html>

Test: monerod args --zmq-pub=tcp://127.0.0.1:18083 --disable-dns-checkpoints
Expected: monerod will not restart / mining begins
Result:

• Win10 pass
• Linux pass

Video

no_restart_1.webm

Test: monerod args --zmq-pub tcp://127.0.0.1:18083 --disable-dns-checkpoints
Expected: monerod will not restart / mining begins
Result:

• Win10 pass
• Linux pass

Video

no_restart_2.webm

Test: monerod args --zmq-pub tcp://127.0.0.1:18083 --disable-dns-checkpoints --no-zmq
Expected: monerod will restart with --no-zmq removed
Result:

• Win10 pass
• Linux pass

Video

nozmq_3.webm

Test: monerod args --log-level 0 --zmq-pub=tcp://127.0.0.1:18083
Expected: monerod will restart because --disable-dns-checkpoints not present. --log-level 0 passed also (--prune-blockchain is removed on purpose now as not neccessary) Result:

• Win10 pass
• Linux pass

Video

log_level_4.webm

Test: no args
Expected: monerod restarts with --zmq-pub and --disable-dns-checkpoints set
Result:

• Win10 pass
• Linux: pass

Video

no_args.webm

Test: monerod with only arg --prune-blockchain
Expected: to restart with --prune-blockchain removed (as its not needed) but with p2pool flags set
Result:

• Win10 pass
• Linux pass

Video

prune_removed_5.webm

Test: monerod with only arg --zmq-pub=tcp://127.0.0.1:18083
Expected: monerod to restart with zmq+disable-dns
Result:

• Win10 pass
• Linux pass

Video

zmq_eq_6.webm

Test: monerod with only arg --zmq-pub tcp://127.0.0.1:18083
Expected: monerod to restart with zmq+disable-dns
Result:

• Win10 pass
• Linux pass

Video

zmq_7.webm

[devhyper]

Thanks for the help, I'll test again on Windows and Linux later today.

[devhyper]

Just tested all the cases on Windows (with custom data dir), all passed. Testing Linux next.

[plowsof]

I've also made another pull request to your branch with some extra tweaks that came up during review/testing. I am pleased with this PR so far!

[devhyper]

Linux tested, all cases passed. Testing omitting --disable-dns-checkpoints worked as well.

[devhyper]

@selsta Is --disable-dns-checkpoints needed for p2pool anymore?

[selsta]

@devhyper It shouldn't be necessary anymore.

[plowsof]

After squashing, i approve, all tests passing* sanity checks performed on binaries from the latest workflow run *win-service users will always get their daemon restarted regardless but it works fine.

Test: monerod args --zmq-pub=tcp://127.0.0.1:18083 Expected: monerod will not restart / mining begins
Result:

• Win11 pass
• Linux pass

Test: monerod args --zmq-pub tcp://127.0.0.1:18083
Expected: monerod will not restart / mining begins
Result:

• Win11 pass
• Linux pass

Test: monerod args --zmq-pub tcp://127.0.0.1:18083 --no-zmq
Expected: monerod will restart with --no-zmq removed
Result:

• Win11 pass
• Linux pass

Test: monerod args --log-level 0 --zmq-pub=tcp://127.0.0.1:18083 --no-zmq
Expected: monerod will restart with --no-zmq removed and custom arg --log-level 0 added Result:

• Win10 pass
• Linux pass

Test: no args (so with defaults) Expected: monerod restarts with --zmq-pub set Result:

• Win11 pass
• Linux pass

Test: monerod with only arg --prune-blockchain
Expected: to restart with --prune-blockchain removed (as its not needed) but with p2pool flags set
Result:

• Win11 pass
• Linux pass

Test: monerod running as a service but needs to restart Expected: display an error with instructions

• Win11 pass (the service monerod is closed, and monero-gui spawns its own) (confirming the service is uninstalled by exit, which selsta has a PR to -monero fixing)
• Linux pass

Test: monerod running as a service with --zmq-pub in the unit file Expected: no error / no restart Result:

• Win11 fail (we can only read "" args from the daemon so always must restart. if args == "" and os == win we could show a warning but not worth the hassle as win server is stopped / restarted with correct args fine, whereas systemd on linux will keep restarting it)
• Linux pass

[selsta]

@devhyper please squash

[devhyper]

@selsta @plowsof Done.