Fix for UB in getblocktemplate
Co-authored-by: Axel Gembe derago@gmail.com
Summary
This does code cleanup to fix the UB observed in issue #230 (closed). This bug was a regression introduced in !796 (merged).
The UB can happen with default values of nTimeLimit which lead to signed integer overflow. In order to fix the issue, the code has been switched over to using doubles for the time limit, and the fixed-point math has been changed to floating point math (which is easier to reason about). Also some code nits were added such as const-correcting a few things and more.
These UB issues can manifest themselves as crashes if building with
-DCMAKE_BUILD_TYPE=Debug
. I encourage reviewers to try building against
master in that mode and watching the functional tests that touch
getblocktemplate
, such as mining_basic
fail every time.
How to reproduce:
- Build master with
-DCMAKE_BUILD_TYPE=Debug
on Linux with gcc 8.x+ - Start bitcoind (testnet, testnet4, regtest, doesn't matter).
- Hit the rpc with
getblocktemplate
, see it crash withSIGABRT
.
Explanation: GCC's debug mode traps on overflows, as a conveneince,
that's why it SIGABRTS
.
This commit fixes that.
Test Plan
-
ninja all check-all
-
Attempt to reproduce the bug in issue #230 (closed) (may require your platform raise SIGABRT on overflow, something that some debuggers do, or that some compile modes do).
- For me this involved configuring with
-DCMAKE_BUILD_TYPE=Debug
on Debian 10 with GCC 8.3.0. Then none of the mining tests would work. Do this against master and it e.g.mining_basic
should fail. Do it against this commit and it works again.