From 4b4b9a8de662e6327f78aaf04a1e83d45713c06a Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Thu, 4 Jun 2015 14:22:26 +0200 Subject: [PATCH 1/3] Don't go through double in AmountFromValue and ValueFromAmount My prime gripe with JSON spirit was that monetary values still had to be converted from and to floating point which can cause deviations (see #3759 and https://bitcoin.stackexchange.com/questions/22716/bitcoind-sendfrom-round-amount-error). As UniValue stores internal values as strings, this is no longer necessary. This avoids risky double-to-integer and integer-to-double conversions completely, and results in more elegant code to boot. --- src/rpcserver.cpp | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/src/rpcserver.cpp b/src/rpcserver.cpp index e6bf008618287..11312ff63ea03 100644 --- a/src/rpcserver.cpp +++ b/src/rpcserver.cpp @@ -11,6 +11,7 @@ #include "sync.h" #include "ui_interface.h" #include "util.h" +#include "utilmoneystr.h" #include "utilstrencodings.h" #ifdef ENABLE_WALLET #include "wallet/wallet.h" @@ -118,25 +119,21 @@ void RPCTypeCheckObj(const UniValue& o, } } -static inline int64_t roundint64(double d) -{ - return (int64_t)(d > 0 ? d + 0.5 : d - 0.5); -} - CAmount AmountFromValue(const UniValue& value) { - double dAmount = value.get_real(); - if (dAmount <= 0.0 || dAmount > 21000000.0) - throw JSONRPCError(RPC_TYPE_ERROR, "Invalid amount"); - CAmount nAmount = roundint64(dAmount * COIN); - if (!MoneyRange(nAmount)) + if (!value.isReal() && !value.isNum()) + throw JSONRPCError(RPC_TYPE_ERROR, "Amount is not a number"); + CAmount amount; + if (!ParseMoney(value.getValStr(), amount)) throw JSONRPCError(RPC_TYPE_ERROR, "Invalid amount"); - return nAmount; + if (!MoneyRange(amount)) + throw JSONRPCError(RPC_TYPE_ERROR, "Amount out of range"); + return amount; } UniValue ValueFromAmount(const CAmount& amount) { - return (double)amount / (double)COIN; + return UniValue(UniValue::VREAL, FormatMoney(amount, false)); } uint256 ParseHashV(const UniValue& v, string strName) From a04bdefbebf9c6eee12712ccf8ef0b20e993da4a Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Thu, 4 Jun 2015 14:43:02 +0200 Subject: [PATCH 2/3] Get rid of fPlus argument to FormatMoney It's never used with any other value than false, the default. --- src/rpcserver.cpp | 2 +- src/test/util_tests.cpp | 44 ++++++++++++++++++++--------------------- src/utilmoneystr.cpp | 4 +--- src/utilmoneystr.h | 2 +- 4 files changed, 24 insertions(+), 28 deletions(-) diff --git a/src/rpcserver.cpp b/src/rpcserver.cpp index 11312ff63ea03..3894dd08bba86 100644 --- a/src/rpcserver.cpp +++ b/src/rpcserver.cpp @@ -133,7 +133,7 @@ CAmount AmountFromValue(const UniValue& value) UniValue ValueFromAmount(const CAmount& amount) { - return UniValue(UniValue::VREAL, FormatMoney(amount, false)); + return UniValue(UniValue::VREAL, FormatMoney(amount)); } uint256 ParseHashV(const UniValue& v, string strName) diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp index 1779bcdd952e5..2d445f426c02b 100644 --- a/src/test/util_tests.cpp +++ b/src/test/util_tests.cpp @@ -146,29 +146,27 @@ BOOST_AUTO_TEST_CASE(util_GetArg) BOOST_AUTO_TEST_CASE(util_FormatMoney) { - BOOST_CHECK_EQUAL(FormatMoney(0, false), "0.00"); - BOOST_CHECK_EQUAL(FormatMoney((COIN/10000)*123456789, false), "12345.6789"); - BOOST_CHECK_EQUAL(FormatMoney(COIN, true), "+1.00"); - BOOST_CHECK_EQUAL(FormatMoney(-COIN, false), "-1.00"); - BOOST_CHECK_EQUAL(FormatMoney(-COIN, true), "-1.00"); - - BOOST_CHECK_EQUAL(FormatMoney(COIN*100000000, false), "100000000.00"); - BOOST_CHECK_EQUAL(FormatMoney(COIN*10000000, false), "10000000.00"); - BOOST_CHECK_EQUAL(FormatMoney(COIN*1000000, false), "1000000.00"); - BOOST_CHECK_EQUAL(FormatMoney(COIN*100000, false), "100000.00"); - BOOST_CHECK_EQUAL(FormatMoney(COIN*10000, false), "10000.00"); - BOOST_CHECK_EQUAL(FormatMoney(COIN*1000, false), "1000.00"); - BOOST_CHECK_EQUAL(FormatMoney(COIN*100, false), "100.00"); - BOOST_CHECK_EQUAL(FormatMoney(COIN*10, false), "10.00"); - BOOST_CHECK_EQUAL(FormatMoney(COIN, false), "1.00"); - BOOST_CHECK_EQUAL(FormatMoney(COIN/10, false), "0.10"); - BOOST_CHECK_EQUAL(FormatMoney(COIN/100, false), "0.01"); - BOOST_CHECK_EQUAL(FormatMoney(COIN/1000, false), "0.001"); - BOOST_CHECK_EQUAL(FormatMoney(COIN/10000, false), "0.0001"); - BOOST_CHECK_EQUAL(FormatMoney(COIN/100000, false), "0.00001"); - BOOST_CHECK_EQUAL(FormatMoney(COIN/1000000, false), "0.000001"); - BOOST_CHECK_EQUAL(FormatMoney(COIN/10000000, false), "0.0000001"); - BOOST_CHECK_EQUAL(FormatMoney(COIN/100000000, false), "0.00000001"); + BOOST_CHECK_EQUAL(FormatMoney(0), "0.00"); + BOOST_CHECK_EQUAL(FormatMoney((COIN/10000)*123456789), "12345.6789"); + BOOST_CHECK_EQUAL(FormatMoney(-COIN), "-1.00"); + + BOOST_CHECK_EQUAL(FormatMoney(COIN*100000000), "100000000.00"); + BOOST_CHECK_EQUAL(FormatMoney(COIN*10000000), "10000000.00"); + BOOST_CHECK_EQUAL(FormatMoney(COIN*1000000), "1000000.00"); + BOOST_CHECK_EQUAL(FormatMoney(COIN*100000), "100000.00"); + BOOST_CHECK_EQUAL(FormatMoney(COIN*10000), "10000.00"); + BOOST_CHECK_EQUAL(FormatMoney(COIN*1000), "1000.00"); + BOOST_CHECK_EQUAL(FormatMoney(COIN*100), "100.00"); + BOOST_CHECK_EQUAL(FormatMoney(COIN*10), "10.00"); + BOOST_CHECK_EQUAL(FormatMoney(COIN), "1.00"); + BOOST_CHECK_EQUAL(FormatMoney(COIN/10), "0.10"); + BOOST_CHECK_EQUAL(FormatMoney(COIN/100), "0.01"); + BOOST_CHECK_EQUAL(FormatMoney(COIN/1000), "0.001"); + BOOST_CHECK_EQUAL(FormatMoney(COIN/10000), "0.0001"); + BOOST_CHECK_EQUAL(FormatMoney(COIN/100000), "0.00001"); + BOOST_CHECK_EQUAL(FormatMoney(COIN/1000000), "0.000001"); + BOOST_CHECK_EQUAL(FormatMoney(COIN/10000000), "0.0000001"); + BOOST_CHECK_EQUAL(FormatMoney(COIN/100000000), "0.00000001"); } BOOST_AUTO_TEST_CASE(util_ParseMoney) diff --git a/src/utilmoneystr.cpp b/src/utilmoneystr.cpp index 2fbc048878f32..0f3203432f6e1 100644 --- a/src/utilmoneystr.cpp +++ b/src/utilmoneystr.cpp @@ -11,7 +11,7 @@ using namespace std; -string FormatMoney(const CAmount& n, bool fPlus) +std::string FormatMoney(const CAmount& n) { // Note: not using straight sprintf here because we do NOT want // localized number formatting. @@ -29,8 +29,6 @@ string FormatMoney(const CAmount& n, bool fPlus) if (n < 0) str.insert((unsigned int)0, 1, '-'); - else if (fPlus && n > 0) - str.insert((unsigned int)0, 1, '+'); return str; } diff --git a/src/utilmoneystr.h b/src/utilmoneystr.h index cd9b04810d968..99c3ba83061b7 100644 --- a/src/utilmoneystr.h +++ b/src/utilmoneystr.h @@ -14,7 +14,7 @@ #include "amount.h" -std::string FormatMoney(const CAmount& n, bool fPlus=false); +std::string FormatMoney(const CAmount& n); bool ParseMoney(const std::string& str, CAmount& nRet); bool ParseMoney(const char* pszIn, CAmount& nRet); From 7d8ffac18612fe9a5faea3f8fb41442f9e7a3c1d Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Sat, 6 Jun 2015 09:41:12 +0200 Subject: [PATCH 3/3] Changes necessary now that zero values accepted in AmountFromValue - Add an accept test for zero amounts, and a reject test for negative amounts - Remove ugly hack in `settxfee` that is no longer necessary - Do explicit zero checks in wallet RPC functions - Don't add a check for zero amounts in `createrawtransaction` - this could be seen as a feature --- src/test/rpc_tests.cpp | 3 +++ src/wallet/rpcwallet.cpp | 14 ++++++++++---- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/src/test/rpc_tests.cpp b/src/test/rpc_tests.cpp index 0b33ddb0869fb..08f988fdbf7fc 100644 --- a/src/test/rpc_tests.cpp +++ b/src/test/rpc_tests.cpp @@ -131,6 +131,9 @@ static UniValue ValueFromString(const std::string &str) BOOST_AUTO_TEST_CASE(rpc_parse_monetary_values) { + BOOST_CHECK_THROW(AmountFromValue(ValueFromString("-0.00000001")), UniValue); + BOOST_CHECK_EQUAL(AmountFromValue(ValueFromString("0")), 0LL); + BOOST_CHECK_EQUAL(AmountFromValue(ValueFromString("0.00000000")), 0LL); BOOST_CHECK_EQUAL(AmountFromValue(ValueFromString("0.00000001")), 1LL); BOOST_CHECK_EQUAL(AmountFromValue(ValueFromString("0.17622195")), 17622195LL); BOOST_CHECK_EQUAL(AmountFromValue(ValueFromString("0.5")), 50000000LL); diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index e44940993e10b..d284fcf15c253 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -414,6 +414,8 @@ UniValue sendtoaddress(const UniValue& params, bool fHelp) // Amount CAmount nAmount = AmountFromValue(params[1]); + if (nAmount <= 0) + throw JSONRPCError(RPC_TYPE_ERROR, "Invalid amount for send"); // Wallet comments CWalletTx wtx; @@ -809,6 +811,8 @@ UniValue movecmd(const UniValue& params, bool fHelp) string strFrom = AccountFromValue(params[0]); string strTo = AccountFromValue(params[1]); CAmount nAmount = AmountFromValue(params[2]); + if (nAmount <= 0) + throw JSONRPCError(RPC_TYPE_ERROR, "Invalid amount for send"); if (params.size() > 3) // unused parameter, used to be nMinDepth, keep type-checking it though (void)params[3].get_int(); @@ -888,6 +892,8 @@ UniValue sendfrom(const UniValue& params, bool fHelp) if (!address.IsValid()) throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid Bitcoin address"); CAmount nAmount = AmountFromValue(params[2]); + if (nAmount <= 0) + throw JSONRPCError(RPC_TYPE_ERROR, "Invalid amount for send"); int nMinDepth = 1; if (params.size() > 3) nMinDepth = params[3].get_int(); @@ -987,6 +993,8 @@ UniValue sendmany(const UniValue& params, bool fHelp) CScript scriptPubKey = GetScriptForDestination(address.Get()); CAmount nAmount = AmountFromValue(sendTo[name_]); + if (nAmount <= 0) + throw JSONRPCError(RPC_TYPE_ERROR, "Invalid amount for send"); totalAmount += nAmount; bool fSubtractFeeFromAmount = false; @@ -2168,9 +2176,7 @@ UniValue settxfee(const UniValue& params, bool fHelp) LOCK2(cs_main, pwalletMain->cs_wallet); // Amount - CAmount nAmount = 0; - if (params[0].get_real() != 0.0) - nAmount = AmountFromValue(params[0]); // rejects 0.0 amounts + CAmount nAmount = AmountFromValue(params[0]); payTxFee = CFeeRate(nAmount, 1000); return true; @@ -2352,4 +2358,4 @@ UniValue listunspent(const UniValue& params, bool fHelp) } return results; -} \ No newline at end of file +}