Don't assert(foo()) where foo has side effects.
From assert(3):
If the macro
NDEBUGis defined at the moment<assert.h>was last included, the macroassert()generates no code, and hence does nothing at all.
Bitcoin currently cannot be compiled without assertions, but we shouldn't rely on that.
Is this the only place in the code where this needs fixing?
40 | @@ -41,7 +41,8 @@ static CTxIn MineBlock(const CScript& coinbase_scriptPubKey) 41 | auto block = PrepareBlock(coinbase_scriptPubKey); 42 | 43 | while (!CheckProofOfWork(block->GetHash(), block->nBits, Params().GetConsensus())) { 44 | - assert(++block->nNonce); 45 | + auto nNonce = ++block->nNonce; 46 | + assert(nNonce);
You can just remove the assert. This is regtest, so a wrap is "impossible" anyway.
Like this?
while (!CheckProofOfWork(block->GetHash(), block->nBits, Params().GetConsensus())) {
++block->nNonce;
}
sure
Again, I believe we use this excessively in other parts of the code. What makes you fix this specific instance?
@MarcoFalke This is the only instance I know of where the assert(…) includes a function or expression that has a side effect (or more specifically where the execution would differ between defined(NDEBUG) vs. !defined(NDEBUG) beyond assert(...) obviously being expanded to a noop in the former case). Can you find another example? :-)
Candidates:
git grep '[^_]assert(.*);' | \
grep -vE '^(build-aux/|src/(crypto/ctaes/|leveldb/|univalue/))'
They sound a bit like side-effects, haven't checked:
$ git grep 'assert(' src/bench/checkblock.cpp src/httprpc.cpp
src/bench/checkblock.cpp: assert(stream.Rewind(sizeof(block_bench::block413567)));
src/bench/checkblock.cpp: assert(stream.Rewind(sizeof(block_bench::block413567)));
src/bench/checkblock.cpp: assert(CheckBlock(block, validationState, chainParams->GetConsensus()));
src/httprpc.cpp: assert(EventBase());
@MarcoFalke ++block->nNonce case is special in that block is used after the assert(…) takes place. I've now included the cases you mentioned and two others. Please re-review :-)
Concept ACK.
239 | @@ -240,7 +240,8 @@ bool StartHTTPRPC() 240 | // ifdef can be removed once we switch to better endpoint support and API versioning 241 | RegisterHTTPHandler("/wallet/", false, HTTPReq_JSONRPC); 242 | #endif 243 | - assert(EventBase()); 244 | + auto eventBase = EventBase(); 245 | + assert(eventBase); 246 | httpRPCTimerInterface = MakeUnique<HTTPRPCTimerInterface>(EventBase());
nit: Could reuse eventBase on this line. The type struct event_base* could be informative, given it makes the assert's meaning clear.
I agree. If we assign this to a variable anyway, we should use that.
Note, though, that EventBase() has no side effects, it is fully a getter function.
40 | @@ -41,7 +41,7 @@ static CTxIn MineBlock(const CScript& coinbase_scriptPubKey) 41 | auto block = PrepareBlock(coinbase_scriptPubKey); 42 | 43 | while (!CheckProofOfWork(block->GetHash(), block->nBits, Params().GetConsensus())) { 44 | - assert(++block->nNonce); 45 | + ++block->nNonce;
The ->nNonce access has no effect without the assert. I would be in favor of keeping the assert on the following line. Or something like:
while (!CheckProofOfWork(block->GetHash(), block->nBits, Params().GetConsensus()) && ++block) {
assert(block->nNonce);
}
++block?
Note that this is as: assert(++(block->nNonce));
Thanks, yeah figured that out here: #13534 (review) :P
Concept ACK
39 | @@ -40,8 +40,8 @@ static CTxIn MineBlock(const CScript& coinbase_scriptPubKey) 40 | { 41 | auto block = PrepareBlock(coinbase_scriptPubKey); 42 | 43 | - while (!CheckProofOfWork(block->GetHash(), block->nBits, Params().GetConsensus())) { 44 | - assert(++block->nNonce); 45 | + while (!CheckProofOfWork(block->GetHash(), block->nBits, Params().GetConsensus()) && ++block->nNonce) { 46 | + assert(block->nNonce);
This will never fail on account of ->nNonce being in the while condition.
@Empact Good point. Feedback addressed. Please re-review :-)
39 | @@ -40,8 +40,8 @@ static CTxIn MineBlock(const CScript& coinbase_scriptPubKey) 40 | { 41 | auto block = PrepareBlock(coinbase_scriptPubKey); 42 | 43 | - while (!CheckProofOfWork(block->GetHash(), block->nBits, Params().GetConsensus())) { 44 | - assert(++block->nNonce); 45 | + while (!CheckProofOfWork(block->GetHash(), block->nBits, Params().GetConsensus()) && block->nNonce++) { 46 | + assert(block->nNonce);
Ok I realize I misread the precedence when I originally suggested to put it in the while condition. Sorry about that. Now I think it'd be better to put it in the block and let the assert do its thing. :P
@Empact Please re-review :-)
40 | @@ -41,7 +41,8 @@ static CTxIn MineBlock(const CScript& coinbase_scriptPubKey) 41 | auto block = PrepareBlock(coinbase_scriptPubKey); 42 | 43 | while (!CheckProofOfWork(block->GetHash(), block->nBits, Params().GetConsensus())) { 44 | - assert(++block->nNonce); 45 | + block->nNonce++;
nit: could be ++block->nNonce as before
@Empact Updated. Please re-review :-)
utACK b0040a9
re-utACK 6ad0328
utACK 6ad0328.
per our own style guide:
Assertions should not have side-effects
Several people have in the past swept through the code and fixed these things, and they should keep getting fixed, so CONCEPT ACK. Unfortunately, history has shown they keep creeping back in, thus the refusal to build without asserts. :)
utACK 6ad0328
utACK 6ad0328