Don’t condition on expressions known already at compile time to always evaluate to true (or false). Use assertions instead. #14201

practicalswift commented at 9:23 PM on September 11, 2018: contributor

Don’t condition on expressions known already at compile time to always evaluate to true (or false). Use assertions instead.

Note to reviewers:

Some of these redundant checks might be indications of logical bugs in the existing code. If so they should be solved separately. Let me know if you spot such cases.
Although these conditions are known to be true in some cases we might have good reasons for doing a redundant check for documentation purposes or other reasons. Let me know if you think so.
Some of the assertions might be overkill since the preceding code trivially implies what we assert. The fMaster is such a case.

Feedback welcome :-)

practicalswift renamed this:
~~Don’t condition on expressions we know will be true (or false) already at compile time. Use assertions. [wip]~~
Don’t condition on expressions known already at compile time to always evaluate to true (or false). Use assertions instead. [wip]
on Sep 11, 2018

MarcoFalke commented at 9:37 PM on September 11, 2018: member

If they are known at compile time, they should use static_assert. Otherwise NACK

practicalswift commented at 9:45 PM on September 11, 2018: contributor

@MarcoFalke They are known at compile time by a sufficiently intelligent compiler, but I'm afraid we can't use static_assert since that requires the expressions to be contextually converted constant expression of type bool which they are not.

This PR is WIP: let's find which of these cases that might be indication of flaws in the current logic. fAllOk looks weird as an example. See these cases as smells of logic that could be wrong in the current code :-)

ryanofsky commented at 9:46 PM on September 11, 2018: member

If they are known at compile time, they should use static_assert

Static assert is not applicable in any of these cases.

Otherwise NACK

Would dispute this NACK. Generally these are nice code simplifications, though in many of these cases, I would probably just keep the simplifications and drop the asserts.

MarcoFalke commented at 10:17 PM on September 11, 2018: member

The code with the explicit conditions is robust to refactoring oversights. I don't see why we would want to trade that for run-time assertions that can potentially be triggered remotely after some accidentally careless refactoring.

ryanofsky commented at 10:24 PM on September 11, 2018: member

The code with the explicit conditions is robust to refactoring oversights

Like I said, I definitely would want to drop most of the assertions, but can you point to an example of a redundant condition which makes the code more robust?

MarcoFalke commented at 10:29 PM on September 11, 2018: member

The condition to hit bad-cb-missing allows the if-statement to swap places with the if-statement above it. With or without an assert this is no longer possible and we'd have to rely on tests or review to not break anything.

ryanofsky commented at 10:45 PM on September 11, 2018: member

Agreed in that case, but would still be happy to see code cleaned up in other cases. @practicalswift, it sounds like it's up to you to decide whether you want to make manual cleanups here and then remove the [wip] tag, or abandon the PR given concerns raised about the approach.

fanquake added the label Refactoring on Sep 11, 2018

DrahtBot commented at 11:16 PM on September 11, 2018: member

Note to reviewers: This pull request conflicts with the following ones:

#14156 ([WIP] refactor: Make explicit CMutableTransaction -> CTransaction conversion. by lucash-dev)
#10973 (Refactor: separate wallet from node by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

in src/checkqueue.h:93 in 32177e07dc outdated

  89 | @@ -90,8 +90,8 @@ class CCheckQueue
  90 |                          nTotal--;
  91 |                          bool fRet = fAllOk;
  92 |                          // reset the status for new work later
  93 | -                        if (fMaster)
  94 | -                            fAllOk = true;
  95 | +                        assert(fMaster);

Empact commented at 12:17 AM on September 12, 2018:

How about drop this assert? It's assured here by 89.

in src/net.cpp:1363 in 32177e07dc outdated

1358 | @@ -1359,8 +1359,7 @@ void CConnman::ThreadSocketHandler()
1359 |                  int nBytes = 0;
1360 |                  {
1361 |                      LOCK(pnode->cs_hSocket);
1362 | -                    if (pnode->hSocket == INVALID_SOCKET)
1363 | -                        continue;

Empact commented at 12:26 AM on September 12, 2018:

Could this socket have been closed in the time the lock was not held?

in src/script/sign.cpp:201 in 32177e07dc outdated

 197 | @@ -198,7 +198,8 @@ bool ProduceSignature(const SigningProvider& provider, const BaseSignatureCreato
 198 |          // and then the serialized subscript:
 199 |          subscript = CScript(result[0].begin(), result[0].end());
 200 |          sigdata.redeem_script = subscript;
 201 | -        solved = solved && SignStep(provider, creator, subscript, result, whichType, SigVersion::BASE, sigdata) && whichType != TX_SCRIPTHASH;
 202 | +        assert(solved);

Empact commented at 12:27 AM on September 12, 2018:

drop this?

in src/test/multisig_tests.cpp:127 in 32177e07dc outdated

 121 | @@ -122,8 +122,9 @@ BOOST_AUTO_TEST_CASE(multisig_verify)
 122 |              keys.assign(1,key[i]);
 123 |              keys.push_back(key[j]);
 124 |              s = sign_multisig(escrow, keys, txTo[2], 0);
 125 | -            if (i < j && i < 3 && j < 3)
 126 | +            if (i < j && j < 3)
 127 |              {
 128 | +                assert(i < 3);

Empact commented at 12:28 AM on September 12, 2018:

drop this?

in src/utilstrencodings.cpp:269 in 32177e07dc outdated

 265 | @@ -266,7 +266,8 @@ static bool ParsePrechecks(const std::string& str)
 266 |  {
 267 |      if (str.empty()) // No empty string allowed
 268 |          return false;
 269 | -    if (str.size() >= 1 && (isspace(str[0]) || isspace(str[str.size()-1]))) // No padding allowed
 270 | +    assert(str.size() >= 1);

Empact commented at 12:28 AM on September 12, 2018:

drop this?

practicalswift force-pushed on Sep 12, 2018

practicalswift commented at 8:30 AM on September 12, 2018: contributor

@ryanofsky @Empact @MarcoFalke Updated to address all feedback. Please re-review :-)

practicalswift renamed this:
~~Don’t condition on expressions known already at compile time to always evaluate to true (or false). Use assertions instead. [wip]~~
Don’t condition on expressions known already at compile time to always evaluate to true (or false). Use assertions instead.
on Sep 13, 2018

Don’t condition on expressions we know will be true (or false) already at compile time. Use assertions where appropriate. f478a4b156

practicalswift force-pushed on Sep 13, 2018

laanwj commented at 6:58 PM on September 13, 2018: member

I think it's bad to add assertions here; before you know it, you might introduce a DoS or other crash bug where none exists now. This change is, all in all, too risky.

laanwj closed this on Sep 13, 2018

gmaxwell commented at 7:00 PM on September 13, 2018: contributor

NAK. I just don't see an benefit to this change which would justify the small (but decidedly non-zero) review effort involved. In particular, if any one of these assertions in consensus code aren't guaranteed by the preceding code they can cause network wide DOS and consensus failure. so it must be reviewed carefully even if the review is simple. MarcoFalke's point that they make the code more brittle against refactoring is also worth keeping in mind.

In general we do want to include assertions in places where incorrect execution could cause an RCE or what not, but we need to take pretty good care that they don't introduce crashes where otherwise things would run fine.

sdaftuar commented at 7:11 PM on September 13, 2018: member

fwiw I'm also a concept NACK on this. Aside from the issue around the way we use asserts in this project (which we could probably document better for contributors in the developer notes, if anyone is up for it!), I generally think the review effort for minor changes to consensus critical code is not worth it unless we're achieving some larger goal than code cleanup, like a bugfix, new feature, or a refactor in support of future new feature. I think code comments are a much better way to explain consensus code that is correct but doesn't look pretty, as there is much lower review burden for doc-only PRs, and we can achieve a similar effect of making the code more understandable to a future reader.

practicalswift deleted the branch on Apr 10, 2021

DrahtBot locked this on Aug 18, 2022