@fanquake Refactoring? :-)

Concept ACK. Could enable the functional tests in travis in this commit?

@MarcoFalke Unfortunately it turned out that these were not the only undefined behaviours we're triggering in the functional tests. I'll submit a separate PR that enables the functional tests with suppressions for everything we're triggering :-)

Rebased! :-)

@fanquake Is refactoring really the correct label for this PR?

Definitions:

1. Code refactoring is defined as the process of restructuring existing computer code without changing its external behaviour.
2. Triggering undefined behaviour is defined as executing computer code whose behaviour is not prescribed by the language specification to which the code adheres, for the current state of the program.

Since undefined behaviour is a superset of external behaviour it follows that a PR fixing UB cannot be considered refactoring, no? :-)

I suggest labelling all UB fixes with the bug label so that they get the attention they deserve.

@fanquake Ping? :-)

@fanquake I'm afraid the "refactoring" label is causing the UB:s (this and others) to go under the radar and thus remain unfixed.

Please consider giving the UB PR:s a more appropriate label :-) Please at least talk to me my fellow contributor – I'm your friend :-)

Could remove the suppression from the file in contrib?

Added commit 8ed2af7ca980345995ed4115570e4677ed0b2f47 which remove the UBSan suppressions for the fixed files.

Please re-review :-)

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

Rebased!

@gmaxwell

The value representation of floating-point types is implementation-defined.

For an IEEE 754 implementation (std::numeric_limits<double>::is_iec559 == true), floating point division by zero is well defined.

Otherwise it is undefined.

Please correct me if I'm wrong.

I am not familiar with the code base so take this with a grain of salt. I was curious about the change in validation.cpp though, and I thought another pair of eyes cannot hurt. From my observation:

1. A search of the repo indicates nBlocksTotal is only referenced in validation.cpp.
2. nBlocksTotal is only used in 2 functions, ConnectBlock and ConnectTip.
3. nBlocksTotal is initialized to 0, is incremented on another line of code, and the other 11 instances it is a divisor in a LogPrint.
4. The change made causes the nBlocksTotal counter to include the genesis block, so it will always be one higher than prior to this change. Since it is only used in logging statements, this should not affect functionality at all.
5. The logging statements in ConnectBlock would not have been vulnerable to this division by zero risk as they would not have been executed for the genesis block due to the return statement for that special case.
6. The logging statements in ConnectTip might be affected by division by zero as ConnectTip is called by ActivateBestChain, which is called from several places in the code base. I didn't study it further to see if it would ever be called prior to ConnectBlock() for the second time, but given the several instances of calling it sprinkled throughout the code base, it is a good idea to protect against division by zero. [as an aside, would division by zero in a logging statement terminate the process and kill the node? although in this particular example, it'd happen immediately after validating just the first couple of blocks so not really that critical]

This change appears worthwhile and doesn't seem harmful to me.

NAK

@gmaxwell Why?

@gmaxwell In C++11, division by zero is undefined behavior. Of course all architectures we currently support very likely follow IEEE 754 which makes it well-defined, so calling this a fix for actual UB is an exaggeration.

Still, I think it's an improvement to the codebase in my opinion.

Because the behaviour isn't undefined. If you cannot manage to get the basis motivation for the change factually correct what confidence should we have that the change does not break consensus or somehow introduce a vulnerability? Given that the PR doesn't give a (valid) justification for an improvement to the software, it isn't clear to me that it's worth review efforts to determine that its actually free of harmful side-effects and certainly shouldn't be made without such review, it certainly isn't worth yet another worthless language wonking debate.

@gmaxwell You didn't see my reply from yesterday? :-)

The value representation of floating-point types is implementation-defined.

For an IEEE 754 implementation (std::numeric_limits<double>::is_iec559 == true), floating point division by zero is well defined.

Otherwise it is undefined.

Please correct me if I'm wrong.

@practicalswift Please, don't turn a discussion about language semantics into an absolute what is UB and what not.

I am fairly confident that on all platforms we support, this is not UB, as they follow IEEE 754.

It may be desirable to strictly follow C++11 here, but that's a code quality discussion, not one about software bugs.

@sipa I simply refuted @gmaxwell's technically incorrect claim. I'm allowed intellectual self-defence, right? :-)

@practicalswift I don't see any incorrect claim or refutation of that. I only see a disagreement about what language requirements to assume.

@sipa That was the point I made yesterday :-)

@sipa In C++11, while allowing all possible implementations and ignoring language annexes, assuming an "int" can represent a value outside of [-32768, 32767] is undefined behaviour. Do you intend to accept PRs going through the codebase to fix all code relative to that?

With these change the codebase is still not unconditionally correct or safe on some hypothetical system which has arbitrary non-IEEE-754-like semantics (e.g. go look at the fee estimator calculations). Nor do I think it's plausible to make it so without just eliminating the floating point entirely. (and, as an aside, if we wanted to limit the codebase to the language without optional appendexes like annex f then we'd have to eliminate floating point regardless)

This is a silence-a-tool make-work change-- of low but non-zero value-- being disguised as a fix of a serious and concerning problem. The result of handling it that way is that it both gets inadequate review and diverts attention from more useful activities, causing an increased risk of actual bugs.

@practicalswift I understand you're on a crusade against anything that smells like UB under the strictest definitions of the language. I support improvements in that regard, but discussions like this only piss people off.

We're writing software for real systems, and even have the luxury of rather tight control over what architectures are supported. There are plenty of assumptions made that restrict correct operation to certain systems. A certain subset of those also mean that things detected by ubsan and others are spurious.

Sorry, but claiming that a fix is needed "because UB in C++" is crying wolf. Especially when not taking the reality into account, it is overly dramatic, distracts from far more important issues, and won't get you much goodwill from reviewers.

I think this is a meaningful code quality improvement, but you just justify it as "this lets us remove the silencing of divide-by-zero in ubsan, making it a more powerful check for future issues, and it's just a small change" - not as "UB! Must fix at all costs", and then turn it into an endless semantics discussion.

Alternative suggestion:

Add a static_assert(std::numeric_limits<double>::is_iec559 == true) to the code, and remove the explicit enabling of float division by zero in ubsan.

@sipa Yes, that's a good idea -- I thought about taking that route too. As long as we're verifying the assumptions we're making I'm happy (obviously).