diff --git a/qa/replace-by-fee/rbf-tests.py b/qa/replace-by-fee/rbf-tests.py index 5173da6412..b48748fb4b 100755 --- a/qa/replace-by-fee/rbf-tests.py +++ b/qa/replace-by-fee/rbf-tests.py @@ -35,11 +35,11 @@ class Test_ReplaceByFee(unittest.TestCase): 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 @@ -47,10 +47,18 @@ class Test_ReplaceByFee(unittest.TestCase): 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 @@ class Test_ReplaceByFee(unittest.TestCase): 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 @@ class Test_ReplaceByFee(unittest.TestCase): 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() diff --git a/src/main.cpp b/src/main.cpp index 10d661b2a8..6e238f552e 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -1009,6 +1009,7 @@ bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState &state, const CTransa LOCK(pool.cs); CFeeRate newFeeRate(nFees, nSize); + set 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.