mempool: Fix unintended unsigned integer wraparound in CTxMemPool::UpdateAncestorsOf(bool add, …) when add is false

[practicalswift]

Fix unintended unsigned integer wraparound in CTxMemPool::UpdateAncestorsOf(bool add, …) when add is false.

txmempool.cpp:219:44: runtime error: unsigned integer overflow: 18446744073709551615 * 155 cannot be represented in type 'unsigned long' !=

[sdaftuar]

Why did you decide to close this PR? After some digging I believe this change correctly fixes a bug, though I think the problem is not in the unsigned integer overflow (which is well-defined), but in casting the resulting unsigned integer to a signed type (which I have recently learned is not defined behavior, when the unsigned value would be negative).

So I think this change, which casts to int64_t first before multiplying by -1, is exactly correct.

[practicalswift]

@sdaftuar I received some feedback about my PR:s creating too much review work – that's the background to the close. But I'm happy to re-open :-)

[maflcko]

I haven't reviewed the claim that @sdaftuar makes, but I checked that on my machine the bitcoind compiled with clang doesn't change at all, and for gcc the objdump is the same.

[DrahtBot]

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #13804 (WIP: Transaction Pool Layer by MarcoFalke)

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.

[sdaftuar]

utACK

@ryanofsky pointed me to this: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2013/n3690.pdf, from section 4.7.3

If the destination type is signed, the value is unchanged if it can be represented in the destination type (and bit-field width); otherwise, the value is implementation-defined.

[practicalswift]

@sdaftuar @ryanofsky You might want to take a look at #11551 (Fix unsigned integer wrap-around in GetBlockProofEquivalentTime) and #11535 :-)

[DrahtBot]

Coverage	Change (pull 14226)	Reference (master)
Lines	-0.0552 %	87.0361 %
Functions	-0.0309 %	84.1130 %
Branches	-0.0464 %	51.5451 %