Invalid integer negation in FormatMoney(CAmount n) and ValueFromAmount (CAmount n) when n = std::numeric_limits<CAmount>::min()

[practicalswift]

FormatMoney(CAmount n) is not being properly defined for n = std::numeric_limits<CAmount>::min(). This is due to an invalid integer negation: the negation of -9223372036854775808 cannot be represented in type CAmount (which has a max value of 9223372036854775807):

bitcoin/src/util/moneystr.cpp

Lines 12 to 31 in 54f812d

	std::string FormatMoney(const CAmount& n)
	{
	// Note: not using straight sprintf here because we do NOT want
	// localized number formatting.
	int64_t n_abs = (n > 0 ? n : -n);
	int64_t quotient = n_abs/COIN;
	int64_t remainder = n_abs%COIN;
	std::string str = strprintf("%d.%08d", quotient, remainder);
	// Right-trim excess zeros before the decimal point:
	int nTrim = 0;
	for (int i = str.size()-1; (str[i] == '0' && IsDigit(str[i-2])); --i)
	++nTrim;
	if (nTrim)
	str.erase(str.size()-nTrim, nTrim);
	if (n < 0)
	str.insert((unsigned int)0, 1, '-');
	return str;
	}

FWIW FormatMoney(-9223372036854775808) returns --92233720368.-54775808 when compiled with UBSan (LLVM/Clang, -fsanitize=undefined).

Context: #20383 (comment)

It would be nice if FormatMoney(CAmount n) was properly defined for all CAmount n :)

These are the "expectations of least surprise":

BOOST_CHECK_EQUAL(FormatMoney(std::numeric_limits<CAmount>::max()), "92233720368.54775807");       //  9223372036854775807
BOOST_CHECK_EQUAL(FormatMoney(std::numeric_limits<CAmount>::max() - 1), "92233720368.54775806");   //  9223372036854775806
BOOST_CHECK_EQUAL(FormatMoney(std::numeric_limits<CAmount>::max() - 2), "92233720368.54775805");   //  9223372036854775805
BOOST_CHECK_EQUAL(FormatMoney(std::numeric_limits<CAmount>::max() - 3), "92233720368.54775804");   //  9223372036854775804
BOOST_CHECK_EQUAL(FormatMoney(std::numeric_limits<CAmount>::min() + 3), "-92233720368.54775805");  // -9223372036854775805
BOOST_CHECK_EQUAL(FormatMoney(std::numeric_limits<CAmount>::min() + 2), "-92233720368.54775806");  // -9223372036854775806
BOOST_CHECK_EQUAL(FormatMoney(std::numeric_limits<CAmount>::min() + 1), "-92233720368.54775807");  // -9223372036854775807
BOOST_CHECK_EQUAL(FormatMoney(std::numeric_limits<CAmount>::min()), "-92233720368.54775808");      // -9223372036854775808

These assertions are guaranteed to hold for all cases above except for the last one.

[practicalswift]

I think we have exactly the same small problem for UniValue ValueFromAmount(const CAmount& amount).

It needs the same fix :)

[practicalswift]

When fuzzing the RPC interface I stumbled upon this case again: decoderawtransaction ff0000000001000000000000008004ff0400fffe001f00 will trigger the problematic code path :)

$ git checkout master
$ CC=clang CXX=clang++ ./configure --with-sanitizers=address,undefined
$ make
$ src/bitcoind &
$ src/bitcoin-cli decoderawtransaction ff0000000001000000000000008004ff0400fffe001f00
core_write.cpp:21:29: runtime error: negation of -9223372036854775808 cannot be represented in type 'CAmount' (aka 'long'); cast to an unsigned type to negate this value to itself
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior core_write.cpp:21:29 in
error: couldn't parse reply from server

[practicalswift]

The JSON response by the Bitcoin Core RPC interface:

{
   "result" : {
      "size" : 23,
      "txid" : "cc0e5ecf6a06e4587bd011770c3ea4ceab58e86a234dd049ea3a27fab6fccfaa",
      "locktime" : 2031870,
      "hash" : "cc0e5ecf6a06e4587bd011770c3ea4ceab58e86a234dd049ea3a27fab6fccfaa",
      "vsize" : 23,
      "weight" : 92,
      "vout" : [
         {
            "scriptPubKey" : {
               "hex" : "ff0400ff",
               "asm" : "OP_INVALIDOPCODE [error]",
               "type" : "nonstandard"
            },
            "n" : 0,
            "value" : --92233720368.-54775808
         }
      ],
      "version" : 255,
      "vin" : []
   },
   "error" : null,
   "id" : 1
}

Notice the malformed result.vout[0].value field (--92233720368.-54775808) :)