Merge #18383: refactor: Check for overflow when calculating sum of tx outputs

f65c9ad40f2f5cdc581bdaf72e7dc68e9d7f7a80 Check for overflow when calculating sum of outputs (Elichai Turkel)

Pull request description:

  This was reported by practicalswift here #18046
  The exact order of the if, is important, we first do `!MoneyRange(tx_out.nValue)` to make sure the amount is non-negative. and then `std::numeric_limits<CAmount>::max() - tx_out.nValue < nValueOut` checks that the addition cannot overflow (if we won't check that the amount is positive this check can also overflow! (by doing something like `max - -max`))
  and only then we make sure that the some is also valid `!MoneyRange(nValueOut + tx_out.nValue)`
  if any of these conditions fail we throw.

  the overflowing logic:
  ```
  a + b > max // we want to fail if a+b is more than the maximum -> will overflow
  b > max - a
  max - a < b
  ```

  Closes: #18046

ACKs for top commit:
  MarcoFalke:
    ACK f65c9ad40f2f5cdc581bdaf72e7dc68e9d7f7a80, checked that clang with O2 produces identical binaries 💕
  practicalswift:
    ACK f65c9ad40f2f5cdc581bdaf72e7dc68e9d7f7a80
  instagibbs:
    utACK https://github.com/bitcoin/bitcoin/pull/18383/commits/f65c9ad40f2f5cdc581bdaf72e7dc68e9d7f7a80
  vasild:
    ACK f65c9ad40f2f5cdc581bdaf72e7dc68e9d7f7a80 modulo `s/assert.h/cassert/`

Tree-SHA512: 512d6cf4762f24c41cf9a38da486b17b19c634fa3f4efbdebfe6608779e96fc3014d5d2d29adb8001e113152c0217bbd5b3900ac4edc7b8abe77f82f36209e33

1 files changed, 5 insertions, 2 deletions

diff --git a/src/primitives/transaction.cpp b/src/primitives/transaction.cpp
index 28c145f71d..6e72c1f15c 100644
--- a/src/primitives/transaction.cpp
+++ b/src/primitives/transaction.cpp
@@ -9,6 +9,8 @@
 #include <tinyformat.h>
 #include <util/strencodings.h>
 
+#include <assert.h>
+
 std::string COutPoint::ToString() const
 {
     return strprintf("COutPoint(%s, %u)", hash.ToString().substr(0,10), n);
@@ -84,10 +86,11 @@ CAmount CTransaction::GetValueOut() const
 {
     CAmount nValueOut = 0;
     for (const auto& tx_out : vout) {
-        nValueOut += tx_out.nValue;
-        if (!MoneyRange(tx_out.nValue) || !MoneyRange(nValueOut))
+        if (!MoneyRange(tx_out.nValue) || !MoneyRange(nValueOut + tx_out.nValue))
             throw std::runtime_error(std::string(__func__) + ": value out of range");
+        nValueOut += tx_out.nValue;
     }
+    assert(MoneyRange(nValueOut));
     return nValueOut;
 }