Implement new policy for VERIFY_CHECK and #ifdef VERIFY (issue #1381)

theStack commented at 3:00 pm on August 4, 2023: contributor

As suggested in #1381, this PR reworks the policy for VERIFY_CHECK and when to use #ifdef VERIFY, by:

redefining VERIFY_CHECK to empty in production (non-VERIFY) mode
removing many then superflous #ifdef VERIFY blocks (if they exclusively contained VERIFY_CHECKs)
introducing uppercase macros around verify_ functions and using them for better readabiliy

What is not included yet is the proposed renaming from “_check” to “_assert”:

And while we’re touching this anyway, we could consider renaming “check” to “assert”, which is a more precise term. (In fact, if we redefine VERIFY_CHECK to be empty in production, we have almost reimplemented assert.h…)

This should be easy to achieve with simple search-and-replace (e.g. using sed), but I was hesitant as this would probably case annoying merge conflicts on some of the open PRs. Happy to add this if the rename if desired (#1381 didn’t get any feedback about the renaming idea yet).

theStack force-pushed on Aug 16, 2023

theStack commented at 12:02 pm on August 16, 2023: contributor

Rebased on master (since #1348 has been merged) and updated the PR description accordingly.

real-or-random added the label refactor/smell on Aug 16, 2023

theStack force-pushed on Aug 18, 2023

theStack commented at 12:40 pm on August 18, 2023: contributor

Rebased on master again and introduced an uppercase macro for secp256k1_scalar_verify as well (introduced in recently merged #1373).

theStack force-pushed on Aug 18, 2023

theStack force-pushed on Sep 21, 2023

theStack commented at 2:35 am on September 21, 2023: contributor

Rebased on master (CI was failing previously).

in src/util.h:141 in c723b85285 outdated

142 #define VERIFY_CHECK CHECK
143 #define VERIFY_SETUP(stmt) do { stmt; } while(0)
144 #else
145-#define VERIFY_CHECK(cond) do { (void)(cond); } while(0)
146+#define VERIFY_CHECK(cond)
147 #define VERIFY_SETUP(stmt)

stratospher commented at 3:01 am on November 30, 2023:

c723b85: what does VERIFY_SETUP do? it looks really similar to VERIFY_CHECK and used in only 2 places in the codebase.

real-or-random commented at 11:35 am on November 30, 2023:

The only two uses trace back to 72ae443afb536221a8523646ea4a88162341c3df where the macro was introduced.

My thinking is that the purpose of VERIFY_SETUP is to wrap code whose only purpose is to perform side effects (“setup”) for further VERIFY_CHECKs. But assuming this interpretation, I don’t really understand 72ae443afb536221a8523646ea4a88162341c3df.

The effect of the two existing VERIFY_SETUP uses is that they initialize r->x and r->y but that’s done anyway in the current code, immediately afterward: https://github.com/bitcoin-core/secp256k1/blob/5814d8485cbb074115c03b587b6799a812e0f267/src/ecmult_const_impl.h#L90-L95 @sipa Is there a reason why you kept these in #1184?

Anyway, I suggest:

Drop the two uses of VERIFY_SETUP.
Drop VERIFY_SETUP. (Our implicit convention for such code is to nest it in an #ifdef VERIFY block.)

sipa commented at 1:20 pm on November 30, 2023:

The VERIFY_SETUP calls in ecmult_const_impl.h were made obsolete long before that, in #754, but somehow survived since. They can clearly be removed, indeed.

in src/scalar_8x32_impl.h:183 in 33314ca3b1 outdated

178@@ -179,9 +179,8 @@ static void secp256k1_scalar_cadd_bit(secp256k1_scalar *r, unsigned int bit, int
179     r->d[7] = t & 0xFFFFFFFFULL;
180 
181     secp256k1_scalar_verify(r);
182-#ifdef VERIFY
183     VERIFY_CHECK((t >> 32) == 0);
184-#endif
185+    VERIFY_CHECK(secp256k1_scalar_check_overflow(r) == 0);

stratospher commented at 3:14 am on November 30, 2023:

33314ca: can we remove this since it’s already done inside secp256k1_scalar_verify?

real-or-random commented at 11:31 am on November 30, 2023:

This will be resolved by a rebase because it has already been done in #1184, see #1184 (review).

theStack commented at 1:00 am on December 1, 2023:

33314ca: can we remove this since it’s already done inside secp256k1_scalar_verify?

Oops, no idea why I added this line in a commit which is supposed to only remove lines (and only add blank ones for better readability). Removed now.

This will be resolved by a rebase because it has already been done in #1184, see #1184 (comment). @real-or-random: The comment link refers to the same expression, but in a different function. The one in the review comment (being in secp256k1_scalar_cadd_bit) was indeed still there after the rebase.

redefine VERIFY_CHECK to empty in production (non-VERIFY) mode

As suggested in issue #1381, this will make things simpler and
improve code readability, as we don't need to force omitting of
evaluations on a case-by-case basis anymore and hence can remove
lots of `#ifdef VERIFY`/`#endif` lines (see next commit). Plus,
VERIFY_CHECK behaves now identical in both non-VERIFY and coverage mode,
making the latter not special anymore and hopefully decreasing
maintenance burden. The idea of "side-effect safety" is given up.

Note that at two places in the ellswift module void-casts of return
values have to be inserted for non-VERIFY builds, in order to avoid
   "variable ... set but not used [-Wunused-but-set-variable]"
warnings.

c2688f8de9

remove superfluous `#ifdef VERIFY`/`#endif` preprocessor conditions

Now that the `VERIFY_CHECK` compiles to empty in non-VERIFY mode, blocks
that only consist of these macros don't need surrounding `#ifdef VERIFY`
conditions anymore.

At some places intentional blank lines are inserted for grouping and
better readadbility.

5d89bc031b

introduce and use SECP256K1_{FE,GE,GEJ}_VERIFY macros

By providing an uppercase variant of these verification functions, it is
better visible that it is test code and surrounding `#ifdef VERIFY`
blocks can be removed (if there is no other code around that could
remain in production mode), as they don't serve their purpose any more.

At some places intentional blank lines are inserted for grouping and
better readadbility.

cf25c86d05

introduce and use SECP256K1_SCALAR_VERIFY macro

By providing an uppercase variant of these verification functions,
it is better visible that it is test code.

a0fb68a2e7

remove unneeded VERIFY_SETUP uses in ECMULT_CONST_TABLE_GET_GE macro

As the fields r->x and r->y are set immediately after (three lines
below), there is no need to clear them.

a3a3e11acd

remove VERIFY_SETUP define

This define was seemingly introduced for VERIFY mode code with side
effects (for setup purposes), that should just be executed without any
checks. The same can be achieved by putting it in an `#if VERIFY` block,
so we can remove it.

bb4672342e

theStack force-pushed on Dec 1, 2023

theStack commented at 0:49 am on December 1, 2023: contributor

Rebased on master (taking the new function _scalar_half into account, introduced in #1184), fixed #1393 (review) and added commits to drop VERIFY_SETUP as suggested.

stratospher commented at 6:24 am on December 1, 2023: contributor

ACK bb46723.

real-or-random approved

real-or-random commented at 11:57 am on December 1, 2023: contributor

utACK bb4672342efce7fae1cfd30e007c6835a25286a7

It looks a bit strange that the new uppercase macros have a SECP256K1_ prefix, but our existing macros don’t. If you ask me, we should probably add a prefix to everything, but that could be controversial since it touches many lines. Let’s not hold this PR up. edit: Tracked in https://github.com/bitcoin-core/secp256k1/issues/1449

real-or-random merged this on Dec 1, 2023

real-or-random closed this on Dec 1, 2023

theStack deleted the branch on Dec 1, 2023

Implement new policy for VERIFY_CHECK and #ifdef VERIFY (issue #1381) #1393