Do not run functions with necessary side-effects in assert() #9344

gmaxwell commented at 1:51 AM on December 14, 2016: contributor

No description provided.

Do not run functions with necessary side-effects in assert() da9cdd2c9c

dcousens approved

fanquake added the label Refactoring on Dec 14, 2016

dcousens commented at 2:03 AM on December 14, 2016: contributor

Does the compiler, when removing asserts, remove the side effects too?

sipa commented at 2:16 AM on December 14, 2016: member

Does the compiler, when removing asserts, remove the side effects too?

Not the compiler, but assert(X); is defined as nothing in the C library headers when compiling without debug.

dcousens commented at 2:27 AM on December 14, 2016: contributor

s/compiler/C library headers

Right, ouch.

luke-jr commented at 2:37 AM on December 14, 2016: member

Is this going to produce "variable set but not used" warnings when compiled with NDEBUG? Is there a clean way to avoid those?

gmaxwell commented at 3:07 AM on December 14, 2016: contributor

Yes, turn off that nearly useless warning. :P The way to avoid it in gcc is to cast the variable to void in a statement of its own-- but doing so makes MSVC warn. There is no winning with overly paternalistic compiler warnings. Besides, being silently incorrect is a lot worse than a warning.

paveljanik commented at 6:44 AM on December 14, 2016: contributor

@luke-jr Is it producing this warning? In that case, the macro assert is written wrong.

But, anyway: we do not support compilation with NDEBUG ;-)

paveljanik commented at 6:46 AM on December 14, 2016: contributor

ACK da9cdd2

laanwj commented at 8:21 AM on December 14, 2016: member

utACK https://github.com/bitcoin/bitcoin/commit/da9cdd2c9cb58e3737b727c21f070f074b1247a7

This is not the first time side-effects snuck into assertions, and were subsequently removed. How can we avoid these from being introduced again?

The way to avoid it in gcc is to cast the variable to void in a statement of its own-- but doing so makes MSVC warn

The way to avoid it would be to do something with the result - e.g. at least logging a message if the result is negative, at most shutting down the node. But as long as we rely on asserts to be always enabled I guess that's not necessary - and the side-effects will also always happen.

in src/net_processing.cpp:None in da9cdd2c9c

1517 | @@ -1518,7 +1518,8 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
1518 |          }
1519 |  
1520 |          CBlock block;
1521 | -        assert(ReadBlockFromDisk(block, it->second, chainparams.GetConsensus()));
1522 | +        bool ret = ReadBlockFromDisk(block, it->second, chainparams.GetConsensus());

laanwj commented at 8:24 AM on December 14, 2016:

Was introduced in https://github.com/bitcoin/bitcoin/commit/da9cdd2c9cb58e3737b727c21f070f074b1247a7 by @TheBlueMatt

in src/net_processing.cpp:None in da9cdd2c9c

2730 | @@ -2730,7 +2731,8 @@ bool SendMessages(CNode* pto, CConnman& connman)
2731 |                              vHeaders.front().GetHash().ToString(), pto->id);
2732 |                      //TODO: Shouldn't need to reload block from disk, but requires refactor
2733 |                      CBlock block;
2734 | -                    assert(ReadBlockFromDisk(block, pBestIndex, consensusParams));

laanwj commented at 8:24 AM on December 14, 2016:

Introduced in same commit as above.

in src/validation.cpp:None in da9cdd2c9c

2099 | @@ -2100,7 +2100,8 @@ bool static DisconnectTip(CValidationState& state, const CChainParams& chainpara
2100 |          CCoinsViewCache view(pcoinsTip);
2101 |          if (!DisconnectBlock(block, state, pindexDelete, view))
2102 |              return error("DisconnectTip(): DisconnectBlock %s failed", pindexDelete->GetBlockHash().ToString());
2103 | -        assert(view.Flush());

laanwj commented at 8:26 AM on December 14, 2016:

Introduced by @sipa in 75f51f2a63e0ebe34ab290c2b7141dd240b98c3b

in src/validation.cpp:None in da9cdd2c9c

2189 | @@ -2189,7 +2190,8 @@ bool static ConnectTip(CValidationState& state, const CChainParams& chainparams,
2190 |          }
2191 |          nTime3 = GetTimeMicros(); nTimeConnectTotal += nTime3 - nTime2;
2192 |          LogPrint("bench", "  - Connect total: %.2fms [%.2fs]\n", (nTime3 - nTime2) * 0.001, nTimeConnectTotal * 0.000001);
2193 | -        assert(view.Flush());
2194 | +        bool flushed = view.Flush();
2195 | +        assert(flushed);

laanwj commented at 8:28 AM on December 14, 2016:

Introduced in same commit as above.

laanwj approved

laanwj merged this on Dec 14, 2016

laanwj closed this on Dec 14, 2016

laanwj referenced this in commit 82ccac739e on Dec 14, 2016

codablock referenced this in commit d84a074a7b on Jan 16, 2018

codablock referenced this in commit 2cfe601cc3 on Jan 16, 2018

codablock referenced this in commit 91dc005154 on Jan 17, 2018

gladcow referenced this in commit 6237b52635 on Mar 5, 2018

gladcow referenced this in commit e1109caf24 on Mar 8, 2018

gladcow referenced this in commit ec0c05074d on Mar 13, 2018

gladcow referenced this in commit 9f4d6843d3 on Mar 14, 2018

gladcow referenced this in commit 272d0e2c30 on Mar 15, 2018

gladcow referenced this in commit 1720c92bf2 on Mar 15, 2018

gladcow referenced this in commit 1a9fb6d50f on Mar 15, 2018

gladcow referenced this in commit 81b34d6327 on Mar 15, 2018

gladcow referenced this in commit 6925d767ac on Mar 24, 2018

gladcow referenced this in commit c7a4fc9671 on Apr 4, 2018

UdjinM6 referenced this in commit bc45a2f87a on Apr 11, 2018

andvgal referenced this in commit 33fef47fc8 on Jan 6, 2019

andvgal referenced this in commit fd5c50bc2b on Jan 6, 2019

CryptoCentric referenced this in commit 7da0aaf18f on Feb 25, 2019

CryptoCentric referenced this in commit dd3fd51204 on Feb 28, 2019

MarcoFalke locked this on Sep 8, 2021