util: Add Assume() identity function by maflcko · Pull Request #20255 · bitcoin/bitcoin

maflcko · 2020-10-27T18:29:17Z

This is needed for #20138. Please refer to the added documentation for motivation.

laanwj · 2020-10-27T19:09:10Z

Concept ACK. It's mildly interesting that we're sort of reinventing the original assert() here, which gets disabled on NDEBUG, which we had to always force on, but I think it's fine to reinvent the wheel here instead of assuming any specific compiler semantics.

maflcko · 2020-10-27T19:14:12Z

Indeed, this Assume is like the traditional assert.

jnewbery · 2020-10-27T19:29:47Z

Concept ACK. If you're willing to entertain paint color requests, I prefer the name Check(), as was originally proposed in #16136.

maflcko · 2020-10-27T19:36:10Z

I think @ryanofsky also likes check, so I might change to that

jnewbery · 2020-10-27T19:40:55Z

I think Russ doesn't care about the name, but would prefer a pair of check/dcheck macros: #16136 (comment)

sipa · 2020-10-27T19:43:34Z

FWIW, libsecp256k1 has CHECK() (always enabled) and VERIFY_CHECK() functions (enabled with -DVERIFY, used in test builds).

practicalswift · 2020-10-27T21:41:32Z

Concept ACK

maflcko · 2020-10-28T08:16:23Z

FWIW, libsecp256k1 has CHECK() (always enabled) and VERIFY_CHECK() functions (enabled with -DVERIFY, used in test builds).

That is good to know, because that means we can't use that name due to name-clashing.

ajtowns

ACK fa96726

I think this is a good choice of behaviour that should be easy to use correctly. Would like to see the ability to turn on logging of violated Assumptions even in production builds in a followup.

doc/developer-notes.md

practicalswift · 2020-10-29T06:37:54Z

ACK fa96726: patch looks correct

hebasto

Approach ACK fa96726

Are there any cases when assert is preferable to Assert. If "yes", it would be nice to have an example in the docs. If "no", why do we mention assert in the docs?

Fixes the compile error when used inside operator[]: ./chain.h:404:23: error: C++11 only allows consecutive left square brackets when introducing an attribute return (*this)[Assert(pindex)->nHeight] == pindex; ^

The TODO has been added by me, but I don't remember how to solve it. The current code works fine, so just remove the TODO.

maflcko · 2020-11-24T09:13:52Z

Are there any cases when assert is preferable to Assert

No

If "no", why do we mention assert in the docs?

It documents how the assert is used in existing code

practicalswift · 2020-11-24T09:25:55Z

cr ACK faa0585

jnewbery · 2020-11-24T09:35:02Z

utACK faa0585

jonatack

Approach ACK, some doc nits if you retouch

jonatack · 2020-11-24T09:42:44Z

doc/developer-notes.md

+  failure, it will throw an exception, which can be caught to recover from the
+  error.
+   - For example, a nullptr dereference or any other logic bug in RPC code
+     means that the RPC code is faulty and can not be executed. However, the


s/can not/cannot/

jonatack · 2020-11-24T09:43:21Z

doc/developer-notes.md

+  behaves like `Assert`/`assert` to notify developers and testers about
+  nonfatal errors. In production it doesn't warn or log anything, though the
+  expression is always evaluated.
+   - For example it can be assumed that a variable is only initialized once,


s/For example it can/For example, it may/

maflcko · 2020-11-25T12:54:39Z

Will leave style nits alone to preserve ACKs

maflcko · 2020-12-04T08:10:00Z

I think this is ready, but I am waiting on one more (re?)ACK before merge

hebasto

ACK faa0585, I have reviewed the code and it looks OK, I agree it can be merged.

faa0585 util: Remove probably misleading TODO (MarcoFalke) fac5efe util: Add Assume() identity function (MarcoFalke) fa86156 util: Allow Assert(...) to be used in all contexts (practicalswift) Pull request description: This is needed for bitcoin#20138. Please refer to the added documentation for motivation. ACKs for top commit: practicalswift: cr ACK faa0585 jnewbery: utACK faa0585 hebasto: ACK faa0585, I have reviewed the code and it looks OK, I agree it can be merged. Tree-SHA512: 72165fbd898b92ab9a79b070993fa1faa86c2e3545b6645e72c652bda295d5107bc298d0482bf3aaf0926fc0c3e6418a445c0e073b08568c44231f547f76a688

Summary: ``` This is needed for #20138. Please refer to the added documentation for motivation. ``` Backport of [[bitcoin/bitcoin#20255 | core#20255]]. Depends on D9681. Comes with a linter fix for an edge case where a macro defining `((void)(val))` would be linted as `(()(val))`. Ref T1611. Test Plan: With and without debug: ninja all check Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Maniphest Tasks: T1611 Differential Revision: https://reviews.bitcoinabc.org/D9680

maflcko mentioned this pull request Oct 27, 2020

net: Assume that SetCommonVersion is called at most once per peer #20138

Merged

DrahtBot added Build system Docs Utils/log/libs labels Oct 27, 2020

maflcko removed Build system Docs labels Oct 28, 2020

ajtowns reviewed Oct 29, 2020

View reviewed changes

doc/developer-notes.md Outdated Show resolved Hide resolved

doc/developer-notes.md Show resolved Hide resolved

hebasto mentioned this pull request Oct 30, 2020

Add an optional extra level of checking: ASSUME(...) - an opt-in side-effect safe assert(...) #16136

Closed

hebasto reviewed Oct 30, 2020

View reviewed changes

jnewbery mentioned this pull request Nov 22, 2020

Periodically make block-relay connections and sync headers #19858

Merged

practicalswift and others added 3 commits November 24, 2020 09:46

util: Allow Assert(...) to be used in all contexts

fa86156

Fixes the compile error when used inside operator[]: ./chain.h:404:23: error: C++11 only allows consecutive left square brackets when introducing an attribute return (*this)[Assert(pindex)->nHeight] == pindex; ^

util: Add Assume() identity function

fac5efe

util: Remove probably misleading TODO

faa0585

The TODO has been added by me, but I don't remember how to solve it. The current code works fine, so just remove the TODO.

maflcko force-pushed the 2010-assume branch from fa96726 to faa0585 Compare November 24, 2020 09:10

jonatack reviewed Nov 24, 2020

View reviewed changes

hebasto approved these changes Dec 4, 2020

View reviewed changes

maflcko merged commit dca80ff into bitcoin:master Dec 4, 2020

maflcko deleted the 2010-assume branch December 4, 2020 10:09

sdaftuar mentioned this pull request Jan 8, 2021

locks and docs in ATMP and CheckInputsFromMempoolAndCache #20834

Merged

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Conversation

maflcko commented Oct 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Oct 27, 2020

Uh oh!

maflcko commented Oct 27, 2020

Uh oh!

jnewbery commented Oct 27, 2020

Uh oh!

maflcko commented Oct 27, 2020

Uh oh!

jnewbery commented Oct 27, 2020

Uh oh!

sipa commented Oct 27, 2020

Uh oh!

practicalswift commented Oct 27, 2020

Uh oh!

maflcko commented Oct 28, 2020

Uh oh!

ajtowns left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

practicalswift commented Oct 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Nov 24, 2020

Uh oh!

practicalswift commented Nov 24, 2020

Uh oh!

jnewbery commented Nov 24, 2020

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack Nov 24, 2020

Choose a reason for hiding this comment

Uh oh!

jonatack Nov 24, 2020

Choose a reason for hiding this comment

Uh oh!

maflcko commented Nov 25, 2020

Uh oh!

maflcko commented Dec 4, 2020

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

maflcko commented Oct 27, 2020 •

edited

Loading

practicalswift commented Oct 29, 2020 •

edited

Loading