Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Aug 31, 2018
Merged

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

merged 2 commits into from
Aug 31, 2018

Conversation

practicalswift
Copy link
Contributor

@practicalswift practicalswift commented Aug 28, 2018 •
edited
Loading

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.

@practicalswift practicalswift changed the title tests: Don't assert(...) with side effects tests: Don't assert(...) with side effects Aug 28, 2018
@fanquake fanquake added the Tests label Aug 28, 2018
@practicalswift practicalswift mentioned this pull request Aug 28, 2018
Merged
@ken2812221
Copy link
Contributor

utACK 1916b4e

@skeees
Copy link
Contributor

skeees commented Aug 28, 2018

Thanks :) utACK 1916b4e
I don't know how I feel about the linter though - it is too narrowly scoped to ++|-- and won't catch anything inside of a BOOST_* style assert either.

If you're worried about somebody trying to run a node with NDEBUG - maybe add a more explanatory warning message https://github.com/bitcoin/bitcoin/blob/master/src/validation.cpp#L51 about why this could dangerous - instead of the very vague one that's there now?

@practicalswift
Copy link
Contributor Author

practicalswift commented Aug 28, 2018 •
edited
Loading

@skeees Yes, the regression test will only catch this specific type of side effect. The regression test is not meant to be exhaustive (regression tests seldom are! :-)).

FWIW, this is the third time this specific type of side effect is fixed during the past few months so having a check for this is better than no checking :-)

@practicalswift practicalswift force-pushed the assertions-with-side-effects branch from 1916b4e to ca1a093 Compare August 28, 2018 12:28
@practicalswift
Copy link
Contributor Author

practicalswift commented Aug 28, 2018 •
edited
Loading

@skeees Updated version based on your feedback with improved comments. Now catching also accidental assignment operations in assertions :-)

From PRE31-C (SEI CERT C Coding Standard):

Assertions should not contain assignments, increment, or decrement operators.

Now catching all three.

@practicalswift
Copy link
Contributor Author

@skeees Regarding BOOST_ASSERT(...) – we're not using it are we? :-)

@skeees
Copy link
Contributor

skeees commented Aug 28, 2018

Cool - utACK - I think this change makes things strictly better. Regarding BOOST_CHECK_* stuff - used in unit tests but not production code I believe

I do think its worth changing the #error warning message (or adding a comment nearby) to something that explains why bitcoin cannot be compiled without assertions. Right now it just states so without any reason. Perhaps:

In addition to providing important sanity checks on the internal state of the system, is possible that assert statements throughout bitcoin may contain side-effecting code that is consensus critical, therefore bitcoin may not be compiled without assertions

@practicalswift
Copy link
Contributor Author

@skeees BOOST_CHECK_* is ever used outside of the tests:

$ git grep BOOST_CHECK_ ":(exclude)*/test/*"
$

And I don't see any scenario where BOOST_CHECK_* could be removed at run-time like assert(…) under NDEBUG. Do you? :-)

@practicalswift practicalswift mentioned this pull request Aug 31, 2018
Closed
@laanwj
Copy link
Member

laanwj commented Aug 31, 2018

utACK ca1a093

And I don't see any scenario where BOOST_CHECK_* could be removed at run-time like assert(…) under NDEBUG. Do you? :-)

No—the problem does not exist for BOOST_CHECK_*, leave those alone please.

@laanwj laanwj merged commit ca1a093 into bitcoin:master Aug 31, 2018
laanwj added a commit that referenced this pull request Aug 31, 2018
@laanwj
Merge #14088: tests: Don't assert(...) with side effects
709a15b 
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
@Bushstar Bushstar mentioned this pull request Sep 4, 2018
Merged
@practicalswift practicalswift deleted the assertions-with-side-effects branch April 10, 2021 19:35
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 27, 2021
@laanwj @PastaPastaPasta
Merge bitcoin#14088: tests: Don't assert(...) with side effects
969de22 
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
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 28, 2021
@laanwj @PastaPastaPasta
Merge bitcoin#14088: tests: Don't assert(...) with side effects
b5a3283 
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
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 29, 2021
@laanwj @PastaPastaPasta
Merge bitcoin#14088: tests: Don't assert(...) with side effects
5303574 
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
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 29, 2021
@laanwj @PastaPastaPasta
Merge bitcoin#14088: tests: Don't assert(...) with side effects
60c7221 
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
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 29, 2021
@laanwj @PastaPastaPasta
Merge bitcoin#14088: tests: Don't assert(...) with side effects
689230b 
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
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 29, 2021
@laanwj @PastaPastaPasta
Merge bitcoin#14088: tests: Don't assert(...) with side effects
658d86e 
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
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 29, 2021
@laanwj @PastaPastaPasta
Merge bitcoin#14088: tests: Don't assert(...) with side effects
989e018 
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
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 1, 2021
@laanwj @PastaPastaPasta
Merge bitcoin#14088: tests: Don't assert(...) with side effects
cd5a39c 
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
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
Tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@practicalswift @ken2812221 @skeees @laanwj @fanquake