Make -checkmempool=1 not fail through int32 overflow #6896

pull sipa wants to merge 1 commits into bitcoin:master from sipa:fixchainsize changing 1 files +1 −1

sipa commented at 2:09 AM on October 28, 2015: member

Fix a bug in #6776 discovered by @gmaxwell: -checkmempool=1 causes the internal 32-bit variable to overflow to zero.
Make -checkmempool=1 not fail through int32 overflow e9e616323b
in src/txmempool.h:None in e9e616323b
```
 359 | @@ -360,7 +360,7 @@ class CTxMemPool
 360 |       * check does nothing.
 361 |       */
 362 |      void check(const CCoinsViewCache *pcoins) const;
 363 | -    void setSanityCheck(double dFrequency = 1.0) { nCheckFrequency = dFrequency * 4294967296.0; }
 364 | +    void setSanityCheck(double dFrequency = 1.0) { nCheckFrequency = dFrequency * 4294967295.0; }
```
dcousens commented at 3:45 AM on October 28, 2015:

Any reason not to cast explicitly? std::static_cast<int>(dFrequency * 4294967295.0)?

IMHO it might have made this more obvious to reviewers.

jgarzik commented at 12:44 AM on October 29, 2015:

Agree w/ @dcousens

Additionally, raw big numbers with no explanation comment should be avoided. I know it's 2^32 but it's not immediately obvious to every code reader.

This should be uint_max-1 and getdouble() should be similar.

Diapolo commented at 9:02 AM on October 31, 2015:

@jgarzik Agreed
dcousens commented at 3:47 AM on October 28, 2015: contributor

ACK
laanwj commented at 9:13 AM on October 28, 2015: member

Oops. Probably needs a range check, >1 and <0 will still silently overflow.
dcousens commented at 10:04 AM on October 28, 2015: contributor

@laanwj I think the issue was that it would overflow to 0. At least that was my understanding.

In any case, a range check would be sane. Or just min(max(dFrequency, 0), 1)
laanwj commented at 10:25 AM on October 28, 2015: member

Isn't any overflow an issue?
instagibbs commented at 2:37 PM on October 28, 2015: member

Right now it doesn't appear overflow as currently used(post-fix), but probably a good idea to internally check.
laanwj commented at 11:37 PM on October 30, 2015: member

Apparently a range clamp is already done in init.cpp. That's good enough, going to merge this.
laanwj merged this on Oct 30, 2015
laanwj closed this on Oct 30, 2015
laanwj referenced this in commit d482c0a7b2 on Oct 30, 2015
furszy referenced this in commit eb00d0f62f on Jun 14, 2020
DrahtBot locked this on Sep 8, 2021

Contributors

Linked (view graph)

#6776 Support -checkmempool=N, which runs checks once every N transactions