Ignore -mempooltxinputlimit once Overwinter activates

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.

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

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.

src/init.cpp

@@ -355,7 +355,7 @@ std::string HelpMessage(HelpMessageMode mode)
     strUsage += HelpMessageOpt("-dbcache=<n>", strprintf(_("Set database cache size in megabytes (%d to %d, default: %d)"), nMinDbCache, nMaxDbCache, nDefaultDbCache));
     strUsage += HelpMessageOpt("-loadblock=<file>", _("Imports blocks from external blk000??.dat file") + " " + _("on startup"));
     strUsage += HelpMessageOpt("-maxorphantx=<n>", strprintf(_("Keep at most <n> unconnectable transactions in memory (default: %u)"), DEFAULT_MAX_ORPHAN_TRANSACTIONS));
-    strUsage += HelpMessageOpt("-mempooltxinputlimit=<n>", _("Set the maximum number of transparent inputs in a transaction that the mempool will accept (default: 0 = no limit applied)"));
+    strUsage += HelpMessageOpt("-mempooltxinputlimit=<n>", _("[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=<n>", 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

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<size_t>::max() >= std::numeric_limits<int64_t>::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) {

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",

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

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",

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<ShieldCoinbaseUTXO> 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<CBitcoinAddress> 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<MergeToAddressInputUTXO> utxoInputs;
     std::vector<MergeToAddressInputNote> 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;
     }
 

src/wallet/wallet.cpp

@@ -2775,6 +2775,12 @@ bool CWallet::CreateTransaction(const vector<CRecipient>& 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) {