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

1. 
   practicalswift commented at 3:32 pm on November 16, 2020: contributor

   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):

   https://github.com/bitcoin/bitcoin/blob/54f812d9d29893c690ae06b84aaeab128186aa36/src/util/moneystr.cpp#L12-L31

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

   Context: #20383 (review)

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

   These are the “expectations of least surprise”:

   0BOOST_CHECK_EQUAL(FormatMoney(std::numeric_limits<CAmount>::max()), "92233720368.54775807");       //  9223372036854775807
   1BOOST_CHECK_EQUAL(FormatMoney(std::numeric_limits<CAmount>::max() - 1), "92233720368.54775806");   //  9223372036854775806
   2BOOST_CHECK_EQUAL(FormatMoney(std::numeric_limits<CAmount>::max() - 2), "92233720368.54775805");   //  9223372036854775805
   3BOOST_CHECK_EQUAL(FormatMoney(std::numeric_limits<CAmount>::max() - 3), "92233720368.54775804");   //  9223372036854775804
   4BOOST_CHECK_EQUAL(FormatMoney(std::numeric_limits<CAmount>::min() + 3), "-92233720368.54775805");  // -9223372036854775805
   5BOOST_CHECK_EQUAL(FormatMoney(std::numeric_limits<CAmount>::min() + 2), "-92233720368.54775806");  // -9223372036854775806
   6BOOST_CHECK_EQUAL(FormatMoney(std::numeric_limits<CAmount>::min() + 1), "-92233720368.54775807");  // -9223372036854775807
   7BOOST_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.

2. practicalswift renamed this:
   Invalid integer negation in FormatMoney(CAmount n) when n == std::numeric_limits<CAmount>::min()
   Invalid integer negation in FormatMoney(CAmount n) when n = std::numeric_limits<CAmount>::min()
   on Nov 16, 2020
3. 
   practicalswift commented at 4:25 pm on November 16, 2020: contributor

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

   It needs the same fix :)

4. practicalswift renamed this:
   Invalid integer negation in FormatMoney(CAmount n) when n = std::numeric_limits<CAmount>::min()
   Invalid integer negation in FormatMoney(CAmount n) and ValueFromAmount (CAmount n) when n = std::numeric_limits<CAmount>::min()
   on Nov 16, 2020
5. 
   practicalswift commented at 2:54 pm on December 2, 2020: contributor

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

   0$ git checkout master
   1$ CC=clang CXX=clang++ ./configure --with-sanitizers=address,undefined
   2$ make
   3$ src/bitcoind &
   4$ src/bitcoin-cli decoderawtransaction ff0000000001000000000000008004ff0400fffe001f00
   5core_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
   6SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior core_write.cpp:21:29 in
   7error: couldn't parse reply from server
   

6. 
   practicalswift commented at 3:11 pm on December 2, 2020: contributor

   The JSON response by the Bitcoin Core RPC interface:

    0{
    1   "result" : {
    2      "size" : 23,
    3      "txid" : "cc0e5ecf6a06e4587bd011770c3ea4ceab58e86a234dd049ea3a27fab6fccfaa",
    4      "locktime" : 2031870,
    5      "hash" : "cc0e5ecf6a06e4587bd011770c3ea4ceab58e86a234dd049ea3a27fab6fccfaa",
    6      "vsize" : 23,
    7      "weight" : 92,
    8      "vout" : [
    9         {
   10            "scriptPubKey" : {
   11               "hex" : "ff0400ff",
   12               "asm" : "OP_INVALIDOPCODE [error]",
   13               "type" : "nonstandard"
   14            },
   15            "n" : 0,
   16            "value" : --92233720368.-54775808
   17         }
   18      ],
   19      "version" : 255,
   20      "vin" : []
   21   },
   22   "error" : null,
   23   "id" : 1
   24}
   

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

7. laanwj closed this on Mar 3, 2021

   ---

8. DrahtBot locked this on Aug 18, 2022

Contributors
practicalswift