From da6d93916d1d1080e6be7f589dd84a8b5848e1c6 Mon Sep 17 00:00:00 2001 From: Simon Date: Thu, 4 May 2017 11:35:08 -0700 Subject: [PATCH 1/8] Add option 'mempooltxinputlimit' so the mempool can reject a transaction based on the number of transparent inputs. --- src/init.cpp | 12 ++++++++++++ src/main.cpp | 11 +++++++++++ 2 files changed, 23 insertions(+) diff --git a/src/init.cpp b/src/init.cpp index ec3553c4..c3b4dc45 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -332,6 +332,7 @@ std::string HelpMessage(HelpMessageMode mode) strUsage += HelpMessageOpt("-dbcache=", strprintf(_("Set database cache size in megabytes (%d to %d, default: %d)"), nMinDbCache, nMaxDbCache, nDefaultDbCache)); strUsage += HelpMessageOpt("-loadblock=", _("Imports blocks from external blk000??.dat file") + " " + _("on startup")); strUsage += HelpMessageOpt("-maxorphantx=", strprintf(_("Keep at most unconnectable transactions in memory (default: %u)"), DEFAULT_MAX_ORPHAN_TRANSACTIONS)); + strUsage += HelpMessageOpt("-mempooltxinputlimit=", _("Set the maximum number of transparent inputs in a transaction that the mempool will accept (default: 0 = no limit applied)")); strUsage += HelpMessageOpt("-par=", strprintf(_("Set the number of script verification threads (%u to %d, 0 = auto, <0 = leave that many cores free, default: %d)"), -(int)boost::thread::hardware_concurrency(), MAX_SCRIPTCHECK_THREADS, DEFAULT_SCRIPTCHECK_THREADS)); #ifndef WIN32 @@ -1009,6 +1010,17 @@ bool AppInit2(boost::thread_group& threadGroup, CScheduler& scheduler) } #endif + // Default value of 0 for mempooltxinputlimit means no limit is applied + if (mapArgs.count("-mempooltxinputlimit")) { + int64_t limit = GetArg("-mempooltxinputlimit", 0); + if (limit < 0) { + return InitError(_("Mempool limit on transparent inputs to a transaction cannot be negative")); + } else if (limit > 0) { + LogPrintf("Mempool configured to reject transactions with greater than %lld transparent inputs\n", limit); + } + } + + // ********************************************************* Step 4: application initialization: dir lock, daemonize, pidfile, debug log // Initialize libsodium diff --git a/src/main.cpp b/src/main.cpp index ab91ee41..e6897030 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -1057,6 +1057,17 @@ bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState &state, const CTransa if (pfMissingInputs) *pfMissingInputs = false; + // Node operator can choose to reject tx by number of transparent inputs + static_assert(std::numeric_limits::max() >= std::numeric_limits::max(), "size_t too small"); + size_t limit = (size_t) GetArg("-mempooltxinputlimit", 0); + if (limit > 0) { + size_t n = tx.vin.size(); + if (n > limit) { + LogPrint("mempool", "Dropping txid %s : too many transparent inputs %zu > limit %zu\n", tx.GetHash().ToString(), n, limit ); + return false; + } + } + auto verifier = libzcash::ProofVerifier::Strict(); if (!CheckTransaction(tx, state, verifier)) return error("AcceptToMemoryPool: CheckTransaction failed"); From 5799c5f8c02cabe964970207b1d00708e7bbe25a Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Sat, 17 Jun 2017 14:59:16 +1200 Subject: [PATCH 2/8] Add test for -mempooltxinputlimit --- src/gtest/test_mempool.cpp | 45 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/src/gtest/test_mempool.cpp b/src/gtest/test_mempool.cpp index 88b0e259..e84b2dea 100644 --- a/src/gtest/test_mempool.cpp +++ b/src/gtest/test_mempool.cpp @@ -1,10 +1,13 @@ #include #include +#include "consensus/validation.h" #include "core_io.h" +#include "main.h" #include "primitives/transaction.h" #include "txmempool.h" #include "policy/fees.h" +#include "util.h" // Fake the input of transaction 5295156213414ed77f6e538e7e8ebe14492156906b9fe995b242477818789364 // - 532639cc6bebed47c1c69ae36dd498c68a012e74ad12729adbd3dbb56f8f3f4a, 0 @@ -87,3 +90,45 @@ TEST(Mempool, PriorityStatsDoNotCrash) { EXPECT_EQ(dPriority, MAX_PRIORITY); } + +TEST(Mempool, TxInputLimit) { + CTxMemPool pool(::minRelayTxFee); + bool missingInputs; + + // Create an obviously-invalid transaction + CMutableTransaction mtx; + mtx.nVersion = 0; + mtx.vin.resize(10); + + // Check it fails as expected + CValidationState state1; + CTransaction tx1(mtx); + EXPECT_FALSE(AcceptToMemoryPool(pool, state1, tx1, false, &missingInputs)); + EXPECT_EQ(state1.GetRejectReason(), "bad-txns-version-too-low"); + + // Set a limit + mapArgs["-mempooltxinputlimit"] = "10"; + + // Check it stil fails as expected + CValidationState state2; + EXPECT_FALSE(AcceptToMemoryPool(pool, state2, tx1, false, &missingInputs)); + EXPECT_EQ(state2.GetRejectReason(), "bad-txns-version-too-low"); + + // Resize the transaction + mtx.vin.resize(11); + + // Check it now fails due to exceeding the limit + CValidationState state3; + CTransaction tx3(mtx); + EXPECT_FALSE(AcceptToMemoryPool(pool, state3, tx3, false, &missingInputs)); + EXPECT_NE(state3.GetRejectReason(), "bad-txns-version-too-low"); + + // Clear the limit + mapArgs.erase("-mempooltxinputlimit"); + + // Check it no longer fails due to exceeding the limit + CValidationState state4; + EXPECT_FALSE(AcceptToMemoryPool(pool, state4, tx3, false, &missingInputs)); + EXPECT_EQ(state4.GetRejectReason(), "bad-txns-version-too-low"); +} + From 9e84b5aa0db99815b49ddc4195d43769d1949197 Mon Sep 17 00:00:00 2001 From: Simon Date: Sat, 17 Jun 2017 14:41:25 -0700 Subject: [PATCH 3/8] Check mempooltxinputlimit when creating a transaction to avoid local mempool rejection. --- src/wallet/asyncrpcoperation_sendmany.cpp | 13 ++++++++++++- src/wallet/wallet.cpp | 10 ++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/src/wallet/asyncrpcoperation_sendmany.cpp b/src/wallet/asyncrpcoperation_sendmany.cpp index 1691452b..3bbbfcc0 100644 --- a/src/wallet/asyncrpcoperation_sendmany.cpp +++ b/src/wallet/asyncrpcoperation_sendmany.cpp @@ -203,7 +203,18 @@ bool AsyncRPCOperation_sendmany::main_impl() { if (isfromzaddr_ && !find_unspent_notes()) { throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, "Insufficient funds, no unspent notes found for zaddr from address."); } - + + // Check mempooltxinputlimit to avoid creating a transaction which the local mempool rejects + if (isfromtaddr_) { + size_t limit = (size_t)GetArg("-mempooltxinputlimit", 0); + if (limit > 0) { + size_t n = t_inputs_.size(); + if (n > limit) { + throw JSONRPCError(RPC_WALLET_ERROR, strprintf("Too many transparent inputs %zu > limit %zu", n, limit)); + } + } + } + CAmount t_inputs_total = 0; for (SendManyInputUTXO & t : t_inputs_) { t_inputs_total += std::get<2>(t); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 98deedf6..a61c1e2f 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2717,6 +2717,16 @@ bool CWallet::CreateTransaction(const vector& vecSend, CWalletTx& wt txNew.vin.push_back(CTxIn(coin.first->GetHash(),coin.second,CScript(), std::numeric_limits::max()-1)); + // Check mempooltxinputlimit to avoid creating a transaction which the local mempool rejects + size_t limit = (size_t)GetArg("-mempooltxinputlimit", 0); + if (limit > 0) { + size_t n = txNew.vin.size(); + if (n > limit) { + strFailReason = _(strprintf("Too many transparent inputs %zu > limit %zu", n, limit).c_str()); + return false; + } + } + // Sign int nIn = 0; CTransaction txNewConst(txNew); From 4ef014151d0c2d0f9c151a96b7ae8fb521f56282 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Tue, 20 Jun 2017 15:58:46 +1200 Subject: [PATCH 4/8] Additional testing of -mempooltxinputlimit --- qa/pull-tester/rpc-tests.sh | 1 + qa/rpc-tests/mempool_tx_input_limit.py | 138 +++++++++++++++++++++++++ src/gtest/test_mempool.cpp | 6 +- 3 files changed, 144 insertions(+), 1 deletion(-) create mode 100755 qa/rpc-tests/mempool_tx_input_limit.py diff --git a/qa/pull-tester/rpc-tests.sh b/qa/pull-tester/rpc-tests.sh index 08ff3fe7..99706f17 100755 --- a/qa/pull-tester/rpc-tests.sh +++ b/qa/pull-tester/rpc-tests.sh @@ -26,6 +26,7 @@ testScripts=( 'rest.py' 'mempool_spendcoinbase.py' 'mempool_coinbase_spends.py' + 'mempool_tx_input_limit.py' 'httpbasics.py' 'zapwallettxes.py' 'proxy_test.py' diff --git a/qa/rpc-tests/mempool_tx_input_limit.py b/qa/rpc-tests/mempool_tx_input_limit.py new file mode 100755 index 00000000..ed0bf206 --- /dev/null +++ b/qa/rpc-tests/mempool_tx_input_limit.py @@ -0,0 +1,138 @@ +#!/usr/bin/env python2 +# Copyright (c) 2017 The Zcash developers +# Distributed under the MIT software license, see the accompanying +# file COPYING or http://www.opensource.org/licenses/mit-license.php. + +from test_framework.test_framework import BitcoinTestFramework +from test_framework.util import * +import os +import shutil +from time import sleep + +# Create one-input, one-output, no-fee transaction: +class MempoolTxInputLimitTest(BitcoinTestFramework): + + alert_filename = None # Set by setup_network + + def setup_network(self): + args = ["-checkmempool", "-debug=mempool", "-mempooltxinputlimit=2"] + self.nodes = [] + self.nodes.append(start_node(0, self.options.tmpdir, args)) + self.nodes.append(start_node(1, self.options.tmpdir, args)) + connect_nodes(self.nodes[1], 0) + self.is_network_split = False + self.sync_all + + def setup_chain(self): + print "Initializing test directory "+self.options.tmpdir + initialize_chain_clean(self.options.tmpdir, 2) + + def call_z_sendmany(self, from_addr, to_addr, amount): + recipients = [] + recipients.append({"address": to_addr, "amount": amount}) + myopid = self.nodes[0].z_sendmany(from_addr, recipients) + + opids = [] + opids.append(myopid) + + timeout = 120 + status = None + for x in xrange(1, timeout): + results = self.nodes[0].z_getoperationresult(opids) + if len(results)==0: + sleep(1) + else: + status = results[0]["status"] + assert_equal("success", status) + return results[0]["result"]["txid"] + + def run_test(self): + start_count = self.nodes[0].getblockcount() + + self.nodes[0].generate(100) + self.sync_all() + # Mine three blocks. After this, nodes[0] blocks + # 1, 2, and 3 are spend-able. + self.nodes[1].generate(3) + self.sync_all() + + # Check 1: z_sendmany is limited by -mempooltxinputlimit + + # Add zaddr to node 0 + node0_zaddr = self.nodes[0].z_getnewaddress() + + # Send three inputs from node 0 taddr to zaddr to get out of coinbase + node0_taddr = self.nodes[0].getnewaddress(); + recipients = [] + recipients.append({"address":node0_zaddr, "amount":Decimal('30.0')-Decimal('0.0001')}) # utxo amount less fee + myopid = self.nodes[0].z_sendmany(node0_taddr, recipients) + + opids = [] + opids.append(myopid) + + # Spend should fail due to -mempooltxinputlimit + timeout = 120 + status = None + for x in xrange(1, timeout): + results = self.nodes[0].z_getoperationresult(opids) + if len(results)==0: + sleep(1) + else: + status = results[0]["status"] + msg = results[0]["error"]["message"] + assert_equal("failed", status) + assert_equal("Too many transparent inputs 3 > limit 2", msg) + break + + # Mempool should be empty. + assert_equal(set(self.nodes[0].getrawmempool()), set()) + + # Reduce amount to only use two inputs + spend_zaddr_amount = Decimal('20.0') - Decimal('0.0001') + spend_zaddr_id = self.call_z_sendmany(node0_taddr, node0_zaddr, spend_zaddr_amount) # utxo amount less fee + self.sync_all() + + # Spend should be in the mempool + assert_equal(set(self.nodes[0].getrawmempool()), set([ spend_zaddr_id ])) + + self.nodes[0].generate(1) + self.sync_all() + + # mempool should be empty. + assert_equal(set(self.nodes[0].getrawmempool()), set()) + + # Check 2: sendfrom is limited by -mempooltxinputlimit + node1_taddr = self.nodes[1].getnewaddress(); + recipients = [] + spend_taddr_amount = spend_zaddr_amount - Decimal('0.0001') + spend_taddr_output = Decimal('8') + # Create three outputs + self.call_z_sendmany(node0_zaddr, node1_taddr, spend_taddr_output - Decimal('0.0001')) + self.nodes[1].generate(1) + self.sync_all() + self.call_z_sendmany(node0_zaddr, node1_taddr, spend_taddr_output - Decimal('0.0001')) + self.nodes[1].generate(1) + self.sync_all() + self.call_z_sendmany(node0_zaddr, node1_taddr, spend_taddr_amount - spend_taddr_output - spend_taddr_output - Decimal('0.0001')) # note amount less fees + self.nodes[1].generate(1) + self.sync_all() + + # Should use three UTXOs and fail + try: + self.nodes[1].sendtoaddress(node0_taddr, spend_taddr_amount - Decimal('1')) + assert(False) + except JSONRPCException,e: + msg = e.error['message'] + assert_equal("Too many transparent inputs 3 > limit 2", msg) + + # mempool should be empty. + assert_equal(set(self.nodes[1].getrawmempool()), set()) + + # Should use two UTXOs and succeed + spend_taddr_id2 = self.nodes[1].sendtoaddress(node0_taddr, spend_taddr_output + spend_taddr_output - Decimal('1')) + + # Spend should be in the mempool + assert_equal(set(self.nodes[1].getrawmempool()), set([ spend_taddr_id2 ])) + +if __name__ == '__main__': + MempoolTxInputLimitTest().main() diff --git a/src/gtest/test_mempool.cpp b/src/gtest/test_mempool.cpp index e84b2dea..14b4d83b 100644 --- a/src/gtest/test_mempool.cpp +++ b/src/gtest/test_mempool.cpp @@ -96,6 +96,9 @@ TEST(Mempool, TxInputLimit) { bool missingInputs; // Create an obviously-invalid transaction + // We intentionally set tx.nVersion = 0 to reliably trigger an error, as + // it's the first check that occurs after the -mempooltxinputlimit check, + // and it means that we don't have to mock out a lot of global state. CMutableTransaction mtx; mtx.nVersion = 0; mtx.vin.resize(10); @@ -121,7 +124,8 @@ TEST(Mempool, TxInputLimit) { CValidationState state3; CTransaction tx3(mtx); EXPECT_FALSE(AcceptToMemoryPool(pool, state3, tx3, false, &missingInputs)); - EXPECT_NE(state3.GetRejectReason(), "bad-txns-version-too-low"); + // The -mempooltxinputlimit check doesn't set a reason + EXPECT_EQ(state3.GetRejectReason(), ""); // Clear the limit mapArgs.erase("-mempooltxinputlimit"); From d8616d012a5738b893f6ab4ae53a41792402fe96 Mon Sep 17 00:00:00 2001 From: Simon Date: Mon, 19 Jun 2017 21:11:34 -0700 Subject: [PATCH 5/8] Partial revert & fix for commit 9e84b5a ; code block in wrong location. --- src/wallet/asyncrpcoperation_sendmany.cpp | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/src/wallet/asyncrpcoperation_sendmany.cpp b/src/wallet/asyncrpcoperation_sendmany.cpp index 3bbbfcc0..df48fb77 100644 --- a/src/wallet/asyncrpcoperation_sendmany.cpp +++ b/src/wallet/asyncrpcoperation_sendmany.cpp @@ -204,17 +204,6 @@ bool AsyncRPCOperation_sendmany::main_impl() { throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, "Insufficient funds, no unspent notes found for zaddr from address."); } - // Check mempooltxinputlimit to avoid creating a transaction which the local mempool rejects - if (isfromtaddr_) { - size_t limit = (size_t)GetArg("-mempooltxinputlimit", 0); - if (limit > 0) { - size_t n = t_inputs_.size(); - if (n > limit) { - throw JSONRPCError(RPC_WALLET_ERROR, strprintf("Too many transparent inputs %zu > limit %zu", n, limit)); - } - } - } - CAmount t_inputs_total = 0; for (SendManyInputUTXO & t : t_inputs_) { t_inputs_total += std::get<2>(t); @@ -292,6 +281,15 @@ bool AsyncRPCOperation_sendmany::main_impl() { t_inputs_ = selectedTInputs; t_inputs_total = selectedUTXOAmount; + // Check mempooltxinputlimit to avoid creating a transaction which the local mempool rejects + size_t limit = (size_t)GetArg("-mempooltxinputlimit", 0); + if (limit > 0) { + size_t n = t_inputs_.size(); + if (n > limit) { + throw JSONRPCError(RPC_WALLET_ERROR, strprintf("Too many transparent inputs %zu > limit %zu", n, limit)); + } + } + // update the transaction with these inputs CMutableTransaction rawTx(tx_); for (SendManyInputUTXO & t : t_inputs_) { From b1eb4f251a5e144609f16761ea3a15668f3fc1d2 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Tue, 20 Jun 2017 19:54:11 +1200 Subject: [PATCH 6/8] Fix comment --- qa/rpc-tests/mempool_tx_input_limit.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/qa/rpc-tests/mempool_tx_input_limit.py b/qa/rpc-tests/mempool_tx_input_limit.py index ed0bf206..14bac9f8 100755 --- a/qa/rpc-tests/mempool_tx_input_limit.py +++ b/qa/rpc-tests/mempool_tx_input_limit.py @@ -9,7 +9,7 @@ import os import shutil from time import sleep -# Create one-input, one-output, no-fee transaction: +# Test -mempooltxinputlimit class MempoolTxInputLimitTest(BitcoinTestFramework): alert_filename = None # Set by setup_network From 99f6d5da6caf714f185c1a943291508b3ef7116c Mon Sep 17 00:00:00 2001 From: Simon Date: Tue, 20 Jun 2017 22:49:03 +0000 Subject: [PATCH 7/8] Fix #b1eb4f2 so test checks sendfrom as originally intended. Also reduce number of z_sendmany calls made so test runs quicker. --- qa/rpc-tests/mempool_tx_input_limit.py | 38 ++++++++++++++++++-------- 1 file changed, 26 insertions(+), 12 deletions(-) diff --git a/qa/rpc-tests/mempool_tx_input_limit.py b/qa/rpc-tests/mempool_tx_input_limit.py index 14bac9f8..b2d71cf3 100755 --- a/qa/rpc-tests/mempool_tx_input_limit.py +++ b/qa/rpc-tests/mempool_tx_input_limit.py @@ -31,20 +31,34 @@ class MempoolTxInputLimitTest(BitcoinTestFramework): recipients = [] recipients.append({"address": to_addr, "amount": amount}) myopid = self.nodes[0].z_sendmany(from_addr, recipients) + return self.wait_and_assert_operationid_status(myopid) + def wait_and_assert_operationid_status(self, myopid, in_status='success', in_errormsg=None): + print('waiting for async operation {}'.format(myopid)) opids = [] opids.append(myopid) - - timeout = 120 + timeout = 300 status = None + errormsg = None + txid = None for x in xrange(1, timeout): results = self.nodes[0].z_getoperationresult(opids) if len(results)==0: sleep(1) else: status = results[0]["status"] - assert_equal("success", status) - return results[0]["result"]["txid"] + if status == "failed": + errormsg = results[0]['error']['message'] + elif status == "success": + txid = results[0]['result']['txid'] + break + print('...returned status: {}'.format(status)) + assert_equal(in_status, status) + if errormsg is not None: + assert(in_errormsg is not None) + assert_equal(in_errormsg in errormsg, True) + print('...returned error: {}'.format(errormsg)) + return txid def run_test(self): start_count = self.nodes[0].getblockcount() @@ -106,20 +120,20 @@ class MempoolTxInputLimitTest(BitcoinTestFramework): recipients = [] spend_taddr_amount = spend_zaddr_amount - Decimal('0.0001') spend_taddr_output = Decimal('8') + # Create three outputs - self.call_z_sendmany(node0_zaddr, node1_taddr, spend_taddr_output - Decimal('0.0001')) - self.nodes[1].generate(1) - self.sync_all() - self.call_z_sendmany(node0_zaddr, node1_taddr, spend_taddr_output - Decimal('0.0001')) - self.nodes[1].generate(1) - self.sync_all() - self.call_z_sendmany(node0_zaddr, node1_taddr, spend_taddr_amount - spend_taddr_output - spend_taddr_output - Decimal('0.0001')) # note amount less fees + recipients.append({"address":self.nodes[1].getnewaddress(), "amount": spend_taddr_output}) + recipients.append({"address":self.nodes[1].getnewaddress(), "amount": spend_taddr_output}) + recipients.append({"address":self.nodes[1].getnewaddress(), "amount": spend_taddr_amount - spend_taddr_output - spend_taddr_output}) + + myopid = self.nodes[0].z_sendmany(node0_zaddr, recipients) + self.wait_and_assert_operationid_status(myopid) self.nodes[1].generate(1) self.sync_all() # Should use three UTXOs and fail try: - self.nodes[1].sendtoaddress(node0_taddr, spend_taddr_amount - Decimal('1')) + self.nodes[1].sendfrom("", node0_taddr, spend_taddr_amount - Decimal('1')) assert(False) except JSONRPCException,e: msg = e.error['message'] From 6ea58d15315dae2f65825221f1844856fa49df7e Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Thu, 22 Jun 2017 09:34:10 +1200 Subject: [PATCH 8/8] Use sendfrom for both t-addr calls --- qa/rpc-tests/mempool_tx_input_limit.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/qa/rpc-tests/mempool_tx_input_limit.py b/qa/rpc-tests/mempool_tx_input_limit.py index b2d71cf3..89a96a0c 100755 --- a/qa/rpc-tests/mempool_tx_input_limit.py +++ b/qa/rpc-tests/mempool_tx_input_limit.py @@ -143,7 +143,7 @@ class MempoolTxInputLimitTest(BitcoinTestFramework): assert_equal(set(self.nodes[1].getrawmempool()), set()) # Should use two UTXOs and succeed - spend_taddr_id2 = self.nodes[1].sendtoaddress(node0_taddr, spend_taddr_output + spend_taddr_output - Decimal('1')) + spend_taddr_id2 = self.nodes[1].sendfrom("", node0_taddr, spend_taddr_output + spend_taddr_output - Decimal('1')) # Spend should be in the mempool assert_equal(set(self.nodes[1].getrawmempool()), set([ spend_taddr_id2 ]))