Conservatively accept RBF bumps bumping one tx at the package limits

Accept RBF bumps of single transactions (ie which evict exactly one
transaction) even when that transaction is a member of a package
which is currently at the package limit iff the new transaction
does not add any additional mempool dependencies from the original.

This could be made a bit looser in the future and still be safe,
but for now this fixes the case that a transaction which was
accepted by the carve-out rule will not be directly RBF'able.

src/validation.cpp

@@ -602,6 +602,7 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool
                 REJECT_HIGHFEE, "absurdly-high-fee",
                 strprintf("%d > %d", nFees, nAbsurdFee));
 
+        const CTxMemPool::setEntries setIterConflicting = pool.GetIterSet(setConflicts);
         // Calculate in-mempool ancestors, up to a limit.
         CTxMemPool::setEntries setAncestors;
         size_t nLimitAncestors = gArgs.GetArg("-limitancestorcount", DEFAULT_ANCESTOR_LIMIT);
@@ -611,19 +612,48 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool
         std::string errString;
         if (!pool.CalculateMemPoolAncestors(entry, setAncestors, nLimitAncestors, nLimitAncestorSize, nLimitDescendants, nLimitDescendantSize, errString)) {
             setAncestors.clear();
-            // If the new transaction is relatively small (up to 40k weight)
-            // and has at most one ancestor (ie ancestor limit of 2, including
-            // the new transaction), allow it if its parent has exactly the
-            // descendant limit descendants.
-            //
-            // This allows protocols which rely on distrusting counterparties
-            // being able to broadcast descendants of an unconfirmed transaction
-            // to be secure by simply only having two immediately-spendable
-            // outputs - one for each counterparty. For more info on the uses for
-            // this, see https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2018-November/016518.html
-            if (nSize >  EXTRA_DESCENDANT_TX_SIZE_LIMIT ||
-                    !pool.CalculateMemPoolAncestors(entry, setAncestors, 2, nLimitAncestorSize, nLimitDescendants + 1, nLimitDescendantSize + EXTRA_DESCENDANT_TX_SIZE_LIMIT, errString)) {
-                return state.Invalid(ValidationInvalidReason::TX_MEMPOOL_POLICY, false, REJECT_NONSTANDARD, "too-long-mempool-chain", errString);
+            if (setConflicts.size() == 1) {
+                // If we are an RBF transaction which will evict exactly one other transaction, conservatively accept
+                // transactions which are not adding any new mempool dependencies. Such transactions are clearly not
+                // violating the package limits, and thus the CalculateMemPoolAncestors call is set with values which
+                // will allow such transactions even if they originally got in using the carve-out special rule in
+                // the next else branch.
+                assert(setIterConflicting.size() == 1);
+                CTxMemPool::txiter conflict = *setIterConflicting.begin();
+
+                std::set<uint256> conflict_existing_mempool_inputs;
+                for (const CTxIn& input : conflict->GetTx().vin) {
+                    if (mempool.exists(input.prevout.hash)) {
+                        conflict_existing_mempool_inputs.insert(input.prevout.hash);
+                    }
+                }
+
+                for (const CTxIn& new_input: tx.vin) {
+                    if (mempool.exists(new_input.prevout.hash)) {
+                        if (conflict_existing_mempool_inputs.count(new_input.prevout.hash) == 0) {
+                            return state.Invalid(ValidationInvalidReason::TX_MEMPOOL_POLICY, false, REJECT_NONSTANDARD, "too-long-mempool-chain", errString);
+                        }
+                    }
+                }
+                if (!pool.CalculateMemPoolAncestors(entry, setAncestors, nLimitAncestors, nLimitAncestorSize + conflict->GetTxSize(),
+                            nLimitDescendants + 2, nLimitDescendantSize + conflict->GetTxSize() + EXTRA_DESCENDANT_TX_SIZE_LIMIT, errString)) {
+                    return state.Invalid(ValidationInvalidReason::TX_MEMPOOL_POLICY, false, REJECT_NONSTANDARD, "too-long-mempool-chain", errString);
+                }
+            } else {
+                // If the new transaction is relatively small (up to 40k weight)
+                // and has at most one ancestor (ie ancestor limit of 2, including
+                // the new transaction), allow it if its parent has exactly the
+                // descendant limit descendants.
+                //
+                // This allows protocols which rely on distrusting counterparties
+                // being able to broadcast descendants of an unconfirmed transaction
+                // to be secure by simply only having two immediately-spendable
+                // outputs - one for each counterparty. For more info on the uses for
+                // this, see https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2018-November/016518.html
+                if (nSize >  EXTRA_DESCENDANT_TX_SIZE_LIMIT ||
+                        !pool.CalculateMemPoolAncestors(entry, setAncestors, 2, nLimitAncestorSize, nLimitDescendants + 1, nLimitDescendantSize + EXTRA_DESCENDANT_TX_SIZE_LIMIT, errString)) {
+                    return state.Invalid(ValidationInvalidReason::TX_MEMPOOL_POLICY, false, REJECT_NONSTANDARD, "too-long-mempool-chain", errString);
+                }
             }
         }
 
@@ -659,7 +689,6 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool
             CFeeRate newFeeRate(nModifiedFees, nSize);
             std::set<uint256> setConflictsParents;
             const int maxDescendantsToVisit = 100;
-            const CTxMemPool::setEntries setIterConflicting = pool.GetIterSet(setConflicts);
             for (const auto& mi : setIterConflicting) {
                 // Don't allow the replacement to reduce the feerate of the
                 // mempool.

test/functional/mempool_package_onemore.py

@@ -33,7 +33,7 @@ def chain_transaction(self, node, parent_txids, vouts, value, fee, num_outputs):
         outputs = {}
         for i in range(num_outputs):
             outputs[node.getnewaddress()] = send_value
-        rawtx = node.createrawtransaction(inputs, outputs)
+        rawtx = node.createrawtransaction(inputs, outputs, 0, True)
         signedtx = node.signrawtransactionwithwallet(rawtx)
         txid = node.sendrawtransaction(signedtx['hex'])
         fulltx = node.getrawtransaction(txid, 1)
@@ -75,10 +75,16 @@ def run_test(self):
         # ...especially if its > 40k weight
         assert_raises_rpc_error(-26, "too-long-mempool-chain", self.chain_transaction, self.nodes[0], [chain[0][0]], [1], chain[0][1], fee, 350)
         # But not if it chains directly off the first transaction
-        self.chain_transaction(self.nodes[0], [chain[0][0]], [1], chain[0][1], fee, 1)
+        (replacable_txid, replacable_orig_value) = self.chain_transaction(self.nodes[0], [chain[0][0]], [1], chain[0][1], fee, 1)
         # and the second chain should work just fine
         self.chain_transaction(self.nodes[0], [second_chain], [0], second_chain_value, fee, 1)
 
+        # Make sure we can RBF the chain which used our carve-out rule
+        second_tx_outputs = {self.nodes[0].getrawtransaction(replacable_txid, True)["vout"][0]['scriptPubKey']['addresses'][0]: replacable_orig_value - (Decimal(1) / Decimal(100))}
+        second_tx = self.nodes[0].createrawtransaction([{'txid': chain[0][0], 'vout': 1}], second_tx_outputs)
+        signed_second_tx = self.nodes[0].signrawtransactionwithwallet(second_tx)
+        self.nodes[0].sendrawtransaction(signed_second_tx['hex'])
+
         # Finally, check that we added two transactions
         assert_equal(len(self.nodes[0].getrawmempool(True)), MAX_ANCESTORS + 3)