diff --git a/doc/release-notes.md b/doc/release-notes.md index a29094b51..8d7614c8b 100644 --- a/doc/release-notes.md +++ b/doc/release-notes.md @@ -4,3 +4,19 @@ release-notes at release time) Notable changes =============== +`-mempooltxinputlimit` deprecation +---------------------------------- + +The configuration option `-mempooltxinputlimit` was added in release 1.0.10 as a +short-term fix for the quadratic hashing problem inherited from Bitcoin. At the +time, transactions with many inputs were causing performance issues for miners. +Since then, several performance improvements have been merged from the Bitcoin +Core codebase that significantly reduce these issues. + +The Overwinter network upgrade includes changes that solve the quadratic hashing +problem, and so `-mempooltxinputlimit` will no longer be needed - a transaction +with 1000 inputs will take just as long to validate as 10 transactions with 100 +inputs each. Starting from this release, `-mempooltxinputlimit` will be enforced +before the Overwinter activation height is reached, but will be ignored once +Overwinter activates. The option will be removed entirely in a future release +after Overwinter has activated. diff --git a/qa/rpc-tests/mempool_tx_input_limit.py b/qa/rpc-tests/mempool_tx_input_limit.py index c48d73be0..ce4b2096d 100755 --- a/qa/rpc-tests/mempool_tx_input_limit.py +++ b/qa/rpc-tests/mempool_tx_input_limit.py @@ -17,7 +17,7 @@ class MempoolTxInputLimitTest(BitcoinTestFramework): alert_filename = None # Set by setup_network def setup_network(self): - args = ["-checkmempool", "-debug=mempool", "-mempooltxinputlimit=2"] + args = ["-checkmempool", "-debug=mempool", "-mempooltxinputlimit=2", "-nuparams=5ba81b19:110"] self.nodes = [] self.nodes.append(start_node(0, self.options.tmpdir, args)) self.nodes.append(start_node(1, self.options.tmpdir, args)) @@ -120,5 +120,31 @@ class MempoolTxInputLimitTest(BitcoinTestFramework): # Spend should be in the mempool assert_equal(set(self.nodes[1].getrawmempool()), set([ spend_taddr_id2 ])) + # Mine three blocks + self.nodes[1].generate(3) + self.sync_all() + # The next block to be mined, 109, is the last Sprout block + bci = self.nodes[0].getblockchaininfo() + assert_equal(bci['consensus']['chaintip'], '00000000') + assert_equal(bci['consensus']['nextblock'], '00000000') + + # z_sendmany should be limited by -mempooltxinputlimit + 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) + wait_and_assert_operationid_status(self.nodes[0], myopid, 'failed', 'Too many transparent inputs 3 > limit 2') + + # Mine one block + self.nodes[1].generate(1) + self.sync_all() + # The next block to be mined, 110, is the first Overwinter block + bci = self.nodes[0].getblockchaininfo() + assert_equal(bci['consensus']['chaintip'], '00000000') + assert_equal(bci['consensus']['nextblock'], '5ba81b19') + + # z_sendmany should no longer be limited by -mempooltxinputlimit + myopid = self.nodes[0].z_sendmany(node0_taddr, recipients) + wait_and_assert_operationid_status(self.nodes[0], myopid) + if __name__ == '__main__': MempoolTxInputLimitTest().main() diff --git a/src/gtest/test_mempool.cpp b/src/gtest/test_mempool.cpp index 9256f196a..34bc204fa 100644 --- a/src/gtest/test_mempool.cpp +++ b/src/gtest/test_mempool.cpp @@ -96,7 +96,7 @@ TEST(Mempool, PriorityStatsDoNotCrash) { } TEST(Mempool, TxInputLimit) { - SelectParams(CBaseChainParams::TESTNET); + SelectParams(CBaseChainParams::REGTEST); CTxMemPool pool(::minRelayTxFee); bool missingInputs; @@ -133,13 +133,30 @@ TEST(Mempool, TxInputLimit) { // The -mempooltxinputlimit check doesn't set a reason EXPECT_EQ(state3.GetRejectReason(), ""); - // Clear the limit - mapArgs.erase("-mempooltxinputlimit"); + // Activate Overwinter + UpdateNetworkUpgradeParameters(Consensus::UPGRADE_OVERWINTER, Consensus::NetworkUpgrade::ALWAYS_ACTIVE); // 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"); + + // Deactivate Overwinter + UpdateNetworkUpgradeParameters(Consensus::UPGRADE_OVERWINTER, Consensus::NetworkUpgrade::NO_ACTIVATION_HEIGHT); + + // Check it now fails due to exceeding the limit + CValidationState state5; + EXPECT_FALSE(AcceptToMemoryPool(pool, state5, tx3, false, &missingInputs)); + // The -mempooltxinputlimit check doesn't set a reason + EXPECT_EQ(state5.GetRejectReason(), ""); + + // Clear the limit + mapArgs.erase("-mempooltxinputlimit"); + + // Check it no longer fails due to exceeding the limit + CValidationState state6; + EXPECT_FALSE(AcceptToMemoryPool(pool, state6, tx3, false, &missingInputs)); + EXPECT_EQ(state6.GetRejectReason(), "bad-txns-version-too-low"); } // Valid overwinter v3 format tx gets rejected because overwinter hasn't activated yet. diff --git a/src/init.cpp b/src/init.cpp index 8c4a320cb..76e6f35dc 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -355,7 +355,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("-mempooltxinputlimit=", _("[DEPRECATED FROM OVERWINTER] 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)"), -GetNumCores(), MAX_SCRIPTCHECK_THREADS, DEFAULT_SCRIPTCHECK_THREADS)); #ifndef WIN32 diff --git a/src/main.cpp b/src/main.cpp index eb2aacc78..ca3a98d3d 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -1169,6 +1169,9 @@ bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState &state, const CTransa // 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 (NetworkUpgradeActive(nextBlockHeight, Params().GetConsensus(), Consensus::UPGRADE_OVERWINTER)) { + limit = 0; + } if (limit > 0) { size_t n = tx.vin.size(); if (n > limit) { diff --git a/src/wallet/asyncrpcoperation_mergetoaddress.cpp b/src/wallet/asyncrpcoperation_mergetoaddress.cpp index 30ef560df..bba75c438 100644 --- a/src/wallet/asyncrpcoperation_mergetoaddress.cpp +++ b/src/wallet/asyncrpcoperation_mergetoaddress.cpp @@ -204,6 +204,12 @@ bool AsyncRPCOperation_mergetoaddress::main_impl() // Check mempooltxinputlimit to avoid creating a transaction which the local mempool rejects size_t limit = (size_t)GetArg("-mempooltxinputlimit", 0); + { + LOCK(cs_main); + if (NetworkUpgradeActive(chainActive.Height() + 1, Params().GetConsensus(), Consensus::UPGRADE_OVERWINTER)) { + limit = 0; + } + } if (limit > 0 && numInputs > limit) { throw JSONRPCError(RPC_WALLET_ERROR, strprintf("Number of transparent inputs %d is greater than mempooltxinputlimit of %d", diff --git a/src/wallet/asyncrpcoperation_sendmany.cpp b/src/wallet/asyncrpcoperation_sendmany.cpp index 8181bdc3b..281433b0b 100644 --- a/src/wallet/asyncrpcoperation_sendmany.cpp +++ b/src/wallet/asyncrpcoperation_sendmany.cpp @@ -310,6 +310,12 @@ bool AsyncRPCOperation_sendmany::main_impl() { // Check mempooltxinputlimit to avoid creating a transaction which the local mempool rejects size_t limit = (size_t)GetArg("-mempooltxinputlimit", 0); + { + LOCK(cs_main); + if (NetworkUpgradeActive(chainActive.Height() + 1, Params().GetConsensus(), Consensus::UPGRADE_OVERWINTER)) { + limit = 0; + } + } if (limit > 0) { size_t n = t_inputs_.size(); if (n > limit) { diff --git a/src/wallet/asyncrpcoperation_shieldcoinbase.cpp b/src/wallet/asyncrpcoperation_shieldcoinbase.cpp index 527f810bc..6aef57949 100644 --- a/src/wallet/asyncrpcoperation_shieldcoinbase.cpp +++ b/src/wallet/asyncrpcoperation_shieldcoinbase.cpp @@ -186,6 +186,12 @@ bool AsyncRPCOperation_shieldcoinbase::main_impl() { // Check mempooltxinputlimit to avoid creating a transaction which the local mempool rejects size_t limit = (size_t)GetArg("-mempooltxinputlimit", 0); + { + LOCK(cs_main); + if (NetworkUpgradeActive(chainActive.Height() + 1, Params().GetConsensus(), Consensus::UPGRADE_OVERWINTER)) { + limit = 0; + } + } if (limit>0 && numInputs > limit) { throw JSONRPCError(RPC_WALLET_ERROR, strprintf("Number of inputs %d is greater than mempooltxinputlimit of %d", diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index b242580b1..fe09274f5 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -3580,8 +3580,9 @@ UniValue z_shieldcoinbase(const UniValue& params, bool fHelp) "\nShield transparent coinbase funds by sending to a shielded zaddr. This is an asynchronous operation and utxos" "\nselected for shielding will be locked. If there is an error, they are unlocked. The RPC call `listlockunspent`" "\ncan be used to return a list of locked utxos. The number of coinbase utxos selected for shielding can be limited" - "\nby the caller. If the limit parameter is set to zero, the -mempooltxinputlimit option will determine the number" - "\nof uxtos. Any limit is constrained by the consensus rule defining a maximum transaction size of " + "\nby the caller. If the limit parameter is set to zero, and Overwinter is not yet active, the -mempooltxinputlimit" + "\noption will determine the number of uxtos. Any limit is constrained by the consensus rule defining a maximum" + "\ntransaction size of " + strprintf("%d bytes.", MAX_TX_SIZE) + HelpRequiringPassphrase() + "\n" "\nArguments:\n" @@ -3590,7 +3591,7 @@ UniValue z_shieldcoinbase(const UniValue& params, bool fHelp) "3. fee (numeric, optional, default=" + strprintf("%s", FormatMoney(SHIELD_COINBASE_DEFAULT_MINERS_FEE)) + ") The fee amount to attach to this transaction.\n" "4. limit (numeric, optional, default=" - + strprintf("%d", SHIELD_COINBASE_DEFAULT_LIMIT) + ") Limit on the maximum number of utxos to shield. Set to 0 to use node option -mempooltxinputlimit.\n" + + strprintf("%d", SHIELD_COINBASE_DEFAULT_LIMIT) + ") Limit on the maximum number of utxos to shield. Set to 0 to use node option -mempooltxinputlimit (before Overwinter), or as many as will fit in the transaction (after Overwinter).\n" "\nResult:\n" "{\n" " \"remainingUTXOs\": xxx (numeric) Number of coinbase utxos still available for shielding.\n" @@ -3644,6 +3645,9 @@ UniValue z_shieldcoinbase(const UniValue& params, bool fHelp) } } + int nextBlockHeight = chainActive.Height() + 1; + bool overwinterActive = NetworkUpgradeActive(nextBlockHeight, Params().GetConsensus(), Consensus::UPGRADE_OVERWINTER); + // Prepare to get coinbase utxos std::vector inputs; CAmount shieldedValue = 0; @@ -3651,7 +3655,7 @@ UniValue z_shieldcoinbase(const UniValue& params, bool fHelp) size_t estimatedTxSize = 2000; // 1802 joinsplit description + tx overhead + wiggle room size_t utxoCounter = 0; bool maxedOutFlag = false; - size_t mempoolLimit = (nLimit != 0) ? nLimit : (size_t)GetArg("-mempooltxinputlimit", 0); + size_t mempoolLimit = (nLimit != 0) ? nLimit : (overwinterActive ? 0 : (size_t)GetArg("-mempooltxinputlimit", 0)); // Set of addresses to filter utxos by set setAddress = {}; @@ -3730,13 +3734,12 @@ UniValue z_shieldcoinbase(const UniValue& params, bool fHelp) contextInfo.push_back(Pair("fee", ValueFromAmount(nFee))); // Contextual transaction we will build on - int nextBlockHeight = chainActive.Height() + 1; CMutableTransaction contextualTx = CreateNewContextualCMutableTransaction( Params().GetConsensus(), nextBlockHeight); if (contextualTx.nVersion == 1) { contextualTx.nVersion = 2; // Tx format should support vjoinsplits } - if (NetworkUpgradeActive(nextBlockHeight, Params().GetConsensus(), Consensus::UPGRADE_OVERWINTER)) { + if (overwinterActive) { contextualTx.nExpiryHeight = nextBlockHeight + expiryDelta; } @@ -3782,8 +3785,8 @@ UniValue z_mergetoaddress(const UniValue& params, bool fHelp) "\n\nThis is an asynchronous operation, and UTXOs selected for merging will be locked. If there is an error, they" "\nare unlocked. The RPC call `listlockunspent` can be used to return a list of locked UTXOs." "\n\nThe number of UTXOs and notes selected for merging can be limited by the caller. If the transparent limit" - "\nparameter is set to zero, the -mempooltxinputlimit option will determine the number of UTXOs. Any limit is" - "\nconstrained by the consensus rule defining a maximum transaction size of " + "\nparameter is set to zero, and Overwinter is not yet active, the -mempooltxinputlimit option will determine the" + "\nnumber of UTXOs. Any limit is constrained by the consensus rule defining a maximum transaction size of " + strprintf("%d bytes.", MAX_TX_SIZE) + HelpRequiringPassphrase() + "\n" "\nArguments:\n" @@ -3801,7 +3804,7 @@ UniValue z_mergetoaddress(const UniValue& params, bool fHelp) "3. fee (numeric, optional, default=" + strprintf("%s", FormatMoney(MERGE_TO_ADDRESS_OPERATION_DEFAULT_MINERS_FEE)) + ") The fee amount to attach to this transaction.\n" "4. transparent_limit (numeric, optional, default=" - + strprintf("%d", MERGE_TO_ADDRESS_DEFAULT_TRANSPARENT_LIMIT) + ") Limit on the maximum number of UTXOs to merge. Set to 0 to use node option -mempooltxinputlimit.\n" + + strprintf("%d", MERGE_TO_ADDRESS_DEFAULT_TRANSPARENT_LIMIT) + ") Limit on the maximum number of UTXOs to merge. Set to 0 to use node option -mempooltxinputlimit (before Overwinter), or as many as will fit in the transaction (after Overwinter).\n" "4. shielded_limit (numeric, optional, default=" + strprintf("%d", MERGE_TO_ADDRESS_DEFAULT_SHIELDED_LIMIT) + ") Limit on the maximum number of notes to merge. Set to 0 to merge as many as will fit in the transaction.\n" "5. \"memo\" (string, optional) Encoded as hex. When toaddress is a z-addr, this will be stored in the memo field of the new note.\n" @@ -3935,6 +3938,9 @@ UniValue z_mergetoaddress(const UniValue& params, bool fHelp) MergeToAddressRecipient recipient(destaddress, memo); + int nextBlockHeight = chainActive.Height() + 1; + bool overwinterActive = NetworkUpgradeActive(nextBlockHeight, Params().GetConsensus(), Consensus::UPGRADE_OVERWINTER); + // Prepare to get UTXOs and notes std::vector utxoInputs; std::vector noteInputs; @@ -3946,7 +3952,7 @@ UniValue z_mergetoaddress(const UniValue& params, bool fHelp) size_t noteCounter = 0; bool maxedOutUTXOsFlag = false; bool maxedOutNotesFlag = false; - size_t mempoolLimit = (nUTXOLimit != 0) ? nUTXOLimit : (size_t)GetArg("-mempooltxinputlimit", 0); + size_t mempoolLimit = (nUTXOLimit != 0) ? nUTXOLimit : (overwinterActive ? 0 : (size_t)GetArg("-mempooltxinputlimit", 0)); size_t estimatedTxSize = 200; // tx overhead + wiggle room if (isToZaddr) { @@ -4065,7 +4071,6 @@ UniValue z_mergetoaddress(const UniValue& params, bool fHelp) contextInfo.push_back(Pair("fee", ValueFromAmount(nFee))); // Contextual transaction we will build on - int nextBlockHeight = chainActive.Height() + 1; CMutableTransaction contextualTx = CreateNewContextualCMutableTransaction( Params().GetConsensus(), nextBlockHeight); @@ -4073,7 +4078,7 @@ UniValue z_mergetoaddress(const UniValue& params, bool fHelp) if (contextualTx.nVersion == 1 && isShielded) { contextualTx.nVersion = 2; // Tx format should support vjoinsplit } - if (NetworkUpgradeActive(nextBlockHeight, Params().GetConsensus(), Consensus::UPGRADE_OVERWINTER)) { + if (overwinterActive) { contextualTx.nExpiryHeight = nextBlockHeight + expiryDelta; } diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index e2cd01637..cd2a8e1a4 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2775,6 +2775,12 @@ bool CWallet::CreateTransaction(const vector& vecSend, CWalletTx& wt // Check mempooltxinputlimit to avoid creating a transaction which the local mempool rejects size_t limit = (size_t)GetArg("-mempooltxinputlimit", 0); + { + LOCK(cs_main); + if (NetworkUpgradeActive(chainActive.Height() + 1, Params().GetConsensus(), Consensus::UPGRADE_OVERWINTER)) { + limit = 0; + } + } if (limit > 0) { size_t n = txNew.vin.size(); if (n > limit) {