Don't assert(foo()) where foo() has side effects #13534

practicalswift · 2018-06-25T12:13:41Z

Don't assert(foo()) where foo has side effects.

From assert(3):

If the macro NDEBUG is defined at the moment <assert.h> was last included, the macro assert() generates no code, and hence does nothing at all.

Bitcoin currently cannot be compiled without assertions, but we shouldn't rely on that.

maflcko · 2018-06-25T12:20:55Z

Is this the only place in the code where this needs fixing?

maflcko · 2018-06-25T12:22:03Z

src/bench/block_assemble.cpp

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; }

maflcko · 2018-06-25T21:56:06Z

Again, I believe we use this excessively in other parts of the code. What makes you fix this specific instance?

practicalswift · 2018-06-26T12:53:30Z

@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/))'

maflcko · 2018-06-26T13:55:22Z

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());

practicalswift · 2018-06-26T15:21:57Z

@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 :-)

promag · 2018-06-29T07:06:35Z

Concept ACK.

Empact · 2018-07-02T20:37:35Z

src/httprpc.cpp

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.

Empact · 2018-07-02T20:39:01Z

src/bench/block_assemble.cpp

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 (comment) :P

Empact · 2018-07-02T20:39:35Z

Concept ACK

practicalswift · 2018-07-06T08:33:55Z

@Empact @laanwj @promag Thanks for reviewing. Updated. Please re-review :-)

Empact · 2018-07-06T10:47:20Z

src/bench/block_assemble.cpp

This will never fail on account of ->nNonce being in the while condition.

practicalswift · 2018-07-06T16:41:17Z

@Empact Good point. Feedback addressed. Please re-review :-)

Empact · 2018-07-06T21:20:55Z

src/bench/block_assemble.cpp

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

practicalswift · 2018-07-06T22:08:53Z

@Empact Please re-review :-)

Empact · 2018-07-07T01:11:40Z

src/bench/block_assemble.cpp

nit: could be ++block->nNonce as before

@Empact Updated. Please re-review :-)

Empact · 2018-07-07T01:12:12Z

utACK b0040a9

Empact · 2018-07-07T14:20:15Z

re-utACK 6ad0328

ca1a093 Add regression test: Don't assert(...) with side effects (practicalswift) 4c3c9c3 Don't assert(...) with side effects (practicalswift) Pull request description: Don't `assert(...)` with side effects. From the developer notes: > **Assertions should not have side-effects** > > Rationale: Even though the source code is set to refuse to compile with assertions disabled, having side-effects in assertions is unexpected and makes the code harder to understand These assertions were introduced quite recently (in #14069 which was merged two days ago) and since this is a recurring thing (see #13534 – "Don't assert(foo()) where foo() has side effects" from May) I added a simple regression test for the most obvious common side effect. Tree-SHA512: be65db9d8d5d0f5752152ba73fe3fbb0531880f156d3cd7dfdf1752709979b63214e46ae64b1adbe1e09fa121278f4087f4ae49bff16cf8f5aec16ea6bde3650

Summary: 6ad0328f1c Don't assert(foo()) where foo has side effects (practicalswift) Pull request description: Don't `assert(foo())` where `foo` has side effects. From `assert(3)`: > If the macro `NDEBUG` is defined at the moment `<assert.h>` was last included, the macro `assert()` generates no code, and hence does nothing at all. Bitcoin currently cannot be compiled without assertions, but we shouldn't rely on that. Tree-SHA512: 28cff0c6d1c2fb612ca58c9c94142ed01c5cfd0a2fecb8e59cdb6c270374b215d952ed3491d921d84dc1b439fa49da4f0e75e080f6adcbc6b0e08be14e54c170 Partial backport of Core PR13534 bitcoin/bitcoin#13534 Test Plan: make check Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc Reviewed By: Fabien, O1 Bitcoin ABC, #bitcoin_abc Differential Revision: https://reviews.bitcoinabc.org/D4736

6ad0328 Don't assert(foo()) where foo has side effects (practicalswift) Pull request description: Don't `assert(foo())` where `foo` has side effects. From `assert(3)`: > If the macro `NDEBUG` is defined at the moment `<assert.h>` was last included, the macro `assert()` generates no code, and hence does nothing at all. Bitcoin currently cannot be compiled without assertions, but we shouldn't rely on that. Tree-SHA512: 28cff0c6d1c2fb612ca58c9c94142ed01c5cfd0a2fecb8e59cdb6c270374b215d952ed3491d921d84dc1b439fa49da4f0e75e080f6adcbc6b0e08be14e54c170 # Conflicts: # src/bench/block_assemble.cpp # src/bench/checkblock.cpp # src/script/sign.cpp

ca1a093 Add regression test: Don't assert(...) with side effects (practicalswift) 4c3c9c3 Don't assert(...) with side effects (practicalswift) Pull request description: Don't `assert(...)` with side effects. From the developer notes: > **Assertions should not have side-effects** > > Rationale: Even though the source code is set to refuse to compile with assertions disabled, having side-effects in assertions is unexpected and makes the code harder to understand These assertions were introduced quite recently (in bitcoin#14069 which was merged two days ago) and since this is a recurring thing (see bitcoin#13534 – "Don't assert(foo()) where foo() has side effects" from May) I added a simple regression test for the most obvious common side effect. Tree-SHA512: be65db9d8d5d0f5752152ba73fe3fbb0531880f156d3cd7dfdf1752709979b63214e46ae64b1adbe1e09fa121278f4087f4ae49bff16cf8f5aec16ea6bde3650

6ad0328 Don't assert(foo()) where foo has side effects (practicalswift) Pull request description: Don't `assert(foo())` where `foo` has side effects. From `assert(3)`: > If the macro `NDEBUG` is defined at the moment `<assert.h>` was last included, the macro `assert()` generates no code, and hence does nothing at all. Bitcoin currently cannot be compiled without assertions, but we shouldn't rely on that. Tree-SHA512: 28cff0c6d1c2fb612ca58c9c94142ed01c5cfd0a2fecb8e59cdb6c270374b215d952ed3491d921d84dc1b439fa49da4f0e75e080f6adcbc6b0e08be14e54c170 # Conflicts: # src/bench/block_assemble.cpp # src/bench/checkblock.cpp # src/script/sign.cpp

practicalswift changed the title ~~Don't assert(foo()) where foo has side effects~~ Don't assert(foo()) where foo() has side effects Jun 25, 2018

fanquake added the Refactoring label Jun 25, 2018

maflcko reviewed Jun 25, 2018

View reviewed changes

practicalswift force-pushed the assert-with-a-side-effect branch from 5df902d to 6421b84 Compare June 25, 2018 14:47

practicalswift force-pushed the assert-with-a-side-effect branch from 6421b84 to 4728a85 Compare June 26, 2018 15:17

practicalswift force-pushed the assert-with-a-side-effect branch from 4728a85 to 9047ddb Compare June 26, 2018 15:23

Empact reviewed Jul 2, 2018

View reviewed changes

practicalswift force-pushed the assert-with-a-side-effect branch 2 times, most recently from 874dc9e to 9025145 Compare July 6, 2018 08:32

Empact reviewed Jul 6, 2018

View reviewed changes

src/bench/block_assemble.cpp Outdated

Copy link

Contributor

Empact Jul 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will never fail on account of ->nNonce being in the while condition.

practicalswift force-pushed the assert-with-a-side-effect branch from 9025145 to acd9c04 Compare July 6, 2018 16:34

Empact reviewed Jul 6, 2018

View reviewed changes

practicalswift force-pushed the assert-with-a-side-effect branch 2 times, most recently from 2ec358e to b0040a9 Compare July 6, 2018 22:07

Empact reviewed Jul 7, 2018

View reviewed changes

Don't assert(foo()) where foo has side effects

6ad0328

practicalswift force-pushed the assert-with-a-side-effect branch from b0040a9 to 6ad0328 Compare July 7, 2018 08:09

practicalswift mentioned this pull request Aug 28, 2018

tests: Don't assert(...) with side effects #14088

Merged

practicalswift deleted the assert-with-a-side-effect branch April 10, 2021 19:35

kwvg added a commit to kwvg/dash that referenced this pull request Oct 25, 2021

merge bitcoin#13534: Don't assert(foo()) where foo() has side effects

a86b67d

kwvg mentioned this pull request Oct 25, 2021

merge bitcoin#13219...#15779: benchmarks dashpay/dash#4511

Merged

11 tasks

pravblockc pushed a commit to pravblockc/dash that referenced this pull request Nov 18, 2021

merge bitcoin#13534: Don't assert(foo()) where foo() has side effects

cc96d5b

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

Don't assert(foo()) where foo() has side effects #13534

Don't assert(foo()) where foo() has side effects #13534

Uh oh!

Conversation

practicalswift commented Jun 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Jun 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Jun 25, 2018

Uh oh!

practicalswift commented Jun 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Jun 26, 2018

Uh oh!

practicalswift commented Jun 26, 2018

Uh oh!

promag commented Jun 29, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj Jul 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Empact Jul 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Empact commented Jul 2, 2018

Uh oh!

practicalswift commented Jul 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

practicalswift commented Jul 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

practicalswift commented Jul 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Empact commented Jul 7, 2018

Uh oh!

Empact commented Jul 7, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

practicalswift commented Jun 25, 2018 •

edited

Loading

practicalswift commented Jun 26, 2018 •

edited

Loading

laanwj Jul 4, 2018 •

edited

Loading

Empact Jul 2, 2018 •

edited

Loading