From e5aa9f617ba7091eb539e295aed4095ab2a7fddb Mon Sep 17 00:00:00 2001 From: Duke Leto Date: Fri, 22 Jun 2018 05:18:22 +0000 Subject: [PATCH 1/4] Fix absurd fee bug reported in #3281, with tests --- qa/rpc-tests/wallet.py | 56 ++++++++++++++++++++++++++++++++++++++++ src/wallet/rpcwallet.cpp | 19 +++++++++++--- 2 files changed, 71 insertions(+), 4 deletions(-) diff --git a/qa/rpc-tests/wallet.py b/qa/rpc-tests/wallet.py index 158a77c88..4578b1025 100755 --- a/qa/rpc-tests/wallet.py +++ b/qa/rpc-tests/wallet.py @@ -463,6 +463,62 @@ class WalletTest (BitcoinTestFramework): assert_equal("not an integer" in errorString, True); + myzaddr = self.nodes[0].z_getnewaddress() + recipients = [ {"address": myzaddr, "amount": Decimal('0.0') } ] + errorString = '' + + # Make sure that amount=0 transactions can use the default fee + # without triggering "absurd fee" errors + try: + myopid = self.nodes[0].z_sendmany(myzaddr, recipients) + assert(myopid) + except JSONRPCException,e: + errorString = e.error['message'] + print errorString + assert(False) + + # This fee is larger than the default fee and since amount=0 + # it should trigger error + fee = Decimal('0.1') + recipients = [ {"address": myzaddr, "amount": Decimal('0.0') } ] + minconf = 1 + errorString = '' + + try: + myopid = self.nodes[0].z_sendmany(myzaddr, recipients, minconf, fee) + except JSONRPCException,e: + errorString = e.error['message'] + assert('Small transaction amount' in errorString) + + #### This fee is less than default and greater than amount, but still valid + fee = Decimal('0.0000001') + recipients = [ {"address": myzaddr, "amount": Decimal('0.00000001') } ] + minconf = 1 + errorString = '' + + try: + myopid = self.nodes[0].z_sendmany(myzaddr, recipients, minconf, fee) + assert(myopid) + except JSONRPCException,e: + errorString = e.error['message'] + print errorString + assert(False) + + ### Make sure amount=0, fee=0 transaction are valid to add to mempool + # though miners decide whether to add to a block + fee = Decimal('0.0') + minconf = 1 + recipients = [ {"address": myzaddr, "amount": Decimal('0.0') } ] + errorString = '' + + try: + myopid = self.nodes[0].z_sendmany(myzaddr, recipients, minconf, fee) + assert(myopid) + except JSONRPCException,e: + errorString = e.error['message'] + print errorString + assert(False) + if __name__ == '__main__': WalletTest ().main () diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 5476ce247..cef9f569e 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -3698,7 +3698,9 @@ UniValue z_sendmany(const UniValue& params, bool fHelp) } // Fee in Zatoshis, not currency format) - CAmount nFee = ASYNC_RPC_OPERATION_DEFAULT_MINERS_FEE; + CAmount nFee = ASYNC_RPC_OPERATION_DEFAULT_MINERS_FEE; + CAmount nDefaultFee = nFee; + if (params.size() > 3) { if (params[3].get_real() == 0.0) { nFee = 0; @@ -3707,9 +3709,18 @@ UniValue z_sendmany(const UniValue& params, bool fHelp) } // Check that the user specified fee is sane. - if (nFee > nTotalOut) { - throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Fee %s is greater than the sum of outputs %s", FormatMoney(nFee), FormatMoney(nTotalOut))); - } + // This allows amount=0 (and all amount < nDefaultFee) transactions to use the default network fee + // or anything less than nDefaultFee instead of being forced to use a custom fee and leak metadata + if (nTotalOut < nDefaultFee) { + if (nFee > nDefaultFee) { + throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Small transaction amount %s has fee %s that is greater than the default fee %s", FormatMoney(nTotalOut), FormatMoney(nFee), FormatMoney(nDefaultFee))); + } + } else { + // Check that the user specified fee is sane. + if (nFee > nTotalOut) { + throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Fee %s is greater than the sum of outputs %s", FormatMoney(nFee), FormatMoney(nTotalOut))); + } + } } // Use input parameters as the optional context info to be returned by z_getoperationstatus and z_getoperationresult. From 75bb5f94f4b7a348d81545d3d360c81826ab7a58 Mon Sep 17 00:00:00 2001 From: Duke Leto Date: Fri, 13 Jul 2018 19:19:16 +0000 Subject: [PATCH 2/4] Update comment as per @arielgabizon --- src/wallet/rpcwallet.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index cef9f569e..22f203494 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -3716,7 +3716,7 @@ UniValue z_sendmany(const UniValue& params, bool fHelp) throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Small transaction amount %s has fee %s that is greater than the default fee %s", FormatMoney(nTotalOut), FormatMoney(nFee), FormatMoney(nDefaultFee))); } } else { - // Check that the user specified fee is sane. + // Check that the user specified fee is sane and also greater than the default fee. if (nFee > nTotalOut) { throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Fee %s is greater than the sum of outputs %s", FormatMoney(nFee), FormatMoney(nTotalOut))); } From 4b8c52c65c2219ee0bcecd23487419c83a0c70e6 Mon Sep 17 00:00:00 2001 From: Duke Leto Date: Sat, 14 Jul 2018 05:16:28 +0000 Subject: [PATCH 3/4] Improve error message --- src/wallet/rpcwallet.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 22f203494..9ef51b9db 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -3716,9 +3716,9 @@ UniValue z_sendmany(const UniValue& params, bool fHelp) throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Small transaction amount %s has fee %s that is greater than the default fee %s", FormatMoney(nTotalOut), FormatMoney(nFee), FormatMoney(nDefaultFee))); } } else { - // Check that the user specified fee is sane and also greater than the default fee. + // Check that the user specified fee is sane. if (nFee > nTotalOut) { - throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Fee %s is greater than the sum of outputs %s", FormatMoney(nFee), FormatMoney(nTotalOut))); + throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Fee %s is greater than the sum of outputs %s and also greater than the default fee", FormatMoney(nFee), FormatMoney(nTotalOut))); } } } From 0b6eeac33020073b8b92849023be37a8416bbf00 Mon Sep 17 00:00:00 2001 From: Simon Date: Tue, 17 Jul 2018 13:00:42 -0700 Subject: [PATCH 4/4] Update and fix per review comments, the test for absurd fee. --- qa/rpc-tests/wallet.py | 6 +++--- src/wallet/rpcwallet.cpp | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/qa/rpc-tests/wallet.py b/qa/rpc-tests/wallet.py index 4578b1025..642590b81 100755 --- a/qa/rpc-tests/wallet.py +++ b/qa/rpc-tests/wallet.py @@ -488,9 +488,9 @@ class WalletTest (BitcoinTestFramework): myopid = self.nodes[0].z_sendmany(myzaddr, recipients, minconf, fee) except JSONRPCException,e: errorString = e.error['message'] - assert('Small transaction amount' in errorString) + assert('Small transaction amount' in errorString) - #### This fee is less than default and greater than amount, but still valid + # This fee is less than default and greater than amount, but still valid fee = Decimal('0.0000001') recipients = [ {"address": myzaddr, "amount": Decimal('0.00000001') } ] minconf = 1 @@ -504,7 +504,7 @@ class WalletTest (BitcoinTestFramework): print errorString assert(False) - ### Make sure amount=0, fee=0 transaction are valid to add to mempool + # Make sure amount=0, fee=0 transaction are valid to add to mempool # though miners decide whether to add to a block fee = Decimal('0.0') minconf = 1 diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 9ef51b9db..7564d7753 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -3708,7 +3708,7 @@ UniValue z_sendmany(const UniValue& params, bool fHelp) nFee = AmountFromValue( params[3] ); } - // Check that the user specified fee is sane. + // Check that the user specified fee is not absurd. // This allows amount=0 (and all amount < nDefaultFee) transactions to use the default network fee // or anything less than nDefaultFee instead of being forced to use a custom fee and leak metadata if (nTotalOut < nDefaultFee) { @@ -3716,7 +3716,7 @@ UniValue z_sendmany(const UniValue& params, bool fHelp) throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Small transaction amount %s has fee %s that is greater than the default fee %s", FormatMoney(nTotalOut), FormatMoney(nFee), FormatMoney(nDefaultFee))); } } else { - // Check that the user specified fee is sane. + // Check that the user specified fee is not absurd. if (nFee > nTotalOut) { throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Fee %s is greater than the sum of outputs %s and also greater than the default fee", FormatMoney(nFee), FormatMoney(nTotalOut))); }