p2pool: Restart monerod only when needed and with proper args #3936

devhyper · 2022-06-01T07:26:33Z

Resolves #3911 and may resolve #3935

SChernykh · 2022-06-01T08:26:27Z

Depends on #3913

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

selsta · 2022-06-07T18:18:22Z

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 · 2022-06-07T21:17:40Z

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 · 2022-10-11T01:27:55Z

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

devhyper · 2022-10-11T03:42:27Z

@selsta Just tested it, it works.

devhyper · 2022-12-21T06:00:40Z

@selsta

selsta · 2022-12-22T15:39:23Z

@devhyper will try to get it merged

plowsof · 2022-12-24T07:00:16Z

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 · 2022-12-24T15:21:35Z

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

plowsof · 2022-12-24T17:49:04Z

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 · 2022-12-24T17:57:22Z

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

plowsof · 2022-12-24T18:08:34Z

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 · 2022-12-26T06:30:26Z

@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 · 2022-12-28T04:24:12Z

Win 10 using docker-windows-static 5a4554f
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 · 2022-12-29T14:28:32Z

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

devhyper · 2022-12-31T09:43:52Z

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

plowsof · 2022-12-31T10:35:59Z

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 · 2023-01-01T02:40:07Z

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

devhyper · 2023-01-01T02:40:56Z

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

selsta · 2023-01-01T02:41:39Z

@devhyper It shouldn't be necessary anymore.

pages/Mining.qml

plowsof · 2023-01-31T23:22:23Z

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 · 2023-01-31T23:39:52Z

@devhyper please squash

devhyper · 2023-02-01T02:24:09Z

@selsta @plowsof Done.

devhyper mentioned this pull request Jun 1, 2022

p2pool: fix monerod sometimes restarting with invalid arguments #3913

Closed

devhyper force-pushed the only-restart-monerod-if-zmq-not-present branch from f1a52de to f9f58c3 Compare June 1, 2022 23:06

devhyper changed the title ~~Only restart monerod if zmq not present~~ p2pool: Restart monerod only when needed and with proper args Jun 1, 2022

devhyper force-pushed the only-restart-monerod-if-zmq-not-present branch 2 times, most recently from 597ccce to 83c4506 Compare October 11, 2022 03:41

devhyper force-pushed the only-restart-monerod-if-zmq-not-present branch 3 times, most recently from 8e7b047 to 9169dbb Compare October 11, 2022 03:51

plowsof mentioned this pull request Dec 28, 2022

parse daemon args / handle all test cases devhyper/monero-gui#1

Merged

plowsof reviewed Jan 27, 2023

View reviewed changes

pages/Mining.qml Outdated Show resolved Hide resolved

p2pool: Restart monerod only when needed and with proper args

757bc7d

devhyper force-pushed the only-restart-monerod-if-zmq-not-present branch from 1ca8df6 to 757bc7d Compare February 1, 2023 02:23

selsta approved these changes Feb 2, 2023

View reviewed changes

plowsof approved these changes Feb 2, 2023

View reviewed changes

luigi1111 merged commit af368c9 into monero-project:master Feb 15, 2023

p2pool: Restart monerod only when needed and with proper args #3936

p2pool: Restart monerod only when needed and with proper args #3936

Uh oh!

Conversation

devhyper commented Jun 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SChernykh commented Jun 1, 2022

Uh oh!

selsta commented Jun 7, 2022

Uh oh!

devhyper commented Jun 7, 2022

Uh oh!

devhyper commented Oct 11, 2022

Uh oh!

devhyper commented Oct 11, 2022

Uh oh!

devhyper commented Dec 21, 2022

Uh oh!

selsta commented Dec 22, 2022

Uh oh!

plowsof commented Dec 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

devhyper commented Dec 24, 2022

Uh oh!

plowsof commented Dec 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

devhyper commented Dec 24, 2022

Uh oh!

plowsof commented Dec 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

plowsof commented Dec 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

plowsof commented Dec 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

devhyper commented Dec 29, 2022

Uh oh!

devhyper commented Dec 31, 2022

Uh oh!

plowsof commented Dec 31, 2022

Uh oh!

devhyper commented Jan 1, 2023

Uh oh!

devhyper commented Jan 1, 2023

Uh oh!

selsta commented Jan 1, 2023

Uh oh!

Uh oh!

plowsof commented Jan 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

selsta commented Jan 31, 2023

Uh oh!

devhyper commented Feb 1, 2023

Uh oh!

Uh oh!

devhyper commented Jun 1, 2022 •

edited

Loading

plowsof commented Dec 24, 2022 •

edited

Loading

plowsof commented Dec 24, 2022 •

edited

Loading

plowsof commented Dec 24, 2022 •

edited

Loading

plowsof commented Dec 26, 2022 •

edited

Loading

plowsof commented Dec 28, 2022 •

edited

Loading

plowsof commented Jan 31, 2023 •

edited

Loading