Reject replacements that add new unconfirmed inputs

Author: petertodd

qa/replace-by-fee/rbf-tests.py

@@ -35,22 +35,30 @@ def setUpClass(cls):
             cls.proxy = bitcoin.rpc.Proxy()
 
     @classmethod
-    def tearDownClass(cls):
-        # Make sure mining works
+    def mine_mempool(cls):
+        """Mine until mempool is empty"""
         mempool_size = 1
         while mempool_size:
-            cls.proxy.call('generate',1)
+            cls.proxy.call('generate', 1)
             new_mempool_size = len(cls.proxy.getrawmempool())
 
             # It's possible to get stuck in a loop here if the mempool has
             # transactions that can't be mined.
             assert(new_mempool_size != mempool_size)
             mempool_size = new_mempool_size
 
-    def make_txout(self, amount, scriptPubKey=CScript([1])):
+    @classmethod
+    def tearDownClass(cls):
+        # Make sure mining works
+        cls.mine_mempool()
+
+    def make_txout(self, amount, confirmed=True, scriptPubKey=CScript([1])):
         """Create a txout with a given amount and scriptPubKey
 
         Mines coins as needed.
+
+        confirmed - txouts created will be confirmed in the blockchain;
+                    unconfirmed otherwise.
         """
         fee = 1*COIN
         while self.proxy.getbalance() < amount + fee:
@@ -72,6 +80,10 @@ def make_txout(self, amount, scriptPubKey=CScript([1])):
 
         tx2_txid = self.proxy.sendrawtransaction(tx2, True)
 
+        # If requested, ensure txouts are confirmed.
+        if confirmed:
+            self.mine_mempool()
+
         return COutPoint(tx2_txid, 0)
 
     def test_simple_doublespend(self):
@@ -276,5 +288,24 @@ def test_spends_of_conflicting_outputs(self):
         else:
             self.fail()
 
+    def test_new_unconfirmed_inputs(self):
+        """Replacements that add new unconfirmed inputs are rejected"""
+        confirmed_utxo = self.make_txout(1.1*COIN)
+        unconfirmed_utxo = self.make_txout(0.1*COIN, False)
+
+        tx1 = CTransaction([CTxIn(confirmed_utxo)],
+                           [CTxOut(1.0*COIN, CScript([b'a']))])
+        tx1_txid = self.proxy.sendrawtransaction(tx1, True)
+
+        tx2 = CTransaction([CTxIn(confirmed_utxo), CTxIn(unconfirmed_utxo)],
+                           tx1.vout)
+
+        try:
+            tx2_txid = self.proxy.sendrawtransaction(tx2, True)
+        except bitcoin.rpc.JSONRPCException as exp:
+            self.assertEqual(exp.error['code'], -26)
+        else:
+            self.fail()
+
 if __name__ == '__main__':
     unittest.main()

src/main.cpp

@@ -1009,6 +1009,7 @@ bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState &state, const CTransa
             LOCK(pool.cs);
 
             CFeeRate newFeeRate(nFees, nSize);
+            set<uint256> setConflictsParents;
             BOOST_FOREACH(const uint256 hashConflicting, setConflicts)
             {
                 CTxMemPool::txiter mi = pool.mapTx.find(hashConflicting);
@@ -1042,6 +1043,11 @@ bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState &state, const CTransa
                             REJECT_INSUFFICIENTFEE, "insufficient fee");
                 }
 
+                BOOST_FOREACH(const CTxIn &txin, mi->GetTx().vin)
+                {
+                    setConflictsParents.insert(txin.prevout.hash);
+                }
+
                 // For efficiency we simply sum up the pre-calculated
                 // fees/size-with-descendants values from the mempool package
                 // tracking; this does mean the pathological case of diamond tx
@@ -1050,6 +1056,24 @@ bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState &state, const CTransa
                 nConflictingSize += mi->GetSizeWithDescendants();
             }
 
+            for (unsigned int j = 0; j < tx.vin.size(); j++)
+            {
+                // We don't want to accept replacements that require low
+                // feerate junk to be mined first. Ideally we'd keep track of
+                // the ancestor feerates and make the decision based on that,
+                // but for now requiring all new inputs to be confirmed works.
+                if (!setConflictsParents.count(tx.vin[j].prevout.hash))
+                {
+                    // Rather than check the UTXO set - potentially expensive -
+                    // it's cheaper to just check if the new input refers to a
+                    // tx that's in the mempool.
+                    if (pool.mapTx.find(tx.vin[j].prevout.hash) != pool.mapTx.end())
+                        return state.DoS(0, error("AcceptToMemoryPool: replacement %s adds unconfirmed input, idx %d",
+                                                  hash.ToString(), j),
+                                         REJECT_NONSTANDARD, "replacement-adds-unconfirmed");
+                }
+            }
+
             // The replacement must pay greater fees than the transactions it
             // replaces - if we did the bandwidth used by those conflicting
             // transactions would not be paid for.