rpc: Allow shutdown while in generateblocks #16262

pstratem commented at 5:28 PM on June 21, 2019: contributor

By checking the shutdown flag every loop we can use the entire 32 bit nonce space instead of breaking every 16 bits to check the flag.

This is possible now because the shutdown flag is an atomic where before it was controlled by a condition variable and lock.

kallewoof commented at 5:31 PM on June 21, 2019: member

ACK 8f0291f

Thank you. :)

MarcoFalke renamed this:
~~Slightly simplify the mining loop.~~
rpc: Slightly simplify the mining loop
on Jun 21, 2019

in src/rpc/mining.cpp:130 in 8f0291f962 outdated

 122 | @@ -124,14 +123,17 @@ static UniValue generateBlocks(const CScript& coinbase_script, int nGenerate, ui
 123 |              LOCK(cs_main);
 124 |              IncrementExtraNonce(pblock, ::ChainActive().Tip(), nExtraNonce);
 125 |          }
 126 | -        while (nMaxTries > 0 && pblock->nNonce < nInnerLoopCount && !CheckProofOfWork(pblock->GetHash(), pblock->nBits, Params().GetConsensus())) {
 127 | +        while (nMaxTries > 0 && pblock->nNonce < std::numeric_limits<uint32_t>::max() && !CheckProofOfWork(pblock->GetHash(), pblock->nBits, Params().GetConsensus()) && !ShutdownRequested()) {
 128 |              ++pblock->nNonce;
 129 |              --nMaxTries;
 130 |          }
 131 | +        if(ShutdownRequested()) {

kallewoof commented at 5:33 PM on June 21, 2019:

Nit: space after if

pstratem commented at 8:40 PM on June 21, 2019:

nit picked

MarcoFalke renamed this:
~~rpc: Slightly simplify the mining loop~~
rpc: Allow shutdown while in generateblocks
on Jun 21, 2019

MarcoFalke added the label Mining on Jun 21, 2019

MarcoFalke added the label RPC/REST/ZMQ on Jun 21, 2019

MarcoFalke added the label Tests on Jun 21, 2019

pstratem force-pushed on Jun 21, 2019

in src/rpc/mining.cpp:132 in aa566d282b outdated

 128 |              ++pblock->nNonce;
 129 |              --nMaxTries;
 130 |          }
 131 | +        if (ShutdownRequested()) {
 132 | +            break;
 133 | +        }

kallewoof commented at 2:00 AM on June 22, 2019:

No biggie but you could just do

if (nMaxTries == 0 || ShutdownRequested()) {
    break;
}

i.e. merge the above and below blocks into one.

pstratem commented at 2:53 PM on June 22, 2019:

nit picked

kallewoof commented at 2:01 AM on June 22, 2019: member

re-ACK aa566d282bca71f569f4ffb1b001fd58e1cf3263

practicalswift commented at 8:26 AM on June 22, 2019: contributor

The replacement of 0x10000 (65536) with std::numeric_limits<uint32_t>::max() (4294967295) looks incorrect.

0x10000 is std::numeric_limits<uint16_t>::max() + 1, right? :-)

$ cling
****************** CLING ******************
* Type C++ code and press enter to run it *
*             Type .q to exit             *
*******************************************
[cling]$ #include <cstdint>
[cling]$ #include <limits>
[cling]$ 0x10000 == std::numeric_limits<uint16_t>::max() + 1
(bool) true

Edit: OP has now been updated. This comment is no longer relevant.

kallewoof commented at 8:40 AM on June 22, 2019: member

@practicalswift That's the point. Previous code only uses up to 16 bits of the 32 bit nonce, because it wanted to regularly check ShutdownRequested() state. We can use all 32 bits by just checking every iteration.

Edit: I realize OP did not actually mention this detail.

practicalswift commented at 2:14 PM on June 22, 2019: contributor

@kallewoof Oh, thanks for filling in. That's important context! :-)

pstratem force-pushed on Jun 22, 2019

rpc: Allow shutdown while in generateblocks

By checking the shutdown flag every loop we can use the entire nonce space
instead of breaking every 16 bits to check the shutdown flag.

This has been possible since the shutdown flag was switched to an atomic,
before that change it was controlled by a condition variable and lock.

3b9bf0eb0e

pstratem force-pushed on Jun 24, 2019

kallewoof commented at 4:42 AM on June 24, 2019: member

Re-ACK 3b9bf0e

promag commented at 1:34 PM on June 24, 2019: member

#12448 added the shutdown check in the outer loop, with this change it could be removed?

kallewoof commented at 10:48 PM on June 24, 2019: member

@promag The check is happening every loop now instead of every 65536'th loop.

promag commented at 11:05 PM on June 24, 2019: member

I mean that now the check && !ShutdownRequested() could be removed from: https://github.com/bitcoin/bitcoin/blob/e115a21f79c4121a78e7c889c17c3b5d3680d15b/src/rpc/mining.cpp#L117

kallewoof commented at 11:11 PM on June 24, 2019: member

Ahh, yes indeed it can.

pstratem commented at 5:34 PM on June 26, 2019: contributor

@promag yes, but I don't see any reason not to check in the outer loop as well

kallewoof commented at 3:18 AM on June 27, 2019: member

@pstratem It's pointless because you are already breaking the outer loop if ShutdownRequested() is true. And if it happens to loop around, the inner loop will immediately break.

pstratem commented at 12:05 AM on June 29, 2019: contributor

@kallewoof it's pointless to remove the check, so it's probably better to have it

promag commented at 12:15 AM on June 29, 2019: member

It's pointless to have both. How is extra redundant code better?

pstratem commented at 1:13 AM on June 29, 2019: contributor

@promag iono, if someone refactors the loop into a function it would break without it.

there just doesn't seem to be a reason to change it, since it makes the code more not less brittle

kallewoof commented at 3:31 PM on June 29, 2019: member

Suggesting that we should keep redundant code because someone might break it by writing invalid code in the future is an invalid argument.

laanwj merged this on Jul 3, 2019

laanwj closed this on Jul 3, 2019

laanwj referenced this in commit 7d7b832d67 on Jul 3, 2019

sidhujag referenced this in commit 968aa63558 on Jul 5, 2019

jasonbcox referenced this in commit d7c587d711 on Oct 9, 2020

PastaPastaPasta referenced this in commit ae1bfbccbb on Jun 27, 2021

PastaPastaPasta referenced this in commit 9135f3829f on Jun 28, 2021

PastaPastaPasta referenced this in commit 449b8918b1 on Jun 29, 2021

PastaPastaPasta referenced this in commit 4522e15e2b on Jul 1, 2021

PastaPastaPasta referenced this in commit ca76a636f1 on Jul 1, 2021

PastaPastaPasta referenced this in commit 5e1c618484 on Jul 12, 2021

PastaPastaPasta referenced this in commit 915435ecbc on Jul 13, 2021

DrahtBot locked this on Dec 16, 2021